From 238633ae319939135dfd799c4ce6ab06b726c776 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:18:20 +0000 Subject: [PATCH 1/3] feat(credentials): check endpoints of stored GCP WIF configs before use A stored external_account config names the endpoints the resolver talks to when it exchanges and impersonates tokens. Only Google's STS and IAM Credentials endpoints are ever expected there (gcloud and the gcp-target IaC write exactly those, and the secret-free federated path hard-codes them), so the resolver now checks them before it builds a token source: token_url must be https://sts.googleapis.com/..., and service_account_impersonation_url, when set, must be https://iamcredentials.googleapis.com/..., with no user info or port. The check decodes the JSON the same way golang.org/x/oauth2/google does, so it sees the values that library would use. A config that fails it makes the load return a clear error naming the field, without the stored value, instead of a token source. ValidateGCPWIFEndpoints is exported so the API can apply the same rule when a config is submitted. Co-Authored-By: Claude Opus 5.5 --- internal/credentials/gcp_endpoints.go | 81 ++++++++ internal/credentials/gcp_endpoints_test.go | 209 +++++++++++++++++++++ internal/credentials/resolver.go | 3 + 3 files changed, 293 insertions(+) create mode 100644 internal/credentials/gcp_endpoints.go create mode 100644 internal/credentials/gcp_endpoints_test.go diff --git a/internal/credentials/gcp_endpoints.go b/internal/credentials/gcp_endpoints.go new file mode 100644 index 000000000..847958be3 --- /dev/null +++ b/internal/credentials/gcp_endpoints.go @@ -0,0 +1,81 @@ +package credentials + +import ( + "encoding/json" + "fmt" + "net/url" +) + +// Hosts a GCP external_account (workload identity federation) config may name +// for its token exchange and service-account impersonation endpoints. These +// are the values gcloud writes into a generated credential config, and the +// same endpoints BuildGCPFederatedCredential uses. +const ( + gcpSTSHost = "sts.googleapis.com" + gcpIAMCredentialsHost = "iamcredentials.googleapis.com" +) + +// gcpExternalAccountType is the "type" of a GCP external_account credential +// config. +const gcpExternalAccountType = "external_account" + +// ValidateGCPWIFEndpoints checks the endpoint URLs of a GCP external_account +// credential config. token_url is required and must be an https URL whose +// host is exactly sts.googleapis.com. service_account_impersonation_url is +// optional; when set it must be an https URL whose host is exactly +// iamcredentials.googleapis.com. Neither URL may carry user info or an +// explicit port. The returned error names the field and the expected form +// only, never the submitted value. +func ValidateGCPWIFEndpoints(tokenURL, impersonationURL string) error { + if tokenURL == "" { + return fmt.Errorf("token_url is required") + } + if err := checkGCPEndpoint("token_url", tokenURL, gcpSTSHost); err != nil { + return err + } + if impersonationURL == "" { + return nil + } + return checkGCPEndpoint("service_account_impersonation_url", impersonationURL, gcpIAMCredentialsHost) +} + +// checkGCPEndpoint requires raw to parse as an absolute https URL whose +// authority is exactly host: no user info, no port, no other host. +func checkGCPEndpoint(field, raw, host string) error { + u, err := url.Parse(raw) + if err != nil || u.Scheme != "https" || u.User != nil || u.Host != host { + return fmt.Errorf("%s must be an https URL on %s with no user info or port", field, host) + } + return nil +} + +// storedGCPExternalAccount holds the fields of a stored GCP credential JSON +// that ValidateGCPWIFEndpoints checks. The JSON tags match the ones +// golang.org/x/oauth2/google decodes the same file with, and decoding into a +// struct (not a map) matches keys the same way, so both read the same values. +type storedGCPExternalAccount struct { + Type string `json:"type"` + TokenURL string `json:"token_url"` + ServiceAccountImpersonationURL string `json:"service_account_impersonation_url"` +} + +// checkStoredGCPEndpoints checks a stored GCP credential JSON before a token +// source is built from it: when its type is external_account, its endpoints +// must pass ValidateGCPWIFEndpoints. Other types are left to the parser. +// Empty input passes so the caller can report the missing credential itself. +func checkStoredGCPEndpoints(accountID string, raw []byte) error { + if len(raw) == 0 { + return nil + } + var cfg storedGCPExternalAccount + if err := json.Unmarshal(raw, &cfg); err != nil { + return fmt.Errorf("credentials: parse gcp credentials for account %s: %w", accountID, err) + } + if cfg.Type != gcpExternalAccountType { + return nil + } + if err := ValidateGCPWIFEndpoints(cfg.TokenURL, cfg.ServiceAccountImpersonationURL); err != nil { + return fmt.Errorf("credentials: stored gcp workload identity config for account %s is not usable: %w; save the config again", accountID, err) + } + return nil +} diff --git a/internal/credentials/gcp_endpoints_test.go b/internal/credentials/gcp_endpoints_test.go new file mode 100644 index 000000000..b91415c81 --- /dev/null +++ b/internal/credentials/gcp_endpoints_test.go @@ -0,0 +1,209 @@ +package credentials + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testSTSURL = "https://sts.googleapis.com/v1/token" + testImpersonationURL = "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/sa@test-project.iam.gserviceaccount.com:generateAccessToken" +) + +// endpointCase is a token_url or service_account_impersonation_url value +// that ValidateGCPWIFEndpoints must refuse. +type endpointCase struct{ name, url string } + +// badTokenURLs are token_url values that are not exactly +// https://sts.googleapis.com/...: other schemes, look-alike hosts, user info +// and explicit ports. +var badTokenURLs = []endpointCase{ + {"http scheme", "http://sts.googleapis.com/v1/token"}, + {"other scheme", "ftp://sts.googleapis.com/v1/token"}, + {"no scheme", "sts.googleapis.com/v1/token"}, + {"scheme-relative", "//sts.googleapis.com/v1/token"}, + {"suffix look-alike", "https://sts.googleapis.com.example.net/v1/token"}, + {"prefix look-alike", "https://my-sts.googleapis.com/v1/token"}, + {"other google host", "https://iamcredentials.googleapis.com/v1/token"}, + {"other host", "https://token.example.net/v1/token"}, + {"trailing dot", "https://sts.googleapis.com./v1/token"}, + {"userinfo before host", "https://sts.googleapis.com@token.example.net/v1/token"}, + {"userinfo on google host", "https://user:pass@sts.googleapis.com/v1/token"}, + {"explicit default port", "https://sts.googleapis.com:443/v1/token"}, + {"explicit other port", "https://sts.googleapis.com:8443/v1/token"}, + {"empty port", "https://sts.googleapis.com:/v1/token"}, + {"ip literal", "https://127.0.0.1/v1/token"}, + {"ipv6 literal", "https://[::1]/v1/token"}, + {"uppercase host", "https://STS.GOOGLEAPIS.COM/v1/token"}, + {"unparsable", "https://sts.googleapis.com/%zz"}, +} + +var badImpersonationURLs = []endpointCase{ + {"http scheme", "http://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/sa:generateAccessToken"}, + {"suffix look-alike", "https://iamcredentials.googleapis.com.example.net/v1/x"}, + {"prefix look-alike", "https://my-iamcredentials.googleapis.com/v1/x"}, + {"sts host", "https://sts.googleapis.com/v1/x"}, + {"userinfo before host", "https://iamcredentials.googleapis.com@token.example.net/v1/x"}, + {"explicit port", "https://iamcredentials.googleapis.com:443/v1/x"}, +} + +func TestValidateGCPWIFEndpoints_AcceptsGoogleEndpoints(t *testing.T) { + require.NoError(t, ValidateGCPWIFEndpoints(testSTSURL, testImpersonationURL)) + require.NoError(t, ValidateGCPWIFEndpoints(testSTSURL, ""), "impersonation URL is optional") + require.NoError(t, ValidateGCPWIFEndpoints("https://sts.googleapis.com/v1/token?x=1", "")) +} + +func TestValidateGCPWIFEndpoints_RequiresTokenURL(t *testing.T) { + err := ValidateGCPWIFEndpoints("", testImpersonationURL) + require.Error(t, err) + assert.Contains(t, err.Error(), "token_url is required") +} + +func TestValidateGCPWIFEndpoints_RejectsBadTokenURL(t *testing.T) { + for _, tc := range badTokenURLs { + t.Run(tc.name, func(t *testing.T) { + err := ValidateGCPWIFEndpoints(tc.url, testImpersonationURL) + require.Error(t, err) + assert.Contains(t, err.Error(), "token_url must be an https URL on sts.googleapis.com") + }) + } +} + +func TestValidateGCPWIFEndpoints_RejectsBadImpersonationURL(t *testing.T) { + for _, tc := range badImpersonationURLs { + t.Run(tc.name, func(t *testing.T) { + err := ValidateGCPWIFEndpoints(testSTSURL, tc.url) + require.Error(t, err) + assert.Contains(t, err.Error(), "service_account_impersonation_url must be an https URL on iamcredentials.googleapis.com") + }) + } +} + +// wifConfigJSON returns a gcloud-style external_account config (the shape +// `gcloud iam workload-identity-pools create-cred-config` writes) with the +// given endpoints. An empty impersonation URL leaves the key out. +func wifConfigJSON(t *testing.T, tokenURL, impersonationURL string) []byte { + t.Helper() + cfg := map[string]any{ + "universe_domain": "googleapis.com", + "type": "external_account", + "audience": "//iam.googleapis.com/projects/1/locations/global/workloadIdentityPools/pool/providers/provider", + "subject_token_type": "urn:ietf:params:oauth:token-type:jwt", + "token_url": tokenURL, + "credential_source": map[string]any{ + "file": "/var/run/secrets/azure/tokens/azure-identity-token", + "format": map[string]any{"type": "text"}, + }, + } + if impersonationURL != "" { + cfg["service_account_impersonation_url"] = impersonationURL + } + raw, err := json.Marshal(cfg) + require.NoError(t, err) + return raw +} + +// loadStoredWIF resolves a WIF-mode account whose stored config is raw. With +// federated set, the options and audience would allow the secret-free path, +// which a stored config takes priority over. +func loadStoredWIF(t *testing.T, raw []byte, federated bool) error { + t.Helper() + store := newMockStore() + store.data["acct1/"+CredTypeGCPWIFConfig] = raw + account := &config.CloudAccount{ID: "acct1", GCPAuthMode: "workload_identity_federation"} + opts := GCPResolveOptions{} + if federated { + account.GCPWIFAudience = "//iam.googleapis.com/projects/1/locations/global/workloadIdentityPools/pool/providers/provider" + opts = GCPResolveOptions{Signer: stubOIDCSigner{}, IssuerURL: "https://issuer.example.com"} + } + src, err := ResolveGCPTokenSourceWithOpts(context.Background(), account, store, opts) + if err == nil { + require.NotNil(t, src) + } else { + require.Nil(t, src) + } + return err +} + +func TestLoadStoredGCPTokenSource_WIF_AcceptsGcloudConfig(t *testing.T) { + require.NoError(t, loadStoredWIF(t, wifConfigJSON(t, testSTSURL, testImpersonationURL), false)) + require.NoError(t, loadStoredWIF(t, wifConfigJSON(t, testSTSURL, ""), false)) + require.NoError(t, loadStoredWIF(t, wifConfigJSON(t, testSTSURL, testImpersonationURL), true)) +} + +func TestLoadStoredGCPTokenSource_WIF_RejectsBadEndpoints(t *testing.T) { + type stored struct{ name, tokenURL, impURL string } + cases := make([]stored, 0, len(badTokenURLs)+len(badImpersonationURLs)+1) + for _, tc := range badTokenURLs { + cases = append(cases, stored{"token_url/" + tc.name, tc.url, testImpersonationURL}) + } + for _, tc := range badImpersonationURLs { + cases = append(cases, stored{"impersonation/" + tc.name, testSTSURL, tc.url}) + } + cases = append(cases, stored{"token_url/missing", "", testImpersonationURL}) + for _, tc := range cases { + for _, federated := range []bool{false, true} { + t.Run(tc.name, func(t *testing.T) { + err := loadStoredWIF(t, wifConfigJSON(t, tc.tokenURL, tc.impURL), federated) + require.Error(t, err) + assert.Contains(t, err.Error(), "stored gcp workload identity config for account acct1 is not usable") + assert.NotContains(t, err.Error(), "example.net", "the error must not echo the stored URL") + }) + } + } +} + +// Decoding matches x/oauth2: JSON keys match struct tags case-insensitively +// and the last duplicate wins, so the checked value is the one it would use. +func TestLoadStoredGCPTokenSource_WIF_ChecksTheValueTheParserUses(t *testing.T) { + cases := map[string]string{ + "case variant key": `{"type":"external_account","audience":"a","subject_token_type":"s",` + + `"token_url":"` + testSTSURL + `","Token_URL":"https://token.example.net/v1/token","credential_source":{"file":"x"}}`, + "duplicate key": `{"type":"external_account","audience":"a","subject_token_type":"s",` + + `"token_url":"` + testSTSURL + `","token_url":"https://token.example.net/v1/token","credential_source":{"file":"x"}}`, + } + for name, raw := range cases { + t.Run(name, func(t *testing.T) { + err := loadStoredWIF(t, []byte(raw), false) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not usable") + }) + } +} + +func TestLoadStoredGCPTokenSource_WIF_MalformedFailsWithoutPanic(t *testing.T) { + for name, raw := range map[string]string{ + "not json": "not valid json", + "token_url not text": `{"type":"external_account","token_url":7}`, + "type not text": `{"type":["external_account"]}`, + "top level not object": `["external_account"]`, + } { + t.Run(name, func(t *testing.T) { + err := loadStoredWIF(t, []byte(raw), false) + require.Error(t, err) + assert.Contains(t, err.Error(), "parse gcp credentials") + }) + } +} + +// The service-account slot never needs an external_account config; if one +// is stored there it gets the same endpoint check before it is used. +func TestLoadStoredGCPTokenSource_ServiceAccountSlot_ChecksExternalAccountEndpoints(t *testing.T) { + store := newMockStore() + store.data["acct1/"+CredTypeGCPServiceAccount] = wifConfigJSON(t, "https://token.example.net/v1/token", "") + account := &config.CloudAccount{ID: "acct1", GCPAuthMode: "service_account_key"} + _, err := ResolveGCPTokenSource(context.Background(), account, store) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not usable") +} + +func TestLoadStoredGCPTokenSource_WIF_EmptyStillReportsMissing(t *testing.T) { + err := loadStoredWIF(t, nil, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "no gcp credentials stored") +} diff --git a/internal/credentials/resolver.go b/internal/credentials/resolver.go index d2863c3b9..865364172 100644 --- a/internal/credentials/resolver.go +++ b/internal/credentials/resolver.go @@ -471,6 +471,9 @@ func loadStoredGCPTokenSource( if err != nil { return nil, fmt.Errorf("credentials: load gcp credentials for account %s: %w", account.ID, err) } + if checkErr := checkStoredGCPEndpoints(account.ID, raw); checkErr != nil { + return nil, checkErr + } if raw == nil { return nil, fmt.Errorf("credentials: no gcp credentials stored for account %s", account.ID) } From 3f818a3d41077c85b7f7428ff8dd1e91fe3393ac Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:18:20 +0000 Subject: [PATCH 2/3] feat(api): check endpoint URLs in submitted GCP WIF configs validateGCPWIFPayload checked the shape of a gcp_workload_identity_config but not its URLs. It now runs credentials.ValidateGCPWIFEndpoints on token_url and service_account_impersonation_url, the same rule the resolver applies before using a stored config, so a config that could never be used is refused with a 400 when it is saved. The optional impersonation URL must also be a non-empty string when present. Co-Authored-By: Claude Opus 5.5 --- internal/api/validation.go | 24 ++++++++ internal/api/validation_gcpwif_test.go | 76 ++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 internal/api/validation_gcpwif_test.go diff --git a/internal/api/validation.go b/internal/api/validation.go index dd6575dbb..20476d47f 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -15,6 +15,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/credentials" ) // Security constants. @@ -286,6 +287,29 @@ func validateGCPWIFPayload(payload map[string]interface{}) error { return err } } + return validateGCPWIFEndpointKeys(payload) +} + +// validateGCPWIFEndpointKeys checks the config's token exchange and +// service-account impersonation URLs with credentials.ValidateGCPWIFEndpoints, +// the same check the resolver runs before it uses a stored config. Runs after +// the required-key checks, so token_url is a non-empty string here. +func validateGCPWIFEndpointKeys(payload map[string]interface{}) error { + tokenURL, ok := payload["token_url"].(string) + if !ok { + return NewClientError(400, "credentials payload key \"token_url\" must be a non-empty string") + } + impersonationURL := "" + if v, ok := payload["service_account_impersonation_url"]; ok { + s, isStr := v.(string) + if !isStr || strings.TrimSpace(s) == "" { + return NewClientError(400, "credentials payload key \"service_account_impersonation_url\" must be a non-empty string") + } + impersonationURL = s + } + if err := credentials.ValidateGCPWIFEndpoints(tokenURL, impersonationURL); err != nil { + return NewClientError(400, "credentials payload for gcp_workload_identity_config: "+err.Error()) + } return nil } diff --git a/internal/api/validation_gcpwif_test.go b/internal/api/validation_gcpwif_test.go new file mode 100644 index 000000000..61d65870a --- /dev/null +++ b/internal/api/validation_gcpwif_test.go @@ -0,0 +1,76 @@ +package api + +// Tests for the endpoint URLs validateCredentialPayload accepts in a +// gcp_workload_identity_config payload. + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const regCredSAEmail = "cudly@test-project.iam.gserviceaccount.com" + +// gcloudWIFConfig is the external_account config the gcp-target IaC +// (iac/federation/gcp-target/terraform/registration.tf) and gcloud generate +// for an AWS-hosted deployment. +func gcloudWIFConfig() map[string]any { + return map[string]any{ + "type": "external_account", + "audience": "//iam.googleapis.com/projects/1/locations/global/workloadIdentityPools/cudly/providers/cudly", + "subject_token_type": "urn:ietf:params:aws:token-type:aws4_request", + "service_account_impersonation_url": "https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/" + regCredSAEmail + ":generateAccessToken", + "token_url": "https://sts.googleapis.com/v1/token", + "credential_source": map[string]any{ + "environment_id": "aws1", + "region_url": "http://169.254.169.254/latest/meta-data/placement/availability-zone", + "url": "http://169.254.169.254/latest/meta-data/iam/security-credentials", + "regional_cred_verification_url": "https://sts.{region}.amazonaws.com?Action=GetCallerIdentity&Version=2011-06-15", + }, + } +} + +// gcloudWIFConfigWith returns gcloudWIFConfig with key set to value. +func gcloudWIFConfigWith(key string, value any) map[string]any { + cfg := gcloudWIFConfig() + cfg[key] = value + return cfg +} + +func require400(t *testing.T, err error, msg string) { + t.Helper() + ce, ok := IsClientError(err) + require.True(t, ok, "want a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, msg) +} + +func TestValidateCredentialPayload_GCPWIFEndpoints(t *testing.T) { + require.NoError(t, validateCredentialPayload("gcp_workload_identity_config", gcloudWIFConfig())) + + noImpersonation := gcloudWIFConfig() + delete(noImpersonation, "service_account_impersonation_url") + require.NoError(t, validateCredentialPayload("gcp_workload_identity_config", noImpersonation)) + + cases := map[string]struct { + key string + value any + msg string + }{ + "token_url look-alike suffix": {"token_url", "https://sts.googleapis.com.example.net/v1/token", "token_url must be an https URL on sts.googleapis.com"}, + "token_url look-alike prefix": {"token_url", "https://my-sts.googleapis.com/v1/token", "token_url must be an https URL on sts.googleapis.com"}, + "token_url userinfo": {"token_url", "https://sts.googleapis.com@token.example.net/v1/token", "token_url must be an https URL"}, + "token_url port": {"token_url", "https://sts.googleapis.com:8443/v1/token", "token_url must be an https URL"}, + "token_url http": {"token_url", "http://sts.googleapis.com/v1/token", "token_url must be an https URL"}, + "impersonation url other host": {"service_account_impersonation_url", "https://token.example.net/v1/x", "service_account_impersonation_url must be an https URL"}, + "impersonation url not a string": {"service_account_impersonation_url", 42, `"service_account_impersonation_url" must be a non-empty string`}, + "impersonation url empty": {"service_account_impersonation_url", " ", `"service_account_impersonation_url" must be a non-empty string`}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := validateCredentialPayload("gcp_workload_identity_config", gcloudWIFConfigWith(tc.key, tc.value)) + require400(t, err, tc.msg) + }) + } +} From 7d1fb80ad4a8d305038a9e2f3cb1f0549d39903b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sun, 25 Oct 2026 16:18:20 +0000 Subject: [PATCH 3/3] feat(api): validate credentials submitted with account registrations POST /api/register accepts an optional credential_type and credential_payload, which approval saves into account_credentials. They were stored as sent, while POST /api/accounts/:id/credentials checks the type and the payload shape. Registrations now get the same checks: - credential_type and credential_payload come together or not at all; - the type must be a known one and belong to the registration's provider; - the payload must be a JSON object that passes validateCredentialPayload (including the GCP WIF endpoint check). The stored payload is re-encoded from the checked object, so the saved bytes are the ones that were checked. Registrations without credentials are accepted as before. Approval checks the stored credential again, against the provider of the account being created, before the registration leaves "pending". A credential that fails stops the approval with a 400 telling the admin to reject the registration; nothing is created or saved. Previously a decrypt failure or a type-less payload was skipped with a log line while the account was still created and enabled. Without a credential store the payload was already dropped at submit; the type is now dropped with it so the record stays consistent. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_registrations.go | 67 +++--- .../api/handler_registrations_credentials.go | 102 ++++++++ .../handler_registrations_credentials_test.go | 222 ++++++++++++++++++ 3 files changed, 361 insertions(+), 30 deletions(-) create mode 100644 internal/api/handler_registrations_credentials.go create mode 100644 internal/api/handler_registrations_credentials_test.go diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index b101d4cda..8a3231226 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -93,14 +93,17 @@ func (h *Handler) submitRegistration(ctx context.Context, req *events.LambdaFunc GCPClientEmail: body.GCPClientEmail, GCPAuthMode: body.GCPAuthMode, GCPWIFAudience: body.GCPWIFAudience, - RegCredentialType: body.CredentialType, } encrypted, encErr := h.encryptRegistrationCredential(body.CredentialPayload) if encErr != nil { return nil, fmt.Errorf("registrations: encrypt credential: %w", encErr) } - reg.RegCredentialPayload = encrypted + if encrypted != "" { + // Without a credential store nothing is kept, so record no type either. + reg.RegCredentialType = body.CredentialType + reg.RegCredentialPayload = encrypted + } if err := h.insertRegistration(ctx, reg); err != nil { return nil, err @@ -233,22 +236,6 @@ func (h *Handler) setReviewMetadata(ctx context.Context, reg *config.AccountRegi } } -// storeRegistrationCredentials decrypts and stores credentials from the registration -// record into the account_credentials table. Errors are logged, not propagated. -func (h *Handler) storeRegistrationCredentials(ctx context.Context, reg *config.AccountRegistration, accountID string) { - if reg.RegCredentialPayload == "" || reg.RegCredentialType == "" || h.credStore == nil { - return - } - plaintext, err := h.credStore.DecryptPayload(reg.RegCredentialPayload) - if err != nil { - logging.Warnf("registration %s: failed to decrypt credential payload: %v", reg.ID, err) - return - } - if err := h.credStore.SaveCredential(ctx, accountID, reg.RegCredentialType, plaintext); err != nil { - logging.Warnf("registration %s: failed to store credential: %v", reg.ID, err) - } -} - // encryptRegistrationCredential encrypts the credential payload for storage in the // registration table. Returns empty string if no credential is provided. func (h *Handler) encryptRegistrationCredential(payload string) (string, error) { @@ -278,15 +265,9 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return nil, err } - var acctReq CloudAccountRequest - if err := json.Unmarshal([]byte(httpReq.Body), &acctReq); err != nil { - return nil, NewClientError(400, "invalid account request body") - } - if err := validateCloudAccountRequest(acctReq); err != nil { - return nil, err - } - // Before the transition, so a rejected config leaves the registration pending. - if err := h.requireHostForEmptyRoleARN(ctx, "(registration "+reg.ID+")", &acctReq); err != nil { + // Before the transition, so a rejected request leaves the registration pending. + acctReq, regCred, err := h.parseApprovalRequest(ctx, reg, httpReq) + if err != nil { return nil, err } @@ -312,7 +293,7 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd // KMS-backed federated. Without this, every federated-path // approval leaves the account disabled and the operator has to // follow up with a manual PUT enabled=true. - account.Enabled = reg.RegCredentialPayload != "" || accountHasCredentialFreePath(account) + account.Enabled = regCred != nil || accountHasCredentialFreePath(account) account.CreatedAt = now account.UpdatedAt = now @@ -321,7 +302,7 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd } // Auto-store credentials if the registration included them. - h.storeRegistrationCredentials(ctx, reg, account.ID) + h.storeRegistrationCredentials(ctx, reg, account.ID, regCred) // Link the cloud account and wipe the credential payload from the registration record. reg.CloudAccountID = &account.ID @@ -338,6 +319,25 @@ func (h *Handler) approveRegistration(ctx context.Context, httpReq *events.Lambd return account, nil } +// parseApprovalRequest reads and checks an approval body for reg: the +// CloudAccountRequest itself, the host-account rule for an empty role ARN, and +// the credential stored with the registration (see registrationCredential). +// It returns the request and the checked credential, nil when there is none. +func (h *Handler) parseApprovalRequest(ctx context.Context, reg *config.AccountRegistration, httpReq *events.LambdaFunctionURLRequest) (CloudAccountRequest, []byte, error) { + var acctReq CloudAccountRequest + if err := json.Unmarshal([]byte(httpReq.Body), &acctReq); err != nil { + return acctReq, nil, NewClientError(400, "invalid account request body") + } + if err := validateCloudAccountRequest(acctReq); err != nil { + return acctReq, nil, err + } + if err := h.requireHostForEmptyRoleARN(ctx, "(registration "+reg.ID+")", &acctReq); err != nil { + return acctReq, nil, err + } + regCred, err := h.registrationCredential(reg, acctReq.Provider) + return acctReq, regCred, err +} + // accountHasCredentialFreePath returns true when the account's auth // mode resolves a credential without any stored secret — either via // ambient instance credentials (role assumption, managed identity, @@ -432,7 +432,9 @@ func (h *Handler) deleteRegistration(ctx context.Context, id string) (any, error const maxAccountNameLen = 256 // validateRegistrationRequest checks required fields for a registration -// submission. It also normalizes account_name in place: CR/LF characters are +// submission, and the optional credential with parseRegistrationCredential, +// replacing credential_payload in place with the checked encoding. It also +// normalizes account_name in place: CR/LF characters are // stripped (defense-in-depth against email header injection at the data // source, complementing sanitizeHeader on the email subject path) and the // result is length-capped. @@ -464,6 +466,11 @@ func validateRegistrationRequest(req *RegistrationRequest) error { if !strings.Contains(req.ContactEmail, "@") || len(req.ContactEmail) < 5 { return NewClientError(400, "contact_email must be a valid email address") } + credential, err := parseRegistrationCredential(req.Provider, req.CredentialType, req.CredentialPayload) + if err != nil { + return err + } + req.CredentialPayload = string(credential) return nil } diff --git a/internal/api/handler_registrations_credentials.go b/internal/api/handler_registrations_credentials.go new file mode 100644 index 000000000..a9bb24ebc --- /dev/null +++ b/internal/api/handler_registrations_credentials.go @@ -0,0 +1,102 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// credentialTypeProviders maps each credential_type to the provider whose +// accounts use it. +var credentialTypeProviders = map[string]string{ + "aws_access_keys": "aws", + "azure_client_secret": "azure", + "gcp_service_account": "gcp", + "gcp_workload_identity_config": "gcp", +} + +// parseRegistrationCredential checks the optional credential_type and +// credential_payload of a registration for provider. With both empty there is +// no credential and it returns (nil, nil). Otherwise both are required, the +// type must be one POST /api/accounts/:id/credentials accepts and must belong +// to provider, and the payload must be a JSON object that passes +// validateCredentialPayload, the same shape check that endpoint runs. It +// returns the payload re-encoded from the checked object, so the bytes that +// are stored are exactly the ones that were checked. +func parseRegistrationCredential(provider, credType, payload string) ([]byte, error) { + if credType == "" && payload == "" { + return nil, nil + } + if err := checkRegistrationCredentialType(provider, credType, payload); err != nil { + return nil, err + } + var fields map[string]interface{} + if err := json.Unmarshal([]byte(payload), &fields); err != nil || fields == nil { + return nil, NewClientError(400, "credential_payload must be a JSON object") + } + if err := validateCredentialPayload(credType, fields); err != nil { + return nil, err + } + canonical, err := json.Marshal(fields) + if err != nil { + return nil, NewClientError(400, "credential_payload must be a JSON object") + } + return canonical, nil +} + +// checkRegistrationCredentialType requires credential_type and +// credential_payload together, and a credential_type that is known and +// belongs to provider. +func checkRegistrationCredentialType(provider, credType, payload string) error { + if credType == "" || payload == "" { + return NewClientError(400, "credential_type and credential_payload must be provided together") + } + if !validCredentialTypes[credType] { + return NewClientError(400, "credential_type must be one of: aws_access_keys, azure_client_secret, gcp_service_account, gcp_workload_identity_config") + } + if credentialTypeProviders[credType] != provider { + return NewClientError(400, fmt.Sprintf("credential_type %q cannot be used with provider %q", credType, provider)) + } + return nil +} + +// registrationCredential decrypts the credential stored with a pending +// registration and checks it again with parseRegistrationCredential against +// the provider of the account being approved. It runs before the +// registration leaves "pending", so a credential that fails the check stops +// the approval with a 400 the admin sees, and nothing is created or saved. +// Returns (nil, nil) when the registration carries no credential. +func (h *Handler) registrationCredential(reg *config.AccountRegistration, provider string) ([]byte, error) { + if reg.RegCredentialPayload == "" { + return nil, nil + } + if h.credStore == nil { + return nil, fmt.Errorf("registrations: credential store not configured") + } + plaintext, err := h.credStore.DecryptPayload(reg.RegCredentialPayload) + if err != nil { + return nil, fmt.Errorf("registrations: decrypt credential: %w", err) + } + canonical, err := parseRegistrationCredential(provider, reg.RegCredentialType, string(plaintext)) + if err != nil { + return nil, NewClientError(400, "the credentials submitted with this registration are not valid ("+err.Error()+ + "); reject the registration and ask the registrant to submit it again") + } + return canonical, nil +} + +// storeRegistrationCredentials saves a registration credential that +// registrationCredential has checked into the account_credentials table. +// The account already exists at this point, so a store error is logged, not +// returned. +func (h *Handler) storeRegistrationCredentials(ctx context.Context, reg *config.AccountRegistration, accountID string, payload []byte) { + if payload == nil || h.credStore == nil { + return + } + if err := h.credStore.SaveCredential(ctx, accountID, reg.RegCredentialType, payload); err != nil { + logging.Warnf("registration %s: failed to store credential: %v", reg.ID, err) + } +} diff --git a/internal/api/handler_registrations_credentials_test.go b/internal/api/handler_registrations_credentials_test.go new file mode 100644 index 000000000..567bdb085 --- /dev/null +++ b/internal/api/handler_registrations_credentials_test.go @@ -0,0 +1,222 @@ +package api + +// Tests for the credential that may come with POST /api/register: it is +// checked when submitted and again when the registration is approved, with +// the same rules as POST /api/accounts/:id/credentials. + +import ( + "context" + "encoding/json" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const regCredRegID = "6f1c2a8e-3b4d-4e5f-8a9b-0c1d2e3f4a5b" + +func mustJSON(t *testing.T, v any) string { + t.Helper() + b, err := json.Marshal(v) + require.NoError(t, err) + return string(b) +} + +// registrationBody builds a POST /api/register body; credential fields are +// left out when empty. +func registrationBody(t *testing.T, provider, credType, credPayload string) string { + t.Helper() + body := map[string]any{ + "provider": provider, "external_id": "test-project", + "account_name": "Test", "contact_email": "owner@example.com", + } + if credType != "" { + body["credential_type"] = credType + } + if credPayload != "" { + body["credential_payload"] = credPayload + } + return mustJSON(t, body) +} + +// submitWithCredential runs submitRegistration with a store that records the +// created registration. It returns the stored registration (nil when none +// was created) and the handler error. +func submitWithCredential(t *testing.T, h *Handler, body string) (*config.AccountRegistration, error) { + t.Helper() + ctx := context.Background() + store := new(MockConfigStore) + var created *config.AccountRegistration + store.On("CreateAccountRegistration", ctx, mock.Anything).Run(func(args mock.Arguments) { + created = args.Get(1).(*config.AccountRegistration) + }).Return(nil).Maybe() + h.config = store + _, err := h.submitRegistration(ctx, &events.LambdaFunctionURLRequest{Body: body}) + return created, err +} + +func TestSubmitRegistration_WithoutCredentialIsAccepted(t *testing.T) { + reg, err := submitWithCredential(t, &Handler{credStore: &MockCredentialStore{}}, registrationBody(t, "gcp", "", "")) + require.NoError(t, err) + require.NotNil(t, reg) + assert.Empty(t, reg.RegCredentialType) + assert.Empty(t, reg.RegCredentialPayload) +} + +func TestSubmitRegistration_AcceptsValidCredential(t *testing.T) { + cases := map[string]struct{ provider, credType, payload string }{ + "gcp wif from gcloud": {"gcp", "gcp_workload_identity_config", mustJSON(t, gcloudWIFConfig())}, + "gcp wif with file source": {"gcp", "gcp_workload_identity_config", mustJSON(t, gcloudWIFConfigWith( + "credential_source", map[string]any{"file": "/var/run/secrets/azure/tokens/azure-identity-token"}))}, + "aws access keys": {"aws", "aws_access_keys", `{"access_key_id":"AKIAEXAMPLE","secret_access_key":"placeholder"}`}, + "azure secret": {"azure", "azure_client_secret", `{"client_secret":"placeholder"}`}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + reg, err := submitWithCredential(t, &Handler{credStore: &MockCredentialStore{}}, + registrationBody(t, tc.provider, tc.credType, tc.payload)) + require.NoError(t, err) + require.NotNil(t, reg) + assert.Equal(t, tc.credType, reg.RegCredentialType) + // MockCredentialStore "encrypts" to the plaintext. + assert.JSONEq(t, tc.payload, reg.RegCredentialPayload) + }) + } +} + +func TestSubmitRegistration_RejectsInvalidCredential(t *testing.T) { + wif := "gcp_workload_identity_config" + cases := map[string]struct{ provider, credType, payload, msg string }{ + "type without payload": {"gcp", wif, "", "must be provided together"}, + "payload without type": {"gcp", "", mustJSON(t, gcloudWIFConfig()), "must be provided together"}, + "unknown type": {"gcp", "gcp_oauth_client", `{"a":"b"}`, "credential_type must be one of"}, + "gcp type for aws account": {"aws", wif, mustJSON(t, gcloudWIFConfig()), `cannot be used with provider "aws"`}, + "aws type for gcp account": {"gcp", "aws_access_keys", `{"access_key_id":"a","secret_access_key":"b"}`, `cannot be used with provider "gcp"`}, + "azure type for aws account": {"aws", "azure_client_secret", `{"client_secret":"x"}`, `cannot be used with provider "aws"`}, + "junk payload": {"gcp", wif, "not json at all", "must be a JSON object"}, + "truncated payload": {"gcp", wif, `{"type":"external_account"`, "must be a JSON object"}, + "array payload": {"gcp", wif, `["external_account"]`, "must be a JSON object"}, + "null payload": {"gcp", wif, "null", "must be a JSON object"}, + "string payload": {"gcp", wif, `"external_account"`, "must be a JSON object"}, + "empty object": {"gcp", wif, "{}", "must not be empty"}, + "missing token_url": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("token_url", "")), `"token_url" must be a non-empty string`}, + "unknown key": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("token_info_url", "https://sts.googleapis.com/v1/introspect")), `unknown key "token_info_url"`}, + "wrong wif type": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("type", "service_account")), `type="external_account"`}, + "non-google token_url": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("token_url", "https://token.example.net/v1/token")), "token_url must be an https URL on sts.googleapis.com"}, + "http token_url": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("token_url", "http://sts.googleapis.com/v1/token")), "token_url must be an https URL"}, + "non-google impersonation": {"gcp", wif, mustJSON(t, gcloudWIFConfigWith("service_account_impersonation_url", + "https://iamcredentials.googleapis.com.example.net/v1/x")), "service_account_impersonation_url must be an https URL"}, + "service account missing key": {"gcp", "gcp_service_account", `{"type":"service_account","project_id":"p","client_email":"e"}`, `missing required key "private_key"`}, + "aws keys missing secret": {"aws", "aws_access_keys", `{"access_key_id":"a"}`, `missing required key "secret_access_key"`}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + reg, err := submitWithCredential(t, &Handler{credStore: &MockCredentialStore{}}, + registrationBody(t, tc.provider, tc.credType, tc.payload)) + require400(t, err, tc.msg) + assert.Nil(t, reg, "an invalid credential must not create a registration") + }) + } +} + +// Without a credential store nothing can be kept, so neither field is +// recorded on the registration (the payload was already dropped before). +func TestSubmitRegistration_NoCredentialStoreRecordsNoCredential(t *testing.T) { + reg, err := submitWithCredential(t, &Handler{}, + registrationBody(t, "gcp", "gcp_workload_identity_config", mustJSON(t, gcloudWIFConfig()))) + require.NoError(t, err) + require.NotNil(t, reg) + assert.Empty(t, reg.RegCredentialType) + assert.Empty(t, reg.RegCredentialPayload) +} + +// recordingCredStore records SaveCredential calls. +type recordingCredStore struct { + MockCredentialStore + saved map[string][]byte +} + +func (r *recordingCredStore) SaveCredential(_ context.Context, accountID, credType string, payload []byte) error { + if r.saved == nil { + r.saved = map[string][]byte{} + } + r.saved[accountID+"/"+credType] = payload + return nil +} + +const gcpApprovalBody = `{"name":"Test","provider":"gcp","external_id":"test-project",` + + `"gcp_auth_mode":"workload_identity_federation","gcp_client_email":"` + regCredSAEmail + `"}` + +// approveWithCredential approves a pending registration whose stored +// credential is credType/payload (MockCredentialStore "decrypts" to the +// stored text). +func approveWithCredential(t *testing.T, credType, payload, approvalBody string) (*MockConfigStore, *recordingCredStore, any, error) { + t.Helper() + ctx := context.Background() + store := new(MockConfigStore) + store.On("GetAccountRegistration", mock.Anything, regCredRegID).Return(&config.AccountRegistration{ + ID: regCredRegID, Status: "pending", Provider: "gcp", ExternalID: "test-project", + RegCredentialType: credType, RegCredentialPayload: payload, + }, nil) + store.On("TransitionRegistrationStatus", mock.Anything, mock.Anything, "pending", mock.Anything).Return(nil).Maybe() + store.On("UpdateAccountRegistration", mock.Anything, mock.Anything).Return(nil).Maybe() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + creds := &recordingCredStore{} + h := &Handler{auth: mockAuth, config: store, credStore: creds} + result, err := h.approveRegistration(ctx, &events.LambdaFunctionURLRequest{Body: approvalBody}, regCredRegID) + return store, creds, result, err +} + +func TestApproveRegistration_SavesCheckedCredential(t *testing.T) { + payload := mustJSON(t, gcloudWIFConfig()) + store, creds, result, err := approveWithCredential(t, "gcp_workload_identity_config", payload, gcpApprovalBody) + require.NoError(t, err) + account, ok := result.(*config.CloudAccount) + require.True(t, ok) + assert.True(t, account.Enabled) + store.AssertCalled(t, "CreateCloudAccount", mock.Anything, mock.Anything) + require.Contains(t, creds.saved, account.ID+"/gcp_workload_identity_config") + assert.JSONEq(t, payload, string(creds.saved[account.ID+"/gcp_workload_identity_config"])) +} + +func TestApproveRegistration_WithoutCredentialSavesNothing(t *testing.T) { + _, creds, _, err := approveWithCredential(t, "", "", gcpApprovalBody) + require.NoError(t, err) + assert.Empty(t, creds.saved) +} + +// A registration stored before submit-time checks existed can still hold a +// credential that fails them. Approval refuses it with a 400 and leaves the +// registration pending: nothing is transitioned, created or saved. +func TestApproveRegistration_RefusesInvalidStoredCredential(t *testing.T) { + wif := "gcp_workload_identity_config" + cases := map[string]struct{ credType, payload, body, msg string }{ + "non-google token_url": {wif, mustJSON(t, gcloudWIFConfigWith("token_url", "https://token.example.net/v1/token")), + gcpApprovalBody, "token_url must be an https URL on sts.googleapis.com"}, + "non-google impersonation url": {wif, mustJSON(t, gcloudWIFConfigWith("service_account_impersonation_url", "https://my-iamcredentials.googleapis.com/v1/x")), + gcpApprovalBody, "service_account_impersonation_url must be an https URL"}, + "junk payload": {wif, "not json", gcpApprovalBody, "must be a JSON object"}, + "payload without type": {"", mustJSON(t, gcloudWIFConfig()), gcpApprovalBody, "must be provided together"}, + "unknown type": {"gcp_oauth_client", `{"a":"b"}`, gcpApprovalBody, "credential_type must be one of"}, + "wrong type in wif slot": {wif, `{"type":"service_account","project_id":"p","private_key":"k","client_email":"e"}`, gcpApprovalBody, "unknown key"}, + "type does not match approved provider": {wif, mustJSON(t, gcloudWIFConfig()), + `{"name":"Test","provider":"azure","external_id":"00000000-0000-0000-0000-000000000000","azure_auth_mode":"managed_identity"}`, + `cannot be used with provider "azure"`}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + store, creds, result, err := approveWithCredential(t, tc.credType, tc.payload, tc.body) + assert.Nil(t, result) + require400(t, err, "the credentials submitted with this registration are not valid") + require400(t, err, tc.msg) + store.AssertNotCalled(t, "TransitionRegistrationStatus", mock.Anything, mock.Anything, mock.Anything, mock.Anything) + store.AssertNotCalled(t, "CreateCloudAccount", mock.Anything, mock.Anything) + assert.Empty(t, creds.saved) + }) + } +}