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

[IAM] Ability to add conditions to AccountPrincipal #5855

Closed
2 tasks
robertd opened this issue Jan 18, 2020 · 5 comments · Fixed by #7015
Closed
2 tasks

[IAM] Ability to add conditions to AccountPrincipal #5855

robertd opened this issue Jan 18, 2020 · 5 comments · Fixed by #7015
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.

Comments

@robertd
Copy link
Contributor

robertd commented Jan 18, 2020

It'd be great if we can add conditions to AccountPrincipal when creating iam.Role. Currently, AccountPrincipal only accepts accountId param. We're creating an iam role for 3rd party SaaS and we need to provide a Condition property.

Other

Current workaround is using addPropertyOverride.

const role = new iam.Role(this, "role", {
  assumedBy: new iam.AccountPrincipal('1234567890')
});

let roleRef = role.node.defaultChild as iam.CfnRole;
roleRef.addPropertyOverride("AssumeRolePolicyDocument.Statement.0.Condition.StringEquals.foo”,"baz");
Resources:
  Role1EF426C6:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRole
            Effect: Allow
            Principal:
              AWS:
                Fn::Join:
                  - ""
                  - - "arn:"
                    - Ref: AWS::Partition
                    - :iam::1234567890:root
            Condition:
              StringEquals:
                foo: baz
        Version: "2012-10-17"
  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@robertd robertd added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 18, 2020
@robertd robertd changed the title Ability to add conditions to AccountPrincipal [IAM] Ability to add conditions to AccountPrincipal Jan 18, 2020
@SomayaB SomayaB added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Jan 21, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 23, 2020

Probably a wrapper principal that adds conditions will be more reusable.

@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md labels Jan 23, 2020
@mrpackethead
Copy link

Thanks Robertd, i'm in teh same situation. ANd this will be very useful.

@nija-at
Copy link
Contributor

nija-at commented Feb 19, 2020

Our current modeling of conditions on ServicePrincipal is not the right model either. If I recall correctly, adding a conditional to a ServicePrincipal looks like this -

const role = new iam.Role(this, "role", {
  assumedBy: new iam.ServicePrincipal('service-principal.amazonaws.com', {
    conditions: {
      "string:equals": {
        "sts:ExternalId": "<sts-external-id>"
      }
    }
  })
});

I propose we switch the conditional for AccountPrincipal to something like -

conditions: {
  "sts:ExternalId": {
    StringCondition.Equals: "<sts-external-id>"
  }
}

where StringCondition is an enum and has the values listed here.

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 19, 2020

I see how this is more logical, but it deviates from what people are used to in IAM so I'm not use that'd be a great idea.

@nija-at
Copy link
Contributor

nija-at commented Feb 19, 2020

Ok - I originally assumed that there was support for repeated use of the same operator - such as multiple 'StringEquals' - in the conditional. Upon trying this out, it seems I'm mistaken and there can only be one instance of a conditional per policy. I'm ok leaving the structure as is.

However, can we still model the conditional more strongly so I don't have to look at the IAM documentation to know the list of conditionals available?

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Mar 4, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Mar 28, 2020
@mergify mergify bot closed this as completed in #7015 Apr 6, 2020
mergify bot pushed a commit that referenced this issue Apr 6, 2020
Closes #5855 

Adds a `PrincipalWithConditions` wrapper that allows conditions to be added to
any principal.

Behaviour is consistent with the way that `PolicyStatement.addCondition`
and `.addConditions` currently work - most notably in that adding an operator
that is already present will merge their objects, but adding a condition to
an operator/key combination that is already set will overwrite the existing
value (rather than merge the values into an array).

BREAKING CHANGES: every place an IAM Condition was expected it is now typed as `{[key: string]: any}`, instead of plain `any`. You were always supposed to pass a map/dictionary in these locations, but the type system didn't enforce it. It now does. This will not impact correct programs, but may cause compiler errors in programs that were incorrect.
horsmand pushed a commit to horsmand/aws-cdk that referenced this issue Apr 8, 2020
Closes aws#5855 

Adds a `PrincipalWithConditions` wrapper that allows conditions to be added to
any principal.

Behaviour is consistent with the way that `PolicyStatement.addCondition`
and `.addConditions` currently work - most notably in that adding an operator
that is already present will merge their objects, but adding a condition to
an operator/key combination that is already set will overwrite the existing
value (rather than merge the values into an array).

BREAKING CHANGES: every place an IAM Condition was expected it is now typed as `{[key: string]: any}`, instead of plain `any`. You were always supposed to pass a map/dictionary in these locations, but the type system didn't enforce it. It now does. This will not impact correct programs, but may cause compiler errors in programs that were incorrect.
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 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants