diff --git a/go.mod b/go.mod index e0e042895..d4663a7e8 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.17.0 + github.com/mulgadc/bluebottle v1.17.1-0.20260824040243-7b109e8b0ca1 github.com/mulgadc/northstar v1.17.0 github.com/mulgadc/predastore v1.17.1-0.20260820015438-7883d5073cf0 github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481 @@ -62,10 +62,10 @@ require ( github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.37 // indirect github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.38 // indirect github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.17 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.29 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.30 // indirect github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.37 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.37 // indirect - github.com/aws/aws-sdk-go-v2/service/s3 v1.107.1 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.38 // indirect + github.com/aws/aws-sdk-go-v2/service/s3 v1.107.2 // indirect github.com/aws/aws-sdk-go-v2/service/signin v1.5.6 // indirect github.com/aws/aws-sdk-go-v2/service/sso v1.33.6 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.6 // indirect @@ -74,7 +74,7 @@ require ( github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/boltdb/bolt v1.3.1 // indirect - github.com/buraksezer/consistent v0.10.0 // indirect + github.com/buraksezer/consistent v1.0.0 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect github.com/cenkalti/hub v1.0.2 // indirect github.com/cenkalti/rpc2 v1.0.5 // indirect @@ -134,7 +134,7 @@ require ( github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/josharian/native v1.1.0 // indirect github.com/klauspost/compress v1.19.2 // indirect - github.com/klauspost/reedsolomon v1.14.1 // indirect + github.com/klauspost/reedsolomon v1.14.2 // indirect github.com/leodido/go-urn v1.4.0 // indirect github.com/lithammer/fuzzysearch v1.1.8 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect diff --git a/go.sum b/go.sum index 0f7b6d9c1..a67192f7b 100644 --- a/go.sum +++ b/go.sum @@ -562,17 +562,17 @@ github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.1/go.mod h1: github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.12.3/go.mod h1:0yKJC/kb8sAnmlYa6Zs3QVYqaC8ug2AbnNChv5Ox3uA= github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.17 h1:OvYZOB3qA6zvfdRFiRFRzVSiElMYrz3GdntkXZxlp1o= github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.17/go.mod h1:JgR/2Ew50ACfIWau1oeMRX59tMtC0kM+PYQGEaT04cY= -github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.29 h1:E65Hj648dOV6FuUfI0mYXXhQRHbsi7n+B9h6fZPJO/E= -github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.29/go.mod h1:xLrF9yNTCs92VZSpdEd68EJbgcdw3SMR74RO6QDzWHE= +github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.30 h1:5437eMoOwqqQpZn2XJy74mlDCuPYL81texMT3mXqgtU= +github.com/aws/aws-sdk-go-v2/service/internal/checksum v1.9.30/go.mod h1:xfu2m3dOpvW8lj98wQYa8V9ku/Rta59hsbireGzhh3A= github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.23/go.mod h1:9uPh+Hrz2Vn6oMnQYiUi/zbh3ovbnQk19YKINkQny44= github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.2/go.mod h1:Ru7vg1iQ7cR4i7SZ/JTLYN9kaXtbL69UdgG0OQWQxW0= github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.12.15/go.mod h1:SwFBy2vjtA0vZbjjaFtfN045boopadnoVPhu4Fv66vY= github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.37 h1:a3D4AjrOrTrP8+d9ILBthqrElf0z1JNol09Xvnwcys8= github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.37/go.mod h1:ky0gTu+ukvUTuUKFIpp6Wid4oninrkCyvbFkVs0kpHM= -github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.37 h1:KGHa9iZCrgtkOsFfXb0S4ywsjostA/hau7WE9aSb43E= -github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.37/go.mod h1:FV79f0DSnZIEGsQjWenENGtUycrasyAaJZO+zRanLHA= -github.com/aws/aws-sdk-go-v2/service/s3 v1.107.1 h1:VUTtUJMuRNMkb/7NIKmd8NQaeQLPGCMoTJxkYKre4qM= -github.com/aws/aws-sdk-go-v2/service/s3 v1.107.1/go.mod h1:WvUaO0lP5GNMs1R6cs6qvB3mqo16GLta8yfOuf55Rpc= +github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.38 h1:gX8B8y3Ho30B1LPxefDKMi/HZqWEb47U9ogs3DtSG0M= +github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.19.38/go.mod h1:l5WblZlcmGPe4/O7JY2HO25Z+xqTBvyfTyFbRMf8gYw= +github.com/aws/aws-sdk-go-v2/service/s3 v1.107.2 h1:GNU0/xtPEXMKilJZ/a8BedeuQnvu+Usi6qVm9EFfncc= +github.com/aws/aws-sdk-go-v2/service/s3 v1.107.2/go.mod h1:4jYWUecEsQtE73jPl7p3jrbYXH5ffcR4gegyCygagfg= github.com/aws/aws-sdk-go-v2/service/signin v1.5.6 h1:i68sFvXidKlkiSvI7d7Ilc1/UvW4CtBOaivH7jhG4fs= github.com/aws/aws-sdk-go-v2/service/signin v1.5.6/go.mod h1:/h7Obr9WTtzbjTHGASRQwLN7Bupw+TC3x8x7fyx39hE= github.com/aws/aws-sdk-go-v2/service/sso v1.12.3/go.mod h1:jtLIhd+V+lft6ktxpItycqHqiVXrPIRjWIsFIlzMriw= @@ -610,8 +610,8 @@ github.com/boltdb/bolt v1.3.1 h1:JQmyP4ZBrce+ZQu0dY660FMfatumYDLun9hBCUVIkF4= github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps= github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= github.com/boombuler/barcode v1.0.1/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8= -github.com/buraksezer/consistent v0.10.0 h1:hqBgz1PvNLC5rkWcEBVAL9dFMBWz6I0VgUCW25rrZlU= -github.com/buraksezer/consistent v0.10.0/go.mod h1:6BrVajWq7wbKZlTOUPs/XVfR8c0maujuPowduSpZqmw= +github.com/buraksezer/consistent v1.0.0 h1:ggz8fN8QW95rM4D+NlNMSUUCtoubMra3x/4fmFSv+fo= +github.com/buraksezer/consistent v1.0.0/go.mod h1:L/Bczo4XsqEvJLOadAlIhamyNMhIj0iGt0+exc/KeV8= github.com/campoy/embedmd v1.0.0/go.mod h1:oxyr9RCiSXg0M3VJ3ks0UGfp98BpSSGr0kpiX3MzVl8= github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ= github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM= @@ -1223,8 +1223,8 @@ github.com/klauspost/cpuid/v2 v2.2.3/go.mod h1:RVVoqg1df56z8g3pUjL/3lE5UfnlrJX8t github.com/klauspost/cpuid/v2 v2.2.5/go.mod h1:Lcz8mBdAVJIBVzewtcLocK12l3Y+JytZYpaMropDUws= github.com/klauspost/cpuid/v2 v2.4.0 h1:S6Hrbc7+ywsr0r+RLapfGBHfyefhCTwEh3A0tV913Dw= github.com/klauspost/cpuid/v2 v2.4.0/go.mod h1:19jmZ9mjzoF//ddRSUsv0zfBTJWh3QJh9FNxZTMrGxU= -github.com/klauspost/reedsolomon v1.14.1 h1:swE9kzyWXD/wVG+l5Pe8bWnQ0giIY7D1GjCBKk3kG2U= -github.com/klauspost/reedsolomon v1.14.1/go.mod h1:yjqqjgMTQkBUHSG97/rm4zipffCNbCiZcB3kTqr++sQ= +github.com/klauspost/reedsolomon v1.14.2 h1:SafJYwpBBQBI6amHUygcjxZjXeN2HpiENHQDwuPWCCQ= +github.com/klauspost/reedsolomon v1.14.2/go.mod h1:yjqqjgMTQkBUHSG97/rm4zipffCNbCiZcB3kTqr++sQ= github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/konsorten/go-windows-terminal-sequences v1.0.3/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ= github.com/kr/fs v0.1.0/go.mod h1:FFnZGqtBN9Gxj7eW1uZ42v5BccTP0vu6NEaFoC2HwRg= @@ -1327,8 +1327,10 @@ 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.17.0 h1:K+tGfqwI8nnjV2naG5EGpWMO3Y6XpCTl7/5d5HmROiA= -github.com/mulgadc/bluebottle v1.17.0/go.mod h1:kBSgbql3n7FY/emDWP2clK0jyEpOaPSCap+wggILwFQ= +github.com/mulgadc/bluebottle v1.17.1-0.20260824024913-88edf0ac2a24 h1:FTeGBkbQjbMy28+Ul6GcHPz6g8oJ53mfp2m09HhC6d0= +github.com/mulgadc/bluebottle v1.17.1-0.20260824024913-88edf0ac2a24/go.mod h1:1RZGHtu9w1PgxEr6WsnELwQOURjt5il8MNZ2hym+o9o= +github.com/mulgadc/bluebottle v1.17.1-0.20260824040243-7b109e8b0ca1 h1:/PSbT+uGrofKToyIgp8RnKr41k/EfpyUv7jm/jiGRYI= +github.com/mulgadc/bluebottle v1.17.1-0.20260824040243-7b109e8b0ca1/go.mod h1:KkUyzqfKxyI8PvKR27EVqJnVm4BSbajl1Q/u+PQxAYw= github.com/mulgadc/northstar v1.17.0 h1:bUkcGpKw06FcWyibkqP/O5J2gnXkM8EAtGX1AZjrmzc= github.com/mulgadc/northstar v1.17.0/go.mod h1:Qg559eZgF8GIsxhSyfDaZCnvhg1Ml66/nnsAQxE9ybg= github.com/mulgadc/predastore v1.17.1-0.20260820015438-7883d5073cf0 h1:AOe9Q6lwosNOnyE/KbagIcuAnmPjwDUUPmkZ1w+WvFE= diff --git a/spinifex/gateway/conditionkeys_test.go b/spinifex/gateway/conditionkeys_test.go new file mode 100644 index 000000000..f3f27b875 --- /dev/null +++ b/spinifex/gateway/conditionkeys_test.go @@ -0,0 +1,191 @@ +package gateway + +import ( + "context" + "crypto/tls" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mulgadc/bluebottle/pkg/iampolicy" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequestConditionKeys_PopulatesAvailableKeys(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/", nil) + r.TLS = &tls.ConnectionState{} + r.RemoteAddr = "10.4.1.9:52344" + principal := principalContext{ + identity: "alice", + accountID: "000000000001", + principalType: principalTypeUser, + } + + keys := requestConditionKeys(r, principal) + + assert.Equal(t, iampolicy.ConditionKeys{ + iampolicy.KeySourceIP: "10.4.1.9", + iampolicy.KeySecureTransport: "true", + iampolicy.KeyUsername: "alice", + iampolicy.KeyPrincipalAccount: "000000000001", + }, keys) +} + +// 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. +func TestRequestConditionKeys_OmitsUsernameForAssumedRole(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/", nil) + principal := principalContext{ + identity: "alice", + accountID: "000000000001", + principalType: principalTypeAssumedRole, + assumedRoleARN: "arn:aws:sts::000000000001:assumed-role/SharedOps/alice", + } + + keys := requestConditionKeys(r, principal) + + assert.NotContains(t, keys, iampolicy.KeyUsername) + assert.Equal(t, "000000000001", keys[iampolicy.KeyPrincipalAccount]) +} + +// s3:prefix has no meaning on the AWS API path, so a policy conditioned on it +// must not fire here even though the same document works at predastore's door. +func TestRequestConditionKeys_OmitsS3Prefix(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/?prefix=home/", nil) + keys := requestConditionKeys(r, principalContext{ + identity: "alice", + principalType: principalTypeUser, + }) + + assert.NotContains(t, keys, iampolicy.KeyS3Prefix) + assert.Equal(t, "false", keys[iampolicy.KeySecureTransport]) +} + +// An empty value would compare as a real value that matches nothing, which on a +// Deny widens access instead of narrowing it. Absent is the only safe reading. +func TestRequestConditionKeys_OmitsEmptyValues(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/", nil) + r.RemoteAddr = "" + keys := requestConditionKeys(r, principalContext{principalType: principalTypeUser}) + + assert.NotContains(t, keys, iampolicy.KeySourceIP) + assert.NotContains(t, keys, iampolicy.KeyUsername) + assert.NotContains(t, keys, iampolicy.KeyPrincipalAccount) +} + +// The OCI registry chain never runs SigV4AuthMiddleware, so the source address +// must come from the request itself or every aws:SourceIp condition is inert +// on that door while working on the AWS API door. +func TestRequestConditionKeys_SourceIPWithoutAuthMiddleware(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, "/v2/app/blobs/uploads/", nil) + r.RemoteAddr = "10.4.1.9:52344" + + keys := requestConditionKeys(r, principalContext{ + identity: "alice", + principalType: principalTypeUser, + }) + + assert.Equal(t, "10.4.1.9", keys[iampolicy.KeySourceIP]) +} + +// conditionedPolicyIAMService serves one statement carrying cond, so a test can +// drive a real condition through checkPolicyResources. +func conditionedPolicyIAMService(effect string, cond map[string]map[string]handlers_iam.ConditionValue) *policyMockIAMService { + docs := []handlers_iam.PolicyDocument{{ + Version: "2012-10-17", + Statement: []handlers_iam.Statement{ + {Effect: "Allow", Action: handlers_iam.StringOrArr{"*"}, Resource: handlers_iam.StringOrArr{"*"}}, + { + Effect: effect, + Action: handlers_iam.StringOrArr{"ec2:*"}, + Resource: handlers_iam.StringOrArr{"*"}, + Condition: cond, + }, + }, + }} + if effect == "Allow" { + docs[0].Statement = docs[0].Statement[1:] + } + return &policyMockIAMService{ + getUserPoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return docs, nil }, + getRolePoliciesFn: func(_, _ string) ([]handlers_iam.PolicyDocument, error) { return docs, nil }, + } +} + +// Proves the keys built from the request actually reach the evaluator. Passing +// nil keys from checkPolicyResources would leave every other test green while +// silently killing every conditional Allow on this path. +func TestCheckPolicyResources_ConditionalAllowHonoursSourceIP(t *testing.T) { + gw := &GatewayConfig{ + DisableLogging: true, + IAMService: conditionedPolicyIAMService("Allow", map[string]map[string]handlers_iam.ConditionValue{ + iampolicy.OpIPAddress: {iampolicy.KeySourceIP: {"10.0.0.0/8"}}, + }), + } + + tests := []struct { + name string + remote string + wantErr string + }{ + {"in range", "10.4.1.9:52344", ""}, + {"out of range", "192.168.1.1:52344", awserrors.ErrorAccessDenied}, + {"no source address", "", awserrors.ErrorAccessDenied}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/", nil) + req.RemoteAddr = tt.remote + req = withTestIdentity(req) + req = req.WithContext(context.WithValue(req.Context(), ctxAccountID, "000000000001")) + + err := gw.checkPolicyResources(req, "ec2", "DescribeInstances", []string{"*"}) + if tt.wantErr == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + }) + } +} + +// A Deny conditioned on a key this door cannot supply does not fire, which is +// the one direction where an absent key widens access. Pin it so the omission +// is a decision rather than an accident. +func TestCheckPolicyResources_ConditionalDenyHonoursSourceIP(t *testing.T) { + gw := &GatewayConfig{ + DisableLogging: true, + IAMService: conditionedPolicyIAMService("Deny", map[string]map[string]handlers_iam.ConditionValue{ + iampolicy.OpIPAddress: {iampolicy.KeySourceIP: {"203.0.113.0/24"}}, + }), + } + + for _, tt := range []struct { + name string + remote string + wantErr string + }{ + {"denied range", "203.0.113.5:52344", awserrors.ErrorAccessDenied}, + {"other range", "10.4.1.9:52344", ""}, + } { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/", nil) + req.RemoteAddr = tt.remote + req = withTestIdentity(req) + req = req.WithContext(context.WithValue(req.Context(), ctxAccountID, "000000000001")) + + err := gw.checkPolicyResources(req, "ec2", "DescribeInstances", []string{"*"}) + if tt.wantErr == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + }) + } +} diff --git a/spinifex/gateway/ecr_authorization.go b/spinifex/gateway/ecr_authorization.go index 203cd20ba..b8267a80f 100644 --- a/spinifex/gateway/ecr_authorization.go +++ b/spinifex/gateway/ecr_authorization.go @@ -55,6 +55,7 @@ func (gw *GatewayConfig) ecrOperationAuthorization(next http.Handler) http.Handl } if err := gw.evaluatePrincipalPolicyResources( principal, policy.IAMAction("ecr", req.Action), []string{resource}, + requestConditionKeys(r, principal), ); err != nil { if err.Error() == awserrors.ErrorInternalError { // IAM/STS/NATS dependency failure: fail closed, never dispatch. diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index e920e475f..22aa33137 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -11,6 +11,7 @@ import ( "log/slog" "net/http" "net/url" + "strconv" "strings" "sync" "time" @@ -560,7 +561,37 @@ func (gw *GatewayConfig) checkPolicyResources(r *http.Request, service, action s assumedRoleID: mustCtxString(r, ctxAssumedRoleID), underlyingRoleARN: mustCtxString(r, ctxUnderlyingRoleARN), } - return gw.evaluatePrincipalPolicyResources(principal, policy.IAMAction(service, action), resources) + return gw.evaluatePrincipalPolicyResources(principal, policy.IAMAction(service, action), resources, + requestConditionKeys(r, principal)) +} + +// requestConditionKeys resolves the IAM condition context keys available on the +// AWS API path. s3:prefix has no meaning here and is deliberately absent, so a +// policy conditioned on it does not fire. +// +// Every key is omitted rather than set empty when unknown: an empty value reads +// as a real value that matches nothing, and on a Deny that silently widens +// access instead of narrowing it. +func requestConditionKeys(r *http.Request, principal principalContext) iampolicy.ConditionKeys { + keys := iampolicy.ConditionKeys{ + iampolicy.KeySecureTransport: strconv.FormatBool(r.TLS != nil), + } + // aws:username is user-only in AWS. A role session's identity is the + // caller-chosen RoleSessionName, so gating authorization on it here would + // let any principal that may assume the role satisfy the condition at will. + if principal.principalType == principalTypeUser && principal.identity != "" { + keys[iampolicy.KeyUsername] = principal.identity + } + if principal.accountID != "" { + keys[iampolicy.KeyPrincipalAccount] = principal.accountID + } + // 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. + if ip := utils.ClientIP(r.RemoteAddr); ip != "" { + keys[iampolicy.KeySourceIP] = ip + } + return keys } // mustCtxString reads a string context value, defaulting to "" for an absent @@ -573,7 +604,7 @@ func mustCtxString(r *http.Request, key contextKey) string { // evaluatePrincipalPolicyResources resolves policies once and evaluates every // resource in the request against that same snapshot. func (gw *GatewayConfig) evaluatePrincipalPolicyResources( - principal principalContext, iamAction string, resources []string, + principal principalContext, iamAction string, resources []string, keys iampolicy.ConditionKeys, ) error { if gw.IAMService == nil { slog.Error("evaluatePrincipalPolicy: IAM service not available", "action", iamAction) @@ -638,7 +669,7 @@ func (gw *GatewayConfig) evaluatePrincipalPolicyResources( } for _, resource := range resources { - if iampolicy.Evaluate(iamAction, resource, policies) == iampolicy.Deny { + if iampolicy.EvaluateWithKeys(iamAction, resource, policies, keys) == iampolicy.Deny { slog.Info("evaluatePrincipalPolicy: access denied", "identity", logIdentity, "action", iamAction, "resource", resource) return errors.New(awserrors.ErrorAccessDenied) diff --git a/spinifex/gateway/policy/evaluator_test.go b/spinifex/gateway/policy/evaluator_test.go index ed03a5cfb..943e72e31 100644 --- a/spinifex/gateway/policy/evaluator_test.go +++ b/spinifex/gateway/policy/evaluator_test.go @@ -37,7 +37,7 @@ func TestEvaluate_IAMInstanceProfileActionStrings(t *testing.T) { doc("Allow", "*", "*"), } for _, a := range actions { - if got := iampolicy.Evaluate(a, "*", policies); got != iampolicy.Allow { + if got := iampolicy.EvaluateWithKeys(a, "*", policies, nil); got != iampolicy.Allow { t.Errorf("expected Allow for action %q under wildcard policy, got %v", a, got) } } @@ -60,7 +60,7 @@ func TestEvaluate_IAMInstanceProfileActionStrings(t *testing.T) { {"ec2:RunInstances", "*", iampolicy.Deny}, } for _, tt := range scopedTests { - got := iampolicy.Evaluate(tt.action, tt.resource, scoped) + got := iampolicy.EvaluateWithKeys(tt.action, tt.resource, scoped, nil) if got != tt.want { t.Errorf("scoped policy, action=%s resource=%s: expected %v, got %v", tt.action, tt.resource, tt.want, got) @@ -83,17 +83,17 @@ func TestEvaluate_STSActionStrings(t *testing.T) { scoped := []handlers_iam.PolicyDocument{doc("Allow", "sts:*", "*")} for _, a := range actions { - if got := iampolicy.Evaluate(a, "*", wildcard); got != iampolicy.Allow { + if got := iampolicy.EvaluateWithKeys(a, "*", wildcard, nil); got != iampolicy.Allow { t.Errorf("wildcard policy: expected Allow for %q, got %v", a, got) } - if got := iampolicy.Evaluate(a, "*", scoped); got != iampolicy.Allow { + if got := iampolicy.EvaluateWithKeys(a, "*", scoped, nil); got != iampolicy.Allow { t.Errorf("sts:* policy: expected Allow for %q, got %v", a, got) } } // Non-STS action must NOT match an sts:*-scoped policy — guards against a // pattern regression that would over-allow. - if got := iampolicy.Evaluate("ec2:RunInstances", "*", scoped); got != iampolicy.Deny { + if got := iampolicy.EvaluateWithKeys("ec2:RunInstances", "*", scoped, nil); got != iampolicy.Deny { t.Errorf("sts:* policy: expected Deny for ec2:RunInstances, got %v", got) } } diff --git a/spinifex/gateway/rds/authz_test.go b/spinifex/gateway/rds/authz_test.go index 8c58a09f2..4dc26076a 100644 --- a/spinifex/gateway/rds/authz_test.go +++ b/spinifex/gateway/rds/authz_test.go @@ -219,7 +219,7 @@ func TestResourceARN_ResourceScopedDenyReachesTheSnapshotActions(t *testing.T) { require.NoError(t, err) got := iampolicy.Allow for _, resource := range resources { - if iampolicy.Evaluate(policy.IAMAction("rds", tt.action), resource, policies) == iampolicy.Deny { + if iampolicy.EvaluateWithKeys(policy.IAMAction("rds", tt.action), resource, policies, nil) == iampolicy.Deny { got = iampolicy.Deny } } diff --git a/spinifex/handlers/iam/managed_policies_test.go b/spinifex/handlers/iam/managed_policies_test.go index 2fca85ba6..0ed8d86ff 100644 --- a/spinifex/handlers/iam/managed_policies_test.go +++ b/spinifex/handlers/iam/managed_policies_test.go @@ -90,7 +90,7 @@ func TestRDSManagedShims_GrantNoInternalAction(t *testing.T) { doc, ok := builtinManagedPolicyDoc(arn) require.True(t, ok, "arn %q", arn) for _, action := range rdsInternalActions { - assert.Equal(t, iampolicy.Deny, iampolicy.Evaluate(action, "*", []PolicyDocument{doc}), + assert.Equal(t, iampolicy.Deny, iampolicy.EvaluateWithKeys(action, "*", []PolicyDocument{doc}, nil), "%s must not grant %s", arn, action) } } @@ -112,7 +112,7 @@ func TestRDSFullAccessShim_GrantsTheCustomerSurface(t *testing.T) { "rds:DescribeEvents", } for _, action := range granted { - assert.Equal(t, iampolicy.Allow, iampolicy.Evaluate(action, "*", []PolicyDocument{doc}), + assert.Equal(t, iampolicy.Allow, iampolicy.EvaluateWithKeys(action, "*", []PolicyDocument{doc}, nil), "AmazonRDSFullAccess must grant %s", action) } } @@ -125,13 +125,13 @@ func TestRDSReadOnlyAccessShim_GrantsReadsOnly(t *testing.T) { for _, action := range []string{"rds:DescribeDBInstances", "rds:DescribeDBSnapshots", "rds:DescribeDBParameters", "rds:ListTagsForResource"} { - assert.Equal(t, iampolicy.Allow, iampolicy.Evaluate(action, "*", []PolicyDocument{doc}), + assert.Equal(t, iampolicy.Allow, iampolicy.EvaluateWithKeys(action, "*", []PolicyDocument{doc}, nil), "AmazonRDSReadOnlyAccess must grant %s", action) } for _, action := range []string{"rds:CreateDBInstance", "rds:DeleteDBInstance", "rds:ModifyDBInstance", "rds:StopDBInstance", "rds:AddTagsToResource", "rds:RestoreDBInstanceFromDBSnapshot"} { - assert.Equal(t, iampolicy.Deny, iampolicy.Evaluate(action, "*", []PolicyDocument{doc}), + assert.Equal(t, iampolicy.Deny, iampolicy.EvaluateWithKeys(action, "*", []PolicyDocument{doc}, nil), "AmazonRDSReadOnlyAccess must not grant %s", action) } } diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index 45e298fd3..6e40baaad 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "maps" + "net/netip" "slices" "strconv" "strings" @@ -19,6 +20,7 @@ import ( "github.com/nats-io/nats.go" "github.com/nats-io/nats.go/jetstream" + "github.com/mulgadc/bluebottle/pkg/iampolicy" "github.com/mulgadc/bluebottle/pkg/masterkey" "github.com/mulgadc/spinifex/spinifex/admin" "github.com/mulgadc/spinifex/spinifex/awserrors" @@ -2016,11 +2018,65 @@ func ValidatePolicyDocument(docJSON string) (*PolicyDocument, error) { if len(stmt.Resource) == 0 { return nil, fmt.Errorf("statement %d: Resource is required", i) } + if err := validateStatementRestrictions(i, stmt); err != nil { + return nil, err + } } return &doc, nil } +// validateStatementRestrictions rejects the clauses the evaluator cannot enforce, +// so an identity policy is never accepted with an inert restriction on it. +// Conditions inside the supported allowlist are accepted and enforced. +func validateStatementRestrictions(i int, stmt Statement) error { + if isRawJSONNonEmpty(stmt.Principal) { + return fmt.Errorf("statement %d: Principal is not valid on an identity policy; use a resource or trust policy instead", i) + } + if len(stmt.NotAction) > 0 { + return fmt.Errorf("statement %d: NotAction blocks are not supported in this release; use Action with an explicit list instead", i) + } + if len(stmt.NotResource) > 0 { + return fmt.Errorf("statement %d: NotResource blocks are not supported in this release; use Resource with an explicit list instead", i) + } + for op, keys := range stmt.Condition { + for key, values := range keys { + if !iampolicy.SupportedCondition(op, key) { + return fmt.Errorf("statement %d: Condition operator %q on key %q is not supported in this release", i, op, key) + } + if err := validateConditionValues(i, op, key, values); err != nil { + return err + } + } + } + return nil +} + +// validateConditionValues rejects leaf values the matcher can only ever compare +// false. An allowlisted operator over an unparseable value is still an inert +// restriction, which is the failure this validation exists to prevent. +func validateConditionValues(i int, op, key string, values ConditionValue) error { + if len(values) == 0 { + return fmt.Errorf("statement %d: Condition operator %q on key %q has no value", i, op, key) + } + for _, v := range values { + switch op { + case iampolicy.OpIPAddress: + if _, prefixErr := netip.ParsePrefix(v); prefixErr != nil { + if _, addrErr := netip.ParseAddr(v); addrErr != nil { + return fmt.Errorf("statement %d: Condition %s on key %q: %q is not a valid IP address or CIDR block", + i, op, key, v) + } + } + case iampolicy.OpBool: + if !strings.EqualFold(v, "true") && !strings.EqualFold(v, "false") { + return fmt.Errorf("statement %d: Condition %s on key %q: %q is not true or false", i, op, key, v) + } + } + } + return nil +} + // summaryQuotaDefaults holds the static SummaryMap entries returned by // GetAccountSummary. Spinifex's quota system (handlers/quota) only enforces // infrastructure dimensions (vCPUs, VPCs, subnets, EIPs, EBS) and models no IAM diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 53940f24c..99d2390cf 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -1551,6 +1551,108 @@ func TestValidatePolicyDocument_MissingResource(t *testing.T) { assert.Contains(t, err.Error(), "Resource is required") } +func TestValidatePolicyDocument_SupportedCondition(t *testing.T) { + doc, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*", + "Condition":{"IpAddress":{"aws:SourceIp":"10.0.0.0/8"}}}]}`) + require.NoError(t, err) + assert.Equal(t, ConditionValue{"10.0.0.0/8"}, + doc.Statement[0].Condition["IpAddress"]["aws:SourceIp"]) +} + +func TestValidatePolicyDocument_UnsupportedConditionOperator(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*", + "Condition":{"StringEquals":{"aws:SourceIp":"10.0.0.1"}}}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "is not supported in this release") +} + +func TestValidatePolicyDocument_UnsupportedConditionKey(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*", + "Condition":{"StringEquals":{"aws:PrincipalOrgID":"o-1234"}}}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "aws:PrincipalOrgID") +} + +// MFA does not exist in spinifex, so this key could never be true and accepting +// it would mint a grant that silently never fires. +func TestValidatePolicyDocument_RejectsMFACondition(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"*","Resource":"*", + "Condition":{"Bool":{"aws:MultiFactorAuthPresent":"true"}}}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "aws:MultiFactorAuthPresent") +} + +func TestValidatePolicyDocument_RejectsPrincipal(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"*","Resource":"*", + "Principal":{"AWS":"arn:aws:iam::123456789012:root"}}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "Principal is not valid on an identity policy") +} + +func TestValidatePolicyDocument_RejectsNotAction(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Action":"*","Resource":"*", + "NotAction":"sts:AssumeRole"}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "NotAction blocks are not supported") +} + +// An allowlisted operator over a malformed value is still an inert grant: the +// matcher cannot parse it, so the Allow never fires and the operator gets a bare +// AccessDenied with nothing naming the cause. +func TestValidatePolicyDocument_RejectsMalformedConditionValues(t *testing.T) { + tests := []struct { + name string + cond string + wantErr string + }{ + {"truncated CIDR", `{"IpAddress":{"aws:SourceIp":"10.0.0/8"}}`, "not a valid IP address or CIDR block"}, + {"comma string", `{"IpAddress":{"aws:SourceIp":"10.0.0.0/8, 192.168.0.0/16"}}`, "not a valid IP address or CIDR block"}, + {"hostname", `{"IpAddress":{"aws:SourceIp":"office.example.com"}}`, "not a valid IP address or CIDR block"}, + {"numeric bool", `{"Bool":{"aws:SecureTransport":1}}`, "not true or false"}, + {"yes", `{"Bool":{"aws:SecureTransport":"yes"}}`, "not true or false"}, + {"empty array", `{"IpAddress":{"aws:SourceIp":[]}}`, "has no value"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow", + "Action":"s3:*","Resource":"*","Condition":` + tt.cond + `}]}`) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +// D4's lenient leaf parsing must survive at the API surface: a native JSON bool +// is what Terraform emits, and rejecting it would fail ordinary valid policies. +func TestValidatePolicyDocument_AcceptsNativeBoolAndCIDRForms(t *testing.T) { + for _, cond := range []string{ + `{"Bool":{"aws:SecureTransport":true}}`, + `{"Bool":{"aws:SecureTransport":"TRUE"}}`, + `{"IpAddress":{"aws:SourceIp":["10.0.0.0/8","192.168.1.1"]}}`, + `{"IpAddress":{"aws:SourceIp":"2001:db8::/32"}}`, + } { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow", + "Action":"s3:*","Resource":"*","Condition":` + cond + `}]}`) + assert.NoError(t, err, "condition %s must be accepted", cond) + } +} + +// Map iteration order makes a break-instead-of-continue bug non-deterministic, +// so a single-operator document cannot pin the loop. +func TestValidatePolicyDocument_RejectsUnsupportedAlongsideSupported(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*", + "Condition":{"IpAddress":{"aws:SourceIp":"10.0.0.0/8"},"StringEquals":{"aws:PrincipalOrgID":"o-1234"}}}]}`) + require.Error(t, err) + assert.Contains(t, err.Error(), "aws:PrincipalOrgID") +} + +func TestValidatePolicyDocument_RejectsNotResource(t *testing.T) { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Action":"*","Resource":"*", + "NotResource":"arn:aws:s3:::public/*"}]}`) + assert.Error(t, err) + assert.Contains(t, err.Error(), "NotResource blocks are not supported") +} + // ============================================================================ // Sensitive Data Not Logged Tests // ============================================================================ diff --git a/spinifex/handlers/iam/types.go b/spinifex/handlers/iam/types.go index a81ba5aae..ce92b16de 100644 --- a/spinifex/handlers/iam/types.go +++ b/spinifex/handlers/iam/types.go @@ -29,6 +29,7 @@ type ( PolicyDocument = iampolicy.PolicyDocument Statement = iampolicy.Statement StringOrArr = iampolicy.StringOrArr + ConditionValue = iampolicy.ConditionValue ) // User represents an IAM user stored in JetStream KV.