From 2230e92ec2dbfb3cc9e22ee6bf721019477b9704 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 1 Oct 2026 10:49:51 +1000 Subject: [PATCH] feat(iam): accept negated, case-insensitive and NotIpAddress condition operators (#1143) --- go.mod | 2 +- go.sum | 4 +- .../conditionkeys_completeness_test.go | 21 ++++++--- spinifex/handlers/iam/service_impl.go | 15 +++++-- spinifex/handlers/iam/service_impl_test.go | 44 +++++++++++++++++++ 5 files changed, 73 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index ed8372bbe..d096d2cf1 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,7 @@ require ( github.com/klauspost/cpuid/v2 v2.4.0 github.com/miekg/dns v1.1.73 github.com/minio/highwayhash v1.0.4 - github.com/mulgadc/bluebottle v1.21.1-0.20260930235459-f23b29cd08db + github.com/mulgadc/bluebottle v1.21.1-0.20261001004744-156519746c8a github.com/mulgadc/northstar v1.21.0 github.com/mulgadc/predastore v1.21.0 github.com/mulgadc/viperblock v1.21.0 diff --git a/go.sum b/go.sum index 3beca0766..6f845e1fb 100644 --- a/go.sum +++ b/go.sum @@ -312,8 +312,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.21.1-0.20260930235459-f23b29cd08db h1:FyAx2PMpiCz/0xokJQOn8FWkZXyO4wKX965h000cMcs= -github.com/mulgadc/bluebottle v1.21.1-0.20260930235459-f23b29cd08db/go.mod h1:LH0MuQcFxet0dDEISlZsuULIYj592eCN2Fv39A6EH5o= +github.com/mulgadc/bluebottle v1.21.1-0.20261001004744-156519746c8a h1:Qoycvq0OWPk+KpN4Gb+dG3plkWx4AGGJOJWz0ZJsuSM= +github.com/mulgadc/bluebottle v1.21.1-0.20261001004744-156519746c8a/go.mod h1:LH0MuQcFxet0dDEISlZsuULIYj592eCN2Fv39A6EH5o= github.com/mulgadc/northstar v1.21.0 h1:LE/5kiT3kBelCg02cXa3b7awhaYkGHsOXeJ4nHRLNaQ= github.com/mulgadc/northstar v1.21.0/go.mod h1:O446cApNMB6qebqieBhDZfCIGVWX++v3QdOz+wIGhUk= github.com/mulgadc/predastore v1.21.0 h1:pAm4fFWf0vnlwRgnoL2oJ+KSCw8mtjT2rQoBHFmnjzk= diff --git a/spinifex/gateway/conditionkeys_completeness_test.go b/spinifex/gateway/conditionkeys_completeness_test.go index df296537d..eb42dc2e1 100644 --- a/spinifex/gateway/conditionkeys_completeness_test.go +++ b/spinifex/gateway/conditionkeys_completeness_test.go @@ -49,14 +49,23 @@ var allConditionKeys = []string{ // One value per operator that the leaf validator accepts, so a rejection can // only come from the operator/key allowlist and not from the value. var operatorValues = map[string]string{ - iampolicy.OpStringEquals: "alice", - iampolicy.OpStringLike: "ali*", - iampolicy.OpIPAddress: "10.0.0.0/8", - iampolicy.OpBool: "true", + iampolicy.OpStringEquals: "alice", + iampolicy.OpStringNotEquals: "alice", + iampolicy.OpStringEqualsIgnoreCase: "alice", + iampolicy.OpStringNotEqualsIgnoreCase: "alice", + iampolicy.OpStringLike: "ali*", + iampolicy.OpStringNotLike: "ali*", + iampolicy.OpIPAddress: "10.0.0.0/8", + iampolicy.OpNotIPAddress: "10.0.0.0/8", + iampolicy.OpBool: "true", + // Implemented, but no supported key is ARN-valued, so every pair below is + // rejected until one is. + iampolicy.OpArnEquals: "arn:aws:iam::000000000001:user/alice", + iampolicy.OpArnLike: "arn:aws:iam::000000000001:user/*", + iampolicy.OpArnNotEquals: "arn:aws:iam::000000000001:user/alice", + iampolicy.OpArnNotLike: "arn:aws:iam::000000000001:user/*", // Operators the evaluator does not implement. Accepting one would store a // restriction that compares false forever. - "StringNotEquals": "alice", - "ArnLike": "arn:aws:iam::000000000001:user/alice", "NumericLessThan": "3", "DateGreaterThan": "2026-01-01T00:00:00Z", } diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index 82b420112..5d604e0d4 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -2410,15 +2410,22 @@ func validateConditionValues(i int, op, key string, values ConditionValue) error return fmt.Errorf("statement %d: Condition operator %q on key %q has no value", i, op, key) } for _, v := range values { - // Only the string operators expand variables; a reference in a Bool or - // IpAddress value is already rejected as unparseable below. - if op == iampolicy.OpStringEquals || op == iampolicy.OpStringLike { + // Only the string and ARN operators expand variables; a reference in a + // Bool or IpAddress value is already rejected as unparseable below. + switch op { + case iampolicy.OpIPAddress, iampolicy.OpNotIPAddress, iampolicy.OpBool: + default: if err := validatePolicyVariables(i, fmt.Sprintf("Condition %s on key %q", op, key), v); err != nil { return err } } switch op { - case iampolicy.OpIPAddress: + case iampolicy.OpArnEquals, iampolicy.OpArnLike, iampolicy.OpArnNotEquals, iampolicy.OpArnNotLike: + if _, ok := iampolicy.SplitARN(v); !ok { + return fmt.Errorf("statement %d: Condition %s on key %q: %q is not an ARN of six colon-separated components", + i, op, key, v) + } + case iampolicy.OpIPAddress, iampolicy.OpNotIPAddress: 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", diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 6ba903785..17fdf61f4 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -9,6 +9,7 @@ import ( "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/admin" "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -2392,6 +2393,8 @@ func TestValidatePolicyDocument_RejectsMalformedConditionValues(t *testing.T) { {"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"}, + {"NotIpAddress hostname", `{"NotIpAddress":{"aws:SourceIp":"office.example.com"}}`, "not a valid IP address or CIDR block"}, + {"NotIpAddress empty array", `{"NotIpAddress":{"aws:SourceIp":[]}}`, "has no value"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2404,6 +2407,38 @@ func TestValidatePolicyDocument_RejectsMalformedConditionValues(t *testing.T) { } } +// No supported key carries an Arn operator yet, so the write path cannot reach +// this arm; it is exercised directly so the key that unlocks it lands validated. +func TestValidateConditionValues_ArnOperators(t *testing.T) { + t.Parallel() + const key = "aws:SourceArn" + tests := []struct { + name string + value string + wantErr string + }{ + {"exact ARN", "arn:aws:ecs:us-east-1:111122223333:task/c/abc", ""}, + {"wildcard components", "arn:aws:ecs:*:111122223333:*", ""}, + {"resource with colons", "arn:aws:logs:us-east-1:111122223333:log-group:app:*", ""}, + {"variable", "arn:aws:iam::${aws:PrincipalAccount}:role/*", ""}, + {"too few components", "arn:aws:ecs:*", "not an ARN of six colon-separated components"}, + {"bare wildcard", "*", "not an ARN of six colon-separated components"}, + {"unknown variable", "arn:aws:iam::${aws:bogus}:role/*", `references policy variable "aws:bogus"`}, + } + for _, op := range []string{ + iampolicy.OpArnEquals, iampolicy.OpArnLike, iampolicy.OpArnNotEquals, iampolicy.OpArnNotLike, + } { + for _, tt := range tests { + err := validateConditionValues(0, op, key, ConditionValue{tt.value}) + if tt.wantErr == "" { + assert.NoError(t, err, "%s %s", op, tt.name) + continue + } + assert.ErrorContains(t, err, tt.wantErr, "%s %s", op, tt.name) + } + } +} + // A reference the evaluator cannot resolve makes an Allow select nothing and a // Deny select everything it named, so the policy does not do what it reads as. // Rejecting it on write is the only place an operator can be told. @@ -2422,6 +2457,15 @@ func TestValidatePolicyDocument_RejectsUnresolvableVariables(t *testing.T) { {"MFA in a StringLike value", `"Action":"s3:*","Resource":"*","Condition":{"StringLike":{"s3:prefix":"${aws:MultiFactorAuthPresent}/*"}}`, `references policy variable "aws:MultiFactorAuthPresent"`}, + {"MFA in a StringNotEquals value", + `"Action":"s3:*","Resource":"*","Condition":{"StringNotEquals":{"aws:username":"${aws:MultiFactorAuthPresent}"}}`, + `references policy variable "aws:MultiFactorAuthPresent"`}, + {"MFA in a StringNotLike value", + `"Action":"s3:*","Resource":"*","Condition":{"StringNotLike":{"s3:prefix":"${aws:MultiFactorAuthPresent}/*"}}`, + `references policy variable "aws:MultiFactorAuthPresent"`}, + {"unterminated in a StringEqualsIgnoreCase value", + `"Action":"s3:*","Resource":"*","Condition":{"StringEqualsIgnoreCase":{"aws:username":"${aws:username"}}`, + "unterminated policy variable reference"}, {"unknown key", `"Action":"s3:*","Resource":"arn:aws:s3:::home/${aws:bogus}/*"`, `references policy variable "aws:bogus"`}, {"condition key that is not substitutable",