From 211e2b8cfc8e125001d9dc3bbbfb92536bf626d7 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Thu, 1 Oct 2026 11:28:44 +1000 Subject: [PATCH] feat(iam): accept Null and IfExists condition operators at the write path (#1145) --- go.mod | 2 +- go.sum | 4 ++-- .../conditionkeys_completeness_test.go | 13 ++++++++++- spinifex/handlers/iam/service_impl.go | 12 ++++++---- spinifex/handlers/iam/service_impl_test.go | 23 +++++++++++++++++++ 5 files changed, 45 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 087e2eafa..b7a5f66fb 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.20261001011202-4904308df7ef + github.com/mulgadc/bluebottle v1.21.1-0.20261001012726-c8e8261ae573 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 2603c8c85..7aa50952a 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.20261001011202-4904308df7ef h1:7xjmp8lTRhxnZqviKV0tDbNcCnBwp6PGr6eNJm+ZDYs= -github.com/mulgadc/bluebottle v1.21.1-0.20261001011202-4904308df7ef/go.mod h1:LH0MuQcFxet0dDEISlZsuULIYj592eCN2Fv39A6EH5o= +github.com/mulgadc/bluebottle v1.21.1-0.20261001012726-c8e8261ae573 h1:rVlqvXN8+nA1q8MGYtKfY4GTCMgBjQZBMiHwfNjDiB0= +github.com/mulgadc/bluebottle v1.21.1-0.20261001012726-c8e8261ae573/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 eb42dc2e1..155bb95a1 100644 --- a/spinifex/gateway/conditionkeys_completeness_test.go +++ b/spinifex/gateway/conditionkeys_completeness_test.go @@ -58,6 +58,7 @@ var operatorValues = map[string]string{ iampolicy.OpIPAddress: "10.0.0.0/8", iampolicy.OpNotIPAddress: "10.0.0.0/8", iampolicy.OpBool: "true", + iampolicy.OpNull: "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", @@ -70,6 +71,16 @@ var operatorValues = map[string]string{ "DateGreaterThan": "2026-01-01T00:00:00Z", } +// withIfExists adds the IfExists form of every operator, so the gate below covers +// the suffixed operators too, NullIfExists among the ones it must reject. +func withIfExists(values map[string]string) map[string]string { + all := maps.Clone(values) + for op, value := range values { + all[op+iampolicy.IfExistsSuffix] = value + } + return all +} + // emittedKeys drives requestConditionKeys with everything a request can carry, // for both principal types, and returns the union. func emittedKeys(t *testing.T) map[string]string { @@ -145,7 +156,7 @@ func TestRequestConditionKeys_MatchesTheDoorKeySet(t *testing.T) { // policy an operator cannot write. func TestValidatePolicyDocument_AcceptsExactlyTheSupportedConditions(t *testing.T) { for _, key := range allConditionKeys { - for op, value := range operatorValues { + for op, value := range withIfExists(operatorValues) { doc := conditionDocument(t, op, key, value) _, err := handlers_iam.ValidatePolicyDocument(doc) diff --git a/spinifex/handlers/iam/service_impl.go b/spinifex/handlers/iam/service_impl.go index e98d6a44d..9b5309653 100644 --- a/spinifex/handlers/iam/service_impl.go +++ b/spinifex/handlers/iam/service_impl.go @@ -2411,17 +2411,19 @@ 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) } + // An IfExists form takes its base operator's values. + base, _ := iampolicy.BaseOperator(op) for _, v := range values { // 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: + // Bool, Null or IpAddress value is already rejected as unparseable below. + switch base { + case iampolicy.OpIPAddress, iampolicy.OpNotIPAddress, iampolicy.OpBool, iampolicy.OpNull: default: if err := validatePolicyVariables(i, fmt.Sprintf("Condition %s on key %q", op, key), v); err != nil { return err } } - switch op { + switch base { 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", @@ -2434,7 +2436,7 @@ func validateConditionValues(i int, op, key string, values ConditionValue) error i, op, key, v) } } - case iampolicy.OpBool: + case iampolicy.OpBool, iampolicy.OpNull: 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) } diff --git a/spinifex/handlers/iam/service_impl_test.go b/spinifex/handlers/iam/service_impl_test.go index 17fdf61f4..d4b286cb2 100644 --- a/spinifex/handlers/iam/service_impl_test.go +++ b/spinifex/handlers/iam/service_impl_test.go @@ -2285,6 +2285,23 @@ func TestValidatePolicyDocument_SupportedCondition(t *testing.T) { doc.Statement[0].Condition["IpAddress"]["aws:SourceIp"]) } +// Null takes a JSON boolean or its string form, and IfExists forms reach the +// write path on the keys their base operator does. +func TestValidatePolicyDocument_NullAndIfExists(t *testing.T) { + t.Parallel() + for _, cond := range []string{ + `{"Null":{"aws:username":true}}`, + `{"Null":{"s3:prefix":"false"}}`, + `{"StringEqualsIfExists":{"aws:username":"alice"}}`, + `{"NotIpAddressIfExists":{"aws:SourceIp":"10.0.0.0/8"}}`, + `{"BoolIfExists":{"aws:SecureTransport":"true"}}`, + } { + _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow", + "Action":"s3:*","Resource":"*","Condition":` + cond + `}]}`) + assert.NoError(t, err, cond) + } +} + func TestValidatePolicyDocument_UnsupportedConditionOperator(t *testing.T) { t.Parallel() _, err := ValidatePolicyDocument(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:*","Resource":"*", @@ -2395,6 +2412,12 @@ func TestValidatePolicyDocument_RejectsMalformedConditionValues(t *testing.T) { {"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"}, + {"Null yes", `{"Null":{"aws:username":"yes"}}`, "not true or false"}, + {"Null variable", `{"Null":{"aws:username":"${aws:username}"}}`, "not true or false"}, + {"IpAddressIfExists hostname", `{"IpAddressIfExists":{"aws:SourceIp":"office.example.com"}}`, "not a valid IP address or CIDR block"}, + {"BoolIfExists yes", `{"BoolIfExists":{"aws:SecureTransport":"yes"}}`, "not true or false"}, + {"StringEqualsIfExists unknown variable", `{"StringEqualsIfExists":{"aws:username":"${aws:bogus}"}}`, `references policy variable "aws:bogus"`}, + {"NullIfExists", `{"NullIfExists":{"aws:username":"true"}}`, "is not supported in this release"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {