diff --git a/spinifex/gateway/auth.go b/spinifex/gateway/auth.go index 4a18111c3..96d3cce4c 100644 --- a/spinifex/gateway/auth.go +++ b/spinifex/gateway/auth.go @@ -14,6 +14,7 @@ import ( "github.com/mulgadc/bluebottle/pkg/sigv4" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" "github.com/mulgadc/spinifex/spinifex/otelsetup" "github.com/mulgadc/spinifex/spinifex/utils" ) @@ -163,6 +164,14 @@ func (gw *GatewayConfig) SigV4AuthMiddleware() func(http.Handler) http.Handler { return } + // Principal state, same class as the account check above and deferred for + // the same reason: a session name resolves to whichever principal holds it + // now, so it has to be checked against the ID the session was minted for. + if errCode := gw.checkSessionPrincipal(principal, sig.Credential.AccessKeyID, clientIP); errCode != "" { + gw.writeSigV4Error(w, r, errCode) + return + } + // Resolved here, once, rather than per policy check: a fault must fail // the request rather than authorize it against a context missing the key. userID, err := gw.principalUserID(principal) @@ -282,6 +291,9 @@ type principalContext struct { // userID is aws:userid, resolved where the principal is so the value cannot // differ between two policy checks in the same request. userID string + // sessionCred is the rehydrated STS record on the ASIA path, nil on the + // long-lived one. It carries the continuity check past sig.Verify. + sessionCred *handlers_sts.SessionCredential } // resolveLongLivedAKID handles the AKIA path: IAM lookup, status check, secret decrypt. @@ -368,6 +380,7 @@ func (gw *GatewayConfig) resolveSessionAKID(r *http.Request, accessKeyID, client identity: cred.SessionName, accountID: cred.AccountID, principalType: principalTypeUser, + sessionCred: cred, }, "" } @@ -379,9 +392,51 @@ func (gw *GatewayConfig) resolveSessionAKID(r *http.Request, accessKeyID, client assumedRoleARN: cred.AssumedRoleARN, assumedRoleID: cred.AssumedRoleID, underlyingRoleARN: cred.UnderlyingRoleARN, + sessionCred: cred, }, "" } +// checkSessionPrincipal re-verifies that a session's backing principal still +// exists with the immutable ID the session was minted for, returning an AWS +// error code to fail with or "" to proceed. A nil sessionCred (long-lived +// credential) is a no-op. +// +// Deliberately not folded into resolveSessionAKID: that runs pre-signature +// because it must produce the secret, whereas this reads the IAM users and +// roles buckets. An AKID travels in cleartext, so running it there would let +// anyone holding a copied one drive IAM reads with garbage signatures. +func (gw *GatewayConfig) checkSessionPrincipal(principal principalContext, accessKeyID, clientIP string) string { + if principal.sessionCred == nil { + return "" + } + if gw.STSService == nil { + slog.Error("SigV4 auth: STS service not initialized", "accessKeyID", accessKeyID) + return awserrors.ErrorInternalError + } + + if _, err := gw.STSService.VerifySessionPrincipal(principal.sessionCred); err != nil { + if !handlers_sts.IsSessionPrincipalVerdict(err) { + slog.Error("Session principal continuity check faulted, refusing to authorize", + "accessKeyID", accessKeyID, "err", err) + return awserrors.ErrorInternalError + } + // InvalidClientTokenId, not ExpiredToken: the credential has not expired, + // the principal behind it is no longer the one it was minted for. + slog.Warn("Auth failure: session principal continuity check failed", + "accessKeyID", accessKeyID, "sourceIP", clientIP, + "identity", principal.identity, "principalType", principal.principalType, + "reason", err) + // Every legacy record fails at once on the deploy that ships this check, so + // counting them would lock out a shared egress address on distinct-attempt + // volume alone and answer "retry later" to a fleet that cannot act on it. + if !errors.Is(err, handlers_sts.ErrSessionPrincipalLegacy) { + gw.RateLimiter.RecordFailure(clientIP, failureFingerprint("session-principal", accessKeyID)) + } + return awserrors.ErrorInvalidClientTokenId + } + return "" +} + // writeSigV4Error writes an auth-failure error in the service-appropriate format. // Every auth rejection funnels through here, a rate-limit lockout included, so // this is the one place that has to record the verdict onto the request. diff --git a/spinifex/gateway/auth_test.go b/spinifex/gateway/auth_test.go index 7936d13d6..6cfae7c49 100644 --- a/spinifex/gateway/auth_test.go +++ b/spinifex/gateway/auth_test.go @@ -1622,6 +1622,9 @@ type mockSTSService struct { tokens map[string]string // AKID → plaintext wire token for HMAC equivalence lookupErr error lookups atomic.Int32 // counts LookupSessionCredential calls for negative-side-effect assertions + + principalErr error // verdict or fault the continuity check returns + principalChecks atomic.Int32 // counts VerifySessionPrincipal calls, to pin its ordering } func (m *mockSTSService) LookupSessionCredential(accessKeyID string) (*handlers_sts.SessionCredential, error) { @@ -1652,6 +1655,36 @@ func (m *mockSTSService) VerifySessionToken(cred *handlers_sts.SessionCredential return want == wireToken } +// VerifySessionPrincipal defaults to "still the same principal"; principalErr +// drives the rejection-verdict and dependency-fault branches. +func (m *mockSTSService) VerifySessionPrincipal(cred *handlers_sts.SessionCredential) (*handlers_sts.SessionPrincipal, error) { + m.principalChecks.Add(1) + if m.principalErr != nil { + return nil, m.principalErr + } + return stubSessionPrincipal(cred) +} + +// stubSessionPrincipal is the shared "principal unchanged" answer for both +// package doubles. It derives RoleName from the underlying role ARN rather than +// the session name, because that is what the real implementation returns: a +// stub echoing SessionName would let a door read the wrong field and stay green. +func stubSessionPrincipal(cred *handlers_sts.SessionCredential) (*handlers_sts.SessionPrincipal, error) { + if cred == nil { + return nil, errors.New("nil session credential") + } + roleName := cred.UnderlyingRoleARN + if idx := strings.LastIndex(roleName, "/"); idx >= 0 { + roleName = roleName[idx+1:] + } + return &handlers_sts.SessionPrincipal{ + UserID: cred.UserID, + RoleID: cred.RoleID, + RoleName: roleName, + RoleARN: cred.UnderlyingRoleARN, + }, nil +} + const ( testSessionAKID = "ASIATESTSESSIONAAAAA" testSessionToken = "wire-session-token-plain" @@ -1881,6 +1914,149 @@ func TestSigV4Auth_Session_AKIDNotInBucket(t *testing.T) { assert.Contains(t, string(body), "InvalidClientTokenId") } +// A session whose backing principal was deleted, or recreated under the same +// name with a different immutable ID, must not authenticate. InvalidClientTokenId +// rather than ExpiredToken: the credential itself has not expired. +func TestSigV4Auth_Session_StalePrincipalRejected(t *testing.T) { + cases := []struct { + name string + err error + }{ + {"gone", handlers_sts.ErrSessionPrincipalGone}, + {"replaced", handlers_sts.ErrSessionPrincipalReplaced}, + {"legacy", handlers_sts.ErrSessionPrincipalLegacy}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) + stsMock.principalErr = tc.err + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signSessionRequest(t, req, nil, testSessionAKID, testSecretKey, testSessionToken) + + resp := doRequest(handler, req) + body, _ := io.ReadAll(resp.Body) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Contains(t, string(body), awserrors.ErrorInvalidClientTokenId) + }) + } +} + +// A fault is not a verdict: an IAM outage must surface as InternalError so the +// client retries rather than discarding a credential that is still good. +func TestSigV4Auth_Session_PrincipalCheckFault_IsInternalError(t *testing.T) { + handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) + stsMock.principalErr = errors.New("jetstream unavailable") + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signSessionRequest(t, req, nil, testSessionAKID, testSecretKey, testSessionToken) + + resp := doRequest(handler, req) + body, _ := io.ReadAll(resp.Body) + + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + assert.Contains(t, string(body), awserrors.ErrorInternalError) +} + +// Ordering pin: the continuity check reads the IAM users and roles buckets, and +// an AKID travels in the Authorization header in cleartext. It must stay behind +// sig.Verify, so a bad signature is answered without the check running at all. +func TestSigV4Auth_Session_StalePrincipal_BadSignatureWinsFirst(t *testing.T) { + handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) + stsMock.principalErr = handlers_sts.ErrSessionPrincipalReplaced + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signSessionRequest(t, req, nil, testSessionAKID, "wrong-secret-key", testSessionToken) + + resp := doRequest(handler, req) + body, _ := io.ReadAll(resp.Body) + + assert.Contains(t, string(body), awserrors.ErrorSignatureDoesNotMatch) + assert.Zero(t, stsMock.principalChecks.Load(), + "continuity check must not run before the signature verifies") +} + +// The other half of the ordering pin: a good signature does reach the check. +func TestSigV4Auth_Session_ValidSignature_RunsPrincipalCheck(t *testing.T) { + handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signSessionRequest(t, req, nil, testSessionAKID, testSecretKey, testSessionToken) + + resp := doRequest(handler, req) + body, _ := io.ReadAll(resp.Body) + + require.Equalf(t, http.StatusOK, resp.StatusCode, "body: %s", string(body)) + assert.Equal(t, int32(1), stsMock.principalChecks.Load()) +} + +// The check runs past sig.Verify, so everything reaching it holds a real +// secret. Gone and Replaced still count toward the lockout: an attacker +// working through a dump of leaked session credentials looking for one whose +// principal survives presents a distinct AKID each time. Legacy must not, and +// that is not symmetry for its own sake — every record predating the field +// fails on the deploy that ships this check, so a shared egress address would +// cross maxFailures on distinct-attempt volume alone and be told to retry +// later by a fleet that has nothing to retry with. +func TestSigV4Auth_SessionPrincipalVerdict_RateLimitRecording(t *testing.T) { + cases := []struct { + name string + err error + recorded bool + }{ + {"gone", handlers_sts.ErrSessionPrincipalGone, true}, + {"replaced", handlers_sts.ErrSessionPrincipalReplaced, true}, + {"legacy", handlers_sts.ErrSessionPrincipalLegacy, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rl := NewAuthRateLimiter() + defer rl.Stop() + gw := &GatewayConfig{ + DisableLogging: true, + RateLimiter: rl, + STSService: &mockSTSService{principalErr: tc.err}, + } + principal := principalContext{ + identity: "test-session", + principalType: principalTypeAssumedRole, + sessionCred: &handlers_sts.SessionCredential{AccessKeyID: testSessionAKID}, + } + + const ip = "10.15.8.11" + code := gw.checkSessionPrincipal(principal, testSessionAKID, ip) + require.Equal(t, awserrors.ErrorInvalidClientTokenId, code) + + rl.mu.RLock() + defer rl.mu.RUnlock() + rec := rl.records[ip] + if !tc.recorded { + assert.Nil(t, rec, "a legacy verdict must not count toward the lockout") + return + } + require.NotNil(t, rec) + assert.Len(t, rec.failures, 1) + }) + } +} + +// A long-lived credential has no session record to check, so the door skips it. +func TestSigV4Auth_LongLived_SkipsPrincipalCheck(t *testing.T) { + handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signTestRequest(t, req, nil, testAccessKey, testSecretKey) + + doRequest(handler, req) + assert.Zero(t, stsMock.principalChecks.Load()) +} + func TestSigV4Auth_UnknownAKIDPrefix_NoLookup(t *testing.T) { handler, stsMock := setupSessionTestApp(t, time.Now().UTC().Add(time.Hour)) diff --git a/spinifex/gateway/ecr_principal.go b/spinifex/gateway/ecr_principal.go index 8bca4741b..1808f5cad 100644 --- a/spinifex/gateway/ecr_principal.go +++ b/spinifex/gateway/ecr_principal.go @@ -8,10 +8,10 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" - "github.com/mulgadc/bluebottle/pkg/auth" "github.com/mulgadc/spinifex/spinifex/awserrors" gateway_ecrauth "github.com/mulgadc/spinifex/spinifex/gateway/ecrauth" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + handlers_sts "github.com/mulgadc/spinifex/spinifex/handlers/sts" ) // An ECR registry token is a signed pointer to an IAM/STS record, not a @@ -65,7 +65,7 @@ func isECRDependencyFailure(err error) bool { // record is gone (invalid, 401); anything else is a dependency failure (503), // since it says nothing about whether the identity is still valid. func classifyIAMLookupErr(err error, what string) error { - if strings.Contains(err.Error(), awserrors.ErrorIAMNoSuchEntity) { + if awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity) { return ecrInvalidPrincipal("%s not found: %w", what, err) } return ecrDependencyFailure("%s lookup failed: %w", what, err) @@ -139,16 +139,17 @@ func (gw *GatewayConfig) resolveECRLongLivedPrincipal(claims *gateway_ecrauth.Cl }, nil } -// resolveECRSessionPrincipal rehydrates an ASIA (STS session) claim. A -// GetSessionToken user session resolves like resolveECRLongLivedPrincipal's -// user branch; an assumed-role session additionally re-resolves the -// underlying role and rejects the session if the role was deleted and -// recreated (its RoleID changed) even though the role name and ARN are -// unchanged. +// resolveECRSessionPrincipal rehydrates an ASIA (STS session) claim. Both a +// GetSessionToken user session and an assumed-role session are re-resolved +// against their principal's live IAM record and rejected if it was deleted, or +// recreated under the same name with a different immutable ID. func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Claims) (principalContext, error) { if gw.STSService == nil { return principalContext{}, ecrDependencyFailure("STS service not available") } + if gw.IAMService == nil { + return principalContext{}, ecrDependencyFailure("IAM service not available") + } cred, err := gw.STSService.LookupSessionCredential(claims.AccessKeyID) if err != nil { @@ -168,23 +169,24 @@ func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Clai return principalContext{}, err } - if gw.IAMService == nil { - return principalContext{}, ecrDependencyFailure("IAM service not available") + // One read of the live principal, shared by both branches: it both rejects a + // session whose principal was deleted or recreated under the same name and + // supplies the resolved record the branches below would otherwise re-read. + live, err := gw.STSService.VerifySessionPrincipal(cred) + if err != nil { + if handlers_sts.IsSessionPrincipalVerdict(err) { + return principalContext{}, ecrInvalidPrincipal("session principal is no longer valid: %w", err) + } + return principalContext{}, ecrDependencyFailure("session principal check failed: %w", err) } if cred.PrincipalType == principalTypeUser { // GetSessionToken: the session resolves to the same user identity as a // long-lived key, so it is authorized as that user. - userOut, err := gw.IAMService.GetUser(cred.AccountID, &iam.GetUserInput{UserName: aws.String(cred.SessionName)}) - if err != nil { - return principalContext{}, classifyIAMLookupErr(err, "user") - } - identity := aws.StringValue(userOut.User.UserName) - if claims.PrincipalType != principalTypeUser { return principalContext{}, ecrInvalidPrincipal("principalType claim %q does not match resolved session", claims.PrincipalType) } - canonicalARN, err := buildCallerARN(cred.AccountID, identity, principalTypeUser, "") + canonicalARN, err := buildCallerARN(cred.AccountID, cred.SessionName, principalTypeUser, "") if err != nil { return principalContext{}, ecrInvalidPrincipal("cannot build canonical ARN: %w", err) } @@ -193,10 +195,10 @@ func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Clai } return principalContext{ - identity: identity, + identity: cred.SessionName, accountID: cred.AccountID, principalType: principalTypeUser, - userID: aws.StringValue(userOut.User.UserId), + userID: live.UserID, }, nil } @@ -206,27 +208,6 @@ func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Clai return principalContext{}, ecrInvalidPrincipal("principalType claim %q does not match resolved session", claims.PrincipalType) } - // Resolve by the session's underlying role, never by SessionName — the - // caller controls RoleSessionName at AssumeRole time, so trusting it here - // would let an attacker rename their way into another role's ARN shape. - roleAcct, roleName, perr := auth.ParseRoleARN(cred.UnderlyingRoleARN) - if perr != nil || roleAcct != cred.AccountID { - return principalContext{}, ecrInvalidPrincipal("session underlying role ARN is unresolvable or cross-account: %w", perr) - } - roleOut, err := gw.IAMService.GetRole(cred.AccountID, &iam.GetRoleInput{RoleName: aws.String(roleName)}) - if err != nil { - return principalContext{}, classifyIAMLookupErr(err, "role") - } - currentRoleARN := aws.StringValue(roleOut.Role.Arn) - currentRoleID := aws.StringValue(roleOut.Role.RoleId) - if currentRoleARN != cred.UnderlyingRoleARN { - return principalContext{}, ecrInvalidPrincipal("role ARN has changed since the session was minted") - } - if cred.RoleID != "" && currentRoleID != cred.RoleID { - // The role name was deleted and recreated: same ARN, different identity. - return principalContext{}, ecrInvalidPrincipal("role was replaced since the session was minted") - } - canonicalARN, err := buildCallerARN(cred.AccountID, cred.SessionName, principalTypeAssumedRole, cred.AssumedRoleARN) if err != nil { return principalContext{}, ecrInvalidPrincipal("cannot build canonical ARN: %w", err) diff --git a/spinifex/gateway/ecr_principal_test.go b/spinifex/gateway/ecr_principal_test.go index 395296a4e..6abffde0c 100644 --- a/spinifex/gateway/ecr_principal_test.go +++ b/spinifex/gateway/ecr_principal_test.go @@ -108,11 +108,17 @@ func (m *ecrMockIAMService) GetRolePolicies(accountID, roleName string) ([]handl } // ecrMockSTSService implements handlers_sts.STSService for ECR principal -// rehydration tests. Only LookupSessionCredential is wired. +// rehydration tests. Only LookupSessionCredential and VerifySessionPrincipal +// are wired. type ecrMockSTSService struct { handlers_sts.STSService sessions map[string]*handlers_sts.SessionCredential + + // principalErr is the continuity verdict (or dependency fault) the check + // returns. Detection itself is exercised against real IAM/STS services in + // handlers/sts; what these tests pin is how this door maps the answer. + principalErr error } func newECRMockSTSService() *ecrMockSTSService { @@ -123,6 +129,13 @@ func (m *ecrMockSTSService) LookupSessionCredential(accessKeyID string) (*handle return m.sessions[accessKeyID], nil } +func (m *ecrMockSTSService) VerifySessionPrincipal(cred *handlers_sts.SessionCredential) (*handlers_sts.SessionPrincipal, error) { + if m.principalErr != nil { + return nil, m.principalErr + } + return stubSessionPrincipal(cred) +} + const ( ecrPrincipalTestAccount = "000000000042" ecrPrincipalTestAKID = "AKIATESTLONGLIVED0001" @@ -287,6 +300,7 @@ func seedECRSessionUser(iamSvc *ecrMockIAMService, stsSvc *ecrMockSTSService, ac AccountID: accountID, PrincipalType: principalTypeUser, SessionName: userName, + UserID: "AIDA" + strings.ToUpper(userName), ExpiresAt: expiresAt, } } @@ -310,6 +324,39 @@ func TestResolveECRPrincipal_SessionToken_User(t *testing.T) { principalType: principalTypeUser, userID: "AIDADEV"}, got) } +// The user branch had no continuity check at all before: a GetSessionToken +// session survived its user being deleted and recreated under the same name. +func TestResolveECRPrincipal_SessionToken_User_StalePrincipalRejected(t *testing.T) { + cases := []struct { + name string + err error + }{ + {"user deleted", handlers_sts.ErrSessionPrincipalGone}, + {"user recreated same name", handlers_sts.ErrSessionPrincipalReplaced}, + {"legacy record with no immutable UserID", handlers_sts.ErrSessionPrincipalLegacy}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + iamSvc := newECRMockIAMService() + stsSvc := newECRMockSTSService() + seedECRSessionUser(iamSvc, stsSvc, ecrPrincipalTestAccount, "dev", ecrPrincipalTestASID, time.Now().Add(time.Hour)) + stsSvc.principalErr = tc.err + gw := &GatewayConfig{IAMService: iamSvc, STSService: stsSvc} + + claims := &gateway_ecrauth.Claims{ + AccountID: ecrPrincipalTestAccount, + PrincipalType: principalTypeUser, + AccessKeyID: ecrPrincipalTestASID, + } + claims.Subject = "arn:aws:iam::" + ecrPrincipalTestAccount + ":user/dev" + + _, err := gw.resolveECRPrincipal(claims) + require.Error(t, err) + assert.False(t, isECRDependencyFailure(err)) + }) + } +} + func TestResolveECRPrincipal_SessionToken_Expired(t *testing.T) { iamSvc := newECRMockIAMService() stsSvc := newECRMockSTSService() @@ -417,26 +464,38 @@ func TestResolveECRPrincipal_AssumedRole_Rejections(t *testing.T) { return c } - t.Run("role deleted", func(t *testing.T) { - iamSvc := newECRMockIAMService() - stsSvc := newECRMockSTSService() - assumedARN := seedECRAssumedRole(iamSvc, stsSvc, ecrPrincipalTestAccount, "deploy", "AROATESTROLE0001", ecrPrincipalTestASID, "session-1", time.Now().Add(time.Hour)) - delete(iamSvc.roles, ecrPrincipalTestAccount+"|deploy") - gw := &GatewayConfig{IAMService: iamSvc, STSService: stsSvc} - _, err := gw.resolveECRPrincipal(baseClaims(assumedARN)) - require.Error(t, err) - assert.False(t, isECRDependencyFailure(err)) + // Every continuity verdict is client-visible (401 + re-auth challenge), never + // a 503: the identity really is invalid, and answering 503 would tell a + // client to retry a credential that will never work again. + t.Run("continuity verdicts are invalid-principal, not dependency failures", func(t *testing.T) { + verdicts := map[string]error{ + "role deleted": handlers_sts.ErrSessionPrincipalGone, + "role replaced (RoleId changed)": handlers_sts.ErrSessionPrincipalReplaced, + "legacy record with no immutable RoleID": handlers_sts.ErrSessionPrincipalLegacy, + } + for name, verdict := range verdicts { + t.Run(name, func(t *testing.T) { + iamSvc := newECRMockIAMService() + stsSvc := newECRMockSTSService() + assumedARN := seedECRAssumedRole(iamSvc, stsSvc, ecrPrincipalTestAccount, "deploy", "AROATESTROLE0001", ecrPrincipalTestASID, "session-1", time.Now().Add(time.Hour)) + stsSvc.principalErr = verdict + gw := &GatewayConfig{IAMService: iamSvc, STSService: stsSvc} + _, err := gw.resolveECRPrincipal(baseClaims(assumedARN)) + require.Error(t, err) + assert.False(t, isECRDependencyFailure(err)) + }) + } }) - t.Run("role replaced (RoleId changed, ARN unchanged)", func(t *testing.T) { + t.Run("continuity check fault is a dependency failure", func(t *testing.T) { iamSvc := newECRMockIAMService() stsSvc := newECRMockSTSService() assumedARN := seedECRAssumedRole(iamSvc, stsSvc, ecrPrincipalTestAccount, "deploy", "AROATESTROLE0001", ecrPrincipalTestASID, "session-1", time.Now().Add(time.Hour)) - iamSvc.roles[ecrPrincipalTestAccount+"|deploy"].RoleId = aws.String("AROADIFFERENTROLE002") + stsSvc.principalErr = errors.New("nats: no responders") gw := &GatewayConfig{IAMService: iamSvc, STSService: stsSvc} _, err := gw.resolveECRPrincipal(baseClaims(assumedARN)) require.Error(t, err) - assert.False(t, isECRDependencyFailure(err)) + assert.True(t, isECRDependencyFailure(err)) }) t.Run("expired session", func(t *testing.T) { @@ -465,16 +524,6 @@ func TestResolveECRPrincipal_AssumedRole_Rejections(t *testing.T) { require.Error(t, err) }) - t.Run("cross-account underlying role ARN rejected", func(t *testing.T) { - iamSvc := newECRMockIAMService() - stsSvc := newECRMockSTSService() - assumedARN := seedECRAssumedRole(iamSvc, stsSvc, ecrPrincipalTestAccount, "deploy", "AROATESTROLE0001", ecrPrincipalTestASID, "session-1", time.Now().Add(time.Hour)) - stsSvc.sessions[ecrPrincipalTestASID].UnderlyingRoleARN = "arn:aws:iam::000000000099:role/deploy" - gw := &GatewayConfig{IAMService: iamSvc, STSService: stsSvc} - _, err := gw.resolveECRPrincipal(baseClaims(assumedARN)) - require.Error(t, err) - }) - t.Run("nil STS service is a dependency failure", func(t *testing.T) { gw := &GatewayConfig{IAMService: newECRMockIAMService()} _, err := gw.resolveECRPrincipal(baseClaims("arn:aws:sts::" + ecrPrincipalTestAccount + ":assumed-role/deploy/session-1")) diff --git a/spinifex/handlers/sts/assume_role.go b/spinifex/handlers/sts/assume_role.go index d880cb102..0f9add53d 100644 --- a/spinifex/handlers/sts/assume_role.go +++ b/spinifex/handlers/sts/assume_role.go @@ -435,6 +435,11 @@ type sessionEnvelope struct { SessionName string SourceIdentity string + // UserID is the immutable IAM UserId; set only on a user envelope, as RoleID + // is set only on a role one. Both are what a session is bound to, so a + // same-name replacement of the principal cannot inherit it. + UserID string + // Assumed-role-only; empty for a user (GetSessionToken) envelope. AssumedRoleARN string UnderlyingRoleARN string @@ -496,6 +501,7 @@ func (s *STSServiceImpl) mintSession(ctx context.Context, env sessionEnvelope, d RoleID: env.RoleID, AssumedRoleID: env.AssumedRoleID, SessionName: env.SessionName, + UserID: env.UserID, SourceIdentity: env.SourceIdentity, ExpiresAt: expiresAt, CreatedAt: now, diff --git a/spinifex/handlers/sts/get_session_token.go b/spinifex/handlers/sts/get_session_token.go index fb37d6cdb..76fe8ed9a 100644 --- a/spinifex/handlers/sts/get_session_token.go +++ b/spinifex/handlers/sts/get_session_token.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" "github.com/aws/aws-sdk-go/service/sts" "github.com/mulgadc/spinifex/spinifex/awserrors" ) @@ -58,7 +59,25 @@ func (s *STSServiceImpl) GetSessionToken(callerAccountID, callerUserName, caller duration = clampGetSessionTokenDuration(*input.DurationSeconds) } - cred, plainSecret, plainToken, err := s.mintSession(ctx, userEnvelope(callerAccountID, callerUserName), duration) + // Resolved here rather than threaded from the gateway: the gateway's value is + // aws:userid, which reports the account ID for root. Both mint and verify read + // the record's own UserId, so the two ends compare like with like. + userOut, err := s.iamSvc.GetUser(callerAccountID, &iam.GetUserInput{UserName: aws.String(callerUserName)}) + if err != nil || userOut == nil || userOut.User == nil { + slog.Error("GetSessionToken: cannot resolve caller user record", + "account_id", callerAccountID, "user_name", callerUserName, "err", err) + return nil, errors.New(awserrors.ErrorInternalError) + } + userID := aws.StringValue(userOut.User.UserId) + if userID == "" { + // Same fail-loud treatment as a caller missing an account or name: a session + // with no immutable ID cannot be verified against the live record later. + slog.Error("GetSessionToken: caller user record carries no UserId", + "account_id", callerAccountID, "user_name", callerUserName) + return nil, errors.New(awserrors.ErrorInternalError) + } + + cred, plainSecret, plainToken, err := s.mintSession(ctx, userEnvelope(callerAccountID, callerUserName, userID), duration) if err != nil { return nil, err } @@ -94,10 +113,11 @@ func clampGetSessionTokenDuration(requested int64) int64 { // userEnvelope is the session envelope for a GetSessionToken user session: // PrincipalType "user", SessionName = the IAM user name, no assumed-role fields. -func userEnvelope(accountID, userName string) sessionEnvelope { +func userEnvelope(accountID, userName, userID string) sessionEnvelope { return sessionEnvelope{ PrincipalType: principalTypeUser, AccountID: accountID, SessionName: userName, + UserID: userID, } } diff --git a/spinifex/handlers/sts/get_session_token_test.go b/spinifex/handlers/sts/get_session_token_test.go index 4750bcc9e..dcdbeed3c 100644 --- a/spinifex/handlers/sts/get_session_token_test.go +++ b/spinifex/handlers/sts/get_session_token_test.go @@ -18,6 +18,7 @@ import ( func TestGetSessionToken_HappyPath_MintsUserSession(t *testing.T) { svc, _ := newTestSetup(t) + userID := seedUser(t, svc, testCallerAccountID, testCallerUserName) out, err := svc.GetSessionToken(testCallerAccountID, testCallerUserName, principalTypeUser, testCallerAccessKeyID, &sts.GetSessionTokenInput{}) @@ -45,6 +46,9 @@ func TestGetSessionToken_HappyPath_MintsUserSession(t *testing.T) { assert.Equal(t, principalTypeUser, stored.PrincipalType) assert.Equal(t, testCallerUserName, stored.SessionName) assert.Equal(t, testCallerAccountID, stored.AccountID) + // The name alone does not identify the principal: the immutable ID must be + // persisted, or a same-name replacement inherits the session. + assert.Equal(t, userID, stored.UserID) assert.Empty(t, stored.AssumedRoleARN) assert.Empty(t, stored.UnderlyingRoleARN) assert.Empty(t, stored.RoleID) @@ -66,6 +70,7 @@ func TestGetSessionToken_HappyPath_MintsUserSession(t *testing.T) { func TestGetSessionToken_NilInput_DefaultsToTwelveHours(t *testing.T) { svc, _ := newTestSetup(t) + seedUser(t, svc, testCallerAccountID, testCallerUserName) out, err := svc.GetSessionToken(testCallerAccountID, testCallerUserName, principalTypeUser, testCallerAccessKeyID, nil) require.NoError(t, err) @@ -76,6 +81,7 @@ func TestGetSessionToken_NilInput_DefaultsToTwelveHours(t *testing.T) { func TestGetSessionToken_DurationClamp(t *testing.T) { svc, _ := newTestSetup(t) + seedUser(t, svc, testCallerAccountID, testCallerUserName) cases := []struct { name string @@ -155,6 +161,18 @@ func TestGetSessionToken_RejectsMissingUserName(t *testing.T) { assert.Nil(t, out) } +// A session that cannot be bound to an immutable ID is worse than no session: +// it would be indistinguishable from a legacy record and rejected at first use. +func TestGetSessionToken_UnresolvableCaller_FailsLoud(t *testing.T) { + svc, _ := newTestSetup(t) + + out, err := svc.GetSessionToken(testCallerAccountID, "no-such-user", principalTypeUser, testCallerAccessKeyID, + &sts.GetSessionTokenInput{}) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) + assert.Nil(t, out) +} + // assertStoredDuration looks up the minted credential and asserts the persisted // lifetime (ExpiresAt - CreatedAt) equals the expected number of seconds. This // reads the clamp through the stored record rather than re-deriving it. diff --git a/spinifex/handlers/sts/presigned_url_verify.go b/spinifex/handlers/sts/presigned_url_verify.go index 5df955668..7adf78728 100644 --- a/spinifex/handlers/sts/presigned_url_verify.go +++ b/spinifex/handlers/sts/presigned_url_verify.go @@ -76,7 +76,7 @@ func (s *STSServiceImpl) VerifyPresignedGetCallerIdentity(presignedURL, expected return nil, errors.New(awserrors.ErrorInvalidIdentityToken) } - principal, secret, err := s.resolvePrincipalForVerify(req.Credential.AccessKeyID) + principal, secret, sessionCred, err := s.resolvePrincipalForVerify(req.Credential.AccessKeyID) if err != nil { return nil, err } @@ -92,10 +92,42 @@ func (s *STSServiceImpl) VerifyPresignedGetCallerIdentity(presignedURL, expected return nil, errors.New(awserrors.ErrorInvalidIdentityToken) } + // Deferred past req.Verify for the reason the SigV4 door defers it: the check + // reads the IAM users and roles buckets, and an AKID travels in cleartext, so + // running it earlier lets anyone holding a copy drive IAM reads unauthenticated. + if err := s.checkVerifySessionPrincipal(sessionCred, req.Credential.AccessKeyID); err != nil { + return nil, err + } + principal.XK8sAwsID = expectedClusterName return principal, nil } +// checkVerifySessionPrincipal maps a continuity verdict onto this door's +// vocabulary. A nil cred is the long-lived path, which has no session to check. +func (s *STSServiceImpl) checkVerifySessionPrincipal(cred *SessionCredential, accessKeyID string) error { + if cred == nil { + return nil + } + + // Without this the session branch performs no IAM lookup at all, so even a + // deleted principal leaves a working path to its cluster identity mapping. + if _, err := s.VerifySessionPrincipal(cred); err != nil { + if IsSessionPrincipalVerdict(err) { + slog.Warn("VerifyPresignedGetCallerIdentity: session principal continuity check failed", + "akid", accessKeyID, "reason", err) + return errors.New(awserrors.ErrorInvalidIdentityToken) + } + // The NATS responder logs every verify failure at Debug, on the assumption + // that a rejection is the expected case. A fault is not, and would otherwise + // take the fleet down with nothing written at production log level. + slog.Error("VerifyPresignedGetCallerIdentity: session principal check faulted, refusing to authorize", + "akid", accessKeyID, "err", err) + return fmt.Errorf("verify session principal: %w", err) + } + return nil +} + // mapSigv4Err maps sigv4 sentinel errors onto AWS error codes. Time-window failures // surface as ExpiredToken; every other structural failure is an invalid token. func mapSigv4Err(err error) error { @@ -109,47 +141,48 @@ func mapSigv4Err(err error) error { // resolvePrincipalForVerify resolves an access key to its plaintext secret and a // PresignedCallerIdentity skeleton. Branches on AKID prefix (session vs long-lived). -func (s *STSServiceImpl) resolvePrincipalForVerify(accessKeyID string) (*PresignedCallerIdentity, string, error) { +// The session record comes back so the caller can check continuity past the signature. +func (s *STSServiceImpl) resolvePrincipalForVerify(accessKeyID string) (*PresignedCallerIdentity, string, *SessionCredential, error) { switch { case strings.HasPrefix(accessKeyID, SessionAccessKeyIDPrefix): cred, err := s.LookupSessionCredential(accessKeyID) if err != nil { - return nil, "", err + return nil, "", nil, err } if cred == nil { - return nil, "", errors.New(awserrors.ErrorInvalidIdentityToken) + return nil, "", nil, errors.New(awserrors.ErrorInvalidIdentityToken) } if presignedTimeNow().After(cred.ExpiresAt) { - return nil, "", errors.New(awserrors.ErrorExpiredToken) + return nil, "", nil, errors.New(awserrors.ErrorExpiredToken) } secret, err := handlers_iam.DecryptSecret(cred.SecretEncrypted, s.masterKey) if err != nil { - return nil, "", fmt.Errorf("decrypt session secret: %w", err) + return nil, "", nil, fmt.Errorf("decrypt session secret: %w", err) } return &PresignedCallerIdentity{ AccountID: cred.AccountID, ARN: cred.AssumedRoleARN, UserID: cred.AssumedRoleID, PrincipalType: principalTypeAssumedRolePresigned, - }, secret, nil + }, secret, cred, nil case strings.HasPrefix(accessKeyID, longLivedAccessKeyIDPrefix): ak, err := s.iamSvc.LookupAccessKey(accessKeyID) if err != nil { - if strings.Contains(err.Error(), awserrors.ErrorIAMNoSuchEntity) { - return nil, "", errors.New(awserrors.ErrorInvalidIdentityToken) + if awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity) { + return nil, "", nil, errors.New(awserrors.ErrorInvalidIdentityToken) } - return nil, "", err + return nil, "", nil, err } if ak.Status != handlers_iam.AccessKeyStatusActive { - return nil, "", errors.New(awserrors.ErrorInvalidIdentityToken) + return nil, "", nil, errors.New(awserrors.ErrorInvalidIdentityToken) } secret, err := s.iamSvc.DecryptSecret(ak.SecretAccessKey) if err != nil { - return nil, "", fmt.Errorf("decrypt IAM secret: %w", err) + return nil, "", nil, fmt.Errorf("decrypt IAM secret: %w", err) } userOut, err := s.iamSvc.GetUser(ak.AccountID, &iam.GetUserInput{UserName: aws.String(ak.UserName)}) if err != nil { - return nil, "", err + return nil, "", nil, err } userARN := aws.StringValue(userOut.User.Arn) userID := aws.StringValue(userOut.User.UserId) @@ -161,9 +194,9 @@ func (s *STSServiceImpl) resolvePrincipalForVerify(accessKeyID string) (*Presign ARN: userARN, UserID: userID, PrincipalType: principalTypeUserPresigned, - }, secret, nil + }, secret, nil, nil default: - return nil, "", errors.New(awserrors.ErrorInvalidIdentityToken) + return nil, "", nil, errors.New(awserrors.ErrorInvalidIdentityToken) } } diff --git a/spinifex/handlers/sts/presigned_url_verify_test.go b/spinifex/handlers/sts/presigned_url_verify_test.go index e13b94947..eaf8117e3 100644 --- a/spinifex/handlers/sts/presigned_url_verify_test.go +++ b/spinifex/handlers/sts/presigned_url_verify_test.go @@ -1,6 +1,7 @@ package handlers_sts import ( + "errors" "net/http" "net/url" "strings" @@ -44,8 +45,7 @@ func presignTestURL(t *testing.T, accessKeyID, secret, clusterName string, signe // CreateAccessKey path (returns the plaintext once). func seedAccessKey(t *testing.T, svc *STSServiceImpl, accountID, userName string) (akid, secret string) { t.Helper() - _, err := svc.iamSvc.CreateUser(accountID, &iam.CreateUserInput{UserName: aws.String(userName)}) - require.NoError(t, err) + seedUser(t, svc, accountID, userName) out, err := svc.iamSvc.CreateAccessKey(accountID, &iam.CreateAccessKeyInput{UserName: aws.String(userName)}) require.NoError(t, err) return aws.StringValue(out.AccessKey.AccessKeyId), aws.StringValue(out.AccessKey.SecretAccessKey) @@ -107,6 +107,74 @@ func TestVerifyPresignedGetCallerIdentity_HappyPath_SessionCred(t *testing.T) { assert.Equal(t, principalTypeAssumedRolePresigned, got.PrincipalType) } +// Before the continuity check the session branch performed no IAM lookup at +// all, so a stale session left a working path to a cluster identity mapping +// keyed on an ARN whose role no longer exists. +func TestVerifyPresignedGetCallerIdentity_StaleSessionRejected(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "irsa-stale", + trustPolicyAllowingUser(testCallerARN())) + + assumeOut, err := svc.AssumeRole(testCallerAccountID, testCallerARN(), testCallerUserName, + basicAssumeRoleInput(*role.Arn, "sess-stale")) + require.NoError(t, err) + akid := aws.StringValue(assumeOut.Credentials.AccessKeyId) + secret := aws.StringValue(assumeOut.Credentials.SecretAccessKey) + + deleteRole(t, svc, testCallerAccountID, "irsa-stale") + createRoleInAccount(t, svc, testCallerAccountID, "irsa-stale", trustPolicyAllowingWildcard()) + + signedAt := time.Now().UTC().Truncate(time.Second) + withFrozenTime(t, signedAt) + + const cluster = "stale-cluster" + u := presignTestURL(t, akid, secret, cluster, signedAt, 900) + + got, err := svc.VerifyPresignedGetCallerIdentity(u, cluster) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidIdentityToken, err.Error()) + assert.Nil(t, got) +} + +// Ordering pin, the counterpart of the SigV4 door's: the continuity check reads +// the IAM users and roles buckets, and a presigned URL carries its AKID in the +// clear, so it must stay behind the signature. A faulting IAM backend makes the +// check's presence observable — its error is distinct from a signature rejection. +func TestVerifyPresignedGetCallerIdentity_BadSignature_SkipsPrincipalCheck(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "irsa-order", + trustPolicyAllowingUser(testCallerARN())) + + assumeOut, err := svc.AssumeRole(testCallerAccountID, testCallerARN(), testCallerUserName, + basicAssumeRoleInput(*role.Arn, "sess-order")) + require.NoError(t, err) + akid := aws.StringValue(assumeOut.Credentials.AccessKeyId) + goodSecret := aws.StringValue(assumeOut.Credentials.SecretAccessKey) + + signedAt := time.Now().UTC().Truncate(time.Second) + withFrozenTime(t, signedAt) + const cluster = "order-cluster" + + boom := errors.New("jetstream unavailable") + svc.iamSvc = faultingIAMService{err: boom} + + // Wrong secret: the signature must be rejected without the check ever running. + got, err := svc.VerifyPresignedGetCallerIdentity( + presignTestURL(t, akid, "wrong-secret-key", cluster, signedAt, 900), cluster) + require.Error(t, err) + assert.Nil(t, got) + assert.Equal(t, awserrors.ErrorInvalidIdentityToken, err.Error()) + assert.NotErrorIs(t, err, boom, "continuity check must not run before the signature verifies") + + // The other half: a good signature does reach the check, and its fault is not + // laundered into a token rejection. + got, err = svc.VerifyPresignedGetCallerIdentity( + presignTestURL(t, akid, goodSecret, cluster, signedAt, 900), cluster) + require.Error(t, err) + assert.Nil(t, got) + assert.ErrorIs(t, err, boom) +} + // ----- Cross-cluster anti-replay (Q10 mandatory) -------------------------- func TestVerifyPresignedGetCallerIdentity_CrossClusterReplay(t *testing.T) { diff --git a/spinifex/handlers/sts/role_arn.go b/spinifex/handlers/sts/role_arn.go index f3589783d..57b0b7808 100644 --- a/spinifex/handlers/sts/role_arn.go +++ b/spinifex/handlers/sts/role_arn.go @@ -43,7 +43,10 @@ func ResolveRoleByARN(svc RoleGetter, roleARN string) (string, *iam.Role, error) case errors.Is(err, auth.ErrRoleARNMismatch): return "", nil, ErrRoleUnresolved case err != nil: - if err.Error() == awserrors.ErrorIAMNoSuchEntity { + // Matched the same way the user side matches it, so a wrapped NoSuchEntity + // cannot read as a deleted role on one branch and a dependency fault on the + // other. Both directions fail closed; only the answer's shape differs. + if awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity) { return "", nil, ErrRoleUnresolved } return "", nil, err diff --git a/spinifex/handlers/sts/service.go b/spinifex/handlers/sts/service.go index 31b438e6b..a18146311 100644 --- a/spinifex/handlers/sts/service.go +++ b/spinifex/handlers/sts/service.go @@ -40,4 +40,10 @@ type STSService interface { // VerifySessionToken constant-time-compares the wire token against the stored HMAC. // Keeping the HMAC inside STSService prevents the master key from crossing the gateway surface. VerifySessionToken(cred *SessionCredential, wireToken string) bool + + // VerifySessionPrincipal re-resolves the principal a session was minted for and + // returns its live IAM record. Fails closed with ErrSessionPrincipalGone, + // ErrSessionPrincipalReplaced or ErrSessionPrincipalLegacy; any other error is a + // dependency fault. + VerifySessionPrincipal(cred *SessionCredential) (*SessionPrincipal, error) } diff --git a/spinifex/handlers/sts/session_credentials.go b/spinifex/handlers/sts/session_credentials.go index 80a94ec90..5f58e4d82 100644 --- a/spinifex/handlers/sts/session_credentials.go +++ b/spinifex/handlers/sts/session_credentials.go @@ -47,14 +47,20 @@ type SessionCredential struct { // for backward compatibility with in-flight records minted before this field existed. PrincipalType string `json:"principal_type,omitempty"` - AssumedRoleARN string `json:"assumed_role_arn"` - UnderlyingRoleARN string `json:"underlying_role_arn"` - RoleID string `json:"role_id"` - AssumedRoleID string `json:"assumed_role_id"` - SessionName string `json:"session_name"` - SourceIdentity string `json:"source_identity,omitempty"` - ExpiresAt time.Time `json:"expires_at"` - CreatedAt time.Time `json:"created_at"` + AssumedRoleARN string `json:"assumed_role_arn"` + UnderlyingRoleARN string `json:"underlying_role_arn"` + RoleID string `json:"role_id"` + AssumedRoleID string `json:"assumed_role_id"` + SessionName string `json:"session_name"` + + // UserID is the immutable IAM UserId a "user" session was minted for, the + // counterpart of RoleID. Empty on role sessions and on records minted before + // the field existed; VerifySessionPrincipal rejects the latter. + UserID string `json:"user_id,omitempty"` + + SourceIdentity string `json:"source_identity,omitempty"` + ExpiresAt time.Time `json:"expires_at"` + CreatedAt time.Time `json:"created_at"` } // initSessionCredentialsBucket opens (or creates) the session-credentials KV bucket. diff --git a/spinifex/handlers/sts/session_principal.go b/spinifex/handlers/sts/session_principal.go new file mode 100644 index 000000000..b20a7ed3c --- /dev/null +++ b/spinifex/handlers/sts/session_principal.go @@ -0,0 +1,127 @@ +package handlers_sts + +import ( + "errors" + "fmt" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// A session credential names its principal, and a name is not an identity: an +// administrator who deletes an IAM user or role and recreates it under the same +// name leaves every unexpired session for the original resolving to the +// replacement. Binding the session to the principal's immutable ID at mint and +// re-checking it against the live record at every door that rehydrates a session +// is what makes the two tell apart. +var ( + // ErrSessionPrincipalGone reports that the principal no longer exists. + ErrSessionPrincipalGone = errors.New("session principal no longer exists") + + // ErrSessionPrincipalReplaced reports that a principal of that name exists but + // carries a different immutable ID than the session was minted for. + ErrSessionPrincipalReplaced = errors.New("session principal was replaced since the session was minted") + + // ErrSessionPrincipalLegacy reports that the record holds no immutable ID to + // check. Rejected rather than waved through: accepting it preserves exactly + // the gap the check closes, and there is nothing honest to backfill from. + ErrSessionPrincipalLegacy = errors.New("session record carries no immutable principal ID") +) + +// IsSessionPrincipalVerdict reports whether err is one of the three continuity +// verdicts rather than a dependency fault. Doors answer a token error for a +// verdict and InternalError for anything else. +func IsSessionPrincipalVerdict(err error) bool { + return errors.Is(err, ErrSessionPrincipalGone) || + errors.Is(err, ErrSessionPrincipalReplaced) || + errors.Is(err, ErrSessionPrincipalLegacy) +} + +// SessionPrincipal is the live IAM record a session credential still resolves to. +type SessionPrincipal struct { + UserID string // user sessions + RoleID string // role sessions + RoleName string + RoleARN string +} + +// VerifySessionPrincipal re-resolves the principal a session was minted for and +// reports whether it is still the same one. The three verdicts above all fail +// closed; an IAM dependency fault wraps through unwrapped, so a caller can tell +// a fault from a verdict and answer InternalError rather than a token error. +func (s *STSServiceImpl) VerifySessionPrincipal(cred *SessionCredential) (*SessionPrincipal, error) { + if cred == nil { + return nil, errors.New("nil session credential") + } + if cred.PrincipalType == principalTypeUser { + return s.verifySessionUser(cred) + } + // "assumed-role", or empty for a record predating PrincipalType. + return s.verifySessionRole(cred) +} + +// verifySessionUser compares the live user's UserId against the one stored at +// mint. There is no ARN comparison to make: the session persists only the user +// name, and no IAM operation can rename or re-path a user. +func (s *STSServiceImpl) verifySessionUser(cred *SessionCredential) (*SessionPrincipal, error) { + if cred.UserID == "" { + return nil, ErrSessionPrincipalLegacy + } + + out, err := s.iamSvc.GetUser(cred.AccountID, &iam.GetUserInput{UserName: aws.String(cred.SessionName)}) + if err != nil { + if awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity) { + return nil, ErrSessionPrincipalGone + } + return nil, fmt.Errorf("resolve session user: %w", err) + } + if out == nil || out.User == nil { + return nil, ErrSessionPrincipalGone + } + + liveUserID := aws.StringValue(out.User.UserId) + if liveUserID != cred.UserID { + return nil, ErrSessionPrincipalReplaced + } + return &SessionPrincipal{UserID: liveUserID}, nil +} + +// verifySessionRole resolves the session's underlying role ARN and compares the +// live RoleId against the stored one. ResolveRoleByARN also compares the stored +// ARN back, so an invented path cannot resolve to a role its ARN does not name. +func (s *STSServiceImpl) verifySessionRole(cred *SessionCredential) (*SessionPrincipal, error) { + if cred.RoleID == "" { + return nil, ErrSessionPrincipalLegacy + } + + roleAccountID, role, err := ResolveRoleByARN(s.iamSvc, cred.UnderlyingRoleARN) + if err != nil { + // A stored ARN that does not parse names no role, which is a verdict on the + // session rather than a fault: answering InternalError would tell a client + // to retry a credential that can never resolve. + if errors.Is(err, ErrRoleUnresolved) || awserrors.IsErrorCode(err, awserrors.ErrorValidationError) { + return nil, ErrSessionPrincipalGone + } + return nil, fmt.Errorf("resolve session role: %w", err) + } + if role == nil { + return nil, ErrSessionPrincipalGone + } + + // The role's account was the session's account at mint, so a disagreement + // means the record no longer describes the principal it was minted for. + if roleAccountID != cred.AccountID { + return nil, ErrSessionPrincipalReplaced + } + if aws.StringValue(role.RoleId) != cred.RoleID { + return nil, ErrSessionPrincipalReplaced + } + + return &SessionPrincipal{ + RoleID: aws.StringValue(role.RoleId), + RoleName: aws.StringValue(role.RoleName), + RoleARN: aws.StringValue(role.Arn), + }, nil +} diff --git a/spinifex/handlers/sts/session_principal_test.go b/spinifex/handlers/sts/session_principal_test.go new file mode 100644 index 000000000..00aa664bd --- /dev/null +++ b/spinifex/handlers/sts/session_principal_test.go @@ -0,0 +1,330 @@ +// behind the unexported STSServiceImpl.iamSvc, which the fault cases swap out, +// and the credential fixtures set unexported principal-type constants. +// +//test:in-package — the verdicts are asserted against the real IAM backend +package handlers_sts + +import ( + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" + "github.com/aws/aws-sdk-go/service/sts" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// seedUser creates an IAM user and returns its immutable UserId. +func seedUser(t *testing.T, svc *STSServiceImpl, accountID, userName string) string { + t.Helper() + out, err := svc.iamSvc.CreateUser(accountID, &iam.CreateUserInput{UserName: aws.String(userName)}) + require.NoError(t, err) + userID := aws.StringValue(out.User.UserId) + require.NotEmpty(t, userID) + return userID +} + +func deleteUser(t *testing.T, svc *STSServiceImpl, accountID, userName string) { + t.Helper() + _, err := svc.iamSvc.DeleteUser(accountID, &iam.DeleteUserInput{UserName: aws.String(userName)}) + require.NoError(t, err) +} + +func deleteRole(t *testing.T, svc *STSServiceImpl, accountID, roleName string) { + t.Helper() + _, err := svc.iamSvc.DeleteRole(accountID, &iam.DeleteRoleInput{RoleName: aws.String(roleName)}) + require.NoError(t, err) +} + +// mintUserSession runs the real GetSessionToken path and returns the persisted record. +func mintUserSession(t *testing.T, svc *STSServiceImpl, accountID, userName string) *SessionCredential { + t.Helper() + out, err := svc.GetSessionToken(accountID, userName, principalTypeUser, testCallerAccessKeyID, + &sts.GetSessionTokenInput{}) + require.NoError(t, err) + cred, err := svc.LookupSessionCredential(aws.StringValue(out.Credentials.AccessKeyId)) + require.NoError(t, err) + require.NotNil(t, cred) + return cred +} + +// mintRoleSession runs the real AssumeRole path and returns the persisted record. +func mintRoleSession(t *testing.T, svc *STSServiceImpl, accountID, roleARN, sessionName string) *SessionCredential { + t.Helper() + out, err := svc.AssumeRole(accountID, testCallerARN(), testCallerUserName, + basicAssumeRoleInput(roleARN, sessionName)) + require.NoError(t, err) + cred, err := svc.LookupSessionCredential(aws.StringValue(out.Credentials.AccessKeyId)) + require.NoError(t, err) + require.NotNil(t, cred) + return cred +} + +// ----- Unchanged principals verify --------------------------------------- + +func TestVerifySessionPrincipal_UnchangedUser_ReturnsLiveID(t *testing.T) { + svc, _ := newTestSetup(t) + userID := seedUser(t, svc, testCallerAccountID, testCallerUserName) + cred := mintUserSession(t, svc, testCallerAccountID, testCallerUserName) + + got, err := svc.VerifySessionPrincipal(cred) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, userID, got.UserID) + assert.Empty(t, got.RoleID) +} + +func TestVerifySessionPrincipal_UnchangedRole_ReturnsLiveRecord(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), "session-1") + + got, err := svc.VerifySessionPrincipal(cred) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, aws.StringValue(role.RoleId), got.RoleID) + assert.Equal(t, "app", got.RoleName) + assert.Equal(t, aws.StringValue(role.Arn), got.RoleARN) +} + +// ----- Same-name recreation: the defect this check closes ----------------- + +// The reported attack: an administrator deletes a user and recreates it under +// the same name, and the deleted user's unexpired session is authorized with +// the replacement's permissions. The replacement's UserId differs, so the +// session must die on that. +func TestVerifySessionPrincipal_UserRecreatedSameName_Rejected(t *testing.T) { + svc, _ := newTestSetup(t) + originalID := seedUser(t, svc, testCallerAccountID, testCallerUserName) + cred := mintUserSession(t, svc, testCallerAccountID, testCallerUserName) + + deleteUser(t, svc, testCallerAccountID, testCallerUserName) + replacementID := seedUser(t, svc, testCallerAccountID, testCallerUserName) + require.NotEqual(t, originalID, replacementID, "recreation must change the immutable ID") + + // The replacement is granted a permission the original never had; the old + // session must not reach the policy evaluator at all. + _, err := svc.iamSvc.PutUserPolicy(testCallerAccountID, &iam.PutUserPolicyInput{ + UserName: aws.String(testCallerUserName), + PolicyName: aws.String("list-users"), + PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"iam:ListUsers","Resource":"*"}]}`), + }) + require.NoError(t, err) + + got, err := svc.VerifySessionPrincipal(cred) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSessionPrincipalReplaced) + assert.Nil(t, got) +} + +func TestVerifySessionPrincipal_RoleRecreatedSameName_Rejected(t *testing.T) { + svc, _ := newTestSetup(t) + caller := testCallerARN() + original := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(caller)) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(original.Arn), "session-1") + + deleteRole(t, svc, testCallerAccountID, "app") + replacement := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingWildcard()) + require.NotEqual(t, aws.StringValue(original.RoleId), aws.StringValue(replacement.RoleId)) + // Delete-and-recreate at the same name and path reproduces the ARN byte for + // byte, so the ARN comparison alone would not catch this. + require.Equal(t, aws.StringValue(original.Arn), aws.StringValue(replacement.Arn)) + + _, err := svc.iamSvc.PutRolePolicy(testCallerAccountID, &iam.PutRolePolicyInput{ + RoleName: aws.String("app"), + PolicyName: aws.String("admin"), + PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"*","Resource":"*"}]}`), + }) + require.NoError(t, err) + + got, err := svc.VerifySessionPrincipal(cred) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSessionPrincipalReplaced) + assert.Nil(t, got) +} + +// A replacement role whose trust policy excludes the original caller must not +// keep serving that caller's session. Trust is evaluated once, at AssumeRole, +// so the ID mismatch is the only thing standing between the two. +func TestVerifySessionPrincipal_RoleReplacedWithExcludingTrustPolicy_Rejected(t *testing.T) { + svc, _ := newTestSetup(t) + original := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(original.Arn), "session-1") + + deleteRole(t, svc, testCallerAccountID, "app") + createRoleInAccount(t, svc, testCallerAccountID, "app", + trustPolicyAllowingUser("arn:aws:iam::"+testCallerAccountID+":user/someone-else")) + + _, err := svc.VerifySessionPrincipal(cred) + assert.ErrorIs(t, err, ErrSessionPrincipalReplaced) +} + +// ----- Deleted and not recreated ----------------------------------------- + +func TestVerifySessionPrincipal_UserDeleted_Gone(t *testing.T) { + svc, _ := newTestSetup(t) + seedUser(t, svc, testCallerAccountID, testCallerUserName) + cred := mintUserSession(t, svc, testCallerAccountID, testCallerUserName) + + deleteUser(t, svc, testCallerAccountID, testCallerUserName) + + _, err := svc.VerifySessionPrincipal(cred) + assert.ErrorIs(t, err, ErrSessionPrincipalGone) +} + +func TestVerifySessionPrincipal_RoleDeleted_Gone(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), "session-1") + + deleteRole(t, svc, testCallerAccountID, "app") + + _, err := svc.VerifySessionPrincipal(cred) + assert.ErrorIs(t, err, ErrSessionPrincipalGone) +} + +// A session whose stored underlying role ARN names another account resolves to +// no role this account can vouch for, so it must not verify. +func TestVerifySessionPrincipal_CrossAccountRoleARN_Rejected(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), "session-1") + cred.UnderlyingRoleARN = "arn:aws:iam::" + testCrossAccountID + ":role/app" + + _, err := svc.VerifySessionPrincipal(cred) + require.Error(t, err) + assert.True(t, IsSessionPrincipalVerdict(err)) +} + +// A stored ARN that does not parse names no principal, so it is a verdict on +// the session rather than a dependency fault. Answering InternalError here +// would tell the client to retry a credential that can never resolve, and on +// the SigV4 door that answer also skips the rate limiter. +func TestVerifySessionPrincipal_MalformedRoleARN_Gone(t *testing.T) { + svc, _ := newTestSetup(t) + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + cred := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), "session-1") + cred.UnderlyingRoleARN = "not-an-arn" + + _, err := svc.VerifySessionPrincipal(cred) + assert.ErrorIs(t, err, ErrSessionPrincipalGone) + assert.True(t, IsSessionPrincipalVerdict(err)) +} + +// ----- Legacy records fail closed ---------------------------------------- + +// Records minted before the immutable ID was persisted carry nothing to compare, +// and are rejected rather than waved through: "absent evidence passes" preserves +// exactly the gap this check closes, and would swallow any future mint path that +// failed to populate the field. +func TestVerifySessionPrincipal_LegacyRecord_Rejected(t *testing.T) { + svc, _ := newTestSetup(t) + seedUser(t, svc, testCallerAccountID, testCallerUserName) + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + + cases := []struct { + name string + cred *SessionCredential + }{ + {"user session with no UserID", &SessionCredential{ + AccessKeyID: "ASIALEGACYUSER000001", + AccountID: testCallerAccountID, + PrincipalType: principalTypeUser, + SessionName: testCallerUserName, + }}, + {"role session with no RoleID", &SessionCredential{ + AccessKeyID: "ASIALEGACYROLE000001", + AccountID: testCallerAccountID, + PrincipalType: principalTypeAssumedRole, + SessionName: "session-1", + UnderlyingRoleARN: aws.StringValue(role.Arn), + }}, + {"pre-PrincipalType role record", &SessionCredential{ + AccessKeyID: "ASIALEGACYEMPTY00001", + AccountID: testCallerAccountID, + SessionName: "session-1", + UnderlyingRoleARN: aws.StringValue(role.Arn), + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := svc.VerifySessionPrincipal(tc.cred) + assert.ErrorIs(t, err, ErrSessionPrincipalLegacy) + assert.Nil(t, got) + }) + } +} + +// ----- Root ------------------------------------------------------------- + +// Root sessions skip policy evaluation entirely, so continuity is the only gate +// on one. No special case is needed: the seeded record's constant ID matches +// while root exists, and its removal fails closed like any other principal. +func TestVerifySessionPrincipal_Root(t *testing.T) { + svc, _ := newTestSetup(t) + rootID := seedUser(t, svc, testCallerAccountID, "root") + cred := mintUserSession(t, svc, testCallerAccountID, "root") + + got, err := svc.VerifySessionPrincipal(cred) + require.NoError(t, err) + assert.Equal(t, rootID, got.UserID) + + deleteUser(t, svc, testCallerAccountID, "root") + + _, err = svc.VerifySessionPrincipal(cred) + assert.ErrorIs(t, err, ErrSessionPrincipalGone) +} + +// ----- Faults are distinguishable from verdicts -------------------------- + +// A dependency fault must not read as a rejection verdict: the doors answer +// InternalError for the first and a token error for the second, and collapsing +// them would let an IAM outage look like a revoked principal. +func TestVerifySessionPrincipal_DependencyFault_NotAVerdict(t *testing.T) { + svc, _ := newTestSetup(t) + boom := errors.New("jetstream unavailable") + svc.iamSvc = faultingIAMService{err: boom} + + cred := &SessionCredential{ + AccessKeyID: "ASIAFAULTUSER0000001", + AccountID: testCallerAccountID, + PrincipalType: principalTypeUser, + SessionName: testCallerUserName, + UserID: "AIDAEXAMPLEAAAAAAAAA", + ExpiresAt: time.Now().UTC().Add(time.Hour), + } + + got, err := svc.VerifySessionPrincipal(cred) + require.Error(t, err) + assert.Nil(t, got) + assert.ErrorIs(t, err, boom) + assert.False(t, IsSessionPrincipalVerdict(err), "a fault must not read as a rejection verdict") +} + +func TestVerifySessionPrincipal_NilCredential_IsAFault(t *testing.T) { + svc, _ := newTestSetup(t) + + got, err := svc.VerifySessionPrincipal(nil) + require.Error(t, err) + assert.Nil(t, got) + assert.False(t, IsSessionPrincipalVerdict(err)) +} + +// faultingIAMService fails every lookup with a non-NoSuchEntity error, standing +// in for an unreachable IAM backend. +type faultingIAMService struct { + handlers_iam.IAMService + + err error +} + +func (f faultingIAMService) GetUser(string, *iam.GetUserInput) (*iam.GetUserOutput, error) { + return nil, f.err +} + +func (f faultingIAMService) GetRole(string, *iam.GetRoleInput) (*iam.GetRoleOutput, error) { + return nil, f.err +}