(CodeBuild): Add KMS decrypt to policy for secrets from another account

[Kruspe]

#13706 added support for reading secrets from another account through specifying the secrets ARN. The KMS key would also live in the different account and it would be awesome if we could add the kms:Decrypt action to the policy. I am not sure if it would be necessary to check which kms key the secret uses or if the wildcard would suffice here. Meaning would this ressource suffice arn:<SecretPartition>:kms:<SecretRegion>:<SecretAccount>:key/*?

Use Case

When using a secret from another account one get's the error message that Access to KMS is not allowed.
Makes secrets from another account available within CodeBuild without having to modify the policy.

Proposed Solution

When granting the principal for secretsmanager:GetSecretValue we should also add the kms:Decrypt principal.

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

---

This is a 🚀 Feature Request

[skinny85]

Hi @Kruspe ,

thanks for opening the issue. This should actually work already! Can you give an example where passing a Key from a different account to a Project is not working?

Thanks,
Adam

[Kruspe]

Hey @skinny85,

sure we use the the CDKPipeline and for the synthAction we use SimpleSynthAction.standardYarnSynth action with the environment property like this

environmentVariables: {
  SECRET: {
    value: <SECRETS_ARN_FROM_OTHER_ACCOUNT>,
    type: BuildEnvironmentVariableType.SECRETS_MANAGER,
  }
}

This creates a policy which now correctly grants access for GetSecretValue like this:

{
"Action": "secretsmanager:GetSecretValue",
"Resource": [
  "arn:aws:secretsmanager:<REGION>:<OTHER_ACCOUNT>:secret:<SECRETS_ARN>"
],
"Effect": "Allow"
}

it also creates a kms policy like this one:

{
"Action": [
  "kms:Decrypt",
  "kms:DescribeKey",
  "kms:Encrypt",
  "kms:ReEncrypt*",
  "kms:GenerateDataKey*"
],
"Resource": "arn:aws:kms:<REGION>:<ACCOUNT>:key/<KEY>",
"Effect": "Allow"
}

But it is missing one for the <OTHER_ACCOUNT>

Hope this helps. If not let me know and I can figure out a better example!

[skinny85]

I don't quite understand. Where is the Key in OTHER_ACCOUNT in your example? I don't see it anywhere...

The KMS policy that you've shown I'm 99% sure has nothing to do with setting any environment variables (you can remove the environmentVariables property, and it would stay the same).

[Kruspe]

The KMS policy is not in the example. That's true. Just wanted to show it for completeness sake. Sorry if that was confusing! Maybe just scrape that information it's not relevant to the issue.

The other KMS policy I am talking about is used by SecretsManager. Since the secrets lives in ANOTHER_ACCOUNT we also need to allow the kms:DECRYPT action on their account since it is used to when we try to get the secret.

[skinny85]

The other KMS policy I am talking about is used by SecretsManager. Since the secrets lives in ANOTHER_ACCOUNT we also need to allow the kms:DECRYPT action on their account since it is used to when we try to get the secret.

What does it mean to "allows kms:Decrypt on their account"? Can you point me to some documentation, or an example?

[Kruspe]

No problem. Have a look here: https://docs.aws.amazon.com/kms/latest/developerguide/services-secrets-manager.html#asm-encrypt.
Under "Decrypting a secret value" it is described what happens when you request a secret from SecretsManager.

Since KMS in the OTHER_ACCOUNT handles encryption and decryption for a secret in the OTHER_ACCOUNT a call to their KMS service is needed when someone tries to get the secret. :)

[skinny85]

Right. But those talk about specific Keys that are used to perform the encryption:

To decrypt an encrypted secret value, Secrets Manager must first decrypt the encrypted data key.

I don't see any mention of "allows kms:Decrypt on their account" that you mentioned above.

[Kruspe]

Yeah I see what you mean. Let me get more specific and reference the article which is actually talking about cross account access to SecretsManager.
There you can see in step 3 the exact actions that are needed to access a secret from another account. Does this make more sense now?

[Kruspe]

@skinny85 Any update on this? Can I support you somewhere?

[skinny85]

There you can see in step 3 the exact actions that are needed to access a secret from another account.

This is what step 3 contains:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "AllowGetSecretValue",
            "Effect": "Allow",
            "Action": [
                "secretsmanager:GetSecretValue"
            ],
            "Resource": [
                "arn:aws:secretsmanager:your-region:Security_Account:secret:DevSecret-??????"
            ]
        },
        {
            "Sid": "AllowKMSDecrypt",
            "Effect": "Allow",
            "Action": [
                "kms:Decrypt"
            ],
            "Resource": [
                "arn:aws:kms:your-region:Security_Account:key/DevSecretCMK_id"
            ]
        }
    ]
}

The permissions are only granted to a single Key, with ARN arn:aws:kms:your-region:Security_Account:key/DevSecretCMK_id.

So, my question is: how is CDK supposed to know that DevSecretCMK_id is the identifier of the KMS Key that encrypts the secret with ARN arn:aws:secretsmanager:<REGION>:<OTHER_ACCOUNT>:secret:<SECRETS_ARN>, from your example?

[Kruspe]

That is a good question. One way would be to just the wildcard like this arn:aws:kms:your-region:Security_Account:key/*. This is not ideal but since there is also policies setup inside the OTHER_ACCOUNT to explicitly allow access to the key it would be fine.
A better option which I am not sure would work for CDK would be to use describe-secret (https://docs.aws.amazon.com/cli/latest/reference/secretsmanager/describe-secret.html). In the outputs you will get the information which KmsKeyId was used.
Third option would be to give the option to pass the KMS key when passing a secret environmentVariable.

What do you think?

[skinny85]

What do you think?

Here are my takes on the various options:

A better option which I am not sure would work for CDK would be to use describe-secret (https://docs.aws.amazon.com/cli/latest/reference/secretsmanager/describe-secret.html). In the outputs you will get the information which KmsKeyId was used.

That's a non-stater (we can't really do these sort of calls, and it would make your infrastructure non-reproducible).

One way would be to just the wildcard like this arn:aws:kms:your-region:Security_Account:key/*. This is not ideal but since there is also policies setup inside the OTHER_ACCOUNT to explicitly allow access to the key it would be fine.

That's not the worst idea! We can check the account in the ARN that we get passed, and if it's different than the account the Project is in, we could grant kms:Decrypt to arn:aws:kms:<secret-region>:<secret-account>:key/*.

I'm a little worried about the wide policy, but it's true that it shouldn't be such a big deal for cross-account access...

Third option would be to give the option to pass the KMS key when passing a secret environmentVariable.

I guess one potential way we could do it is allow passing an object where only an ARN is allowed now:

// today
const project = new codebuild.Project(this, 'Project', {
  // other properties...
  environmentVariables: {
    SECRET: {
      value: 'arn:aws:secretsmanager:<secret-region>:<secret-account>:secret:<secret-name>',
      type: BuildEnvironmentVariableType.SECRETS_MANAGER,
    },
  },
});

// tommorow?
const project = new codebuild.Project(this, 'Project', {
  // other properties...
  environmentVariables: {
    SECRET: {
      value: {
        secret: 'arn:aws:secretsmanager:<secret-region>:<secret-account>:secret:<secret-name>',
        key: 'arn:aws:kms:<secert-region>:<secret-account>:key/<key-id>',
      },
      type: BuildEnvironmentVariableType.SECRETS_MANAGER,
    },
  },
});

But honestly, I don't love this idea. Just too much strings! It'll be easier to just use the current solution, and then go:

const project = new codebuild.Project(this, 'Project', {
  // other properties...
  environmentVariables: {
    SECRET: {
      value: 'arn:aws:secretsmanager:<secret-region>:<secret-account>:secret:<secret-name>',
      type: BuildEnvironmentVariableType.SECRETS_MANAGER,
    },
  },
});
const key = kms.Key.fromKeyArn(this, 'Key',  'arn:aws:kms:<secert-region>:<secret-account>:key/<key-id>');
key.grantDecrypt(project);

---

There is one other potential solution that I can see: we can allow passing a ISecret as the value when type is BuildEnvironmentVariableType.SECRETS_MANAGER. You can import a Secret with a Key by using the Secret.fromSecretAttributes() method. The code inside Project can then check whether value is a Secret, and do the right thing. It would look like this:

const secret = secretsmanager.Secret.fromSecretAttributes(this, 'Secret', {
  secretCompleteArn: 'arn:aws:secretsmanager:<secret-region>:<secret-account>:secret:<secret-name>',
  encryptionKey: 'arn:aws:kms:<secert-region>:<secret-account>:key/<key-id>',
});
const project = new codebuild.Project(this, 'Project', {
  // other properties...
  environmentVariables: {
    SECRET: {
      value: secret,
      type: BuildEnvironmentVariableType.SECRETS_MANAGER,
    },
  },
});

The downside to this solution is that I don't know how you would pass the additional fields that CodeBuild allows for, like arn:aws:secretsmanager:<secret-region>:<secret-account>:secret:<secret-name>:<json-field>...

---

Thoughts on this @Kruspe?

[Kruspe]

Thoughts on this @Kruspe?

Alright here we go. One thing that crossed my mind is that hard coding the KmsKey into the policy is probably not such a good idea. Since it is not under our control and it could get rotated. Then one would have to redeploy in order for the pipeline to work again.

That's not the worst idea! We can check the account in the ARN that we get passed, and if it's different than the account the Project is in, we could grant kms:Decrypt to arn:aws:kms:::key/*.
I'm a little worried about the wide policy, but it's true that it shouldn't be such a big deal for cross-account access...

I had the same worries but right now I think this might be the best way forward. I had the same approach in my mind.

I guess one potential way we could do it is allow passing an object where only an ARN is allowed now:
But honestly, I don't love this idea. Just too much strings! It'll be easier to just use the current solution, and then go:

I also totally agree on this. I don't like the idea and also the key rotation would be an issue here. Maybe just stating the fact of adding the kms:Decrypt policy to the project would also suffice. But solving the problem all together would be the best experience for users I think.

There is one other potential solution that I can see: we can allow passing a ISecret as the value when type is BuildEnvironmentVariableType.SECRETS_MANAGER.

Interesting solution! I quite like it but it comes again with the draw back when you think about Key rotation.

---

So in conclusion I would go with the wild card option personally. Reading the account and region from the secret is easy
and we already parse the arn. So we could just create another array for the kms resources and push the resources there if needed.

kmsIamResources.push(Stack.of(principal).formatArn({
  service: 'kms',
  resource: 'key',
  resourceName: '*',
  sep: ':',
  partition: parsedArn?.partition,
  account: parsedArn?.account,
  region: parsedArn?.region,
}));

And in the end use addToPrincipalPolicy.

If you are happy with that solution I could go ahead and create a PR. :)

[skinny85]

A PR would be awesome, thanks! 😃

[github-actions]

⚠️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.

[peralmq]

Thanks so much for making this fix. I'd appreciate some more documentation though.

We were struggling with triggering the if (parsedArn && parsedArn.account && Token.compareStrings(parsedArn.account, stack.account) === TokenComparison.DIFFERENT) { condition. Even though we provided a verbatim secret ARN from another account as per https://github.com/aws/aws-cdk/pull/14226/files#diff-a8640958f8311388cd004191bfc1b0939145226e363e90ad1a0c45bc6aa30953R1138 no policy allowing kms:Decrypt was added during cdk synth. We did eventually find the solution though. Turns out we weren't explicitly setting the current stack's account, once we did set the account (similar to this https://github.com/aws/aws-cdk/pull/14226/files#diff-a8640958f8311388cd004191bfc1b0939145226e363e90ad1a0c45bc6aa30953R1130) the policy showed up and everything works! 🥳 But.. it took a day of troubleshooting to get there and we'd have appreciated documentation of this.

And (maybe this should be a separate issue) should it really be required to explicitly set the env.account? I guess it has something to do with the timing of when the env.account is derived in comparison to when the code at https://github.com/aws/aws-cdk/pull/14226/files#diff-e6d435c687eaf68e6d4998f8b9322f8527cf6a2acf15319b3317fe544eed0bdaR806 is evaluated? Could stack.account be derived earlier?