diff --git a/cmd/ecs-agent/credentials/imds.go b/cmd/ecs-agent/credentials/imds.go index 32d2e7c14..ec318683e 100644 --- a/cmd/ecs-agent/credentials/imds.go +++ b/cmd/ecs-agent/credentials/imds.go @@ -9,9 +9,9 @@ import ( ) // IMDSProvider fetches instance-role credentials from IMDS via the AWS SDK's -// EC2 IMDS client and ec2rolecreds provider, cached with aws.CredentialsCache -// (which refetches once the real expiry passes; ec2rolecreds also caps -// Expires to at most an hour out). Safe for concurrent use. +// EC2 IMDS client and ec2rolecreds provider, cached with aws.CredentialsCache, +// which refetches once the real expiry passes. A failed refresh is an error +// here, not an extended expiry. Safe for concurrent use. type IMDSProvider struct { cache *aws.CredentialsCache } diff --git a/cmd/ecs-agent/credentials/imds_test.go b/cmd/ecs-agent/credentials/imds_test.go index 70763f0d3..0c483bc87 100644 --- a/cmd/ecs-agent/credentials/imds_test.go +++ b/cmd/ecs-agent/credentials/imds_test.go @@ -88,6 +88,63 @@ func TestRetrieve_RefetchesWhenExpired(t *testing.T) { } } +// imdsStubFailAfterFirst serves creds on the first credential request and 500s +// on every one after it, so a test can drive a refresh that fails against a +// credential the cache has already stored. +func imdsStubFailAfterFirst(t *testing.T, creds map[string]string) *httptest.Server { + t.Helper() + var hits atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/latest/api/token", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Aws-Ec2-Metadata-Token-Ttl-Seconds", "21600") + _, _ = w.Write([]byte("v2-token")) + }) + mux.HandleFunc("/latest/meta-data/iam/security-credentials/", func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/latest/meta-data/iam/security-credentials/" { + _, _ = w.Write([]byte("node-role")) + return + } + if hits.Add(1) > 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + _ = json.NewEncoder(w).Encode(creds) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + +// A refresh that fails must surface as an error. Left to itself the SDK's +// ec2rolecreds implements HandleFailToRefresh, which hands back the same dead +// key with Expires pushed 5-15 minutes out, so the agent would sign requests +// with a credential the control plane has already rejected — indefinitely, +// since every subsequent refresh fails the same way. +func TestRetrieve_FailedRefreshDoesNotExtendExpiry(t *testing.T) { + srv := imdsStubFailAfterFirst(t, map[string]string{ + "Code": "Success", + "AccessKeyId": "AKIA", + "SecretAccessKey": "secret", + "Token": "session", + // Already expired, so the next Retrieve must go back to IMDS. + "Expiration": time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }) + + p := NewIMDSProvider(srv.Client(), srv.URL+"/latest") + first, err := p.Retrieve(context.Background()) + if err != nil { + t.Fatalf("first Retrieve: %v", err) + } + + got, err := p.Retrieve(context.Background()) + if err == nil { + t.Fatalf("refresh failure was reported as success: %+v", got) + } + if got.Expiration.After(first.Expiration) { + t.Errorf("expiry extended on refresh failure: %v -> %v", first.Expiration, got.Expiration) + } +} + func TestRetrieve_CancelledContext(t *testing.T) { srv := imdsStub(t, nil, map[string]string{"Code": "Success", "AccessKeyId": "A", "SecretAccessKey": "B"}) p := NewIMDSProvider(srv.Client(), srv.URL+"/latest") diff --git a/internal/imdscreds/imdscreds.go b/internal/imdscreds/imdscreds.go index 3d3423bf1..27c1f058f 100644 --- a/internal/imdscreds/imdscreds.go +++ b/internal/imdscreds/imdscreds.go @@ -53,9 +53,35 @@ func Fetch(client *http.Client, base string) (Credentials, error) { // client's default HTTP client. func NewProvider(client *http.Client, base string) aws.CredentialsProvider { imdsClient := NewClient(client, base) - return ec2rolecreds.New(func(o *ec2rolecreds.Options) { + return noExtendOnFailure{provider: ec2rolecreds.New(func(o *ec2rolecreds.Options) { o.Client = imdsClient - }) + })} +} + +// noExtendOnFailure forwards everything ec2rolecreds offers except +// HandleFailToRefresh, which answers a failed refresh by returning the same +// expired key with a fresh Expires so a caller never learns the refresh failed. +type noExtendOnFailure struct { + provider *ec2rolecreds.Provider +} + +// aws.CredentialsCache picks its strategies by asserting on the provider's +// dynamic type, so hiding the method is what disarms it — returning an +// interface from NewProvider is not enough. +var ( + _ aws.CredentialsProvider = noExtendOnFailure{} + _ aws.AdjustExpiresByCredentialsCacheStrategy = noExtendOnFailure{} +) + +func (p noExtendOnFailure) Retrieve(ctx context.Context) (aws.Credentials, error) { + return p.provider.Retrieve(ctx) +} + +// AdjustExpiresBy is forwarded deliberately. It only fires when the cache is +// built with an ExpiryWindow, which no caller does today, but dropping it +// silently would change that caller's behaviour without them touching this. +func (p noExtendOnFailure) AdjustExpiresBy(creds aws.Credentials, dur time.Duration) (aws.Credentials, error) { + return p.provider.AdjustExpiresBy(creds, dur) } // NewClient builds an SDK IMDS client against base, an IMDS root URL such as diff --git a/internal/imdscreds/imdscreds_test.go b/internal/imdscreds/imdscreds_test.go index 36550f4e4..389f4ccae 100644 --- a/internal/imdscreds/imdscreds_test.go +++ b/internal/imdscreds/imdscreds_test.go @@ -6,6 +6,8 @@ import ( "net/http/httptest" "testing" "time" + + "github.com/aws/aws-sdk-go-v2/aws" ) // imdsStub serves the IMDSv2 token + role + credentials endpoints under the @@ -116,3 +118,17 @@ func TestFetch_BadExpirationErrors(t *testing.T) { t.Fatal("expected error on unparseable Expiration") } } + +// The provider must not carry ec2rolecreds' extend-on-failure strategy. +// aws.CredentialsCache selects it by asserting on the dynamic type, so this +// assertion is the property that keeps a failed refresh an error. +func TestNewProvider_WithholdsExtendOnFailure(t *testing.T) { + p := NewProvider(nil, "http://169.254.169.254/latest") + + if _, ok := p.(aws.HandleFailRefreshCredentialsCacheStrategy); ok { + t.Error("provider implements HandleFailRefreshCredentialsCacheStrategy, so a failed refresh will silently extend a dead credential's expiry") + } + if _, ok := p.(aws.AdjustExpiresByCredentialsCacheStrategy); !ok { + t.Error("provider dropped AdjustExpiresBy, which a caller using an ExpiryWindow relies on") + } +}