From 5ebbdcb533e2f1093fe127c61e32d78915f1de56 Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Fri, 2 Oct 2026 09:18:48 +1000 Subject: [PATCH] refactor: compose ecr authorization token action --- .../ecr/awsapi/authorization_token_actions.go | 48 ++++++++++++++++ spinifex/gateway/ecr_getauthorizationtoken.go | 55 ------------------- .../gateway/ecr_getauthorizationtoken_test.go | 44 +++++++++++---- spinifex/gateway/ecr_principal.go | 24 ++++++++ spinifex/gateway/ecr_repository.go | 17 ------ spinifex/gateway/ecrapi.go | 34 ++++++------ .../gateway/ecrapi_authz_completeness_test.go | 20 +++---- spinifex/gateway/gateway.go | 3 + spinifex/gateway/operations.go | 5 +- spinifex/services/awsgw/awsgw.go | 4 +- 10 files changed, 139 insertions(+), 115 deletions(-) create mode 100644 spinifex/domains/ecr/awsapi/authorization_token_actions.go delete mode 100644 spinifex/gateway/ecr_getauthorizationtoken.go delete mode 100644 spinifex/gateway/ecr_repository.go diff --git a/spinifex/domains/ecr/awsapi/authorization_token_actions.go b/spinifex/domains/ecr/awsapi/authorization_token_actions.go new file mode 100644 index 000000000..218be46e0 --- /dev/null +++ b/spinifex/domains/ecr/awsapi/authorization_token_actions.go @@ -0,0 +1,48 @@ +package awsapi + +import ( + "errors" + + "github.com/aws/aws-sdk-go/service/ecr" + ecrauth "github.com/mulgadc/spinifex/spinifex/domains/ecr/auth" + "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" +) + +// AuthorizationTokenActionService composes the credential issuer and advertised +// registry endpoint used by GetAuthorizationToken. Gateway retains canonical +// IAM/STS principal construction before calling this service; ECR owns token +// mint response projection. +type AuthorizationTokenActionService struct { + issuer AuthorizationTokenIssuer + endpoint RepositoryEndpoint +} + +// NewAuthorizationTokenActionService binds the issuer and endpoint profile for +// GetAuthorizationToken dispatch. +func NewAuthorizationTokenActionService(issuer AuthorizationTokenIssuer, endpoint RepositoryEndpoint) *AuthorizationTokenActionService { + return &AuthorizationTokenActionService{issuer: issuer, endpoint: endpoint} +} + +var authorizationTokenActionNames = []string{"GetAuthorizationToken"} + +// AuthorizationTokenActionNames returns the ECR actions served by +// AuthorizationTokenActionService. The copy keeps the coverage inventory from +// changing the service's dispatch set. +func AuthorizationTokenActionNames() []string { + return append([]string(nil), authorizationTokenActionNames...) +} + +// IsAuthorizationTokenAction reports whether action is handled by the +// composed authorization-token capability. +func IsAuthorizationTokenAction(action string) bool { + return action == "GetAuthorizationToken" +} + +// MintFor returns the AWS GetAuthorizationToken result for a canonical +// gateway-authenticated principal. +func (s *AuthorizationTokenActionService) MintFor(principal ecrauth.Principal) (*ecr.GetAuthorizationTokenOutput, error) { + if s == nil { + return nil, errors.New(awserrors.ErrorServerInternal) + } + return GetAuthorizationToken(s.issuer, s.endpoint, principal) +} diff --git a/spinifex/gateway/ecr_getauthorizationtoken.go b/spinifex/gateway/ecr_getauthorizationtoken.go deleted file mode 100644 index a07d8ad88..000000000 --- a/spinifex/gateway/ecr_getauthorizationtoken.go +++ /dev/null @@ -1,55 +0,0 @@ -package gateway - -import ( - "errors" - "log/slog" - "net/http" - - ecrauth "github.com/mulgadc/spinifex/spinifex/domains/ecr/auth" - awsapi "github.com/mulgadc/spinifex/spinifex/domains/ecr/awsapi" - "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" -) - -// handleGetAuthorizationToken adapts an authenticated gateway identity into -// the canonical ECR credential principal. The AWS token response is owned by -// the ECR action adapter, while canonical IAM/STS ARN construction remains a -// generic gateway identity concern. -func (gw *GatewayConfig) handleGetAuthorizationToken(w http.ResponseWriter, r *http.Request) error { - if gw.ECRTokenIssuer == nil { - return errors.New(awserrors.ErrorNotImplemented) - } - ctx := r.Context() - accountID, _ := ctx.Value(ctxAccountID).(string) - if accountID == "" { - slog.Error("GetAuthorizationToken: no account ID in auth context") - return errors.New(awserrors.ErrorServerInternal) - } - accessKey, _ := ctx.Value(ctxAccessKey).(string) - identity, _ := ctx.Value(ctxIdentity).(string) - principalType, _ := ctx.Value(ctxPrincipalType).(string) - assumedRoleARN, _ := ctx.Value(ctxAssumedRoleARN).(string) - - // The minted token names the exact IAM/STS record every /v2/* request will - // later rehydrate against, so its subject must be the same canonical ARN - // buildCallerARN produces everywhere else in the gateway (STS included) — - // not a best-effort approximation that could omit an IAM path. - callerARN, err := buildCallerARN(accountID, identity, principalType, assumedRoleARN) - if err != nil { - slog.Error("GetAuthorizationToken: cannot build canonical caller ARN", "err", err) - return errors.New(awserrors.ErrorServerInternal) - } - - output, err := awsapi.GetAuthorizationToken(gw.ECRTokenIssuer, gw.ecrRepositoryEndpoint(), ecrauth.Principal{ - AccountID: accountID, - ARN: callerARN, - Type: principalType, - AccessKeyID: accessKey, - }) - if err != nil { - slog.Error("GetAuthorizationToken: action failed", "err", err) - return err - } - - awsapi.WriteJSONResponse(w, output) - return nil -} diff --git a/spinifex/gateway/ecr_getauthorizationtoken_test.go b/spinifex/gateway/ecr_getauthorizationtoken_test.go index 2079d3fab..f93e1133f 100644 --- a/spinifex/gateway/ecr_getauthorizationtoken_test.go +++ b/spinifex/gateway/ecr_getauthorizationtoken_test.go @@ -9,16 +9,19 @@ import ( "strings" "testing" + awsapi "github.com/mulgadc/spinifex/spinifex/domains/ecr/awsapi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestHandleGetAuthorizationToken_MintsUsableToken(t *testing.T) { +func TestGetAuthorizationToken_MintsUsableToken(t *testing.T) { iss, verify := newECRAuth(t) + endpoint := awsapi.RepositoryEndpoint{Region: ecrTestRegion, ServicesDomain: ecrTestSuffix} gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, - IAMService: allowAllIAMService(), + ECRTokenAction: awsapi.NewAuthorizationTokenActionService(iss, endpoint), + IAMService: allowAllIAMService(), } req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) @@ -26,9 +29,10 @@ func TestHandleGetAuthorizationToken_MintsUsableToken(t *testing.T) { ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) ctx = context.WithValue(ctx, ctxIdentity, "dev") ctx = context.WithValue(ctx, ctxAccessKey, "AKIAGETAUTHTOKENTEST1") + req.Header.Set("X-Amz-Target", awsapi.TargetPrefix+".GetAuthorizationToken") w := httptest.NewRecorder() - require.NoError(t, gw.handleGetAuthorizationToken(w, req.WithContext(ctx))) + require.NoError(t, gw.ECR_Request(w, req.WithContext(ctx))) require.Equal(t, http.StatusOK, w.Code) var out struct { @@ -55,14 +59,17 @@ func TestHandleGetAuthorizationToken_MintsUsableToken(t *testing.T) { claims, err := verify.Verify(jwtStr) require.NoError(t, err) assert.Equal(t, ecrTestAccount, claims.AccountID) + assert.Equal(t, "arn:aws:iam::"+ecrTestAccount+":user/dev", claims.Subject) } -func TestHandleGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { +func TestGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { iss, verify := newECRAuth(t) + endpoint := awsapi.RepositoryEndpoint{Region: ecrTestRegion, ServicesDomain: ecrTestSuffix, RegistryPort: "9999"} gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, RegistryPort: "9999", ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, - IAMService: allowAllIAMService(), + ECRTokenAction: awsapi.NewAuthorizationTokenActionService(iss, endpoint), + IAMService: allowAllIAMService(), } req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) @@ -70,8 +77,9 @@ func TestHandleGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { ctx = context.WithValue(ctx, ctxPrincipalType, principalTypeUser) ctx = context.WithValue(ctx, ctxIdentity, "dev") ctx = context.WithValue(ctx, ctxAccessKey, "AKIAGETAUTHTOKENTEST1") + req.Header.Set("X-Amz-Target", awsapi.TargetPrefix+".GetAuthorizationToken") w := httptest.NewRecorder() - require.NoError(t, gw.handleGetAuthorizationToken(w, req.WithContext(ctx))) + require.NoError(t, gw.ECR_Request(w, req.WithContext(ctx))) var out struct { AuthorizationData []struct { @@ -83,12 +91,25 @@ func TestHandleGetAuthorizationToken_ProxyEndpointCarriesPort(t *testing.T) { assert.Equal(t, "https://"+ecrTestAccount+".dkr.ecr."+ecrTestRegion+"."+ecrTestSuffix+":9999", out.AuthorizationData[0].ProxyEndpoint) } -func TestHandleGetAuthorizationToken_NoIssuerNotImplemented(t *testing.T) { - gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} - req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader("{}")) +func TestGetAuthorizationToken_NoIssuerNotImplemented(t *testing.T) { + gw := &GatewayConfig{ + DisableLogging: true, + IAMService: allowAllIAMService(), + ECRTokenAction: awsapi.NewAuthorizationTokenActionService(nil, awsapi.RepositoryEndpoint{}), + } + req := setupECRRequest(awsapi.TargetPrefix+".GetAuthorizationToken", "{}") ctx := context.WithValue(req.Context(), ctxAccountID, ecrTestAccount) - err := gw.handleGetAuthorizationToken(httptest.NewRecorder(), req.WithContext(ctx)) + err := gw.ECR_Request(httptest.NewRecorder(), req.WithContext(ctx)) require.Error(t, err) + assert.Equal(t, "NotImplemented", err.Error()) +} + +func TestGetAuthorizationToken_MissingComposition(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true, IAMService: allowAllIAMService()} + req := setupECRRequest(awsapi.TargetPrefix+".GetAuthorizationToken", "{}") + err := gw.ECR_Request(httptest.NewRecorder(), req) + require.Error(t, err) + assert.Equal(t, "ServerInternal", err.Error()) } func TestECRRequest_GetAuthorizationTokenDispatched(t *testing.T) { @@ -96,6 +117,9 @@ func TestECRRequest_GetAuthorizationTokenDispatched(t *testing.T) { gw := &GatewayConfig{ Region: ecrTestRegion, InternalSuffix: ecrTestSuffix, ECRTokenIssuer: iss, ECRTokenVerifier: verify, DisableLogging: true, + ECRTokenAction: awsapi.NewAuthorizationTokenActionService(iss, awsapi.RepositoryEndpoint{ + Region: ecrTestRegion, ServicesDomain: ecrTestSuffix, + }), IAMService: allowAllIAMService(), } w := httptest.NewRecorder() diff --git a/spinifex/gateway/ecr_principal.go b/spinifex/gateway/ecr_principal.go index 58ab1c955..2f8d9c644 100644 --- a/spinifex/gateway/ecr_principal.go +++ b/spinifex/gateway/ecr_principal.go @@ -3,6 +3,7 @@ package gateway import ( "errors" "fmt" + "net/http" "strings" "time" @@ -27,6 +28,29 @@ import ( // authorized by the identical policy-evaluation path as every other AWS // action. +// ecrAuthorizationPrincipal converts the fresh SigV4 identity context into +// the canonical principal a GetAuthorizationToken response signs. It stays in +// gateway because the ECR domain must not know gateway context keys or IAM/STS +// ARN construction rules. +func ecrAuthorizationPrincipal(r *http.Request, accountID string) (ecrauth.Principal, error) { + ctx := r.Context() + accessKey, _ := ctx.Value(ctxAccessKey).(string) + identity, _ := ctx.Value(ctxIdentity).(string) + principalType, _ := ctx.Value(ctxPrincipalType).(string) + assumedRoleARN, _ := ctx.Value(ctxAssumedRoleARN).(string) + + callerARN, err := buildCallerARN(accountID, identity, principalType, assumedRoleARN) + if err != nil { + return ecrauth.Principal{}, err + } + return ecrauth.Principal{ + AccountID: accountID, + ARN: callerARN, + Type: principalType, + AccessKeyID: accessKey, + }, nil +} + // ecrPrincipalError classifies a principal-rehydration failure so the HTTP // layer can fail closed with the right status: an invalid/revoked/mismatched // identity is client-visible (401, with the registry's re-auth challenge), diff --git a/spinifex/gateway/ecr_repository.go b/spinifex/gateway/ecr_repository.go deleted file mode 100644 index a41b1aa93..000000000 --- a/spinifex/gateway/ecr_repository.go +++ /dev/null @@ -1,17 +0,0 @@ -package gateway - -import ( - awsapi "github.com/mulgadc/spinifex/spinifex/domains/ecr/awsapi" -) - -// ecrRepositoryEndpoint adapts gateway composition settings to the ECR token -// action, which still constructs its canonical caller principal in gateway. -// Repository metadata actions receive this profile from their composed service. -func (gw *GatewayConfig) ecrRepositoryEndpoint() awsapi.RepositoryEndpoint { - return awsapi.RepositoryEndpoint{ - Region: gw.Region, - ServicesDomain: gw.InternalSuffix, - RegistryHost: gw.RegistryHost, - RegistryPort: gw.RegistryPort, - } -} diff --git a/spinifex/gateway/ecrapi.go b/spinifex/gateway/ecrapi.go index d37114af1..fa3d44200 100644 --- a/spinifex/gateway/ecrapi.go +++ b/spinifex/gateway/ecrapi.go @@ -1,9 +1,7 @@ package gateway import ( - "bytes" "errors" - "io" "log/slog" "net/http" "strings" @@ -12,16 +10,6 @@ import ( "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" ) -type ecrInlineHandler func(*GatewayConfig, http.ResponseWriter, *http.Request) error - -// ecrInlineActions is the authoritative inventory of ECR operations handled -// directly by the gateway rather than relayed through awsapi.Actions. -// Keeping dispatch and coverage on the same map prevents the generated -// operation report from classifying an inline implementation as a stub. -var ecrInlineActions = map[string]ecrInlineHandler{ - "GetAuthorizationToken": (*GatewayConfig).handleGetAuthorizationToken, -} - // ecrActionFromTarget extracts the action suffix from an X-Amz-Target header. // Any "." or bare "" form is accepted. func ecrActionFromTarget(target string) string { @@ -106,12 +94,22 @@ func (gw *GatewayConfig) ECR_Request(w http.ResponseWriter, r *http.Request) err awsapi.WriteJSONResponse(w, output) return nil } - - if inline, ok := ecrInlineActions[action]; ok { - // The inline handlers read r.Body themselves, so it is rewound over the - // bytes the gate consumed, the same discipline the auth middleware uses. - r.Body = io.NopCloser(bytes.NewReader(body)) - return inline(gw, w, r) + if awsapi.IsAuthorizationTokenAction(action) { + if gw.ECRTokenAction == nil { + slog.Error("ECR authorization-token action: capabilities not configured") + return errors.New(awserrors.ErrorServerInternal) + } + principal, err := ecrAuthorizationPrincipal(r, accountID) + if err != nil { + slog.Error("GetAuthorizationToken: cannot build canonical caller ARN", "err", err) + return errors.New(awserrors.ErrorServerInternal) + } + output, err := gw.ECRTokenAction.MintFor(principal) + if err != nil { + return err + } + awsapi.WriteJSONResponse(w, output) + return nil } output, err := handler(r.Context(), gw.NATSConn, accountID, body) diff --git a/spinifex/gateway/ecrapi_authz_completeness_test.go b/spinifex/gateway/ecrapi_authz_completeness_test.go index e249de823..a2d7eebf7 100644 --- a/spinifex/gateway/ecrapi_authz_completeness_test.go +++ b/spinifex/gateway/ecrapi_authz_completeness_test.go @@ -1,5 +1,4 @@ -//test:in-package — ecrInlineActions is unexported here, and ECR dispatch also -// composes the ECR registry-action capability. +//test:in-package — drives the gateway's ECR action-dispatch invariants. package gateway @@ -26,16 +25,10 @@ func TestECRScopeTableIsExhaustive(t *testing.T) { } } -// ECR_Request resolves the action against the ecrapi namespace before it reaches -// either a transitional HTTP adapter or a composed capability, so an action -// outside that namespace is unreachable — and, more to the point, would never -// have had its resources scoped. +// ECR_Request resolves every composed action against the ecrapi namespace +// before dispatch, so an action outside that namespace is unreachable — and, +// more to the point, would never have had its resources scoped. func TestECRComposedActionsAreInTheNamespace(t *testing.T) { - for action := range ecrInlineActions { - _, ok := awsapi.Actions[action] - assert.True(t, ok, - "inline ECR handler %q is not in awsapi.Actions, so ECR_Request rejects it as InvalidAction", action) - } for _, action := range awsapi.RegistryActionNames() { _, ok := awsapi.Actions[action] assert.True(t, ok, @@ -51,4 +44,9 @@ func TestECRComposedActionsAreInTheNamespace(t *testing.T) { assert.True(t, ok, "composed ECR repository action %q is not in awsapi.Actions, so ECR_Request rejects it as InvalidAction", action) } + for _, action := range awsapi.AuthorizationTokenActionNames() { + _, ok := awsapi.Actions[action] + assert.True(t, ok, + "composed ECR authorization-token action %q is not in awsapi.Actions, so ECR_Request rejects it as InvalidAction", action) + } } diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 62df1a776..b97d963dc 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -171,6 +171,9 @@ type GatewayConfig struct { // ECRRepositoryActions serves repository metadata actions from the composed // metadata store and advertised endpoint profile. ECRRepositoryActions *awsapi.RepositoryActionService + // ECRTokenAction serves GetAuthorizationToken after gateway has constructed + // a canonical IAM/STS principal from the authenticated request context. + ECRTokenAction *awsapi.AuthorizationTokenActionService // ECRTokenIssuer mints GetAuthorizationToken JWTs; ECRTokenVerifier validates // them on /v2/*. Both nil disables the auth bridge (registry mounts open, as // in unit tests of unrelated routes). diff --git a/spinifex/gateway/operations.go b/spinifex/gateway/operations.go index 48a4a8f55..3687b89ba 100644 --- a/spinifex/gateway/operations.go +++ b/spinifex/gateway/operations.go @@ -22,10 +22,9 @@ type ServiceOperationInventory struct { // operation dispatch tables. S3 is intentionally absent: Spinifex does not // dispatch S3 operations and delegates that REST surface to Predastore. func AWSOperationInventory() map[string]ServiceOperationInventory { - ecrInline := mapKeys(ecrInlineActions) ecrComposed := union( - union(union(ecrInline, awsapi.RegistryActionNames()), awsapi.LifecyclePreviewActionNames()), - awsapi.RepositoryActionNames(), + union(awsapi.RegistryActionNames(), awsapi.LifecyclePreviewActionNames()), + union(awsapi.RepositoryActionNames(), awsapi.AuthorizationTokenActionNames()), ) ecrRegistered := union(mapKeys(awsapi.Actions), ecrComposed) ecrStubbed := without(awsapi.StubbedActionNames(), ecrComposed) diff --git a/spinifex/services/awsgw/awsgw.go b/spinifex/services/awsgw/awsgw.go index 47161eada..173b107fe 100644 --- a/spinifex/services/awsgw/awsgw.go +++ b/spinifex/services/awsgw/awsgw.go @@ -441,6 +441,7 @@ func launchService(config *config.ClusterConfig) error { // responder identity rather than by count. nodeIDs := slices.Collect(maps.Keys(config.Nodes)) + ecrIssuer := ecrauth.NewIssuer(signingKey, ecrAudience) gw := gateway.GatewayConfig{ Debug: nodeConfig.AWSGW.Debug, DisableLogging: false, @@ -463,7 +464,8 @@ func launchService(config *config.ClusterConfig) error { ECRRegistryActions: awsapi.NewRegistryActionService(ecrRegistry, ecrRegistry, ecrRegistry, ecrRegistry), ECRLifecyclePreview: awsapi.NewLifecyclePreviewActionService(ecrMeta, ecrRegistry), ECRRepositoryActions: awsapi.NewRepositoryActionService(ecrMeta, ecrEndpoint), - ECRTokenIssuer: ecrauth.NewIssuer(signingKey, ecrAudience), + ECRTokenAction: awsapi.NewAuthorizationTokenActionService(ecrIssuer, ecrEndpoint), + ECRTokenIssuer: ecrIssuer, ECRTokenVerifier: ecrauth.NewVerifier(verifyKeys, ecrAudience), BedrockCredentials: bedrockCredentials, BedrockEndpoints: bedrockEndpoints,