Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion pkg/server/plugin/keymanager/awskms/awskms.go
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,29 @@ func (p *Plugin) refreshAliases(ctx context.Context) error {
defer p.mu.RUnlock()
var errs []string
for _, entry := range p.entries {
_, err := p.kmsClient.UpdateAlias(ctx, &kms.UpdateAliasInput{
// Resolve the alias's current target to avoid reverting a rotation
// performed by another replica sharing the same key_identifier_value.
describeResp, err := p.kmsClient.DescribeKey(ctx, &kms.DescribeKeyInput{
KeyId: aws.String(entry.AliasName),
})
if err != nil {
p.log.Error("Failed to describe key for alias refresh", aliasNameTag, entry.AliasName, reasonTag, err)
errs = append(errs, err.Error())
continue
}
if describeResp == nil || describeResp.KeyMetadata == nil || describeResp.KeyMetadata.Arn == nil {
p.log.Error("Malformed describe key response during alias refresh", aliasNameTag, entry.AliasName)
continue
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this path should also append to errs, similar to the DescribeKey error path.

}

currentArn := *describeResp.KeyMetadata.Arn
if currentArn != entry.Arn {
p.log.Warn("Alias target differs from cache, skipping refresh to avoid reverting a rotation",
aliasNameTag, entry.AliasName, "cached_arn", entry.Arn, "current_arn", currentArn)
continue
}

_, err = p.kmsClient.UpdateAlias(ctx, &kms.UpdateAliasInput{
AliasName: &entry.AliasName,
TargetKeyId: &entry.Arn,
})
Expand Down
94 changes: 94 additions & 0 deletions pkg/server/plugin/keymanager/awskms/awskms_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1396,6 +1396,100 @@ func TestRefreshAliases(t *testing.T) {
}
}

func TestRefreshAliasesSkipsRotatedAlias(t *testing.T) {
// This test verifies that refreshAliasesTask does not revert an alias
// that was rotated by another replica (e.g. the leader). After configure,
// we simulate a rotation by pointing the alias to a new key in the fake
// store. The refresh should detect the mismatch via DescribeKey and skip
// the UpdateAlias call, leaving the alias pointing to the new key.
ts := setupTest(t)

fakeEntries := []fakeKeyEntry{
{
AliasName: aws.String("alias/SPIRE_SERVER/test_example_org/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/id_01"),
KeyID: aws.String("key_id_01"),
KeySpec: types.KeySpecEccNistP256,
Enabled: true,
PublicKey: []byte("foo"),
CreationDate: &unixEpoch,
AliasLastUpdatedDate: &unixEpoch,
},
}
ts.fakeKMSClient.setEntries(fakeEntries)

refreshAliasesSignal := make(chan error)
ts.plugin.hooks.refreshAliasesSignal = refreshAliasesSignal

_, err := ts.plugin.Configure(ctx, configureRequestWithDefaults(t))
require.NoError(t, err)

// Wait for refresh alias task to be initialized
_ = waitForSignal(t, refreshAliasesSignal)

// Simulate a rotation by another replica: create a new key and point
// the alias to it. The plugin's in-memory cache still has key_id_01.
rotatedEntry := fakeKeyEntry{
KeyID: aws.String("key_id_rotated"),
KeySpec: types.KeySpecEccNistP256,
Enabled: true,
PublicKey: []byte("bar"),
CreationDate: &unixEpoch,
AliasLastUpdatedDate: &unixEpoch,
}
ts.fakeKMSClient.store.SaveKeyEntry(&rotatedEntry)
err = ts.fakeKMSClient.store.SaveAlias(*rotatedEntry.KeyID, "alias/SPIRE_SERVER/test_example_org/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/id_01")
require.NoError(t, err)

// Record the alias state before refresh
aliasBefore := ts.fakeKMSClient.store.aliases["alias/SPIRE_SERVER/test_example_org/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/id_01"]
require.Equal(t, "key_id_rotated", *aliasBefore.KeyEntry.KeyID)

// Trigger refreshAliasesTask
ts.clockHook.Add(6 * time.Hour)
err = waitForSignal(t, refreshAliasesSignal)
require.NoError(t, err)

// Assert: the alias must still point to the rotated key, not reverted
aliasAfter := ts.fakeKMSClient.store.aliases["alias/SPIRE_SERVER/test_example_org/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/id_01"]
require.Equal(t, "key_id_rotated", *aliasAfter.KeyEntry.KeyID, "alias should not have been reverted to the stale cached key")
}

func TestRefreshAliasesDescribeKeyError(t *testing.T) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also add a test for the malformed response branch (where DescribeKey succeeds but returns nil KeyMetadata or nil Arn)?

ts := setupTest(t)

fakeEntries := []fakeKeyEntry{
{
AliasName: aws.String("alias/SPIRE_SERVER/test_example_org/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/id_01"),
KeyID: aws.String("key_id_01"),
KeySpec: types.KeySpecEccNistP256,
Enabled: true,
PublicKey: []byte("foo"),
CreationDate: &unixEpoch,
AliasLastUpdatedDate: &unixEpoch,
},
}
ts.fakeKMSClient.setEntries(fakeEntries)

refreshAliasesSignal := make(chan error)
ts.plugin.hooks.refreshAliasesSignal = refreshAliasesSignal

_, err := ts.plugin.Configure(ctx, configureRequestWithDefaults(t))
require.NoError(t, err)

// Wait for refresh alias task to be initialized
_ = waitForSignal(t, refreshAliasesSignal)

// Inject a DescribeKey error after configure
ts.fakeKMSClient.setDescribeKeyErr("describe key failure")

// Trigger refreshAliasesTask
ts.clockHook.Add(6 * time.Hour)
err = waitForSignal(t, refreshAliasesSignal)

require.NotNil(t, err)
require.Equal(t, "describe key failure", err.Error())
}

func TestDisposeAliases(t *testing.T) {
for _, tt := range []struct {
name string
Expand Down