Let user provide full KMS Key ARN and update descriptions #1425
Conversation
…gion keys to be used
Co-authored-by: paul david <423357+toothbrush@users.noreply.github.com>
templates/aws-stack.yml
Outdated
| - !If | ||
| - PipelineSigningKeyIsARN | ||
| - !Ref PipelineSigningKMSKeyId | ||
| - !Sub arn:aws:kms:${AWS::Region}:${AWS::AccountId}:key/${PipelineSigningKMSKeyId} |
There was a problem hiding this comment.
This isn't neccessary as the API call in the Agent can take either KMS Key identifer or ARN. So there is no need to do anything special for the ARN https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/kms#SignInput
And there is a precedent in the API for taking either in the same field, so you can remove the fancy ARN detection logic.
There was a problem hiding this comment.
Hey @wolfeidau, thanks for getting back to us. You are quite right that the agent will have no trouble accepting either the Key ID or full ARN format. We put this fancy conditional here because to specify a KMS key in an IAM policy statement, you must use its key ARN, that's my understanding of the documentation at least.
I'm definitely open to suggestions to simplify this stanza though.
templates/aws-stack.yml
Outdated
| - !If | ||
| - PipelineSigningKeyIsARN | ||
| - !Ref PipelineSigningKMSKeyId | ||
| - !Sub arn:aws:kms:${AWS::Region}:${AWS::AccountId}:key/${PipelineSigningKMSKeyId} |
There was a problem hiding this comment.
We could remove the conditional here. Although given what is described in #1425 (comment), i still believe we'd need the PipelineSigningKeyIsARN check.
Slightly unrelated, but looking at the documentation at https://buildkite.com/docs/agent/v3/elastic-ci-aws/parameters#signed-pipelines-configuration i would suggest clarifying the exact format expected in PipelineSigningKMSKeyId. E.g., do not provide key/abcd-1234 but only abcd-1234 - that kind of thing.
Since the SDK https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/kms#SignInput is fairly lax about what it accepts, we needn't worry about hand-crafting a full ARN of the KMS Key.
|
@toothbrush this looks great, I will wait for this build to go green and then merge it. I will work with the team to update the documentation as you suggested as well. Thanks for bearing with us. |
wolfeidau
changed the title
|
@toothbrush this change can be tested using refs/pull/1425/head/aws-stack.yml if you were interested in sanity checking. |
Description
(this is yet another approach to #1422 and #1424 - pick the one you prefer 😅)
We have a use case where we want to have a single KMS key that Buildkite agents in multiple accounts can use to sign and verify pipeline steps. By letting the user specify the whole key ARN rather than just the key ID, the user is free to use a key wherever they want.
This third attempt has a different set of tradeoffs to the previous two: