fix: fail a credential refresh instead of extending a dead key's expiry (#995)

ec2rolecreds implements aws.HandleFailRefreshCredentialsCacheStrategy, which
aws.CredentialsCache prefers over returning the refresh error. When the cached
credential is expired or within five minutes of expiring and the refresh fails,
it hands back the same access key, secret and session token with Expires pushed
a random five to fifteen minutes out, reported as a success. Every later
refresh fails the same way, so ecs-agent signs with a credential the control
plane has already rejected, indefinitely, and nothing surfaces it.

The cache picks its strategies by asserting on the provider's dynamic type, so
returning aws.CredentialsProvider from NewProvider does not disarm it. Wrap the
provider in a type whose method set omits HandleFailToRefresh, and the cache
falls through to returning the error. AdjustExpiresBy is forwarded on purpose:
it only fires under an ExpiryWindow, which no caller sets today, but dropping
it silently would change behaviour for whoever sets one.

Fixed in imdscreds rather than at the one exposed call site, so a future caller
that wraps this provider in a cache is safe by construction.
ecr-credential-provider was never affected: it calls Retrieve uncached.

Two comments claimed the hazard was ec2rolecreds capping Expires to an hour.
That cap only ever shortens a too-far-future expiry and is unrelated.

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
 }

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")

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

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")
+	}
+}