From b3122db231c6e76de8a7b2eb58eb9e2f312aa8b1 Mon Sep 17 00:00:00 2001 From: David Boslee Date: Wed, 16 Oct 2024 11:27:54 -0600 Subject: [PATCH 1/3] kms: allow multi-region kms keys to be enabled --- lib/auth/keystore/aws_kms.go | 27 +++++++++++++++------------ lib/config/configuration.go | 1 + lib/config/fileconf.go | 5 +++++ lib/service/servicecfg/auth.go | 5 +++++ 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/lib/auth/keystore/aws_kms.go b/lib/auth/keystore/aws_kms.go index 3024dad363832..c8bb2efb01a9e 100644 --- a/lib/auth/keystore/aws_kms.go +++ b/lib/auth/keystore/aws_kms.go @@ -57,12 +57,13 @@ const ( ) type awsKMSKeystore struct { - kms kmsClient - clusterName types.ClusterName - awsAccount string - awsRegion string - clock clockwork.Clock - logger *slog.Logger + kms kmsClient + clusterName types.ClusterName + awsAccount string + awsRegion string + multiRegionEnabled bool + clock clockwork.Clock + logger *slog.Logger } func newAWSKMSKeystore(ctx context.Context, cfg *servicecfg.AWSKMSConfig, opts *Options) (*awsKMSKeystore, error) { @@ -99,12 +100,13 @@ func newAWSKMSKeystore(ctx context.Context, cfg *servicecfg.AWSKMSConfig, opts * clock = clockwork.NewRealClock() } return &awsKMSKeystore{ - clusterName: opts.ClusterName, - awsAccount: cfg.AWSAccount, - awsRegion: cfg.AWSRegion, - kms: kmsClient, - clock: clock, - logger: opts.Logger, + clusterName: opts.ClusterName, + awsAccount: cfg.AWSAccount, + awsRegion: cfg.AWSRegion, + multiRegionEnabled: cfg.MultiRegion.Enabled, + kms: kmsClient, + clock: clock, + logger: opts.Logger, }, nil } @@ -138,6 +140,7 @@ func (a *awsKMSKeystore) generateKey(ctx context.Context, algorithm cryptosuites TagValue: aws.String(a.clusterName.GetClusterName()), }, }, + MultiRegion: aws.Bool(a.multiRegionEnabled), }) if err != nil { return nil, nil, trace.Wrap(err) diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 0f23047f64f74..5ef37af409f81 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -1169,6 +1169,7 @@ func applyAWSKMSConfig(kmsConfig *AWSKMS, cfg *servicecfg.Config) error { return trace.BadParameter("must set region in ca_key_params.aws_kms") } cfg.Auth.KeyStore.AWSKMS.AWSRegion = kmsConfig.Region + cfg.Auth.KeyStore.AWSKMS.MultiRegion = kmsConfig.MultiRegion return nil } diff --git a/lib/config/fileconf.go b/lib/config/fileconf.go index 34ea2a60f2607..8a666b8221803 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -919,6 +919,11 @@ type AWSKMS struct { Account string `yaml:"account"` // Region is the AWS region to use. Region string `yaml:"region"` + // MultiRegion contains configuration for multi-region AWS KMS. + MultiRegion struct { + // Enabled configures new keys to be multi-region. + Enabled bool + } `yaml:"multi_region,omitempty"` } // TrustedCluster struct holds configuration values under "trusted_clusters" key diff --git a/lib/service/servicecfg/auth.go b/lib/service/servicecfg/auth.go index 5c56d18b117c1..3663ea25ae0ea 100644 --- a/lib/service/servicecfg/auth.go +++ b/lib/service/servicecfg/auth.go @@ -278,6 +278,11 @@ type AWSKMSConfig struct { AWSAccount string // AWSRegion is the AWS region where the keys will reside. AWSRegion string + // MultiRegion contains configuration for multi-region AWS KMS. + MultiRegion struct { + // Enabled configures new keys to be multi-region. + Enabled bool + } } // CheckAndSetDefaults checks that required parameters of the config are From 7388e192072d34af7f33e94812f6e9f62b12f724 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 17 Oct 2024 16:09:18 -0700 Subject: [PATCH 2/3] add unit tests --- lib/auth/keystore/aws_kms.go | 4 +- lib/auth/keystore/aws_kms_test.go | 58 ++++++++++++++++ lib/auth/keystore/keystore_test.go | 103 +++++++++++++++++------------ 3 files changed, 122 insertions(+), 43 deletions(-) diff --git a/lib/auth/keystore/aws_kms.go b/lib/auth/keystore/aws_kms.go index c8bb2efb01a9e..108ee93d66772 100644 --- a/lib/auth/keystore/aws_kms.go +++ b/lib/auth/keystore/aws_kms.go @@ -128,7 +128,9 @@ func (a *awsKMSKeystore) generateKey(ctx context.Context, algorithm cryptosuites return nil, nil, trace.Wrap(err) } - a.logger.InfoContext(ctx, "Creating new AWS KMS keypair.", "algorithm", alg) + a.logger.InfoContext(ctx, "Creating new AWS KMS keypair.", + slog.Any("algorithm", algorithm), + slog.Bool("multi-region", a.multiRegionEnabled)) output, err := a.kms.CreateKey(ctx, &kms.CreateKeyInput{ Description: aws.String("Teleport CA key"), diff --git a/lib/auth/keystore/aws_kms_test.go b/lib/auth/keystore/aws_kms_test.go index b09a3f5a11c86..dd5940b170979 100644 --- a/lib/auth/keystore/aws_kms_test.go +++ b/lib/auth/keystore/aws_kms_test.go @@ -211,6 +211,60 @@ func TestAWSKMS_RetryWhilePending(t *testing.T) { require.Error(t, err) } +// TestMultiRegionKeys asserts that a keystore created with multi-region enabled +// correctly passes this argument to the AWS client. This gives very little real +// coverage since the AWS KMS service here is faked, but at least we know the +// keystore passed the bool to the client correctly. TestBackends and +// TestManager are both able to run with a real AWS KMS client and you can +// confirm the keys are really multi-region there. +func TestMultiRegionKeys(t *testing.T) { + ctx := context.Background() + clock := clockwork.NewFakeClock() + + const pageSize int = 4 + fakeKMS := newFakeAWSKMSService(t, clock, "123456789012", "us-west-2", pageSize) + clusterName, err := services.NewClusterNameWithRandomID(types.ClusterNameSpecV2{ClusterName: "test-cluster"}) + require.NoError(t, err) + opts := &Options{ + ClusterName: clusterName, + HostUUID: "uuid", + AuthPreferenceGetter: &fakeAuthPreferenceGetter{types.SignatureAlgorithmSuite_SIGNATURE_ALGORITHM_SUITE_HSM_V1}, + awsKMSClient: fakeKMS, + awsSTSClient: &fakeAWSSTSClient{ + account: "123456789012", + }, + clockworkOverride: clock, + } + + for _, multiRegion := range []bool{false, true} { + t.Run(fmt.Sprint(multiRegion), func(t *testing.T) { + cfg := servicecfg.KeystoreConfig{ + AWSKMS: servicecfg.AWSKMSConfig{ + AWSAccount: "123456789012", + AWSRegion: "us-west-2", + MultiRegion: struct{ Enabled bool }{ + Enabled: multiRegion, + }, + }, + } + keyStore, err := NewManager(ctx, &cfg, opts) + require.NoError(t, err) + + sshKeyPair, err := keyStore.NewSSHKeyPair(ctx, cryptosuites.UserCASSH) + require.NoError(t, err) + + keyID, err := parseAWSKMSKeyID(sshKeyPair.PrivateKey) + require.NoError(t, err) + + if multiRegion { + assert.Contains(t, keyID.arn, "mrk-") + } else { + assert.NotContains(t, keyID.arn, "mrk-") + } + }) + } +} + type fakeAWSKMSService struct { keys []*fakeAWSKMSKey clock clockwork.Clock @@ -239,6 +293,10 @@ type fakeAWSKMSKey struct { func (f *fakeAWSKMSService) CreateKey(_ context.Context, input *kms.CreateKeyInput, _ ...func(*kms.Options)) (*kms.CreateKeyOutput, error) { id := uuid.NewString() + if aws.ToBool(input.MultiRegion) { + // AWS does this https://docs.aws.amazon.com/kms/latest/developerguide/concepts.html#key-id-key-ARN + id = "mrk-" + id + } a := arn.ARN{ Partition: "aws", Service: "kms", diff --git a/lib/auth/keystore/keystore_test.go b/lib/auth/keystore/keystore_test.go index 0b9e880677762..3780c73296157 100644 --- a/lib/auth/keystore/keystore_test.go +++ b/lib/auth/keystore/keystore_test.go @@ -604,6 +604,7 @@ func newTestPack(ctx context.Context, t *testing.T) *testPack { }) } + // Test with real GCP account if environment is enabled. if config, ok := gcpKMSTestConfig(t); ok { opts := baseOpts opts.kmsClient = nil @@ -621,6 +622,7 @@ func newTestPack(ctx context.Context, t *testing.T) *testPack { }.marshal(), }) } + // Always test with fake GCP client. fakeGCPKMSConfig := servicecfg.KeystoreConfig{ GCPKMS: servicecfg.GCPKMSConfig{ ProtectionLevel: "HSM", @@ -640,61 +642,78 @@ func newTestPack(ctx context.Context, t *testing.T) *testPack { }.marshal(), }) - if config, ok := awsKMSTestConfig(t); ok { - opts := baseOpts - // Unset the fake clients so this test can use the real AWS clients. - opts.awsKMSClient = nil - opts.awsSTSClient = nil - - backend, err := newAWSKMSKeystore(ctx, &config.AWSKMS, &opts) + // Test AWS with and without multi-region keys + for _, multiRegion := range []bool{false, true} { + // Test with real AWS account if environment is enabled. + if config, ok := awsKMSTestConfig(t); ok { + config.AWSKMS.MultiRegion.Enabled = multiRegion + opts := baseOpts + // Unset the fake clients so this test can use the real AWS clients. + opts.awsKMSClient = nil + opts.awsSTSClient = nil + + backend, err := newAWSKMSKeystore(ctx, &config.AWSKMS, &opts) + require.NoError(t, err) + name := "aws_kms" + if multiRegion { + name += "_multi_region" + } + backends = append(backends, &backendDesc{ + name: name, + config: config, + opts: &opts, + backend: backend, + expectedKeyType: types.PrivateKeyType_AWS_KMS, + unusedRawKey: awsKMSKeyID{ + arn: arn.ARN{ + Partition: "aws", + Service: "kms", + Region: config.AWSKMS.AWSRegion, + AccountID: config.AWSKMS.AWSAccount, + Resource: "unused", + }.String(), + account: config.AWSKMS.AWSAccount, + region: config.AWSKMS.AWSRegion, + }.marshal(), + }) + } + + // Always test with fake AWS client. + fakeAWSKMSConfig := servicecfg.KeystoreConfig{ + AWSKMS: servicecfg.AWSKMSConfig{ + AWSAccount: "123456789012", + AWSRegion: "us-west-2", + MultiRegion: struct{ Enabled bool }{ + Enabled: multiRegion, + }, + }, + } + fakeAWSKMSBackend, err := newAWSKMSKeystore(ctx, &fakeAWSKMSConfig.AWSKMS, &baseOpts) require.NoError(t, err) + name := "fake_aws_kms" + if multiRegion { + name += "_multi_region" + } backends = append(backends, &backendDesc{ - name: "aws_kms", - config: config, - opts: &opts, - backend: backend, + name: name, + config: fakeAWSKMSConfig, + opts: &baseOpts, + backend: fakeAWSKMSBackend, expectedKeyType: types.PrivateKeyType_AWS_KMS, unusedRawKey: awsKMSKeyID{ arn: arn.ARN{ Partition: "aws", Service: "kms", - Region: config.AWSKMS.AWSRegion, - AccountID: config.AWSKMS.AWSAccount, + Region: "us-west-2", + AccountID: "123456789012", Resource: "unused", }.String(), - account: config.AWSKMS.AWSAccount, - region: config.AWSKMS.AWSRegion, + account: "123456789012", + region: "us-west-2", }.marshal(), }) } - fakeAWSKMSConfig := servicecfg.KeystoreConfig{ - AWSKMS: servicecfg.AWSKMSConfig{ - AWSAccount: "123456789012", - AWSRegion: "us-west-2", - }, - } - fakeAWSKMSBackend, err := newAWSKMSKeystore(ctx, &fakeAWSKMSConfig.AWSKMS, &baseOpts) - require.NoError(t, err) - backends = append(backends, &backendDesc{ - name: "fake_aws_kms", - config: fakeAWSKMSConfig, - opts: &baseOpts, - backend: fakeAWSKMSBackend, - expectedKeyType: types.PrivateKeyType_AWS_KMS, - unusedRawKey: awsKMSKeyID{ - arn: arn.ARN{ - Partition: "aws", - Service: "kms", - Region: "us-west-2", - AccountID: "123456789012", - Resource: "unused", - }.String(), - account: "123456789012", - region: "us-west-2", - }.marshal(), - }) - return &testPack{ backends: backends, clock: clock, From 0c7840e8d548d48ec3f92cee43968efe78d38f44 Mon Sep 17 00:00:00 2001 From: David Boslee Date: Mon, 28 Oct 2024 11:15:30 -0400 Subject: [PATCH 3/3] fix logging key lint --- lib/auth/keystore/aws_kms.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/auth/keystore/aws_kms.go b/lib/auth/keystore/aws_kms.go index 108ee93d66772..fc27478c3e031 100644 --- a/lib/auth/keystore/aws_kms.go +++ b/lib/auth/keystore/aws_kms.go @@ -130,7 +130,7 @@ func (a *awsKMSKeystore) generateKey(ctx context.Context, algorithm cryptosuites a.logger.InfoContext(ctx, "Creating new AWS KMS keypair.", slog.Any("algorithm", algorithm), - slog.Bool("multi-region", a.multiRegionEnabled)) + slog.Bool("multi_region", a.multiRegionEnabled)) output, err := a.kms.CreateKey(ctx, &kms.CreateKeyInput{ Description: aws.String("Teleport CA key"),