Skip to content
New issue

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

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

Already on GitHub? # to your account

iam: SAML principal AssumeRoleAction is hardcoded #29456

Closed
justin8 opened this issue Mar 12, 2024 · 8 comments
Closed

iam: SAML principal AssumeRoleAction is hardcoded #29456

justin8 opened this issue Mar 12, 2024 · 8 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@justin8
Copy link
Contributor

justin8 commented Mar 12, 2024

Describe the bug

Currently when defining an assume role policy on a role the action is hardcoded to sts:AssumeRoleWithSAML' [here](https://github.com/aws/aws-cdk/blob/v2.132.0/packages/aws-cdk-lib/aws-iam/lib/principals.ts#L746). However for some use cases, such as Quicksight's own SAML integration, you also need sts:TagSession` in order to use the email sync feature.

Expected Behavior

You should be able to modify the assume role action and just have a default, not a hardcoded action.

Current Behavior

It's hardcoded and requires the use of escape hatches such as:

    const cfnFederationRole = federationRole.node.defaultChild as iam.CfnRole;
    cfnFederationRole.addOverride('Properties.AssumeRolePolicyDocument.Statement.0.Action', [
      'stsAssumeRoleWithSAML',
      'sts:TagSession',
    ]);

Reproduction Steps

    const federationRolePrincipal = new iam.SamlPrincipal(samlProvider, {
      StringEquals: { 'SAML:aud': 'https://signin.aws.amazon.com/saml' },
    });

    const federationRole = new iam.Role(this, 'QuicksightFederationRole', {
      roleName: 'QuicksightFederationRole',
      assumedBy: federationRolePrincipal,
    });

    const cfnFederationRole = federationRole.node.defaultChild as iam.CfnRole;
    cfnFederationRole.addOverride('Properties.AssumeRolePolicyDocument.Statement.0.Action', [
      'stsAssumeRoleWithSAML',
      'sts:TagSession',
    ]);

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.126.0

Framework Version

No response

Node.js Version

18

OS

Linux

Language

TypeScript

Language Version

No response

Other information

No response

@justin8 justin8 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 12, 2024
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Mar 12, 2024
@pahud
Copy link
Contributor

pahud commented Mar 12, 2024

However for some use cases, such as Quicksight's own SAML integration, you also need sts:TagSession` in order to use the email sync feature.

Do you have any public document link about this?

We probably need to update here

/**
* Principal entity that represents a SAML federated identity provider
*/
export class SamlPrincipal extends FederatedPrincipal {
constructor(samlProvider: ISamlProvider, conditions: Conditions) {
super(samlProvider.samlProviderArn, conditions, 'sts:AssumeRoleWithSAML');
}
public toString() {
return `SamlPrincipal(${this.federated})`;
}
}

@pahud pahud added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 12, 2024
@justin8
Copy link
Contributor Author

justin8 commented Mar 12, 2024

There is an example in the docs here for implementing ABAC using tags and SAML IdP integration: https://docs.aws.amazon.com/IAM/latest/UserGuide/tutorial_abac-saml.html

For Quicksight, it has it in their docs to add sts:TagSession permissions to the SAML integration role as well: https://docs.aws.amazon.com/quicksight/latest/user/jit-email-syncing.html

@msambol
Copy link
Contributor

msambol commented Mar 13, 2024

I'll take this.

@msambol
Copy link
Contributor

msambol commented Mar 13, 2024

@justin8 I think you should be able to use federationRolePrincipal.addToAssumeRolePolicy.....

EDIT: ignore this..not right.

@justin8
Copy link
Contributor Author

justin8 commented Mar 13, 2024

I did the same thing :) I tried that thinking it sounded correct, then ended up reading through the code to see that it wouldn't work

@msambol
Copy link
Contributor

msambol commented Mar 13, 2024

Try adding withSessionTags():

    const federationRolePrincipal = new iam.SamlPrincipal(samlProvider, {
      StringEquals: { 'SAML:aud': 'https://signin.aws.amazon.com/saml' },
    }).withSessionTags();

@justin8
Copy link
Contributor Author

justin8 commented Mar 13, 2024

Ah! Thank you, you're right that does fix the issue, I must've missed that function.

@justin8 justin8 closed this as completed Mar 13, 2024
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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

3 participants