fix: scope ACM policy checks to the certificate a request names (#877)

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:<region>:<account>:certificate/<id>.
+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/<a> authorize a request naming certificate/<a>/<b>.
+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
+}

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))
+}

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

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)
+}

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)
+}

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)
+	}
+}

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())
+}

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)

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/<id>". The gate resolves that
+		// literal, so the handler must miss rather than truncate back to <id>.
+		{"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))
+}

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) {