Skip to content

Commit 41a618d

Browse files
ostermanclaude
andcommitted
fix: address CodeRabbit review feedback for ambient credential support
- Propagate context in AuthenticateStandaloneAmbient instead of using context.Background() - Reject `via` configuration on aws/ambient identity (fail fast in constructor) - Preserve SDK-resolved region when no explicit region is configured - Add pr: 2254 to roadmap milestone Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 21f7a04 commit 41a618d

File tree

4 files changed

+46
-4
lines changed

4 files changed

+46
-4
lines changed

pkg/auth/identities/ambient/ambient.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ func IsStandaloneAmbientChain(chain []string, identities map[string]schema.Ident
138138
}
139139

140140
// AuthenticateStandaloneAmbient handles authentication for standalone ambient identities.
141-
func AuthenticateStandaloneAmbient(_ context.Context, identityName string, identities map[string]types.Identity) (types.ICredentials, error) {
141+
func AuthenticateStandaloneAmbient(ctx context.Context, identityName string, identities map[string]types.Identity) (types.ICredentials, error) {
142142
defer perf.Track(nil, "ambient.AuthenticateStandaloneAmbient")()
143143

144144
log.Debug("Authenticating ambient identity directly", logKeyIdentityAmbient, identityName)
@@ -149,7 +149,7 @@ func AuthenticateStandaloneAmbient(_ context.Context, identityName string, ident
149149
}
150150

151151
// Ambient identities return nil credentials — they don't manage credentials.
152-
credentials, err := identity.Authenticate(context.Background(), nil)
152+
credentials, err := identity.Authenticate(ctx, nil)
153153
if err != nil {
154154
return nil, fmt.Errorf("%w: ambient identity %q authentication failed: %w", errUtils.ErrAuthenticationFailed, identityName, err)
155155
}

pkg/auth/identities/aws/ambient.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ func NewAWSAmbientIdentity(name string, config *schema.Identity) (types.Identity
4040
if config.Kind != awsAmbientKind {
4141
return nil, fmt.Errorf("%w: invalid identity kind for aws/ambient: %s", errUtils.ErrInvalidIdentityKind, config.Kind)
4242
}
43+
if config.Via != nil {
44+
return nil, fmt.Errorf("%w: aws/ambient identity %q must not define via", errUtils.ErrInvalidIdentityConfig, name)
45+
}
4346

4447
return &awsAmbientIdentity{
4548
name: name,
@@ -90,6 +93,12 @@ func (i *awsAmbientIdentity) Authenticate(ctx context.Context, _ types.ICredenti
9093
errUtils.ErrLoadAWSConfig, i.name, err)
9194
}
9295

96+
// Fall back to SDK-resolved region when no explicit region is configured.
97+
resolvedRegion := region
98+
if resolvedRegion == "" {
99+
resolvedRegion = cfg.Region
100+
}
101+
93102
// Retrieve credentials from the resolved config.
94103
creds, err := cfg.Credentials.Retrieve(ctx)
95104
if err != nil {
@@ -108,7 +117,7 @@ func (i *awsAmbientIdentity) Authenticate(ctx context.Context, _ types.ICredenti
108117
AccessKeyID: creds.AccessKeyID,
109118
SecretAccessKey: creds.SecretAccessKey,
110119
SessionToken: creds.SessionToken,
111-
Region: region,
120+
Region: resolvedRegion,
112121
}
113122

114123
// Set expiration if available.

pkg/auth/identities/aws/ambient_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,16 @@ func TestNewAWSAmbientIdentity(t *testing.T) {
4747
wantErr: true,
4848
errSubstr: "invalid identity kind",
4949
},
50+
{
51+
name: "via is rejected",
52+
idName: "bad",
53+
config: &schema.Identity{
54+
Kind: "aws/ambient",
55+
Via: &schema.IdentityVia{Identity: "base-identity"},
56+
},
57+
wantErr: true,
58+
errSubstr: "must not define via",
59+
},
5060
}
5161

5262
for _, tt := range tests {
@@ -317,9 +327,32 @@ func TestAWSAmbientIdentityAuthenticateWithoutRegion(t *testing.T) {
317327
awsCreds, ok := creds.(*types.AWSCredentials)
318328
require.True(t, ok)
319329
assert.Equal(t, "AKIAIOSFODNN7EXAMPLE", awsCreds.AccessKeyID)
330+
// No explicit region and /dev/null config means SDK resolves no region either.
320331
assert.Empty(t, awsCreds.Region)
321332
}
322333

334+
func TestAWSAmbientIdentityAuthenticateSDKResolvedRegion(t *testing.T) {
335+
// Set credentials via env vars but region only via AWS_DEFAULT_REGION
336+
// (not in principal config). The SDK resolves this region, and it should
337+
// be preserved in the returned credentials.
338+
t.Setenv("AWS_ACCESS_KEY_ID", "AKIAIOSFODNN7EXAMPLE")
339+
t.Setenv("AWS_SECRET_ACCESS_KEY", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")
340+
t.Setenv("AWS_DEFAULT_REGION", "ap-southeast-1")
341+
t.Setenv("AWS_CONFIG_FILE", "/dev/null")
342+
t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "/dev/null")
343+
344+
identity, err := NewAWSAmbientIdentity("test", &schema.Identity{Kind: "aws/ambient"})
345+
require.NoError(t, err)
346+
347+
creds, err := identity.Authenticate(context.Background(), nil)
348+
require.NoError(t, err)
349+
require.NotNil(t, creds)
350+
351+
awsCreds, ok := creds.(*types.AWSCredentials)
352+
require.True(t, ok)
353+
assert.Equal(t, "ap-southeast-1", awsCreds.Region, "SDK-resolved region should be preserved")
354+
}
355+
323356
func TestAWSAmbientIdentityCredentialsExist(t *testing.T) {
324357
t.Setenv("AWS_ACCESS_KEY_ID", "AKIAIOSFODNN7EXAMPLE")
325358
t.Setenv("AWS_SECRET_ACCESS_KEY", "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")

website/src/data/roadmap.js

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)