diff --git a/spinifex/arn/acm.go b/spinifex/arn/acm.go new file mode 100644 index 000000000..60cfa0444 --- /dev/null +++ b/spinifex/arn/acm.go @@ -0,0 +1,35 @@ +package arn + +import ( + "fmt" + "strings" +) + +// ACMResourceType is the resource-type segment of an arn:aws:acm ARN. +type ACMResourceType string + +const ACMCertificate ACMResourceType = "certificate" + +// FormatACMCertificate builds arn:aws:acm:::certificate/. +func FormatACMCertificate(region, accountID, id string) string { + return fmt.Sprintf("arn:aws:acm:%s:%s:%s/%s", region, accountID, ACMCertificate, id) +} + +// ParseACMCertificateID returns the certificate id from an ACM certificate ARN. +// The region and account segments are discarded rather than validated: callers +// re-anchor on their own, so a caller-supplied anchor is never consulted. +// ok is false for any other shape. +// +// The id keeps everything after the first "/". Truncating at the last one would +// let a grant on certificate/ authorize a request naming certificate//. +func ParseACMCertificateID(certARN string) (string, bool) { + parts := strings.SplitN(certARN, ":", 6) + if len(parts) != 6 || parts[0] != "arn" || parts[2] != "acm" { + return "", false + } + kind, id, found := strings.Cut(parts[5], "/") + if !found || ACMResourceType(kind) != ACMCertificate || id == "" { + return "", false + } + return id, true +} diff --git a/spinifex/arn/acm_test.go b/spinifex/arn/acm_test.go new file mode 100644 index 000000000..4f1f4930a --- /dev/null +++ b/spinifex/arn/acm_test.go @@ -0,0 +1,98 @@ +package arn_test + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/arn" + "github.com/stretchr/testify/assert" +) + +func TestFormatACMCertificate(t *testing.T) { + assert.Equal(t, "arn:aws:acm:ap-southeast-2:123456789012:certificate/aaaa-1111", + arn.FormatACMCertificate("ap-southeast-2", "123456789012", "aaaa-1111")) +} + +func TestParseACMCertificateID(t *testing.T) { + tests := []struct { + name string + certARN string + want string + wantOK bool + }{ + { + name: "certificate ARN", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate/aaaa-1111", + want: "aaaa-1111", + wantOK: true, + }, + { + name: "another account and region still yields the id", + certARN: "arn:aws:acm:us-east-1:999999999999:certificate/aaaa-1111", + want: "aaaa-1111", + wantOK: true, + }, + { + name: "id keeps everything after the first slash", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate/aaaa-1111/admin", + want: "aaaa-1111/admin", + wantOK: true, + }, + { + name: "a literal star is an id like any other", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate/*", + want: "*", + wantOK: true, + }, + { + name: "another service", + certARN: "arn:aws:acm-pca:ap-southeast-2:123456789012:certificate-authority/aaaa-1111", + wantOK: false, + }, + { + name: "another resource type", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate-authority/aaaa-1111", + wantOK: false, + }, + { + name: "no resource component", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate", + wantOK: false, + }, + { + name: "empty id", + certARN: "arn:aws:acm:ap-southeast-2:123456789012:certificate/", + wantOK: false, + }, + { + name: "too few segments", + certARN: "arn:aws:acm:certificate/aaaa-1111", + wantOK: false, + }, + { + name: "a bare id is not an ARN", + certARN: "aaaa-1111", + wantOK: false, + }, + { + name: "empty", + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := arn.ParseACMCertificateID(tt.certARN) + assert.Equal(t, tt.wantOK, ok) + assert.Equal(t, tt.want, got) + }) + } +} + +// Re-anchoring is the composition the ACM gate relies on: an ARN naming another +// account resolves to one under the caller's own. +func TestParseACMCertificateIDReanchors(t *testing.T) { + id, ok := arn.ParseACMCertificateID("arn:aws:acm:us-east-1:999999999999:certificate/aaaa-1111") + assert.True(t, ok) + assert.Equal(t, "arn:aws:acm:ap-southeast-2:123456789012:certificate/aaaa-1111", + arn.FormatACMCertificate("ap-southeast-2", "123456789012", id)) +} diff --git a/spinifex/gateway/acm.go b/spinifex/gateway/acm.go index a6fab4e54..507d42387 100644 --- a/spinifex/gateway/acm.go +++ b/spinifex/gateway/acm.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "io" "log/slog" "net/http" "strings" @@ -88,7 +87,27 @@ func (gw *GatewayConfig) ACM_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorInvalidAction) } - if err := gw.checkPolicy(r, "acm", action); err != nil { + // Hoisted above the policy check because the resolver builds ARNs from the + // caller's account and from the same body bytes the handler unmarshals. + accountID, _ := r.Context().Value(ctxAccountID).(string) + if accountID == "" { + slog.ErrorContext(r.Context(), "ACM_Request: no account ID in auth context") + // InternalError, not ServerInternal: the policy gate used to reach this + // case first and that is the code the caller has always seen. + return errors.New(awserrors.ErrorInternalError) + } + + body, err := readBoundedBody(r) + if err != nil { + slog.ErrorContext(r.Context(), "ACM_Request: failed to read body", "err", err) + return err + } + + resources, err := gateway_acm.ResourceARNs(action, gw.Region, accountID, body) + if err != nil { + return err + } + if err := gw.checkPolicyResources(r, "acm", action, resources); err != nil { return err } @@ -96,18 +115,6 @@ func (gw *GatewayConfig) ACM_Request(w http.ResponseWriter, r *http.Request) err return errors.New(awserrors.ErrorServerInternal) } - accountID, _ := r.Context().Value(ctxAccountID).(string) - if accountID == "" { - slog.ErrorContext(r.Context(), "ACM_Request: no account ID in auth context") - return errors.New(awserrors.ErrorServerInternal) - } - - body, err := io.ReadAll(r.Body) - if err != nil { - slog.ErrorContext(r.Context(), "ACM_Request: failed to read body", "err", err) - return errors.New(awserrors.ErrorInvalidParameterValue) - } - output, err := handler(r.Context(), gw, accountID, body) if err != nil { return err diff --git a/spinifex/gateway/acm/authz.go b/spinifex/gateway/acm/authz.go new file mode 100644 index 000000000..87af5a63e --- /dev/null +++ b/spinifex/gateway/acm/authz.go @@ -0,0 +1,139 @@ +package gateway_acm + +import ( + "errors" + "log/slog" + "sort" + + "github.com/mulgadc/spinifex/spinifex/arn" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/gateway/bodyscope" +) + +// The resource a policy check evaluates against when the request names nothing +// in particular, or when the identifier cannot be resolved at gate time. +const anyResource = "*" + +// A created certificate has no id until the handler mints one. This is what AWS +// evaluates a create against, and it is a value rather than a pattern, so it +// matches a policy scoped to the type without widening a grant. +const anyCertificate = "*" + +// Where an action's certificate is named in the JSON body. +type resourceSource uint8 + +const ( + sourceAny resourceSource = iota + sourceCertificate + sourceCreateCertificate + sourceNewCertificate +) + +// Every action ACM_Request serves, with the resource AWS evaluates it against. +// Exhaustive by contract: a completeness test compares this table with the +// dispatch table in both directions, so an action cannot be added with a silent +// account-wide grant. +// +// One source per action rather than a list: ACM has a single resource type and +// no action AWS evaluates against several certificates. +var acmScopes = map[string]resourceSource{ + "DescribeCertificate": sourceCertificate, + "GetCertificate": sourceCertificate, + "DeleteCertificate": sourceCertificate, + "ListTagsForCertificate": sourceCertificate, + "AddTagsToCertificate": sourceCertificate, + "RemoveTagsFromCertificate": sourceCertificate, + + // CertificateArn is optional: present means re-import into an existing + // certificate, absent means create a new one. + "ImportCertificate": sourceNewCertificate, + + // Issues a certificate; no id exists at gate time. RequestCertificateInput + // carries no CertificateArn, so the body must not be read for one: a field + // the handler discards cannot choose what the request is checked against. + "RequestCertificate": sourceCreateCertificate, + + // Account-level. AWS documents no resource type for the certificate list. + "ListCertificates": sourceAny, +} + +// HasScope reports whether action has an explicit ACM scope-table entry. +// acmActions lives in package gateway, so the completeness test calls this +// rather than reaching into the table. +func HasScope(action string) bool { + _, ok := acmScopes[action] + return ok +} + +// ScopedActions returns every action represented in the ACM scope table. +func ScopedActions() []string { + actions := make([]string, 0, len(acmScopes)) + for action := range acmScopes { + actions = append(actions, action) + } + sort.Strings(actions) + return actions +} + +// ResourceARNs resolves the resources an ACM request authorizes against from the +// same body bytes the handler will unmarshal. The region and account a +// caller-supplied CertificateArn carries are ignored: the handler only ever acts +// on a certificate in the caller's own account, and honouring the caller's +// anchor would let a request slide out from under a Deny scoped to the real one. +func ResourceARNs(action, region, accountID string, body []byte) ([]string, error) { + source, ok := acmScopes[action] + if !ok { + slog.Error("ACM authz: action is served but absent from the scope table", "action", action) + return nil, errors.New(awserrors.ErrorInvalidAction) + } + + scope, err := bodyscope.Parse(action, body) + if err != nil { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + switch source { + case sourceAny: + return []string{anyResource}, nil + + case sourceCreateCertificate: + return []string{newCertificateARN(region, accountID)}, nil + + case sourceCertificate: + return []string{certificateARN(region, accountID, scope.String("certificateArn"))}, nil + + case sourceNewCertificate: + // An absent identifier means the request creates a certificate, so the + // create form applies rather than the account-wide fallback. + if !scope.Has("certificateArn") { + return []string{newCertificateARN(region, accountID)}, nil + } + return []string{certificateARN(region, accountID, scope.String("certificateArn"))}, nil + + default: + slog.Error("ACM authz: unhandled resource source, failing closed", "action", action, "source", source) + return nil, errors.New(awserrors.ErrorInternalError) + } +} + +// certificateARN re-anchors the caller-supplied ARN on gw.Region and the +// caller's account. An absent or unreadable identifier authorizes account-wide, +// so a malformed request stays the handler's validation fault rather than +// becoming an authorization failure. +func certificateARN(region, accountID, certARN string) string { + if region == "" || accountID == "" { + return anyResource + } + id, ok := arn.ParseACMCertificateID(certARN) + if !ok { + return anyResource + } + return arn.FormatACMCertificate(region, accountID, id) +} + +func newCertificateARN(region, accountID string) string { + if region == "" || accountID == "" { + return anyResource + } + return arn.FormatACMCertificate(region, accountID, anyCertificate) +} diff --git a/spinifex/gateway/acm/authz_test.go b/spinifex/gateway/acm/authz_test.go new file mode 100644 index 000000000..86b70cd4a --- /dev/null +++ b/spinifex/gateway/acm/authz_test.go @@ -0,0 +1,133 @@ +package gateway_acm_test + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_acm "github.com/mulgadc/spinifex/spinifex/gateway/acm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testRegion = "ap-southeast-2" + testAccountID = "123456789012" +) + +func certARN(id string) string { + return "arn:aws:acm:" + testRegion + ":" + testAccountID + ":certificate/" + id +} + +func resolve(t *testing.T, action, body string) []string { + t.Helper() + resources, err := gateway_acm.ResourceARNs(action, testRegion, testAccountID, []byte(body)) + require.NoError(t, err) + return resources +} + +// The ARN handed to the evaluator names the certificate the handler resolves. +// ACM has one resource class, so this is the whole fidelity surface. +func TestResourceARNsFidelity(t *testing.T) { + tests := []struct { + action string + body string + want []string + }{ + {"DescribeCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + {"GetCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + {"DeleteCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + {"ListTagsForCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + {"AddTagsToCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + {"RemoveTagsFromCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + + // AWS JSON 1.1 spells the field lower-camel; the SDK struct spells it + // upper-camel. Both name the same certificate. + {"DescribeCertificate", `{"certificateArn":"` + certARN("aaaa-1111") + `"}`, []string{certARN("aaaa-1111")}}, + + // Creates resolve the type, which is what AWS evaluates them against. + {"RequestCertificate", `{"DomainName":"example.com"}`, []string{certARN("*")}}, + + // RequestCertificateInput carries no CertificateArn, so one in the body + // is discarded by the handler and must not steer the resource the + // request is checked against. + {"RequestCertificate", `{"DomainName":"example.com","CertificateArn":"` + certARN("prod-1111") + `"}`, []string{certARN("*")}}, + {"ImportCertificate", `{"Certificate":"pem"}`, []string{certARN("*")}}, + + // A re-import names the certificate it replaces. + {"ImportCertificate", `{"CertificateArn":"` + certARN("aaaa-1111") + `","Certificate":"pem"}`, []string{certARN("aaaa-1111")}}, + + // Account-level: AWS documents no resource type for the list. + {"ListCertificates", `{}`, []string{"*"}}, + } + + for _, tt := range tests { + t.Run(tt.action+" "+tt.body, func(t *testing.T) { + assert.Equal(t, tt.want, resolve(t, tt.action, tt.body)) + }) + } +} + +// A caller-supplied region or account would let a request slide out from under +// a Deny scoped to the real one. The handler only ever acts on a certificate in +// the caller's own account, so the ARN is rebuilt under it. +func TestResourceARNsIgnoresTheCallersAnchor(t *testing.T) { + foreign := `{"CertificateArn":"arn:aws:acm:us-east-1:999999999999:certificate/aaaa-1111"}` + + assert.Equal(t, []string{certARN("aaaa-1111")}, resolve(t, "DeleteCertificate", foreign)) +} + +// An absent identifier authorizes account-wide, so a malformed request stays the +// handler's validation fault rather than becoming an authorization failure. +func TestResourceARNsAbsentIdentifier(t *testing.T) { + for _, body := range []string{`{}`, ``, `{"CertificateArn":""}`, `{"CertificateArn":"not-an-arn"}`} { + assert.Equal(t, []string{"*"}, resolve(t, "DeleteCertificate", body), "body %q", body) + } +} + +// A body the gate cannot parse resolves account-wide for the same reason. +func TestResourceARNsUnparseableBody(t *testing.T) { + assert.Equal(t, []string{"*"}, resolve(t, "DeleteCertificate", "{not json")) +} + +// A body carrying two spellings of one field is rejected: the gate and the +// handler would otherwise name different certificates. +func TestResourceARNsAmbiguousBody(t *testing.T) { + _, err := gateway_acm.ResourceARNs("DeleteCertificate", testRegion, testAccountID, + []byte(`{"CertificateArn":"`+certARN("aaaa-1111")+`","certificateArn":"`+certARN("bbbb-2222")+`"}`)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) +} + +// The id is a value, not a pattern. An id that is literally "*" builds an ARN +// ending "/*", which neither matches a scoped Deny nor widens a grant, and an id +// containing "/" keeps it rather than being truncated. +func TestResourceARNsIdentifierIsAValue(t *testing.T) { + assert.Equal(t, []string{certARN("*")}, + resolve(t, "DeleteCertificate", `{"CertificateArn":"`+certARN("*")+`"}`)) + assert.Equal(t, []string{certARN("aaaa-1111/admin")}, + resolve(t, "DeleteCertificate", `{"CertificateArn":"`+certARN("aaaa-1111/admin")+`"}`)) +} + +// An action absent from the dispatch table cannot reach the resolver, but if one +// ever did it fails closed rather than authorizing account-wide. +func TestResourceARNsUnknownAction(t *testing.T) { + _, err := gateway_acm.ResourceARNs("BogusAction", testRegion, testAccountID, []byte(`{}`)) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidAction, err.Error()) +} + +// Without a region or an account there is no ARN to build, and every action +// falls back to the account-wide resource. +func TestResourceARNsWithoutAnAnchor(t *testing.T) { + for _, action := range gateway_acm.ScopedActions() { + resources, err := gateway_acm.ResourceARNs(action, "", "", []byte(`{"CertificateArn":"`+certARN("aaaa-1111")+`"}`)) + require.NoError(t, err) + assert.Equal(t, []string{"*"}, resources, "action %q", action) + } +} + +func TestHasScope(t *testing.T) { + assert.True(t, gateway_acm.HasScope("DeleteCertificate")) + assert.False(t, gateway_acm.HasScope("BogusAction")) + assert.Len(t, gateway_acm.ScopedActions(), 9) +} diff --git a/spinifex/gateway/acm_authz_completeness_test.go b/spinifex/gateway/acm_authz_completeness_test.go new file mode 100644 index 000000000..e9517e581 --- /dev/null +++ b/spinifex/gateway/acm_authz_completeness_test.go @@ -0,0 +1,27 @@ +//test:in-package — acmActions is unexported here, and the completeness test +// exists to compare it against the scope table. + +package gateway + +import ( + "testing" + + gateway_acm "github.com/mulgadc/spinifex/spinifex/gateway/acm" + "github.com/stretchr/testify/assert" +) + +// TestACMScopeTableIsExhaustive is what stops the next ACM action being added +// with a silent account-wide grant. It asserts both directions, so a scope left +// behind by a deleted or renamed action fails too. +func TestACMScopeTableIsExhaustive(t *testing.T) { + for action := range acmActions { + assert.True(t, gateway_acm.HasScope(action), + "acm action %q has no resource scope entry: add one to acmScopes in gateway/acm/authz.go", action) + } + + for _, action := range gateway_acm.ScopedActions() { + _, ok := acmActions[action] + assert.True(t, ok, + "acmScopes has an entry for %q, which the dispatch table does not serve: remove it from gateway/acm/authz.go", action) + } +} diff --git a/spinifex/gateway/acm_authz_test.go b/spinifex/gateway/acm_authz_test.go new file mode 100644 index 000000000..342daf073 --- /dev/null +++ b/spinifex/gateway/acm_authz_test.go @@ -0,0 +1,191 @@ +//test:in-package — drives ACM_Request through the gateway's unexported test +// helpers (setupACMRequest, policyMockIAMService) and auth context keys. + +package gateway + +import ( + "context" + "net/http/httptest" + "strings" + "testing" + + "github.com/mulgadc/bluebottle/pkg/sigv4" + "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_acm "github.com/mulgadc/spinifex/spinifex/gateway/acm" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func acmCertARN(id string) string { + return "arn:aws:acm:" + authzRegion + ":" + authzAccountID + ":certificate/" + id +} + +// dispatchACM drives the gateway with no NATS connection. A permitted request +// therefore fails at the NATS-availability guard rather than on authorization, +// which is what proves the policy check ran ahead of the certificate existing. +func dispatchACM(t *testing.T, gw *GatewayConfig, action, body string) error { + t.Helper() + return gw.ACM_Request(httptest.NewRecorder(), setupACMRequest("CertificateManager."+action, body)) +} + +// TestACMRequest_ScopedDenyFires is the bypass this work closes. An operator +// fences a production certificate; before the resolver the fence was inert and +// DeleteCertificate against it was permitted with nothing logged. +func TestACMRequest_ScopedDenyFires(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:*", "*"), + statement("Deny", "acm:DeleteCertificate", acmCertARN("prod-1111")), + ) + + assertDenied(t, dispatchACM(t, gw, "DeleteCertificate", `{"CertificateArn":"`+acmCertARN("prod-1111")+`"}`)) + assertPermitted(t, dispatchACM(t, gw, "DeleteCertificate", `{"CertificateArn":"`+acmCertARN("dev-2222")+`"}`)) +} + +// TestACMRequest_ScopedAllowGrants is the other half: a least-privilege policy +// used to deny everything, so the only working policy shape was Resource "*". +// The sibling is denied with AccessDenied and not a ResourceNotFound, which is +// what proves the gate runs ahead of existence. +func TestACMRequest_ScopedAllowGrants(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:DescribeCertificate", acmCertARN("dev-2222")), + ) + + assertPermitted(t, dispatchACM(t, gw, "DescribeCertificate", `{"CertificateArn":"`+acmCertARN("dev-2222")+`"}`)) + assertDenied(t, dispatchACM(t, gw, "DescribeCertificate", `{"CertificateArn":"`+acmCertARN("prod-1111")+`"}`)) +} + +// A caller-supplied region or account in the ARN would let a request slide out +// from under a Deny scoped to the real one. +func TestACMRequest_CallerSuppliedAnchorIsIgnored(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:*", "*"), + statement("Deny", "acm:DeleteCertificate", acmCertARN("prod-1111")), + ) + + assertDenied(t, dispatchACM(t, gw, "DeleteCertificate", + `{"CertificateArn":"arn:aws:acm:us-east-1:999999999999:certificate/prod-1111"}`)) +} + +// ImportCertificate creates unless it names a certificate to replace, so a grant +// on the type covers the create and a fence on one certificate covers only the +// re-import that names it. +func TestACMRequest_ImportResolvesBothShapes(t *testing.T) { + create := scopedPolicyGateway( + statement("Allow", "acm:ImportCertificate", acmCertARN("*")), + ) + assertPermitted(t, dispatchACM(t, create, "ImportCertificate", `{"Certificate":"pem"}`)) + + fenced := scopedPolicyGateway( + statement("Allow", "acm:*", "*"), + statement("Deny", "acm:ImportCertificate", acmCertARN("prod-1111")), + ) + assertDenied(t, dispatchACM(t, fenced, "ImportCertificate", + `{"CertificateArn":"`+acmCertARN("prod-1111")+`","Certificate":"pem"}`)) + assertPermitted(t, dispatchACM(t, fenced, "ImportCertificate", `{"Certificate":"pem"}`)) +} + +// RequestCertificate has no id at gate time, so it resolves the type. +func TestACMRequest_RequestCertificateResolvesTheType(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:RequestCertificate", acmCertARN("*")), + ) + assertPermitted(t, dispatchACM(t, gw, "RequestCertificate", `{"DomainName":"example.com"}`)) + + scoped := scopedPolicyGateway( + statement("Allow", "acm:RequestCertificate", acmCertARN("prod-1111")), + ) + assertDenied(t, dispatchACM(t, scoped, "RequestCertificate", `{"DomainName":"example.com"}`)) +} + +// A grant scoped to one certificate does not become a licence to mint new ones +// by naming that certificate in a field RequestCertificate discards. +func TestACMRequest_RequestCertificateIgnoresACertificateArnInTheBody(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:*", acmCertARN("team-a-1")), + ) + + assertDenied(t, dispatchACM(t, gw, "RequestCertificate", + `{"DomainName":"evil.example","CertificateArn":"`+acmCertARN("team-a-1")+`"}`)) +} + +// ListCertificates is account-level, so a grant scoped to one certificate does +// not reach it. +func TestACMRequest_ListIsAccountLevel(t *testing.T) { + scoped := scopedPolicyGateway( + statement("Allow", "acm:ListCertificates", acmCertARN("dev-2222")), + ) + assertDenied(t, dispatchACM(t, scoped, "ListCertificates", `{}`)) + + wide := scopedPolicyGateway(statement("Allow", "acm:ListCertificates", "*")) + assertPermitted(t, dispatchACM(t, wide, "ListCertificates", `{}`)) +} + +// The property the per-service rollout rests on: every policy that works today +// carries Resource "*", and passing a real ARN cannot turn it into a denial. +func TestACMRequest_WildcardPolicyStillPermitsEveryAction(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "acm:*", "*")) + + for _, action := range gateway_acm.ScopedActions() { + t.Run(action, func(t *testing.T) { + assertPermitted(t, dispatchACM(t, gw, action, `{"CertificateArn":"`+acmCertARN("aaaa-1111")+`"}`)) + }) + } +} + +// An absent identifier authorizes account-wide, so the handler still reports its +// own validation fault rather than the gate converting it into a denial. +func TestACMRequest_AbsentIdentifierStaysTheHandlersFault(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:*", "*"), + statement("Deny", "acm:DeleteCertificate", acmCertARN("prod-1111")), + ) + + assertPermitted(t, dispatchACM(t, gw, "DeleteCertificate", `{}`)) +} + +// A body the gate cannot parse authorizes account-wide for the same reason. +func TestACMRequest_UnparseableBodyStaysTheHandlersFault(t *testing.T) { + gw := scopedPolicyGateway( + statement("Allow", "acm:*", "*"), + statement("Deny", "acm:DeleteCertificate", acmCertARN("prod-1111")), + ) + + assertPermitted(t, dispatchACM(t, gw, "DeleteCertificate", "{not json")) +} + +// A body spelling one field two ways is rejected rather than authorized against +// whichever spelling the case fold happened to keep. +func TestACMRequest_FieldSpelledTwoWaysIsRejected(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "acm:*", "*")) + + // Repeated: the fold's disagreement is random, the rejection must not be. + for range 50 { + err := dispatchACM(t, gw, "DeleteCertificate", + `{"CertificateArn":"`+acmCertARN("dev-2222")+`","certificateArn":"`+acmCertARN("prod-1111")+`"}`) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + } +} + +// A body past the signed path's cap cannot be used to bypass the gate or to +// exhaust memory: it is rejected before either the gate or the handler. +func TestACMRequest_OversizedBodyIsRejected(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "acm:*", "*")) + body := `{"CertificateArn":"` + strings.Repeat("a", sigv4.MaxPayloadLen) + `"}` + + err := dispatchACM(t, gw, "DeleteCertificate", body) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorRequestEntityTooLarge, err.Error()) +} + +// The account-ID guard moved above the policy gate, which used to reach its own +// InternalError first. That is the code the caller has always seen. +func TestACMRequest_MissingAccountID(t *testing.T) { + gw := scopedPolicyGateway(statement("Allow", "acm:*", "*")) + req := setupACMRequest("CertificateManager.ListCertificates", `{}`) + req = req.WithContext(context.WithValue(req.Context(), ctxAccountID, "")) + + err := gw.ACM_Request(httptest.NewRecorder(), req) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInternalError, err.Error()) +} diff --git a/spinifex/handlers/acm/service_impl.go b/spinifex/handlers/acm/service_impl.go index 350ef196a..2d02f8ad5 100644 --- a/spinifex/handlers/acm/service_impl.go +++ b/spinifex/handlers/acm/service_impl.go @@ -17,6 +17,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/acm" + "github.com/mulgadc/spinifex/spinifex/arn" "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/config" "github.com/nats-io/nats.go" @@ -186,7 +187,7 @@ func (s *ACMServiceImpl) northstarHostsAll(domains []string) bool { // mintCertificateArn generates an ACM-style certificate ARN for accountID. func (s *ACMServiceImpl) mintCertificateArn(accountID string) string { - return fmt.Sprintf("arn:aws:acm:%s:%s:certificate/%s", s.region, accountID, uuid.NewV4().String()) + return arn.FormatACMCertificate(s.region, accountID, uuid.NewV4().String()) } // ImportCertificate validates the PEM material, parses the leaf for metadata, @@ -213,7 +214,11 @@ func (s *ACMServiceImpl) ImportCertificate(ctx context.Context, input *acm.Impor if certArn == "" { certArn = s.mintCertificateArn(accountID) } else { - // Re-import: the ARN must already exist and belong to the caller. + // Re-import: the ARN must be one the gate can read, and must already + // exist and belong to the caller. + if _, ok := arn.ParseACMCertificateID(certArn); !ok { + return nil, errors.New(awserrors.ErrorACMInvalidArn) + } existing, gErr := s.store.GetCert(ctx, certArn) if gErr != nil { return nil, errors.New(awserrors.ErrorInternalError) @@ -546,6 +551,12 @@ func (s *ACMServiceImpl) DeleteCertificate(ctx context.Context, input *acm.Delet // ownership check must not decrypt one. A caller that does need the key // fetches it explicitly via store.GetCert once ownership is established. func (s *ACMServiceImpl) lookupOwned(ctx context.Context, certArn, accountID string) (*CertRecord, error) { + // Anything the policy gate cannot read as a certificate ARN it authorizes + // account-wide, so accepting one here would act on a certificate no + // statement named. + if _, ok := arn.ParseACMCertificateID(certArn); !ok { + return nil, errors.New(awserrors.ErrorACMInvalidArn) + } rec, err := s.store.GetCertMetadata(ctx, certArn) if err != nil { return nil, errors.New(awserrors.ErrorInternalError) diff --git a/spinifex/handlers/acm/service_impl_test.go b/spinifex/handlers/acm/service_impl_test.go index 72c67324a..4da86d842 100644 --- a/spinifex/handlers/acm/service_impl_test.go +++ b/spinifex/handlers/acm/service_impl_test.go @@ -19,6 +19,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/acm" + "github.com/mulgadc/spinifex/spinifex/arn" "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -647,3 +648,71 @@ func TestSensitiveDataNotLogged_ACMPrivateKey(t *testing.T) { assert.NotContains(t, buf.String(), string(keyPEM), "raw private key PEM must not appear in log output") } + +// The gate resolves an ARN it cannot read to "*", so the handler must not act +// on one either: every spelling below names a real certificate under certKey's +// old last-slash rule while resolving account-wide at the gate. +func TestCertificateArnSpellingsTheGateCannotRead(t *testing.T) { + svc := setupACMService(t) + certPEM, keyPEM := genCert(t, "fenced.example.com") + + out, err := svc.ImportCertificate(context.Background(), &acm.ImportCertificateInput{ + Certificate: certPEM, + PrivateKey: keyPEM, + }, testAccountID) + require.NoError(t, err) + realArn := aws.StringValue(out.CertificateArn) + id := realArn[strings.LastIndex(realArn, "/")+1:] + + spellings := []struct { + certArn string + want string + }{ + // Not a certificate ARN at all: the gate reads none of these. + {id, awserrors.ErrorACMInvalidArn}, + {"arn:aws:acm:ap-southeast-2:" + testAccountID + ":cert/" + id, awserrors.ErrorACMInvalidArn}, + {"arn:aws:iam::" + testAccountID + ":certificate/" + id, awserrors.ErrorACMInvalidArn}, + + // A well-formed ARN whose id is "other/". The gate resolves that + // literal, so the handler must miss rather than truncate back to . + {"arn:aws:acm:ap-southeast-2:" + testAccountID + ":certificate/other/" + id, awserrors.ErrorResourceNotFound}, + } + + for _, tt := range spellings { + t.Run(tt.certArn, func(t *testing.T) { + _, err := svc.DescribeCertificate(context.Background(), + &acm.DescribeCertificateInput{CertificateArn: aws.String(tt.certArn)}, testAccountID) + require.Error(t, err) + assert.Equal(t, tt.want, err.Error()) + + _, err = svc.DeleteCertificate(context.Background(), + &acm.DeleteCertificateInput{CertificateArn: aws.String(tt.certArn)}, testAccountID) + require.Error(t, err) + assert.Equal(t, tt.want, err.Error()) + + _, err = svc.ImportCertificate(context.Background(), &acm.ImportCertificateInput{ + CertificateArn: aws.String(tt.certArn), + Certificate: certPEM, + PrivateKey: keyPEM, + }, testAccountID) + require.Error(t, err) + assert.Equal(t, tt.want, err.Error()) + }) + } + + // The certificate the fence names is still there. + _, err = svc.DescribeCertificate(context.Background(), + &acm.DescribeCertificateInput{CertificateArn: aws.String(realArn)}, testAccountID) + require.NoError(t, err) +} + +// The handler mints the ARN the gate builds, or every resource-scoped statement +// names an object that does not exist. +func TestMintedArnRoundTripsThroughTheGatesFormatter(t *testing.T) { + svc := setupACMService(t) + minted := svc.mintCertificateArn(testAccountID) + + id, ok := arn.ParseACMCertificateID(minted) + require.True(t, ok) + assert.Equal(t, minted, arn.FormatACMCertificate(svc.region, testAccountID, id)) +} diff --git a/spinifex/handlers/acm/store.go b/spinifex/handlers/acm/store.go index 3b463dcb2..e0136e7f7 100644 --- a/spinifex/handlers/acm/store.go +++ b/spinifex/handlers/acm/store.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/mulgadc/spinifex/spinifex/arn" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" "github.com/mulgadc/spinifex/spinifex/kvutil" "github.com/nats-io/nats.go" @@ -188,11 +189,15 @@ func NewStore(ctx context.Context, nc *nats.Conn, masterKey []byte) (*Store, err return &Store{kv: kv, masterKey: masterKey}, nil } -// certKey derives the KV key from a certificate ARN using the UUID after "certificate/". +// certKey derives the KV key from a certificate ARN, reading the id exactly as +// the policy gate reads it. Anything else yields "", which every caller treats +// as absent: the gate authorizes an ARN it cannot read account-wide, so a +// lookup accepting a shape the gate rejects would reach a certificate that no +// statement named. func certKey(certArn string) string { - id := certArn - if i := strings.LastIndex(certArn, "/"); i >= 0 { - id = certArn[i+1:] + id, ok := arn.ParseACMCertificateID(certArn) + if !ok { + return "" } return KeyPrefixCert + id } @@ -215,7 +220,11 @@ func (s *Store) PutCert(ctx context.Context, rec *CertRecord) error { if err != nil { return fmt.Errorf("marshal cert: %w", err) } - _, err = s.kv.Put(ctx, certKey(rec.CertificateArn), data) + key := certKey(rec.CertificateArn) + if key == "" { + return fmt.Errorf("acm store: not a certificate ARN: %q", rec.CertificateArn) + } + _, err = s.kv.Put(ctx, key, data) return err } @@ -248,7 +257,11 @@ func (s *Store) GetCertMetadata(ctx context.Context, certArn string) (*CertRecor // caller cannot mistake ciphertext for usable key material in a field typed as // plaintext PEM everywhere else in this package. func (s *Store) getCert(ctx context.Context, certArn string, decrypt bool) (*CertRecord, error) { - entry, err := s.kv.Get(ctx, certKey(certArn)) + key := certKey(certArn) + if key == "" { + return nil, nil + } + entry, err := s.kv.Get(ctx, key) if err != nil { if errors.Is(err, jetstream.ErrKeyNotFound) { return nil, nil @@ -317,13 +330,17 @@ func (s *Store) decryptPrivateKey(rec *CertRecord) error { // DeleteCert removes a certificate by ARN. Returns (false, nil) when absent. func (s *Store) DeleteCert(ctx context.Context, certArn string) (bool, error) { - if _, err := s.kv.Get(ctx, certKey(certArn)); err != nil { + key := certKey(certArn) + if key == "" { + return false, nil + } + if _, err := s.kv.Get(ctx, key); err != nil { if errors.Is(err, jetstream.ErrKeyNotFound) { return false, nil } return false, err } - if err := s.kv.Delete(ctx, certKey(certArn)); err != nil { + if err := s.kv.Delete(ctx, key); err != nil { return false, err } return true, nil @@ -460,6 +477,9 @@ func (s *Store) RemoveInUseBy(ctx context.Context, certArn, resourceArn string) // nil (no-op, no retry) if the certificate does not exist. func (s *Store) updateInUseByCAS(ctx context.Context, certArn string, mutate func(cur []string) []string) error { key := certKey(certArn) + if key == "" { + return nil + } for attempt := range maxInUseByCASAttempts { entry, err := s.kv.Get(ctx, key) if err != nil { @@ -520,6 +540,9 @@ func (s *Store) updateInUseByCAS(ctx context.Context, certArn string, mutate fun // holder finish or the lease expire. func (s *Store) AcquireLease(ctx context.Context, certArn, holderID string, ttl time.Duration, now time.Time) (bool, error) { key := certKey(certArn) + if key == "" { + return false, nil + } entry, err := s.kv.Get(ctx, key) if err != nil { if errors.Is(err, jetstream.ErrKeyNotFound) { @@ -557,6 +580,9 @@ func (s *Store) AcquireLease(ctx context.Context, certArn, holderID string, ttl // the lease for LeaseExpiresAt to reap. func (s *Store) ReleaseLease(ctx context.Context, certArn, holderID string) error { key := certKey(certArn) + if key == "" { + return nil + } entry, err := s.kv.Get(ctx, key) if err != nil { if errors.Is(err, jetstream.ErrKeyNotFound) {