Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Ref types for a Statement's Resource attribute #55

Open
dimpavloff opened this issue Aug 3, 2016 · 7 comments
Open

Allow Ref types for a Statement's Resource attribute #55

dimpavloff opened this issue Aug 3, 2016 · 7 comments

Comments

@dimpavloff
Copy link

dimpavloff commented Aug 3, 2016

Hi,

Currently it seems like awacs.aws.Statement's Resource attr can only be a list (https://github.com/cloudtools/awacs/blob/master/awacs/aws.py#L143)

It would be useful to allow passing in a troposphere.Ref object because then you can have a CommaDelimitedList parameter which contains the resources you want to create the policy for.

I guess this could be done with a callable, like the Effect type check in Statement, although it feels like there could be a more general solution that could be useful for all types, not just Statement.

Thanks

@cwgreene
Copy link

cwgreene commented Aug 11, 2016

I believe that this is valid, and is licensed in the grammar:

http://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_grammar.html

<statement> = { 
    <sid_block?>,
    <principal_block?>,
    <effect_block>,
    <action_block>,
    <resource_block>,
    <condition_block?>
}

<resource_block> = ("Resource" | "NotResource") : 
    ("*" | [<resource_string>, <resource_string>, ...])

The main issue I believe is that awacs doesn't know anything about Refs (and my understanding is that awacs should be viewed as an independent module from troposphere), or other stringlike objects. It might be a good idea to give a "is_string_coercable" method to classes that should be treated as strings for this purpose. This will provide an interface that awacs can then take advantage of.

@emayssat-ms
Copy link

emayssat-ms commented Jan 23, 2017

s3.ARN('mybucket') <== This works and returns the correct ARN
s3.ARN(Ref('CloudtrailBucket')) <<<=== this doesn't work!

In the template, I see
"arn:aws:s3:::<troposphere.Ref object at 0x7f40b8b70590>",

I recommend you document this issue in the docs, since many people are using troposphere and awacs together.

@pbudzon
Copy link

pbudzon commented Jun 23, 2017

Similar problem appears with using conditionals inside Statements. Something like this:

logsStatement = aws.Statement(...)
s3Statement = aws.Statement(...)

iam.PolicyType(
    ...
    PolicyDocument=aws.Policy(
        Statement=[
            logsStatement,
            If(
                "HasSources",
                s3Statement,
                Ref(AWS_NO_VALUE)
            )
        ]
    ),
)

is entirely valid in CloudFormation, but will not work with troposphere.
Raised before here: cloudtools/troposphere#668

@dgouldin
Copy link

dgouldin commented Feb 8, 2018

I ran into this limitation when trying to create a CloudFormation template including a policy document statement whose resource ARN references the template's region and account id (AWS::Region and AWS::AccountId). It would be great to be able to generate wildcard ARNs limited to the account and region of the CF template.

@DrLuke
Copy link
Contributor

DrLuke commented Jul 4, 2018

I also ran into issues because Refs aren't supported. What would be the best way to solve this? Re-implement Refs and all the other string manipulation functions in awacs or try to import it from troposphere?

A workaround is to just write raw JSON, for example:

aws.Statement(
    Effect="Allow",
    Action=[aws.Action("logs", "CreateLogGroup")],
    Resource=[
              {
                  "Fn::Join": ["", [
                      "arn:aws:logs:",
                      {"Ref": "AWS::Region"},
                      ":",
                      {"Ref": "AWS::AccountId"},
                      ":*"
                  ]]
              }
    ]
)

@dgouldin
Copy link

I've created a fork which solves the issue for me but probably isn't sufficiently generalized to be merged upstream: https://github.com/cloudtools/awacs/compare/master...clara-labs:arn-troposphere-join?expand=1

I doubt it's acceptable to always use troposphere to construct ARNs if it can be imported. Maybe this can serve as a straw man for discussion. What if we looked for any troposphere related instance in the ARN's parts and only used it in that case?

@michael-k
Copy link
Contributor

Is there anything that still doesn't work? Can this be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants