Support AWS KMS for signing and verifying pipelines #2960

wolfeidau · 2024-08-29T04:31:54Z

Description

This adds AWS KMS support to pipeline signing, it is a work in progress and I am pushing this up for some feedback.

Context

To outline how this will look I have put together a list of commands with the new AWS KMS option added.

The updated agent start command with a KMS key alias which is used to sign and verify pipelines.

buildkite-agent start --signing-aws-kms-key alias/ES256test --debug --debug-signing

To upload and sign a pipeline with a specific AWS KMS key specified. Note this can be omitted if your key is configured in the agent.

buildkite-agent pipeline upload --signing-aws-kms-key alias/ES256test --debug-signing --debug .buildkite/pipeline.yaml

The updated sign and upload command stored in buildkite with a KMS key alias.

buildkite-agent tool sign --aws-kms-key alias/ES256test \
    --graphql-token bkua_XXXXX --organization-slug bk-whatever --pipeline-slug build-golang-sample \
    --update --debug --debug-signing

This table provides a mapping for the supported KMS KeySpec to JSON Web Algorithms (JWA).

"alg" Param Value	Digital Signature Algorithm	KMS KeySpec
ES256	ECDSA using P-256 and SHA-256	ECC_NIST_P256
ES384	ECDSA using P-384 and SHA-384	ECC_NIST_P384
ES512	ECDSA using P-521 and SHA-512	ECC_NIST_P521
RS256	RSASSA-PKCS1-v1_5 using SHA-256	RSASSA_PKCS1_V1_5_SHA_256
RS384	RSASSA-PKCS1-v1_5 using SHA-384	RSASSA_PKCS1_V1_5_SHA_384
RS512	RSASSA-PKCS1-v1_5 using SHA-512	RSASSA_PKCS1_V1_5_SHA_512

So to create a KMS key which supports JSON Web Algorithms (JWA) ES256 we run the following command. Note the --key-usage SIGN_VERIFY is required to support signing of pipelines.

aws kms create-key --key-spec ECC_NIST_P256 --key-usage SIGN_VERIFY

Changes

Testing

Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
Code is formatted (with go fmt ./...)

patrobinson

This is pretty good, some small improvements to naming of the flags would be good

clicommand/agent_start.go

patrobinson · 2024-08-30T00:15:54Z

clicommand/pipeline_upload.go

@@ -78,6 +81,7 @@ type PipelineUploadConfig struct {
 	// Used for signing
 	JWKSFile     string `cli:"jwks-file"`
 	JWKSKeyID    string `cli:"jwks-key-id"`
+	JWKSKMSKeyID string `cli:"jwks-kms-key-id"`


Suggested change

JWKSKMSKeyID string `cli:"jwks-kms-key-id"`

SigningAWSKMSKey string `cli:"signing-aws-kms-key"`

Spit balling here

patrobinson · 2024-08-30T00:20:47Z

clicommand/pipeline_upload.go

+
+		switch {
+		case cfg.JWKSKMSKeyID != "":
+			awscfg, err := config.LoadDefaultConfig(


There's a small risk that this could resolve credentials in a different order to the how the rest of the agent does it.
But since this is a new feature, and we need to move to this way soon (when v1 of the SDK gets deprecated), I think this is a good place to start using this approach

patrobinson · 2024-08-30T00:31:49Z

internal/cryptosigner/aws/kms.go

+
+	output, err := sv.client.GetPublicKey(context.Background(), &input)
+	if err != nil {
+		return nil, fmt.Errorf(`failed to get public key from KMS: %w`, err)


Since this error just gets ignored should we log it?

patrobinson · 2024-08-30T00:33:12Z

internal/cryptosigner/aws/kms.go

+// needs is setup with the algorithm name to use (see
+// https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/kms/types#SigningAlgorithmSpec),
+// a key ID to use while the AWS SDK makes network
+// requests.


This is a bit hard to read, but you suggested it's now out of date anyway?

DrJosh9000

Some nits for you

DrJosh9000 · 2024-09-02T03:16:58Z

clicommand/agent_start.go

+			// this is currently loaded here to ensure it is ONLY loaded if the agent is using KMS for signing
+			// this will limit the possible impact of this new SDK on the rest of the agent users
+			awscfg, err := config.LoadDefaultConfig(
+				context.Background(),


There is a ctx in this function, may as well use it

Suggested change

context.Background(),

ctx,

DrJosh9000 · 2024-09-02T03:18:16Z

clicommand/agent_start.go

+		case cfg.VerificationJWKSFile != "":
+			if cfg.VerificationJWKSFile != "" {
+				var err error
+				verificationJWKS, err = parseAndValidateJWKS(ctx, "verification", cfg.VerificationJWKSFile)
+				if err != nil {
+					l.Fatal("Verification JWKS failed validation: %v", err)
+				}


Redundant check is redundant:

Suggested change

case cfg.VerificationJWKSFile != "":

if cfg.VerificationJWKSFile != "" {

var err error

verificationJWKS, err = parseAndValidateJWKS(ctx, "verification", cfg.VerificationJWKSFile)

if err != nil {

l.Fatal("Verification JWKS failed validation: %v", err)

}

case cfg.VerificationJWKSFile != "":

var err error

verificationJWKS, err = parseAndValidateJWKS(ctx, "verification", cfg.VerificationJWKSFile)

if err != nil {

l.Fatal("Verification JWKS failed validation: %v", err)

DrJosh9000 · 2024-09-02T03:19:55Z

clicommand/pipeline_upload.go

+			awscfg, err := config.LoadDefaultConfig(
+				context.Background(),
+			)


Suggested change

awscfg, err := config.LoadDefaultConfig(

context.Background(),

)

awscfg, err := config.LoadDefaultConfig(ctx)

DrJosh9000 · 2024-09-02T03:20:30Z

clicommand/tool_sign.go

+			awscfg, err := config.LoadDefaultConfig(
+				context.Background(),
+			)


Suggested change

awscfg, err := config.LoadDefaultConfig(

context.Background(),

)

awscfg, err := config.LoadDefaultConfig(ctx)

DrJosh9000 · 2024-09-02T03:22:27Z

internal/cryptosigner/aws/kms.go

+	ErrInvalidKeyID        = fmt.Errorf(`invalid key ID`)
+)
+
+type KMS struct {


Suggested change

type KMS struct {

// KMS is a crypto.Signer that uses an AWS KMS key for signing.

type KMS struct {

... or similar.

DrJosh9000 · 2024-09-02T03:25:44Z

internal/cryptosigner/aws/kms.go

+
+	keyDesc, err := client.GetPublicKey(context.Background(), &kms.GetPublicKeyInput{KeyId: aws.String(kmsKeyID)})
+	if err != nil {
+		return nil, fmt.Errorf(`failed to describe key %q: %w`, kmsKeyID, err)


We began preferring "-strings where possible instead of backtick-strings for most uses a while ago. If nothing else, this file is inconsistent with the rest of the PR.

Suggested change

return nil, fmt.Errorf(`failed to describe key %q: %w`, kmsKeyID, err)

return nil, fmt.Errorf("failed to describe key %q: %w", kmsKeyID, err)

DrJosh9000 · 2024-09-02T03:27:50Z

internal/cryptosigner/aws/kms.go

+	return pubkey
+}
+
+// This method is an escape hatch for those cases where the user needs


Suggested change

// This method is an escape hatch for those cases where the user needs

// GetPublicKey is an escape hatch for those cases where the user needs

DrJosh9000 · 2024-09-02T03:30:01Z

internal/cryptosigner/aws/kms.go

+	return key, nil
+}
+
+// jwa.ES256


Out of date?

Suggested change

// jwa.ES256

// Algorithm returns the equivalent of the KMS key's signing algorithm as a JWA key algorithm.

DrJosh9000

Nice stuff 👍

wolfeidau · 2024-09-02T05:14:51Z

Some nits for you

Amazing, I have been through and addressed all your feedback 🙇🏻

wolfeidau changed the title ~~feat kms signing and verifying~~ Support AWS KMS for signing and verifying pipelines Aug 29, 2024

patrobinson reviewed Aug 30, 2024

View reviewed changes

wolfeidau added 7 commits August 30, 2024 14:41

WIP signing and verifying using KMS

1b79aeb

added some output for key info

d6b1752

Tidy up the logic which prints the public key hash

a53fdaa

Move the code for awssigner into the project

34392d7

refactor the kms support to auto select algorithym

f75e0c3

Add support for KMS signing in the pipeline upload sub command

8f736eb

Addressed feedback on comments and naming of flags

31c09ee

wolfeidau force-pushed the feat_kms_signing_and_verifying branch from ea8b60e to 31c09ee Compare August 30, 2024 04:59

wolfeidau added 3 commits August 30, 2024 16:35

Adjusted flag names based on feedback and testing

abad0d0

Add support for a range of JWA algorythms

ce9aa9a

Add new go-pipeline release with support for crypto.Signer

c97f8a6

wolfeidau force-pushed the feat_kms_signing_and_verifying branch from d57cc5c to c97f8a6 Compare September 2, 2024 01:24

Added more internal documentation for supported algorithms

757786a

wolfeidau marked this pull request as ready for review September 2, 2024 02:32

wolfeidau requested a review from DrJosh9000 September 2, 2024 02:32

DrJosh9000 reviewed Sep 2, 2024

View reviewed changes

DrJosh9000 approved these changes Sep 2, 2024

View reviewed changes

Fixes for feedback from review

059b464

wolfeidau force-pushed the feat_kms_signing_and_verifying branch from 640a107 to 059b464 Compare September 2, 2024 05:13

wolfeidau merged commit b10f0c3 into main Sep 2, 2024
1 check passed

wolfeidau deleted the feat_kms_signing_and_verifying branch September 2, 2024 06:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support AWS KMS for signing and verifying pipelines #2960

Support AWS KMS for signing and verifying pipelines #2960

wolfeidau commented Aug 29, 2024 •

edited

Loading

patrobinson left a comment

patrobinson Aug 30, 2024

patrobinson Aug 30, 2024

patrobinson Aug 30, 2024

patrobinson Aug 30, 2024

DrJosh9000 left a comment

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 Sep 2, 2024

DrJosh9000 left a comment

wolfeidau commented Sep 2, 2024

	JWKSKMSKeyID string `cli:"jwks-kms-key-id"`
	SigningAWSKMSKey string `cli:"signing-aws-kms-key"`

	type KMS struct {
	// KMS is a crypto.Signer that uses an AWS KMS key for signing.
	type KMS struct {

	return nil, fmt.Errorf(`failed to describe key %q: %w`, kmsKeyID, err)
	return nil, fmt.Errorf("failed to describe key %q: %w", kmsKeyID, err)

	// This method is an escape hatch for those cases where the user needs
	// GetPublicKey is an escape hatch for those cases where the user needs

	// jwa.ES256
	// Algorithm returns the equivalent of the KMS key's signing algorithm as a JWA key algorithm.

Support AWS KMS for signing and verifying pipelines #2960

Support AWS KMS for signing and verifying pipelines #2960

Conversation

wolfeidau commented Aug 29, 2024 • edited Loading

Description

Context

Changes

Testing

patrobinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrJosh9000 left a comment

Choose a reason for hiding this comment

wolfeidau commented Sep 2, 2024

wolfeidau commented Aug 29, 2024 •

edited

Loading