Add AWS STS token exchange for OIDC-to-AWS credential conversion#3659
Add AWS STS token exchange for OIDC-to-AWS credential conversion#3659
Conversation
jhrozek
requested review from
ChrisJBurns,
JAORMX,
dmjb and
yrobla
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3659 +/- ##
=======================================
Coverage ? 65.96%
=======================================
Files ? 417
Lines ? 41196
Branches ? 0
=======================================
Hits ? 27173
Misses ? 11926
Partials ? 2097 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
force-pushed
the
aws_sts-pr-3-tokenexchange
branch
from
88f2721 to
483bd2f
Compare
github-actions
bot
added
size/S
jhrozek
force-pushed
the
aws_sts-pr-3-tokenexchange
branch
from
483bd2f to
4f58f04
Compare
github-actions
bot
added
size/S
jhrozek
force-pushed
the
aws_sts-pr-3-tokenexchange
branch
from
4f58f04 to
ff62fab
Compare
github-actions
bot
added
size/S
This implements the Exchanger component that converts OIDC identity tokens into temporary AWS credentials using STS AssumeRoleWithWebIdentity. The exchanger uses regional STS endpoints for lower latency and maps STS API errors to package-level sentinel errors for consistent error handling. Related: #3567
jhrozek
force-pushed
the
aws_sts-pr-3-tokenexchange
branch
from
ff62fab to
799c85c
Compare
github-actions
bot
added
size/S
ChrisJBurns
left a comment
ChrisJBurns
left a comment
There was a problem hiding this comment.
Looks good, couple of nits, had to ask Claude to check a couple things like the SSRF etc - but I think we should be ok.
| // Using regional endpoints (https://sts.{region}.amazonaws.com) provides lower latency | ||
| // compared to the global endpoint. | ||
| func newRegionalSTSClient(ctx context.Context, region string) (STSClient, error) { | ||
| cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(region)) |
There was a problem hiding this comment.
I don't think you need to call config.LoadDefaultConfig here for AssumeRoleWithWebIdentity?
AssumeRoleWithWebIdentity is designed to work out pre-existing AWS creds, the web-id token is the auth. Calling LoadDefaultConfig does a call to the internal metadata server (IMDS/IMDSv2) reads .aws/credentials, then reads env vars that aren't relevant. You should be able to use an anonymous creds here.
Something like
cfg, err := config.LoadDefaultConfig(ctx,
config.WithRegion(region),
config.WithCredentialsProvider(aws.AnonymousCredentials{}),
)|
|
||
| input := &sts.AssumeRoleWithWebIdentityInput{ | ||
| RoleArn: aws.String(roleArn), | ||
| RoleSessionName: aws.String(sessionName), |
There was a problem hiding this comment.
nit: could we validate the session names? I believe they have to be a certain length and characters
| return nil, fmt.Errorf("failed to load AWS config: %w", err) | ||
| } | ||
|
|
||
| regionalEndpoint := fmt.Sprintf("https://sts.%s.amazonaws.com", region) |
There was a problem hiding this comment.
Probs a nit: is there a SSRF risk here? an unvalidated region value like evil.com/foo?x= as region could produce https://sts.evil.com/foo?x=.amazonaws.com which the HTTP client could follow. There will likely be TLS cert validation so this is probably a nothing burger. I'm not too deep on the details of the caller, so i doubt a user is specifying this directly which makes it even more of a nothing burger. Alternatively, we could use the SDK's built-in endpoint resolution by removing the BaseEndpoint override entirely. the SDK will use regional endpoints when config.WithRegion is set.
| // validateInputs validates the exchange inputs. | ||
| func validateInputs(token, roleArn string, durationSeconds int32) error { | ||
| if token == "" { | ||
| return fmt.Errorf("token is required") |
There was a problem hiding this comment.
nit: a couple of bare errors here, and they can't be matched with errors.Is(). the rest of the package uses sentinel errors, wondering if its worth doing so in this function too?
| ) | ||
|
|
||
| // mockSTSClient implements STSClient for testing. | ||
| type mockSTSClient struct { |
There was a problem hiding this comment.
nit: i assume this was intended, but any reason we didn't use mockgen here? i assume simplicity and overkill
|
|
||
| output, err := e.client.AssumeRoleWithWebIdentity(ctx, input) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("STS AssumeRoleWithWebIdentity failed: %w", err) |
There was a problem hiding this comment.
Yeah so you'd probably not want to wrap errors here. STS errors can contain token fragments and role ARN details. I'd consider mapping to generic errors and logging the error itself at DEBUG.
| return nil, fmt.Errorf("STS AssumeRoleWithWebIdentity failed: %w", err) | ||
| } | ||
|
|
||
| if output.Credentials == nil { |
There was a problem hiding this comment.
nit: although typically the SDK doesn't reutnr nil output on success, should we check output for nil here just in case?
4 participants
This implements the Exchanger component that converts OIDC identity tokens into temporary AWS credentials using STS AssumeRoleWithWebIdentity. The exchanger uses regional STS endpoints for lower latency and maps STS API errors to package-level sentinel errors for consistent error handling.
Related: #3567