From a5fc2b517cabc6dd4152d25c21475ae81bbf7590 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:41:14 +1000 Subject: [PATCH] fix: supply aws:userid on the gateway authorization path (#987) --- go.mod | 2 +- go.sum | 6 +- spinifex/gateway/auth.go | 15 +++ spinifex/gateway/auth_test.go | 84 +++++++++++++- .../conditionkeys_completeness_test.go | 8 ++ spinifex/gateway/conditionkeys_test.go | 109 ++++++++++++++++++ spinifex/gateway/ecr_authorization.go | 6 +- spinifex/gateway/ecr_principal.go | 3 + spinifex/gateway/ecr_principal_test.go | 9 +- spinifex/gateway/gateway.go | 43 +++++++ spinifex/gateway/sts/GetCallerIdentity.go | 33 +++--- spinifex/handlers/iam/service_impl_test.go | 20 ++-- spinifex/handlers/iam/user_inline_test.go | 4 +- 13 files changed, 307 insertions(+), 35 deletions(-) diff --git a/go.mod b/go.mod index 7cb4f304a..f2bb96fac 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,7 @@ require ( github.com/jackc/pgx/v5 v5.10.0 github.com/klauspost/cpuid/v2 v2.4.0 github.com/miekg/dns v1.1.73 - github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0 + github.com/mulgadc/bluebottle v1.18.1-0.20260903003911-3d26c3a8def8 github.com/mulgadc/northstar v1.18.0 github.com/mulgadc/predastore v1.18.1-0.20260831064140-ff6a1dc42441 github.com/mulgadc/viperblock v1.18.0 diff --git a/go.sum b/go.sum index 22cefe695..171413819 100644 --- a/go.sum +++ b/go.sum @@ -1327,10 +1327,8 @@ github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELU github.com/muesli/cancelreader v0.2.2/go.mod h1:3XuTXfFS2VjM+HTLZY9Ak0l6eUKfijIfMUZ4EgX0QYo= github.com/muesli/termenv v0.16.0 h1:S5AlUN9dENB57rsbnkPyfdGuWIlkmzJjbFf0Tf5FWUc= github.com/muesli/termenv v0.16.0/go.mod h1:ZRfOIKPFDYQoDFF4Olj7/QJbW60Ol/kL1pU3VfY/Cnk= -github.com/mulgadc/bluebottle v1.18.1-0.20260901051501-b00bc7828992 h1:ss5/vEr5STIZXBWAZZthlAS6q9i44aOMbaFjZL9ediY= -github.com/mulgadc/bluebottle v1.18.1-0.20260901051501-b00bc7828992/go.mod h1:XBIMI+G2s3wuJNiEdYtlGDvCacw82OXEw50RIQhowYg= -github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0 h1:img64+hlZeHkjPbF/l9FPhiPNW+p0/9sqDpv8PopbpE= -github.com/mulgadc/bluebottle v1.18.1-0.20260902235515-6023971275c0/go.mod h1:XBIMI+G2s3wuJNiEdYtlGDvCacw82OXEw50RIQhowYg= +github.com/mulgadc/bluebottle v1.18.1-0.20260903003911-3d26c3a8def8 h1:eTOKjTcXmX6i9CSzrFhjtc/bpfy6bRizmIiPoVmVask= +github.com/mulgadc/bluebottle v1.18.1-0.20260903003911-3d26c3a8def8/go.mod h1:XBIMI+G2s3wuJNiEdYtlGDvCacw82OXEw50RIQhowYg= github.com/mulgadc/northstar v1.18.0 h1:ksRjIKE6qtrw/ZoYaJh8JRBh+xdnuDw8FsmFLFCiXbg= github.com/mulgadc/northstar v1.18.0/go.mod h1:z6iol77R1vsIQyIBc1JQboUPhSJ6eG4bipzfbZZbLEM= github.com/mulgadc/predastore v1.18.1-0.20260831064140-ff6a1dc42441 h1:5t+iuKpnRWXkKOmh5lLBoziBaWD+H71edWO/O4ceMp8= diff --git a/spinifex/gateway/auth.go b/spinifex/gateway/auth.go index ed8fea781..4a18111c3 100644 --- a/spinifex/gateway/auth.go +++ b/spinifex/gateway/auth.go @@ -163,6 +163,15 @@ func (gw *GatewayConfig) SigV4AuthMiddleware() func(http.Handler) http.Handler { 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) + if err != nil { + gw.writeSigV4Error(w, r, err.Error()) + return + } + principal.userID = userID + // Parse rewound the body; re-read it for query-arg parsing, then rewind // again for the downstream handler. body, err := io.ReadAll(r.Body) @@ -191,6 +200,9 @@ func (gw *GatewayConfig) SigV4AuthMiddleware() func(http.Handler) http.Handler { if principal.underlyingRoleARN != "" { ctx = context.WithValue(ctx, ctxUnderlyingRoleARN, principal.underlyingRoleARN) } + if principal.userID != "" { + ctx = context.WithValue(ctx, ctxUserID, principal.userID) + } // Parse once; dispatchers reuse via ctxQueryArgs. On error the // dispatcher re-parses and returns MalformedQueryString. @@ -267,6 +279,9 @@ type principalContext struct { assumedRoleARN string assumedRoleID string underlyingRoleARN string + // userID is aws:userid, resolved where the principal is so the value cannot + // differ between two policy checks in the same request. + userID string } // resolveLongLivedAKID handles the AKIA path: IAM lookup, status check, secret decrypt. diff --git a/spinifex/gateway/auth_test.go b/spinifex/gateway/auth_test.go index 10a2bc9b6..7936d13d6 100644 --- a/spinifex/gateway/auth_test.go +++ b/spinifex/gateway/auth_test.go @@ -18,6 +18,8 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" v4 "github.com/aws/aws-sdk-go-v2/aws/signer/v4" + awsv1 "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" "github.com/go-chi/chi/v5" "github.com/mulgadc/bluebottle/pkg/sigv4" "github.com/mulgadc/spinifex/spinifex/awserrors" @@ -39,7 +41,7 @@ const ( // mockIAMService implements handlers_iam.IAMService for auth tests. It embeds // the interface so it satisfies the full contract; the SigV4 middleware drives -// only LookupAccessKey and DecryptSecret, so those are the only methods wired. +// LookupAccessKey, DecryptSecret and GetUser, so those are the methods wired. // Any other method nil-panics, surfacing an unexpected call. type mockIAMService struct { handlers_iam.IAMService @@ -51,6 +53,10 @@ type mockIAMService struct { // every account is ACTIVE, which is what most tests here are asserting // around rather than asserting about. accounts map[string]*handlers_iam.Account + + // getUserFn overrides the synthetic user record below, for a test that cares + // what aws:userid resolves to or that it fails to resolve at all. + getUserFn func(accountID string, input *iam.GetUserInput) (*iam.GetUserOutput, error) } func (m *mockIAMService) LookupAccessKey(accessKeyID string) (*handlers_iam.AccessKey, error) { @@ -72,6 +78,19 @@ func (m *mockIAMService) GetAccount(accountID string) (*handlers_iam.Account, er return &handlers_iam.Account{AccountID: accountID, Status: handlers_iam.AccountStatusActive}, nil } +// GetUser answers with a unique ID derived from the name, so aws:userid resolves +// on the authorization path without every test defining a user record. +func (m *mockIAMService) GetUser(accountID string, input *iam.GetUserInput) (*iam.GetUserOutput, error) { + if m.getUserFn != nil { + return m.getUserFn(accountID, input) + } + name := awsv1.StringValue(input.UserName) + return &iam.GetUserOutput{User: &iam.User{ + UserName: awsv1.String(name), + UserId: awsv1.String("AIDA" + strings.ToUpper(name)), + }}, nil +} + // testMasterKey is a fixed 32-byte key for deterministic tests. var testMasterKey []byte @@ -143,6 +162,69 @@ func setupTestApp(accessKey, secretKey string) http.Handler { return r } +// userIDApp signs requests for alice on a real account, so the middleware takes +// the aws:userid branch that a user principal reaches in production. +func userIDApp(t *testing.T, svc *mockIAMService) (http.Handler, *string) { + t.Helper() + encryptedSecret, err := handlers_iam.EncryptSecret(testSecretKey, testMasterKey) + require.NoError(t, err) + svc.masterKey = testMasterKey + svc.accessKeys = map[string]*handlers_iam.AccessKey{ + testAccessKey: { + AccessKeyID: testAccessKey, + SecretAccessKey: encryptedSecret, + UserName: "alice", + AccountID: "000000000001", + Status: "Active", + }, + } + + gw := &GatewayConfig{DisableLogging: true, Region: testRegion, IAMService: svc} + seen := new(string) + r := chi.NewRouter() + r.Use(gw.SigV4AuthMiddleware()) + r.HandleFunc("/*", func(w http.ResponseWriter, r *http.Request) { + *seen = mustCtxString(r, ctxUserID) + w.Write([]byte("OK")) + }) + return r, seen +} + +// aws:userid is resolved once, by the middleware, so every policy check in the +// request evaluates the same value rather than re-reading the user record. +func TestSigV4Auth_StashesUserID(t *testing.T) { + handler, seen := userIDApp(t, &mockIAMService{}) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signTestRequest(t, req, nil, testAccessKey, testSecretKey) + + resp := doRequest(handler, req) + + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "AIDAALICE", *seen) +} + +// An IAM fault must fail the request, not authorize it against a context missing +// aws:userid: that silently narrows an Allow and widens a Deny to everything. +func TestSigV4Auth_UserIDDependencyFaultFailsClosed(t *testing.T) { + handler, _ := userIDApp(t, &mockIAMService{ + getUserFn: func(string, *iam.GetUserInput) (*iam.GetUserOutput, error) { + return nil, errors.New("nats: no responders available for request") + }, + }) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "localhost:9999" + signTestRequest(t, req, nil, testAccessKey, testSecretKey) + + resp := doRequest(handler, req) + + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + body, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(body), awserrors.ErrorInternalError) +} + func TestSigV4Auth_NoAuthorizationHeader(t *testing.T) { handler := setupTestApp(testAccessKey, testSecretKey) diff --git a/spinifex/gateway/conditionkeys_completeness_test.go b/spinifex/gateway/conditionkeys_completeness_test.go index fa3316502..9b4875425 100644 --- a/spinifex/gateway/conditionkeys_completeness_test.go +++ b/spinifex/gateway/conditionkeys_completeness_test.go @@ -22,6 +22,7 @@ import ( var gatewayDoorKeys = []string{ iampolicy.KeySecureTransport, iampolicy.KeyUsername, + iampolicy.KeyUserID, iampolicy.KeyPrincipalAccount, iampolicy.KeySourceIP, } @@ -71,14 +72,21 @@ func emittedKeys(t *testing.T) map[string]string { accountID: "000000000001", principalType: principalTypeAssumedRole, assumedRoleARN: "arn:aws:sts::000000000001:assumed-role/SharedOps/session", + assumedRoleID: "AROASHAREDOPS:session", }, } + gw := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}} union := make(map[string]string) for name, principal := range principals { r := httptest.NewRequest(http.MethodPost, "/?prefix=home/", nil) r.TLS = &tls.ConnectionState{} r.RemoteAddr = "10.4.1.9:52344" + // Resolved the way the middleware resolves it, so the gate cannot pass on + // a value only the test supplies. + userID, err := gw.principalUserID(principal) + require.NoError(t, err) + principal.userID = userID for key := range requestConditionKeys(r, principal) { union[key] = name } diff --git a/spinifex/gateway/conditionkeys_test.go b/spinifex/gateway/conditionkeys_test.go index 81367773f..471978704 100644 --- a/spinifex/gateway/conditionkeys_test.go +++ b/spinifex/gateway/conditionkeys_test.go @@ -7,10 +7,13 @@ package gateway import ( "context" "crypto/tls" + "errors" "net/http" "net/http/httptest" "testing" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/bluebottle/pkg/iampolicy" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" @@ -26,6 +29,7 @@ func TestRequestConditionKeys_PopulatesAvailableKeys(t *testing.T) { identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + userID: "AIDAALICE", } keys := requestConditionKeys(r, principal) @@ -34,10 +38,115 @@ func TestRequestConditionKeys_PopulatesAvailableKeys(t *testing.T) { iampolicy.KeySourceIP: "10.4.1.9", iampolicy.KeySecureTransport: "true", iampolicy.KeyUsername: "alice", + iampolicy.KeyUserID: "AIDAALICE", iampolicy.KeyPrincipalAccount: "000000000001", }, keys) } +// A role session's aws:userid is the ID STS minted for it. Both halves come from +// the resolved role, so unlike aws:username it can carry a decision. +func TestRequestConditionKeys_UserIDForAssumedRole(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/", nil) + + keys := requestConditionKeys(r, principalContext{ + identity: "session", + accountID: "000000000001", + principalType: principalTypeAssumedRole, + assumedRoleID: "AROASHAREDOPS:session", + userID: "AROASHAREDOPS:session", + }) + + assert.Equal(t, "AROASHAREDOPS:session", keys[iampolicy.KeyUserID]) + assert.NotContains(t, keys, iampolicy.KeyUsername) +} + +// An ID the door could not resolve is omitted, not set empty: a policy naming it +// then selects nothing on an Allow rather than selecting the empty-string path. +func TestRequestConditionKeys_OmitsUnresolvableUserID(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/", nil) + + keys := requestConditionKeys(r, principalContext{ + identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + }) + assert.NotContains(t, keys, iampolicy.KeyUserID) +} + +// principalUserID resolves the ID once per request, at the point the principal +// is resolved. A user with no record and a session minted before the ID was +// recorded both yield an omitted key, not a failed request. +func TestPrincipalUserID_ResolvesAndOmits(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{}} + + userID, err := gw.principalUserID(principalContext{ + identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + }) + require.NoError(t, err) + assert.Equal(t, "AIDAALICE", userID) + + userID, err = gw.principalUserID(principalContext{ + identity: "session", accountID: "000000000001", principalType: principalTypeAssumedRole, + assumedRoleID: "AROASHAREDOPS:session", + }) + require.NoError(t, err) + assert.Equal(t, "AROASHAREDOPS:session", userID) + + // Root's aws:userid is the account ID, matching what GetCallerIdentity + // reports for the same principal. + userID, err = gw.principalUserID(principalContext{ + identity: "root", accountID: "000000000000", principalType: principalTypeUser, + }) + require.NoError(t, err) + assert.Equal(t, "000000000000", userID) + + missing := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{ + getUserFn: func(string, *iam.GetUserInput) (*iam.GetUserOutput, error) { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + }, + }} + userID, err = missing.principalUserID(principalContext{ + identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + }) + require.NoError(t, err, "a deleted user omits the key rather than failing the request") + assert.Empty(t, userID) + + // A record predating the field resolves to no ID, which omits the key. + legacy := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{ + getUserFn: func(string, *iam.GetUserInput) (*iam.GetUserOutput, error) { + return &iam.GetUserOutput{User: &iam.User{UserName: aws.String("alice")}}, nil + }, + }} + userID, err = legacy.principalUserID(principalContext{ + identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + }) + require.NoError(t, err) + assert.Empty(t, userID) + + // A session minted before assumed_role_id was recorded takes the same arm. + userID, err = gw.principalUserID(principalContext{ + identity: "session", accountID: "000000000001", principalType: principalTypeAssumedRole, + }) + require.NoError(t, err) + assert.Empty(t, userID) +} + +// A dependency fault must not read as "the principal has no ID": the key would +// be omitted, silently narrowing an Allow and widening a Deny to everything. +func TestPrincipalUserID_DependencyFaultFailsClosed(t *testing.T) { + faulty := &GatewayConfig{DisableLogging: true, IAMService: &mockIAMService{ + getUserFn: func(string, *iam.GetUserInput) (*iam.GetUserOutput, error) { + return nil, errors.New("nats: no responders available for request") + }, + }} + + userID, err := faulty.principalUserID(principalContext{ + identity: "alice", accountID: "000000000001", principalType: principalTypeUser, + }) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) + assert.Empty(t, userID) +} + // RoleSessionName is chosen by the caller of AssumeRole, so aws:username must // stay absent for a role session — otherwise anyone permitted to assume the // role satisfies an aws:username condition just by naming their session. diff --git a/spinifex/gateway/ecr_authorization.go b/spinifex/gateway/ecr_authorization.go index b8267a80f..998cd08a5 100644 --- a/spinifex/gateway/ecr_authorization.go +++ b/spinifex/gateway/ecr_authorization.go @@ -47,6 +47,9 @@ func (gw *GatewayConfig) ecrOperationAuthorization(next http.Handler) http.Handl return } + // Hoisted: the keys depend on the request and the principal, neither of + // which changes across requirements. + keys := requestConditionKeys(r, principal) for _, req := range op.Requirements { resource, err := ecrResourceARN(gw.Region, principal.accountID, req.Scope, op) if err != nil { @@ -54,8 +57,7 @@ func (gw *GatewayConfig) ecrOperationAuthorization(next http.Handler) http.Handl return } if err := gw.evaluatePrincipalPolicyResources( - principal, policy.IAMAction("ecr", req.Action), []string{resource}, - requestConditionKeys(r, principal), + principal, policy.IAMAction("ecr", req.Action), []string{resource}, keys, ); err != nil { if err.Error() == awserrors.ErrorInternalError { // IAM/STS/NATS dependency failure: fail closed, never dispatch. diff --git a/spinifex/gateway/ecr_principal.go b/spinifex/gateway/ecr_principal.go index 237fa6cd9..8bca4741b 100644 --- a/spinifex/gateway/ecr_principal.go +++ b/spinifex/gateway/ecr_principal.go @@ -135,6 +135,7 @@ func (gw *GatewayConfig) resolveECRLongLivedPrincipal(claims *gateway_ecrauth.Cl identity: identity, accountID: ak.AccountID, principalType: principalTypeUser, + userID: aws.StringValue(userOut.User.UserId), }, nil } @@ -195,6 +196,7 @@ func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Clai identity: identity, accountID: cred.AccountID, principalType: principalTypeUser, + userID: aws.StringValue(userOut.User.UserId), }, nil } @@ -240,6 +242,7 @@ func (gw *GatewayConfig) resolveECRSessionPrincipal(claims *gateway_ecrauth.Clai assumedRoleARN: cred.AssumedRoleARN, assumedRoleID: cred.AssumedRoleID, underlyingRoleARN: cred.UnderlyingRoleARN, + userID: cred.AssumedRoleID, }, nil } diff --git a/spinifex/gateway/ecr_principal_test.go b/spinifex/gateway/ecr_principal_test.go index d6b809888..395296a4e 100644 --- a/spinifex/gateway/ecr_principal_test.go +++ b/spinifex/gateway/ecr_principal_test.go @@ -2,6 +2,7 @@ package gateway import ( "errors" + "strings" "testing" "time" @@ -137,6 +138,7 @@ func seedECRTestUser(iamSvc *ecrMockIAMService, accountID, userName, akid string } iamSvc.users[accountID+"|"+userName] = &iam.User{ UserName: aws.String(userName), + UserId: aws.String("AIDA" + strings.ToUpper(userName)), Arn: aws.String("arn:aws:iam::" + accountID + ":user/" + userName), } iamSvc.accounts[accountID] = &handlers_iam.Account{AccountID: accountID, Status: handlers_iam.AccountStatusActive} @@ -156,7 +158,8 @@ func TestResolveECRPrincipal_LongLivedUser(t *testing.T) { got, err := gw.resolveECRPrincipal(claims) require.NoError(t, err) - assert.Equal(t, principalContext{identity: "dev", accountID: ecrPrincipalTestAccount, principalType: principalTypeUser}, got) + assert.Equal(t, principalContext{identity: "dev", accountID: ecrPrincipalTestAccount, + principalType: principalTypeUser, userID: "AIDADEV"}, got) } func TestResolveECRPrincipal_GlobalRoot(t *testing.T) { @@ -275,6 +278,7 @@ func TestResolveECRPrincipal_LongLivedUser_Rejections(t *testing.T) { func seedECRSessionUser(iamSvc *ecrMockIAMService, stsSvc *ecrMockSTSService, accountID, userName, asid string, expiresAt time.Time) { iamSvc.users[accountID+"|"+userName] = &iam.User{ UserName: aws.String(userName), + UserId: aws.String("AIDA" + strings.ToUpper(userName)), Arn: aws.String("arn:aws:iam::" + accountID + ":user/" + userName), } iamSvc.accounts[accountID] = &handlers_iam.Account{AccountID: accountID, Status: handlers_iam.AccountStatusActive} @@ -302,7 +306,8 @@ func TestResolveECRPrincipal_SessionToken_User(t *testing.T) { got, err := gw.resolveECRPrincipal(claims) require.NoError(t, err) - assert.Equal(t, principalContext{identity: "dev", accountID: ecrPrincipalTestAccount, principalType: principalTypeUser}, got) + assert.Equal(t, principalContext{identity: "dev", accountID: ecrPrincipalTestAccount, + principalType: principalTypeUser, userID: "AIDADEV"}, got) } func TestResolveECRPrincipal_SessionToken_Expired(t *testing.T) { diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 5a5f78f14..e013f0ed4 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -30,6 +30,7 @@ import ( gateway_ecr "github.com/mulgadc/spinifex/spinifex/gateway/ecr" gateway_ecrauth "github.com/mulgadc/spinifex/spinifex/gateway/ecrauth" "github.com/mulgadc/spinifex/spinifex/gateway/policy" + gateway_sts "github.com/mulgadc/spinifex/spinifex/gateway/sts" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" handlers_ochrevector "github.com/mulgadc/spinifex/spinifex/handlers/ochrevector" handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" @@ -57,6 +58,9 @@ const ( ctxPrincipalType contextKey = "sigv4.principalType" ctxAssumedRoleARN contextKey = "sigv4.assumedRoleARN" ctxAssumedRoleID contextKey = "sigv4.assumedRoleID" + // ctxUserID carries aws:userid, resolved once by the SigV4 middleware so + // every policy check in a request evaluates the same value. + ctxUserID contextKey = "sigv4.userID" // ctxUnderlyingRoleARN carries the IAM role ARN backing an assumed-role session. // Policy enforcement resolves the role name from this, never from ctxIdentity // (attacker-influenced RoleSessionName). @@ -586,6 +590,7 @@ func (gw *GatewayConfig) checkPolicyResources(r *http.Request, service, action s assumedRoleARN: mustCtxString(r, ctxAssumedRoleARN), assumedRoleID: mustCtxString(r, ctxAssumedRoleID), underlyingRoleARN: mustCtxString(r, ctxUnderlyingRoleARN), + userID: mustCtxString(r, ctxUserID), } return gw.evaluatePrincipalPolicyResources(principal, policy.IAMAction(service, action), resources, requestConditionKeys(r, principal)) @@ -611,6 +616,11 @@ func requestConditionKeys(r *http.Request, principal principalContext) iampolicy if principal.accountID != "" { keys[iampolicy.KeyPrincipalAccount] = principal.accountID } + // Resolved once where the principal was, so a per-check lookup cannot leave + // one check in a request evaluating against a different context than the next. + if principal.userID != "" { + keys[iampolicy.KeyUserID] = principal.userID + } // Derived from the request rather than read from the context: the OCI // registry chain never runs SigV4AuthMiddleware, so a context-carried // address would be absent there and every aws:SourceIp condition inert. @@ -620,6 +630,39 @@ func requestConditionKeys(r *http.Request, principal principalContext) iampolicy return keys } +// principalUserID resolves aws:userid: an IAM user's unique ID, the role ID and +// session name STS minted for a role session, or the account ID for root. Both +// halves of a session's ID come from the resolved role, so unlike aws:username +// it is not caller-chosen. +// +// A principal with no ID on record returns empty and the door omits the key. A +// dependency fault returns InternalError instead: authorizing against a context +// missing the key silently narrows an Allow and widens a Deny. +func (gw *GatewayConfig) principalUserID(principal principalContext) (string, error) { + if principal.identity == "" || principal.accountID == "" { + return "", nil + } + userID, err := gateway_sts.ResolveCallerUserID(principal.accountID, principal.principalType, + principal.identity, principal.assumedRoleID, gw.IAMService) + switch { + case err == nil: + if userID == "" { + slog.Warn("aws:userid unavailable: principal record carries no user ID", + "accountID", principal.accountID, "identity", principal.identity, + "principalType", principal.principalType) + } + return userID, nil + case strings.Contains(err.Error(), awserrors.ErrorIAMNoSuchEntity): + slog.Warn("aws:userid unavailable: no such IAM user", + "accountID", principal.accountID, "user", principal.identity) + return "", nil + default: + slog.Error("aws:userid: IAM dependency fault, refusing to authorize on a degraded context", + "accountID", principal.accountID, "identity", principal.identity, "err", err) + return "", errors.New(awserrors.ErrorInternalError) + } +} + // mustCtxString reads a string context value, defaulting to "" for an absent // or wrong-typed key rather than panicking. func mustCtxString(r *http.Request, key contextKey) string { diff --git a/spinifex/gateway/sts/GetCallerIdentity.go b/spinifex/gateway/sts/GetCallerIdentity.go index 6f50434c9..a49ee32d1 100644 --- a/spinifex/gateway/sts/GetCallerIdentity.go +++ b/spinifex/gateway/sts/GetCallerIdentity.go @@ -35,14 +35,28 @@ func GetCallerIdentity( iamSvc handlers_iam.IAMService, stsSvc handlers_sts.STSService, ) (*sts.GetCallerIdentityOutput, error) { - userID, err := resolveCallerUserID(accountID, callerPrincipalType, identity, assumedRoleID, iamSvc) + userID, err := ResolveCallerUserID(accountID, callerPrincipalType, identity, assumedRoleID, iamSvc) if err != nil { return nil, err } + if userID == "" { + if callerPrincipalType == PrincipalTypeAssumedRole { + // Session vanished between auth and dispatch (expired record); surface as + // InvalidClientTokenId rather than a 500. + slog.Warn("GetCallerIdentity: assumed-role session vanished between auth and dispatch") + return nil, errors.New(awserrors.ErrorInvalidClientTokenId) + } + slog.Error("GetCallerIdentity: IAM GetUser returned empty UserId", "user", identity) + return nil, errors.New(awserrors.ErrorInternalError) + } return stsSvc.GetCallerIdentity(accountID, callerARN, userID, input) } -func resolveCallerUserID( +// ResolveCallerUserID resolves the UserId of an authenticated principal, which +// is also aws:userid at the authorization door. An error is a dependency fault; +// a principal with no ID to report returns empty, which each caller reads on its +// own terms. +func ResolveCallerUserID( accountID, callerPrincipalType, identity, assumedRoleID string, iamSvc handlers_iam.IAMService, ) (string, error) { @@ -55,28 +69,21 @@ func resolveCallerUserID( return accountID, nil } if iamSvc == nil { - slog.Error("GetCallerIdentity: IAM service not initialized") + slog.Error("ResolveCallerUserID: IAM service not initialized") return "", errors.New(awserrors.ErrorInternalError) } out, err := iamSvc.GetUser(accountID, &iam.GetUserInput{UserName: aws.String(identity)}) if err != nil { return "", err } - if out == nil || out.User == nil || aws.StringValue(out.User.UserId) == "" { - slog.Error("GetCallerIdentity: IAM GetUser returned empty UserId", "user", identity) - return "", errors.New(awserrors.ErrorInternalError) + if out == nil || out.User == nil { + return "", nil } return aws.StringValue(out.User.UserId), nil case PrincipalTypeAssumedRole: - if assumedRoleID == "" { - // Session vanished between auth and dispatch (expired record); surface as - // InvalidClientTokenId rather than a 500. - slog.Warn("GetCallerIdentity: assumed-role session vanished between auth and dispatch") - return "", errors.New(awserrors.ErrorInvalidClientTokenId) - } return assumedRoleID, nil default: - slog.Error("GetCallerIdentity: unknown principal type", "principalType", callerPrincipalType) + slog.Error("ResolveCallerUserID: unknown principal type", "principalType", callerPrincipalType) return "", errors.New(awserrors.ErrorInternalError) } } diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 2af27a789..e98b8a4e4 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -1871,14 +1871,14 @@ func TestValidatePolicyDocument_RejectsUnresolvableVariables(t *testing.T) { stmt string wantErr string }{ - {"userid in a resource", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:userid}/*"`, - `references policy variable "aws:userid"`}, - {"userid in a StringEquals value", - `"Action":"s3:*","Resource":"*","Condition":{"StringEquals":{"aws:username":"${aws:userid}"}}`, - `references policy variable "aws:userid"`}, - {"userid in a StringLike value", - `"Action":"s3:*","Resource":"*","Condition":{"StringLike":{"s3:prefix":"${aws:userid}/*"}}`, - `references policy variable "aws:userid"`}, + {"MFA in a resource", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:MultiFactorAuthPresent}/*"`, + `references policy variable "aws:MultiFactorAuthPresent"`}, + {"MFA in a StringEquals value", + `"Action":"s3:*","Resource":"*","Condition":{"StringEquals":{"aws:username":"${aws:MultiFactorAuthPresent}"}}`, + `references policy variable "aws:MultiFactorAuthPresent"`}, + {"MFA in a StringLike value", + `"Action":"s3:*","Resource":"*","Condition":{"StringLike":{"s3:prefix":"${aws:MultiFactorAuthPresent}/*"}}`, + `references policy variable "aws:MultiFactorAuthPresent"`}, {"unknown key", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:bogus}/*"`, `references policy variable "aws:bogus"`}, {"condition key that is not substitutable", @@ -1889,8 +1889,8 @@ func TestValidatePolicyDocument_RejectsUnresolvableVariables(t *testing.T) { {"empty reference", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${}/*"`, "references policy variable"}, {"second resource carries it", - `"Action":"s3:*","Resource":["arn:aws:s3:::a/*","arn:aws:s3:::${aws:userid}/*"]`, - `references policy variable "aws:userid"`}, + `"Action":"s3:*","Resource":["arn:aws:s3:::a/*","arn:aws:s3:::${aws:bogus}/*"]`, + `references policy variable "aws:bogus"`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/spinifex/handlers/iam/user_inline_test.go b/spinifex/handlers/iam/user_inline_test.go index afc241a06..090f577ee 100644 --- a/spinifex/handlers/iam/user_inline_test.go +++ b/spinifex/handlers/iam/user_inline_test.go @@ -112,14 +112,14 @@ func TestPutUserPolicy_UnresolvableVariableNamesTheCause(t *testing.T) { UserName: aws.String("variable-user"), PolicyName: aws.String("Inert"), PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow", - "Action":"s3:GetObject","Resource":"arn:aws:s3:::home/${aws:userid}/*"}]}`), + "Action":"s3:GetObject","Resource":"arn:aws:s3:::home/${aws:teamname}/*"}]}`), }) require.Error(t, err) code, message, ok := awserrors.ResolveErrorDetail(err) require.True(t, ok, "the error carries no registered AWS code, so it reaches the client as a 500") assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code) - assert.Contains(t, message, "aws:userid", "the message does not name the offending variable") + assert.Contains(t, message, "aws:teamname", "the message does not name the offending variable") assert.Contains(t, message, "${aws:username}", "the message does not name the variables that do resolve") // The rejection is also a rejection: nothing was stored.