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

CompositePrincipal does not support conditions #1578

Closed
patrickdomnick opened this issue Jan 20, 2019 · 7 comments · Fixed by #17689
Closed

CompositePrincipal does not support conditions #1578

patrickdomnick opened this issue Jan 20, 2019 · 7 comments · Fixed by #17689
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@patrickdomnick
Copy link

I am trying to reproduces the following Cloudformation Template with CDK:

"AssumeRolePolicyDocument": {
          "Version": "2012-10-17",
          "Statement": [
            {
              "Effect": "Allow",
              "Action": [
                "sts:AssumeRole"
              ],
              "Principal": {
                "AWS": [
                  "arn:aws:iam::<account>:role/MasterRole",
                  "arn:aws:iam::<account>:role/AccountOwner"
                ]
              }
            },
            {
              "Effect": "Allow",
              "Action": [
                "sts:AssumeRoleWithSAML"
              ],
              "Principal": {
                "Federated": "arn:aws:iam::<account>:saml-provider/SAMLIDP"
              },
              "Condition": {
                "StringEquals": {
                  "SAML:aud": "https://signin.aws.amazon.com/saml"
                }
              }
            }
          ]
        },

Either of those (Federated or Arn) work just fine but I am unable to combine them with CompositePrincipal because there is no support for Conditions: feat(iam): CompositePrincipal and allow multiple principal types #1377
The Condition is in a different scope so I don't understand why this would be not possible.

new iam.CompositePrincipal(new iam.FederatedPrincipal(
        `arn:aws:iam::${props.account.account}:saml-provider/SAMLIDP`, {
          "StringEquals": [{"SAML:aud": "https://signin.aws.amazon.com/saml"}],
        }, "sts:AssumeRoleWithSAML"
      ),
      new iam.ArnPrincipal(`arn:aws:iam::${props.default.rootAccount}:saml-provider/MasterRole`),
      new iam.ArnPrincipal(`arn:aws:iam::${props.default.rootAccount}:saml-provider/AccountOwner`)
)
@rix0rrr rix0rrr added feature-request A feature should be added or improved. @aws-cdk/aws-iam Related to AWS Identity and Access Management gap bug This issue is a bug. and removed feature-request A feature should be added or improved. labels Jan 21, 2019
@rix0rrr rix0rrr changed the title IAM CompositePrincipal with FederatedPrincipal CompositePrincipal does not support conditions Jan 21, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 21, 2019

We need to change CompositePrincipal to emit multiple statements.

@reillykw
Copy link
Contributor

reillykw commented Mar 8, 2019

I'm also experiencing this issue. What I need to be able to do is attach multiple actions to a federated policy document, I was hoping composite would help but it would throw an error with conditions. Also would need to see support for multiple federated ARNs, if that is possible. I don't know if a Composite would achieve that end or not.

@Cushdrive
Copy link

I have the same use case, and I'm also blocked from fully leveraging the CDK because it can't be used in the same way as the CloudFormation templates I'm trying to replace.

@rix0rrr rix0rrr added good first issue Related to contributions. See CONTRIBUTING.md and removed good first issue Related to contributions. See CONTRIBUTING.md labels Aug 19, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 21, 2019

Not so easy to fix, actually. Goes together with #3227 as well.

@NGL321 NGL321 added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed status/needs-response labels Sep 16, 2019
@NGL321 NGL321 removed gap response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Oct 4, 2019
@rix0rrr rix0rrr added the p2 label Oct 24, 2019
@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Aug 12, 2020
@cesarpball
Copy link

Any workaround on this topic? I would to work with Federate access with conditions.

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 18, 2021

Escape hatches would be the standard answer

@rix0rrr rix0rrr removed their assignment Jun 3, 2021
l0b0 added a commit to linz/geostore that referenced this issue Jun 22, 2021
Allowing the S3 Batch Operations service was not necessary after all,
and caused non-prod deployment to hit a known CDK limitation
<aws/aws-cdk#1578>.
kodiakhq bot pushed a commit to linz/geostore that referenced this issue Jun 22, 2021
Allowing the S3 Batch Operations service was not necessary after all,
and caused non-prod deployment to hit a known CDK limitation
<aws/aws-cdk#1578>.
rix0rrr added a commit that referenced this issue Nov 24, 2021
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes #15908, closes #16725, fixes #2041, fixes #1578.
@mergify mergify bot closed this as completed in #17689 Dec 16, 2021
mergify bot pushed a commit that referenced this issue Dec 16, 2021
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes #15908, closes #16725, fixes #2041, fixes #1578.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
To allow session tagging, the `sts:TagSession` permission needs to
be added to the role's AssumeRolePolicyDocument.

Introduce a new principal which enables this, and add a convenience
method `.withSessionTags()` to the `PrincipalBase` class so all
built-in principals will have this convenience method by default.

To build this, we had to get rid of some cruft and assumptions around
policy documents and statements, and defer more power to the
`IPrincipal` objects themselves. In order not to break existing
implementors, introduce a new interface `IAssumeRolePrincipal` which
knows how to add itself to an AssumeRolePolicyDocument and gets complete
freedom doing so.

That same new interface could be used to lift some old limitations on
`CompositePrincipal` so did that as well.

Fixes aws#15908, closes aws#16725, fixes aws#2041, fixes aws#1578.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants