From 92f1f97ad6e464c0bcf9719525cb4c2797df8de9 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 1 Oct 2026 15:54:30 +1000 Subject: [PATCH] fix: revoke a deleted IAM user's or role's STS sessions at deletion (#1153) --- spinifex/handlers/iam/roles.go | 12 ++ spinifex/handlers/iam/roles_test.go | 46 ++++++ spinifex/handlers/iam/service_impl.go | 27 ++++ spinifex/handlers/iam/service_impl_test.go | 77 +++++++++ spinifex/handlers/sts/service_impl.go | 5 +- spinifex/handlers/sts/session_credentials.go | 99 +++++++++--- .../handlers/sts/session_credentials_test.go | 152 ++++++++++++++++-- spinifex/services/awsgw/awsgw.go | 3 + 8 files changed, 385 insertions(+), 36 deletions(-) diff --git a/spinifex/handlers/iam/roles.go b/spinifex/handlers/iam/roles.go index 3af4a0033..504deba3f 100644 --- a/spinifex/handlers/iam/roles.go +++ b/spinifex/handlers/iam/roles.go @@ -216,6 +216,18 @@ func (s *IAMServiceImpl) DeleteRole(accountID string, input *iam.DeleteRoleInput } slog.Info("IAM role deleted", "accountID", accountID, "roleName", roleName) + + // Same ordering and failure handling as DeleteUser. + if s.sessionRevoker != nil { + revoked, err := s.sessionRevoker.RevokeRoleSessions(ctx, accountID, role.ARN, role.RoleID) + if err != nil { + slog.Error("Revoking deleted role's sessions failed; janitor will reap them", + "accountID", accountID, "roleName", roleName, "revoked", revoked, "err", err) + } else if revoked > 0 { + slog.Info("Deleted role's sessions revoked", + "accountID", accountID, "roleName", roleName, "revoked", revoked) + } + } return &iam.DeleteRoleOutput{}, nil } diff --git a/spinifex/handlers/iam/roles_test.go b/spinifex/handlers/iam/roles_test.go index 55ae114cc..b0c3391a9 100644 --- a/spinifex/handlers/iam/roles_test.go +++ b/spinifex/handlers/iam/roles_test.go @@ -2,6 +2,7 @@ package handlers_iam import ( "encoding/json" + "errors" "slices" "strconv" "strings" @@ -1656,3 +1657,48 @@ func TestRoleAndPolicy_DescriptionLength(t *testing.T) { }) require.NoError(t, err) } + +func TestDeleteRole_RevokesSessionsOnceRecordIsGone(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc} + svc.SetSessionRevoker(revoker) + role := createTestRole(t, svc, "retired") + + _, err := svc.DeleteRole(testAccountID, &iam.DeleteRoleInput{RoleName: role.RoleName}) + require.NoError(t, err) + + require.Len(t, revoker.calls, 1) + assert.Equal(t, revocation{testAccountID, aws.StringValue(role.Arn), aws.StringValue(role.RoleId), true}, revoker.calls[0]) +} + +func TestDeleteRole_ConflictDoesNotRevoke(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc} + svc.SetSessionRevoker(revoker) + role := createTestRole(t, svc, "attached") + policy := createTestPolicy(t, svc, "RolePolicy") + _, err := svc.AttachRolePolicy(testAccountID, &iam.AttachRolePolicyInput{RoleName: role.RoleName, PolicyArn: policy.Arn}) + require.NoError(t, err) + + _, err = svc.DeleteRole(testAccountID, &iam.DeleteRoleInput{RoleName: role.RoleName}) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorIAMDeleteConflict) + assert.Empty(t, revoker.calls, "a role that survives the delete keeps its sessions") +} + +func TestDeleteRole_RevocationFailureKeepsDelete(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc, err: errors.New("jetstream unavailable")} + svc.SetSessionRevoker(revoker) + role := createTestRole(t, svc, "retired") + + _, err := svc.DeleteRole(testAccountID, &iam.DeleteRoleInput{RoleName: role.RoleName}) + require.NoError(t, err) + require.Len(t, revoker.calls, 1) + + _, err = svc.GetRole(testAccountID, &iam.GetRoleInput{RoleName: role.RoleName}) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity)) +} diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index ded77d7c2..cad33b402 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -110,10 +110,24 @@ type IAMServiceImpl struct { instanceProfilesBucket jetstream.KeyValue groupsBucket jetstream.KeyValue key *masterkey.Key + sessionRevoker SessionRevoker } var _ IAMService = (*IAMServiceImpl)(nil) +// SessionRevoker removes the STS session credentials bound to a deleted user or +// role. STS implements it; IAM cannot import STS, so the gateway installs it. +type SessionRevoker interface { + RevokeUserSessions(ctx context.Context, accountID, userName, userID string) (int, error) + RevokeRoleSessions(ctx context.Context, accountID, roleARN, roleID string) (int, error) +} + +// SetSessionRevoker installs the revoker DeleteUser and DeleteRole call once the +// principal record is gone. Must be called before the service handles requests. +func (s *IAMServiceImpl) SetSessionRevoker(r SessionRevoker) { + s.sessionRevoker = r +} + // NewIAMServiceImpl creates a new IAM service backed by NATS JetStream KV. // The context bounds bucket creation and the schema migrations only. func NewIAMServiceImpl(ctx context.Context, natsConn *nats.Conn, masterKey []byte) (*IAMServiceImpl, error) { @@ -586,6 +600,19 @@ func (s *IAMServiceImpl) DeleteUser(accountID string, input *iam.DeleteUserInput } slog.Info("IAM user deleted", "accountID", accountID, "userName", userName) + + // Revoked after the delete so a racing mint either writes before the scan or + // fails to resolve the user. A failure keeps the delete; the janitor retries. + if s.sessionRevoker != nil { + revoked, err := s.sessionRevoker.RevokeUserSessions(ctx, accountID, userName, user.UserID) + if err != nil { + slog.Error("Revoking deleted user's sessions failed; janitor will reap them", + "accountID", accountID, "userName", userName, "revoked", revoked, "err", err) + } else if revoked > 0 { + slog.Info("Deleted user's sessions revoked", + "accountID", accountID, "userName", userName, "revoked", revoked) + } + } return &iam.DeleteUserOutput{}, nil } diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 5e5fa9227..08ac8cfb6 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -1,7 +1,9 @@ package handlers_iam import ( + "context" "encoding/hex" + "errors" "log/slog" "strings" "testing" @@ -3016,3 +3018,78 @@ func TestAttachUserPolicy_CustomerManagedMustExist(t *testing.T) { }) require.Error(t, err) } + +// recordingRevoker records each revocation and whether the principal record was +// already gone when it ran, which is the ordering a racing mint relies on. +type recordingRevoker struct { + svc *IAMServiceImpl + err error + + calls []revocation +} + +type revocation struct { + accountID, name, id string + recordGone bool +} + +var _ SessionRevoker = (*recordingRevoker)(nil) + +func (r *recordingRevoker) RevokeUserSessions(_ context.Context, accountID, userName, userID string) (int, error) { + _, err := r.svc.GetUser(accountID, &iam.GetUserInput{UserName: aws.String(userName)}) + r.calls = append(r.calls, revocation{accountID, userName, userID, awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity)}) + return 0, r.err +} + +func (r *recordingRevoker) RevokeRoleSessions(_ context.Context, accountID, roleARN, roleID string) (int, error) { + name := roleARN[strings.LastIndex(roleARN, "/")+1:] + _, err := r.svc.GetRole(accountID, &iam.GetRoleInput{RoleName: aws.String(name)}) + r.calls = append(r.calls, revocation{accountID, roleARN, roleID, awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity)}) + return 0, r.err +} + +func TestDeleteUser_RevokesSessionsOnceRecordIsGone(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc} + svc.SetSessionRevoker(revoker) + user := createTestUser(t, svc, "leaver") + + _, err := svc.DeleteUser(testAccountID, &iam.DeleteUserInput{UserName: aws.String("leaver")}) + require.NoError(t, err) + + require.Len(t, revoker.calls, 1) + assert.Equal(t, revocation{testAccountID, "leaver", aws.StringValue(user.UserId), true}, revoker.calls[0]) +} + +func TestDeleteUser_ConflictDoesNotRevoke(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc} + svc.SetSessionRevoker(revoker) + createTestUser(t, svc, "keyholder") + _, err := svc.CreateAccessKey(testAccountID, &iam.CreateAccessKeyInput{UserName: aws.String("keyholder")}) + require.NoError(t, err) + + _, err = svc.DeleteUser(testAccountID, &iam.DeleteUserInput{UserName: aws.String("keyholder")}) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorIAMDeleteConflict) + assert.Empty(t, revoker.calls, "a user that survives the delete keeps its sessions") +} + +// The user is already gone when revocation fails, so failing the call would +// report a delete that happened as one that did not; the janitor retries. +func TestDeleteUser_RevocationFailureKeepsDelete(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + revoker := &recordingRevoker{svc: svc, err: errors.New("jetstream unavailable")} + svc.SetSessionRevoker(revoker) + createTestUser(t, svc, "leaver") + + _, err := svc.DeleteUser(testAccountID, &iam.DeleteUserInput{UserName: aws.String("leaver")}) + require.NoError(t, err) + require.Len(t, revoker.calls, 1) + + _, err = svc.GetUser(testAccountID, &iam.GetUserInput{UserName: aws.String("leaver")}) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorIAMNoSuchEntity)) +} diff --git a/spinifex/handlers/sts/service_impl.go b/spinifex/handlers/sts/service_impl.go index e7f1b334e..a5415c177 100644 --- a/spinifex/handlers/sts/service_impl.go +++ b/spinifex/handlers/sts/service_impl.go @@ -34,7 +34,10 @@ type STSServiceImpl struct { masterKey []byte } -var _ STSService = (*STSServiceImpl)(nil) +var ( + _ STSService = (*STSServiceImpl)(nil) + _ handlers_iam.SessionRevoker = (*STSServiceImpl)(nil) +) // NewSTSServiceImpl constructs an STSServiceImpl. masterKey must be the 32-byte // key shared with IAMServiceImpl. The context bounds bucket creation and the diff --git a/spinifex/handlers/sts/session_credentials.go b/spinifex/handlers/sts/session_credentials.go index 9deb6d7e0..03ca50e66 100644 --- a/spinifex/handlers/sts/session_credentials.go +++ b/spinifex/handlers/sts/session_credentials.go @@ -156,36 +156,91 @@ func (s *STSServiceImpl) RunJanitor(ctx context.Context) { slog.Info("STS session credential janitor stopped") return case <-ticker.C: - s.sweepExpired(ctx, time.Now().UTC()) + s.sweep(ctx, time.Now().UTC()) } } } -// sweepExpired deletes all records whose ExpiresAt is past the grace period. -// Per-key errors are logged and skipped; returns the delete count. -// -// Deliberately on the raw handle rather than Store.List: the sweep needs the key -// to delete it, and one undecodable record must not stop it expiring every -// other credential. -func (s *STSServiceImpl) sweepExpired(ctx context.Context, now time.Time) int { +// sweep deletes every record past the grace period, and every unexpired record +// whose principal no longer verifies: the retry path for a failed revocation at +// deletion and for a mint that raced it. A verification fault keeps the record. +func (s *STSServiceImpl) sweep(ctx context.Context, now time.Time) int { cutoff := now.Add(-janitorGracePeriod) + var orphaned, faults int + deleted, err := s.deleteSessions(ctx, func(cred *SessionCredential) bool { + if cred.ExpiresAt.Before(cutoff) { + return true + } + _, verr := s.VerifySessionPrincipal(cred) + switch { + case verr == nil: + return false + case IsSessionPrincipalVerdict(verr): + orphaned++ + return true + default: + faults++ + return false + } + }) + if err != nil { + slog.Warn("STS janitor: sweep incomplete", "err", err) + } + if faults > 0 { + slog.Warn("STS janitor: session principal verification failed; records kept", "count", faults) + } + if deleted > 0 { + slog.Info("session credentials swept", "count", deleted, "orphaned", orphaned) + } + return deleted +} + +// RevokeUserSessions deletes the session records of a deleted user. A record +// matches on UserID or, for records predating it, on name: names are unique per +// account, so no live user can hold a session under the deleted user's name. +func (s *STSServiceImpl) RevokeUserSessions(ctx context.Context, accountID, userName, userID string) (int, error) { + return s.deleteSessions(ctx, func(cred *SessionCredential) bool { + if cred.PrincipalType != principalTypeUser || cred.AccountID != accountID { + return false + } + return (userID != "" && cred.UserID == userID) || (userName != "" && cred.SessionName == userName) + }) +} + +// RevokeRoleSessions deletes the session records of a deleted role, matching on +// RoleID or, for records predating it, on the underlying role ARN. +func (s *STSServiceImpl) RevokeRoleSessions(ctx context.Context, accountID, roleARN, roleID string) (int, error) { + return s.deleteSessions(ctx, func(cred *SessionCredential) bool { + if cred.PrincipalType == principalTypeUser || cred.AccountID != accountID { + return false + } + return (roleID != "" && cred.RoleID == roleID) || (roleARN != "" && cred.UnderlyingRoleARN == roleARN) + }) +} + +// deleteSessions scans the bucket and deletes every record match selects. The +// bucket is keyed by AKID, so selecting by anything else is a full scan. +// Per-key failures are skipped and joined, so one bad record shields no other. +// +// Deliberately on the raw handle rather than Store.List: the scan needs the key +// to delete it, and one undecodable record must not stop the rest. +func (s *STSServiceImpl) deleteSessions(ctx context.Context, match func(*SessionCredential) bool) (int, error) { bucket, err := s.sessions.KV(ctx) if err != nil { - slog.Warn("STS janitor: open session credential bucket failed", "err", err) - return 0 + return 0, fmt.Errorf("open session credential bucket: %w", err) } keys, err := kvutil.Keys(ctx, bucket) if err != nil { if errors.Is(err, jetstream.ErrNoKeysFound) { - return 0 + return 0, nil } - slog.Warn("STS janitor: list session credential keys failed", "err", err) - return 0 + return 0, fmt.Errorf("list session credential keys: %w", err) } var deleted int + var errs []error for _, key := range keys { if key == utils.VersionKey { continue @@ -195,32 +250,24 @@ func (s *STSServiceImpl) sweepExpired(ctx context.Context, now time.Time) int { if errors.Is(err, jetstream.ErrKeyNotFound) { continue } - slog.Warn("STS janitor: get session credential failed", - "key", key, "err", err) + errs = append(errs, fmt.Errorf("get session credential %s: %w", key, err)) continue } var cred SessionCredential if err := json.Unmarshal(entry.Value(), &cred); err != nil { - slog.Warn("STS janitor: unmarshal session credential failed", - "key", key, "err", err) + errs = append(errs, fmt.Errorf("unmarshal session credential %s: %w", key, err)) continue } - - if !cred.ExpiresAt.Before(cutoff) { + if !match(&cred) { continue } if err := bucket.Delete(ctx, key); err != nil { - slog.Warn("STS janitor: delete expired session credential failed", - "key", key, "err", err) + errs = append(errs, fmt.Errorf("delete session credential %s: %w", key, err)) continue } deleted++ } - - if deleted > 0 { - slog.Info("session credentials swept", "count", deleted) - } - return deleted + return deleted, errors.Join(errs...) } diff --git a/spinifex/handlers/sts/session_credentials_test.go b/spinifex/handlers/sts/session_credentials_test.go index 0711e74c9..936e86bfe 100644 --- a/spinifex/handlers/sts/session_credentials_test.go +++ b/spinifex/handlers/sts/session_credentials_test.go @@ -6,6 +6,8 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go/aws" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/kvstore" "github.com/mulgadc/spinifex/spinifex/kvutil" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -192,14 +194,22 @@ func putCredWithExpiry(t *testing.T, svc *STSServiceImpl, akid string, expiresAt func TestSweepExpired_DeletesPastGraceOnly(t *testing.T) { svc, _ := newTestSetup(t) + // The fixture's role must exist, or the sweep reaps its unexpired records as orphans. + role := createRoleInAccount(t, svc, utils.GlobalAccountID, "app-role", trustPolicyAllowingWildcard()) + put := func(akid string, expiresAt time.Time) { + cred := newTestSessionCredential(akid) + cred.RoleID = aws.StringValue(role.RoleId) + cred.ExpiresAt = expiresAt + require.NoError(t, putSessionCredential(t.Context(), svc.sessions, cred)) + } now := time.Now().UTC() - putCredWithExpiry(t, svc, "ASIALIVE000000000001", now.Add(time.Hour)) // live - putCredWithExpiry(t, svc, "ASIAJUSTEXPIRED00002", now.Add(-30*time.Minute)) // expired, within grace - putCredWithExpiry(t, svc, "ASIAPASTGRACE0000003", now.Add(-janitorGracePeriod-time.Minute)) // past grace - putCredWithExpiry(t, svc, "ASIAANCIENT000000004", now.Add(-24*time.Hour)) // long past grace + put("ASIALIVE000000000001", now.Add(time.Hour)) // live + put("ASIAJUSTEXPIRED00002", now.Add(-30*time.Minute)) // expired, within grace + put("ASIAPASTGRACE0000003", now.Add(-janitorGracePeriod-time.Minute)) // past grace + put("ASIAANCIENT000000004", now.Add(-24*time.Hour)) // long past grace - deleted := svc.sweepExpired(t.Context(), now) + deleted := svc.sweep(t.Context(), now) assert.Equal(t, 2, deleted) // Live and within-grace must still exist; past-grace must be gone. @@ -216,13 +226,13 @@ func TestSweepExpired_DeletesPastGraceOnly(t *testing.T) { func TestSweepExpired_EmptyBucketIsNoop(t *testing.T) { svc, _ := newTestSetup(t) - assert.Equal(t, 0, svc.sweepExpired(t.Context(), time.Now().UTC())) + assert.Equal(t, 0, svc.sweep(t.Context(), time.Now().UTC())) } func TestSweepExpired_SkipsCorruptRecord(t *testing.T) { // A single unmarshalable record must not stall the sweep — neighbouring // expired records still need to be cleaned up. Asserts the per-key error - // path in sweepExpired is log-and-continue, not abort. + // path in sweep is log-and-continue, not abort. svc, _ := newTestSetup(t) now := time.Now().UTC() @@ -230,7 +240,7 @@ func TestSweepExpired_SkipsCorruptRecord(t *testing.T) { require.NoError(t, err) putCredWithExpiry(t, svc, "ASIAEXPIRED000000002", now.Add(-24*time.Hour)) - deleted := svc.sweepExpired(t.Context(), now) + deleted := svc.sweep(t.Context(), now) assert.Equal(t, 1, deleted) _, err = sessionsKV(t, svc).Get(t.Context(), "ASIAEXPIRED000000002") @@ -246,7 +256,7 @@ func TestSweepExpired_IgnoresVersionKey(t *testing.T) { // iterator must skip it (it's not a SessionCredential and unmarshal // would fail every sweep). svc, _ := newTestSetup(t) - assert.Equal(t, 0, svc.sweepExpired(t.Context(), time.Now().UTC())) + assert.Equal(t, 0, svc.sweep(t.Context(), time.Now().UTC())) _, err := sessionsKV(t, svc).Get(t.Context(), utils.VersionKey) require.NoError(t, err, "version key must survive the sweep") @@ -269,3 +279,127 @@ func TestRunJanitor_StopsOnContextCancel(t *testing.T) { t.Fatal("RunJanitor did not return after context cancel") } } + +// ----- Revocation at deletion -------------------------------------------- + +// wireRevoker installs svc as its IAM service's session revoker, as awsgw does. +func wireRevoker(t *testing.T, svc *STSServiceImpl) { + t.Helper() + iamSvc, ok := svc.iamSvc.(*handlers_iam.IAMServiceImpl) + require.True(t, ok) + iamSvc.SetSessionRevoker(svc) +} + +func putSession(t *testing.T, svc *STSServiceImpl, cred *SessionCredential) string { + t.Helper() + cred.ExpiresAt = time.Now().UTC().Add(time.Hour) + require.NoError(t, putSessionCredential(t.Context(), svc.sessions, cred)) + return cred.AccessKeyID +} + +func assertSessionsGone(t *testing.T, svc *STSServiceImpl, akids ...string) { + t.Helper() + for _, akid := range akids { + cred, err := svc.LookupSessionCredential(akid) + require.NoError(t, err) + assert.Nil(t, cred, "session %s must be revoked", akid) + } +} + +func assertSessionsKept(t *testing.T, svc *STSServiceImpl, akids ...string) { + t.Helper() + for _, akid := range akids { + cred, err := svc.LookupSessionCredential(akid) + require.NoError(t, err) + assert.NotNil(t, cred, "session %s must survive", akid) + } +} + +// Deleting a user removes its session records at deletion time and nothing +// else: a role session the user assumed belongs to the role, as on AWS, and a +// role session that merely shares the user's name is not the user's. +func TestDeleteUser_RevokesOnlyTheUsersSessions(t *testing.T) { + svc, _ := newTestSetup(t) + wireRevoker(t, svc) + seedUser(t, svc, testCallerAccountID, testCallerUserName) + seedUser(t, svc, testCallerAccountID, "bob") + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + + first := mintUserSession(t, svc, testCallerAccountID, testCallerUserName).AccessKeyID + second := mintUserSession(t, svc, testCallerAccountID, testCallerUserName).AccessKeyID + legacy := putSession(t, svc, &SessionCredential{ + AccessKeyID: "ASIALEGACYUSER000001", AccountID: testCallerAccountID, + PrincipalType: principalTypeUser, SessionName: testCallerUserName, + }) + bob := mintUserSession(t, svc, testCallerAccountID, "bob").AccessKeyID + assumed := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), testCallerUserName).AccessKeyID + otherAccount := putSession(t, svc, &SessionCredential{ + AccessKeyID: "ASIAOTHERACCOUNT0001", AccountID: testCrossAccountID, + PrincipalType: principalTypeUser, SessionName: testCallerUserName, UserID: "AIDAOTHERACCOUNT0001", + }) + + deleteUser(t, svc, testCallerAccountID, testCallerUserName) + + assertSessionsGone(t, svc, first, second, legacy) + assertSessionsKept(t, svc, bob, assumed, otherAccount) +} + +func TestDeleteRole_RevokesOnlyTheRolesSessions(t *testing.T) { + svc, _ := newTestSetup(t) + wireRevoker(t, svc) + seedUser(t, svc, testCallerAccountID, testCallerUserName) + app := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + other := createRoleInAccount(t, svc, testCallerAccountID, "other", trustPolicyAllowingUser(testCallerARN())) + + first := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(app.Arn), "session-1").AccessKeyID + second := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(app.Arn), "session-2").AccessKeyID + legacy := putSession(t, svc, &SessionCredential{ + AccessKeyID: "ASIALEGACYROLE000001", AccountID: testCallerAccountID, + SessionName: "session-1", UnderlyingRoleARN: aws.StringValue(app.Arn), + }) + otherRole := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(other.Arn), "session-1").AccessKeyID + user := mintUserSession(t, svc, testCallerAccountID, testCallerUserName).AccessKeyID + + deleteRole(t, svc, testCallerAccountID, "app") + + assertSessionsGone(t, svc, first, second, legacy) + assertSessionsKept(t, svc, otherRole, user) +} + +// ----- The janitor reaps what revocation missed -------------------------- + +// With no revoker wired, deletion leaves the records behind, which is the state +// a failed revocation or a mint racing the delete produces. The sweep must reap +// them well before they expire, and leave a live principal's sessions alone. +func TestSweep_ReapsSessionsOfDeletedPrincipals(t *testing.T) { + svc, _ := newTestSetup(t) + seedUser(t, svc, testCallerAccountID, testCallerUserName) + seedUser(t, svc, testCallerAccountID, "bob") + role := createRoleInAccount(t, svc, testCallerAccountID, "app", trustPolicyAllowingUser(testCallerARN())) + + orphanUser := mintUserSession(t, svc, testCallerAccountID, "bob").AccessKeyID + orphanRole := mintRoleSession(t, svc, testCallerAccountID, aws.StringValue(role.Arn), "session-1").AccessKeyID + live := mintUserSession(t, svc, testCallerAccountID, testCallerUserName).AccessKeyID + + deleteUser(t, svc, testCallerAccountID, "bob") + deleteRole(t, svc, testCallerAccountID, "app") + assertSessionsKept(t, svc, orphanUser, orphanRole) + + assert.Equal(t, 2, svc.sweep(t.Context(), time.Now().UTC())) + assertSessionsGone(t, svc, orphanUser, orphanRole) + assertSessionsKept(t, svc, live) +} + +// An IAM outage is not evidence the principal is gone; reaping on it would +// revoke every live session in the cluster. +func TestSweep_KeepsSessionsWhenVerificationFaults(t *testing.T) { + svc, _ := newTestSetup(t) + akid := putSession(t, svc, &SessionCredential{ + AccessKeyID: "ASIAFAULTUSER0000001", AccountID: testCallerAccountID, + PrincipalType: principalTypeUser, SessionName: testCallerUserName, UserID: "AIDAEXAMPLEAAAAAAAAA", + }) + svc.iamSvc = faultingIAMService{err: errors.New("jetstream unavailable")} + + assert.Equal(t, 0, svc.sweep(t.Context(), time.Now().UTC())) + assertSessionsKept(t, svc, akid) +} diff --git a/spinifex/services/awsgw/awsgw.go b/spinifex/services/awsgw/awsgw.go index 97aa32fde..57f6ebe7e 100644 --- a/spinifex/services/awsgw/awsgw.go +++ b/spinifex/services/awsgw/awsgw.go @@ -199,6 +199,9 @@ func launchService(config *config.ClusterConfig) error { return fmt.Errorf("initialize STS service: %w", err) } + // DeleteUser and DeleteRole revoke the deleted principal's sessions through STS. + iamService.SetSessionRevoker(stsService) + // Janitor sweeps expired session credentials. go stsService.RunJanitor(janitorCtx)