fix(pipelines): cross-region/cross-account key permissions are wrong #10222

rix0rrr · 2020-09-07T14:07:31Z

In #8280 we made a resource's account/region distinct
from the stack in which the construct was defined, to account for
accounts and regions from imported resources.

The pipelines module used to define imported roles in a separate
in-memory Stack so that the old, broken "cross-environment" logic
would do the right thing. That crutch was removed as part of #8280.

The new logic hasn't been carried through everywhere though. For
example, the logic in the grants of KMS keys had not been updated
to match, leading to cross-account/cross-region deployments
being broken (as reported in #10166) because the cross-region support
stack's KMS key had the wrong permissions.

In fact, it switched from:

{
    "Action": [ "kms:Decrypt", "kms:DescribeKey" ],
    "Principal": {
        "AWS": {
            "Fn::Sub": "arn:${AWS::Partition}:iam::561462023695:role/cdk-hnb659fds-deploy-role-561462023695-us-east-2"
        }
    },
    "Resource": "*"
}

to

{
    "Action": [ "kms:Decrypt", "kms:DescribeKey" ],
    "Principal": {
        "AWS": {
            "Fn::Join": [ "", [
                "arn:", { "Ref": "AWS::Partition" }, ":iam::355421412380:root"
            ] ]
        }
    },
    "Resource": "*"
}

Ignoring the switch from Fn::Sub to Fn::Join, it switched from the
deploy-role in a DIFFERENT account to the root principal of the SAME
account.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

In #8280 we made a resource's account/region distinct from the stack in which the construct was defined, to account for accounts and regions from imported resources. The pipelines module used to define imported roles in a separate in-memory Stack so that the old, broken "cross-environment" logic would do the right thing. That crutch was removed as part of #8280. The new logic hasn't been carried through everywhere though. For example, the logic in the grants of KMS keys had not been updated to match, leading to cross-account/cross-region deployments being broken (as reported in #10166) because the cross-region support stack's KMS key had the wrong permissions. In fact, it switched from: ``` { "Action": [ "kms:Decrypt", "kms:DescribeKey" ], "Principal": { "AWS": { "Fn::Sub": "arn:${AWS::Partition}:iam::561462023695:role/cdk-hnb659fds-deploy-role-561462023695-us-east-2" } }, "Resource": "*" } ``` to ``` { "Action": [ "kms:Decrypt", "kms:DescribeKey" ], "Principal": { "AWS": { "Fn::Join": [ "", [ "arn:", { "Ref": "AWS::Partition" }, ":iam::355421412380:root" ] ] } }, "Resource": "*" } ``` Ignoring the switch from `Fn::Sub` to `Fn::Join`, it switched from the `deploy-role` in a DIFFERENT account to the root principal of the SAME account.

rix0rrr · 2020-09-07T14:10:30Z

This issue is not fixed yet! I added a test that shows the brokenness (which should have been there before but wasn't :( ), but I haven't been able to trace the problem to its source yet.

It seems like it might have something to do with the SingletonPolicy being used here:

aws-cdk/packages/@aws-cdk/aws-codepipeline-actions/lib/cloudformation/pipeline-actions.ts

Lines 87 to 92 in 118dbfb

    
           const singletonPolicy = SingletonPolicy.forRole(options.role); 
        
           if ((this.actionProperties.outputs || []).length > 0) { 
        
             options.bucket.grantReadWrite(singletonPolicy); 
        
           } else if ((this.actionProperties.inputs || []).length > 0) { 
        
             options.bucket.grantRead(singletonPolicy);

Or with the logic in key.grant() that I already touched in this PR because it didn't take into account the principal's ACTUAL account.

Also principals don't really have a region, so I could refactor the one method but not the other :/

aws-cdk-automation · 2020-09-07T14:14:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 118dbfb
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

skinny85 · 2020-09-08T23:24:17Z

@rix0rrr I've worked on this problem today, and it's quite a hairy one. I believe the issue is in the granteeStackDependsOnKeyStack method.

Basically, the deploy Role is imported into the Pipeline Stack scope, while the replication Key is in a support Stack. Of course, the Pipeline Stack depends on the replication Stack. So, that method triggers, changing the principal from the Role ARN to the account's root. However, this is incorrect of course, because in this case, the grantee is an imported Role, not a new one, so there's no need to trigger this logic (the principal, the bootstrap deploy role, will exist when the Key policy is updated).

I was aware of that problem before, but it seems like we have reached critical mass, and need to solve it now. The question is how? Do we need a concept of an "imported" Resource, that we can check for in granteeStackDependsOnKeyStack()? I'm open to suggestions.

rix0rrr · 2020-09-09T09:05:01Z

Sooo... hmm. I don't think we need a generic concept of imported resources. It's only really about whether the principal is imported or not.

Ultimately it's about the principal of the grantee (not the grantee itself). If principal had a definingResource?: Resource, I guess we could then use granteeStackDependsOnKeyStack on the stack of THAT (if it exists at all, which it won't for imported principals or ARN/Service principals).

A cheaper, hackier way is to make something like isOwnedIamIdentity(principal) which checks if the principal is a User, Group or Role, because in those cases they have defining resource and that defining resource is themselves.

skinny85 · 2020-09-09T22:15:53Z

Ultimately it's about the principal of the grantee (not the grantee itself). If principal had a definingResource?: Resource, I guess we could then use granteeStackDependsOnKeyStack on the stack of THAT (if it exists at all, which it won't for imported principals or ARN/Service principals).

Isn't that a very weird API though? Why would imported Roles have definingResource as undefined, when they themselves extend Resource, so you could easily have this there?

And of course, it will actually be this for non-imported Roles.

rix0rrr · 2020-09-10T08:43:25Z

Isn't that a very weird API though? Why would imported Roles have definingResource as undefined, when they themselves extend Resource, so you could easily have this there?

Well... isn't that the point? Because an imported role is not defining the resource?

(The term defining here being the one that Elad favors, substitute creating if "defining" means something else to you--I personally agree with him that defining is a fine term for this and it has precedent in for example C's distinction between declaring and defining)

Do you have a better name or idea? The simpler change is probably type-checking the IPrincipal to see if it's actually an instance of Role, User or Group. That's not an open protocol so it's going to break if people define their own subclasses of IPrincipal, but might be a good enough and low-risk approximation for the time being?

skinny85 · 2020-09-11T00:18:21Z

Moved to #10299 (you can't approve your own PRs @rix0rrr ).

rix0rrr · 2020-09-11T08:18:37Z

I have shipped that other one but I can't help but think there's more left to do.

While investigating this I saw that much of the code of KMS still uses Stack.of(principal).account instead of prinicpal.principalAccount to get the account that the principal represents, which is wrong in the fact of imported principals.

Don't you think we should fix those up while we're at it?

skinny85 · 2020-09-11T21:03:07Z

I have shipped that other one but I can't help but think there's more left to do.

While investigating this I saw that much of the code of KMS still uses Stack.of(principal).account instead of prinicpal.principalAccount to get the account that the principal represents, which is wrong in the fact of imported principals.

Don't you think we should fix those up while we're at it?

Sure. Do you know of the specific cases when the behavior is incorrect?

rix0rrr · 2020-09-14T11:44:17Z

Sure. Do you know of the specific cases when the behavior is incorrect?

No, but a grep should cover a bunch?

skinny85 · 2020-09-14T19:00:00Z

Sure. Do you know of the specific cases when the behavior is incorrect?

No, but a grep should cover a bunch?

Sorry, I don't follow. A grep of what?

rix0rrr requested a review from skinny85 September 7, 2020 14:07

rix0rrr assigned skinny85 and rix0rrr Sep 7, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 7, 2020

skinny85 mentioned this pull request Sep 11, 2020

fix(kms): do not change the principal to root for imported resources in dependent Stacks #10299

Merged

rix0rrr closed this Sep 17, 2020

rix0rrr deleted the huijbers/x-env-key-permissions-broken branch October 23, 2020 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pipelines): cross-region/cross-account key permissions are wrong #10222

fix(pipelines): cross-region/cross-account key permissions are wrong #10222

rix0rrr commented Sep 7, 2020

rix0rrr commented Sep 7, 2020

aws-cdk-automation commented Sep 7, 2020

skinny85 commented Sep 8, 2020 •

edited

Loading

rix0rrr commented Sep 9, 2020

skinny85 commented Sep 9, 2020 •

edited

Loading

rix0rrr commented Sep 10, 2020 •

edited

Loading

skinny85 commented Sep 11, 2020

rix0rrr commented Sep 11, 2020

skinny85 commented Sep 11, 2020

rix0rrr commented Sep 14, 2020

skinny85 commented Sep 14, 2020

fix(pipelines): cross-region/cross-account key permissions are wrong #10222

fix(pipelines): cross-region/cross-account key permissions are wrong #10222

Conversation

rix0rrr commented Sep 7, 2020

rix0rrr commented Sep 7, 2020

aws-cdk-automation commented Sep 7, 2020

AWS CodeBuild CI Report

skinny85 commented Sep 8, 2020 • edited Loading

rix0rrr commented Sep 9, 2020

skinny85 commented Sep 9, 2020 • edited Loading

rix0rrr commented Sep 10, 2020 • edited Loading

skinny85 commented Sep 11, 2020

rix0rrr commented Sep 11, 2020

skinny85 commented Sep 11, 2020

rix0rrr commented Sep 14, 2020

skinny85 commented Sep 14, 2020

skinny85 commented Sep 8, 2020 •

edited

Loading

skinny85 commented Sep 9, 2020 •

edited

Loading

rix0rrr commented Sep 10, 2020 •

edited

Loading