From a6951d66246f13ce8d66ad272bc3089f7902abf2 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Thu, 27 Aug 2026 16:12:44 -0700 Subject: [PATCH 1/2] fix(provx/gcp): trust the issuer the caller names, not the one compiled in The GCP provisioner resolved its issuer from the provx.Endpoint constant, so the workload identity provider it created always trusted https://oidc.cloud.formae.ai no matter which issuer the caller had been given. The AWS provisioner has taken the issuer as a parameter since it was written, for the reason that applies equally here: the issuer is produced by the control plane and travels verbatim into the trust artifacts, so a provisioner that substitutes its own establishes trust for an issuer nobody named. The caller was already carrying one and had nowhere to put it: formae's connect path takes an issuer argument, validates it against what the control plane reports, and then drops it on the floor at the call to gcp.New. Production hides this because both values are the same string there; anywhere else -- a staging installation, an acceptance test with its own issuer -- provisioning reports success and establishes trust that can never accept a token. New now takes the issuer and validates it with provx.ParseIssuer, the same canonical-origin rule the AWS side holds it to: a value with a port or a path cannot be compared against what Google stores, so it is refused at construction rather than producing a provider that silently never matches. The absence of a test asserting the created provider's issuerUri is what let this stand, so the fake-backed tests now trust a non-production issuer and one of them asserts the provider carries it. --- cmd/oox/cli/provision_gcp.go | 5 +++-- provx/gcp/fake_test.go | 22 +++++++++++++++++----- provx/gcp/gcp.go | 24 ++++++++++++++++++------ provx/gcp/gcp_test.go | 31 +++++++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 13 deletions(-) diff --git a/cmd/oox/cli/provision_gcp.go b/cmd/oox/cli/provision_gcp.go index 946548a..372c1c5 100644 --- a/cmd/oox/cli/provision_gcp.go +++ b/cmd/oox/cli/provision_gcp.go @@ -6,6 +6,7 @@ import ( "log/slog" "time" + "github.com/platform-engineering-labs/oox/provx" "github.com/platform-engineering-labs/oox/provx/gcp" "github.com/spf13/cobra" ) @@ -48,7 +49,7 @@ var ProvisionGCPCreate = &cobra.Command{ ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - prov, err := gcp.New(ctx, slog.New(Logger), project, tenantId, installationId) + prov, err := gcp.New(ctx, slog.New(Logger), project, tenantId, installationId, "https://"+provx.Endpoint) if err != nil { return err } @@ -87,7 +88,7 @@ var ProvisionGCPDelete = &cobra.Command{ ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() - prov, err := gcp.New(ctx, slog.New(Logger), project, tenantId, installationId) + prov, err := gcp.New(ctx, slog.New(Logger), project, tenantId, installationId, "https://"+provx.Endpoint) if err != nil { return err } diff --git a/provx/gcp/fake_test.go b/provx/gcp/fake_test.go index 9fd1c6f..265bf8f 100644 --- a/provx/gcp/fake_test.go +++ b/provx/gcp/fake_test.go @@ -238,20 +238,32 @@ func errorResponse(e *googleError) (*http.Response, error) { return jsonResponse(e.status, payload) } +// testIssuer is the issuer the fake-backed provisioners trust. It is not the +// production one on purpose: an issuer compiled in rather than taken from the +// caller would pass every test that used the production value. +const testIssuer = "https://issuer.test.example" + // newTestGCP builds a GCP provisioner wired to the fake. func newTestGCP(t *testing.T, f *fakeGoogle, tenant, installation string) *GCP { t.Helper() - g, err := New(t.Context(), discardLogger(), "test-project", tenant, installation, - option.WithoutAuthentication(), - option.WithEndpoint("https://example.invalid/"), - option.WithHTTPClient(&http.Client{Transport: f}), - ) + g, err := newTestGCPWithIssuer(t, f, tenant, installation, testIssuer) if err != nil { t.Fatalf("New: %v", err) } return g } +// newTestGCPWithIssuer is newTestGCP with the issuer left to the caller and +// the construction error returned, for the cases that are about the issuer. +func newTestGCPWithIssuer(t *testing.T, f *fakeGoogle, tenant, installation, issuer string) (*GCP, error) { + t.Helper() + return New(t.Context(), discardLogger(), "test-project", tenant, installation, issuer, + option.WithoutAuthentication(), + option.WithEndpoint("https://example.invalid/"), + option.WithHTTPClient(&http.Client{Transport: f}), + ) +} + func mustCreate(t *testing.T, g *GCP) *Result { t.Helper() res, err := g.Create(t.Context()) diff --git a/provx/gcp/gcp.go b/provx/gcp/gcp.go index 827961e..ce75522 100644 --- a/provx/gcp/gcp.go +++ b/provx/gcp/gcp.go @@ -50,16 +50,29 @@ type GCP struct { project string tenantId string installationId string + issuer provx.Issuer } // New builds a provisioner for one installation's connection to one project. // +// issuer is the outbound identity issuer the provider will trust, taken from +// the caller and validated here rather than compiled in. The AWS provisioner +// has always worked this way, and the reason is the same on both clouds: the +// issuer is produced by the control plane and travels verbatim into the trust +// artifacts, so a build that substituted its own would provision trust for an +// issuer the caller never named. +// // The context is the caller's: it bounds the client construction, and the // credential lookup that construction performs. opts are passed through to the // underlying Google clients, which is how tests reach a fake without a network // or a credential. -func New(ctx context.Context, logger *slog.Logger, project, tenantId, installationId string, +func New(ctx context.Context, logger *slog.Logger, project, tenantId, installationId, issuer string, opts ...option.ClientOption) (*GCP, error) { + iss, err := provx.ParseIssuer(issuer) + if err != nil { + return nil, err + } + client, err := provider.NewClient(ctx, opts...) if err != nil { return nil, err @@ -71,6 +84,7 @@ func New(ctx context.Context, logger *slog.Logger, project, tenantId, installati project: project, tenantId: tenantId, installationId: installationId, + issuer: iss, }, nil } @@ -181,12 +195,12 @@ func (gcp *GCP) assertProviderIsOurs(ctx context.Context, pool provider.PoolSpec if existing == nil || existing.Oidc == nil { return nil // nothing there yet, or nothing to disagree with } - if existing.Oidc.IssuerUri == issuerURI() { + if existing.Oidc.IssuerUri == gcp.issuer.URL() { return nil } return &ProviderNotOursError{ Name: existing.Name, - IssuerWanted: issuerURI(), + IssuerWanted: gcp.issuer.URL(), IssuerFound: existing.Oidc.IssuerUri, PoolID: poolID, ProviderID: providerID, @@ -207,7 +221,7 @@ func (gcp *GCP) spec(projectNumber string) (*provider.PoolSpec, *provider.OIDCPr oidcProvider := &provider.OIDCProviderSpec{ ProviderID: providerID, DisplayName: "formae ai Cloud OIDC", - IssuerURI: issuerURI(), + IssuerURI: gcp.issuer.URL(), AttributeMapping: map[string]string{ "google.subject": "assertion.sub", }, @@ -222,8 +236,6 @@ func (gcp *GCP) spec(projectNumber string) (*provider.PoolSpec, *provider.OIDCPr return pool, oidcProvider } -func issuerURI() string { return fmt.Sprintf("https://%s", provx.Endpoint) } - // subjectNamespaceCondition is the CEL expression Google evaluates against an // incoming assertion. It admits every subject this issuer mints and nothing // else. diff --git a/provx/gcp/gcp_test.go b/provx/gcp/gcp_test.go index 90f6d18..083a15e 100644 --- a/provx/gcp/gcp_test.go +++ b/provx/gcp/gcp_test.go @@ -200,6 +200,37 @@ func TestCreateReturnsTheProviderName(t *testing.T) { } } +// TestProviderTrustsTheIssuerItWasGiven holds the issuer to the caller's. It +// is the whole reason New takes one: an issuer resolved inside this package +// would provision trust for whatever this build was compiled against, no +// matter which issuer the control plane named. +func TestProviderTrustsTheIssuerItWasGiven(t *testing.T) { + f := newFakeGoogle() + mustCreate(t, newTestGCP(t, f, "tenant-a", "install-1")) + + for _, p := range f.createdProviders { + if p.Oidc.IssuerUri != testIssuer { + t.Errorf("provider issuerUri = %q, want %q", p.Oidc.IssuerUri, testIssuer) + } + } + if len(f.createdProviders) == 0 { + t.Fatal("no provider was created") + } +} + +// TestRefusesAnIssuerThatIsNotACanonicalOrigin fails the construction rather +// than the provisioning: an issuer with a path or a port cannot be compared +// against what Google stores, so accepting one here would defer the failure to +// a provider that silently never matches. +func TestRefusesAnIssuerThatIsNotACanonicalOrigin(t *testing.T) { + f := newFakeGoogle() + for _, issuer := range []string{"", "http://issuer.test.example", "https://issuer.test.example/", "https://issuer.test.example:8443"} { + if _, err := newTestGCPWithIssuer(t, f, "tenant-a", "install-1", issuer); err == nil { + t.Errorf("New accepted issuer %q", issuer) + } + } +} + // TestRefusesAProviderThatIsNotOurs guards the shared singleton: the pool and // provider ids are fixed, so an object of that name belonging to a different // deployment must not be adopted and rewritten. From 13e7bb4a74bef7c1c1febce42c4bfc9a4a678c37 Mon Sep 17 00:00:00 2001 From: Jeroen Soeters Date: Thu, 27 Aug 2026 17:32:29 -0700 Subject: [PATCH 2/2] test(provx/aws): stop testing the issuer against the value it would be hardcoded to The AWS provisioner takes its issuer as a parameter and threads it into the provider URL and the trust policy, and the tests pin both. But testIssuer was the production issuer, so every one of those assertions would pass just as well against an issuer resolved from provx.Endpoint instead of from the argument. They pinned the issuer's shape, never its provenance. That is the hole the GCP provisioner fell through: it did resolve its issuer from the constant, and no test noticed, because a test written with the production value cannot tell the two apart. Pointing testIssuer at a non-production origin closes it here. Nothing in the production code changes and the suite still passes, which is the result worth having: it confirms the parameter is genuinely what reaches the artifacts, rather than leaving that unproven and true by luck. --- provx/aws/aws_test.go | 7 ++++++- provx/aws/connect_provider_test.go | 6 +++--- provx/aws/create_test.go | 2 +- provx/aws/role_test.go | 2 +- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/provx/aws/aws_test.go b/provx/aws/aws_test.go index 23f25bf..594ceac 100644 --- a/provx/aws/aws_test.go +++ b/provx/aws/aws_test.go @@ -21,7 +21,12 @@ func (f *fakeSTS) GetCallerIdentity(ctx context.Context, in *sts.GetCallerIdenti return &sts.GetCallerIdentityOutput{Account: &f.account, Arn: &f.arn}, nil } -const testIssuer = "https://oidc.cloud.formae.ai" +// testIssuer is deliberately not the production issuer. The production code +// takes the issuer as a parameter, and the assertions below pin it as it +// appears in the provider URL and the trust policy — but written with the +// production value they would pass just as well against an issuer resolved +// from provx.Endpoint, which is the regression they exist to catch. +const testIssuer = "https://issuer.test.example" func TestNewVerifiesAccount(t *testing.T) { _, err := newWithClients(context.Background(), diff --git a/provx/aws/connect_provider_test.go b/provx/aws/connect_provider_test.go index f5b46d6..c20a909 100644 --- a/provx/aws/connect_provider_test.go +++ b/provx/aws/connect_provider_test.go @@ -33,7 +33,7 @@ func TestEnsureProviderCreates(t *testing.T) { if err != nil || out != ProviderCreated { t.Fatalf("out=%v err=%v", out, err) } - if gotURL != "https://oidc.cloud.formae.ai" || len(gotClients) != 1 || gotClients[0] != "sts.amazonaws.com" { + if gotURL != "https://issuer.test.example" || len(gotClients) != 1 || gotClients[0] != "sts.amazonaws.com" { t.Fatalf("create input: url=%q clients=%v", gotURL, gotClients) } } @@ -45,7 +45,7 @@ func TestEnsureProviderExistsValid(t *testing.T) { }, getOIDCProvider: func(in *iam.GetOpenIDConnectProviderInput) (*iam.GetOpenIDConnectProviderOutput, error) { return &iam.GetOpenIDConnectProviderOutput{ - Url: awssdk.String("oidc.cloud.formae.ai"), + Url: awssdk.String("issuer.test.example"), ClientIDList: []string{"sts.amazonaws.com"}, }, nil }, @@ -64,7 +64,7 @@ func TestEnsureProviderAddsClientID(t *testing.T) { return nil, &types.EntityAlreadyExistsException{} }, getOIDCProvider: func(*iam.GetOpenIDConnectProviderInput) (*iam.GetOpenIDConnectProviderOutput, error) { - return &iam.GetOpenIDConnectProviderOutput{Url: awssdk.String("oidc.cloud.formae.ai"), ClientIDList: []string{"other"}}, nil + return &iam.GetOpenIDConnectProviderOutput{Url: awssdk.String("issuer.test.example"), ClientIDList: []string{"other"}}, nil }, addClientID: func(in *iam.AddClientIDToOpenIDConnectProviderInput) (*iam.AddClientIDToOpenIDConnectProviderOutput, error) { if awssdk.ToString(in.ClientID) != "sts.amazonaws.com" { diff --git a/provx/aws/create_test.go b/provx/aws/create_test.go index 34c6faf..7ccb357 100644 --- a/provx/aws/create_test.go +++ b/provx/aws/create_test.go @@ -52,7 +52,7 @@ func TestCreateRerunConverges(t *testing.T) { }, getOIDCProvider: func(*iam.GetOpenIDConnectProviderInput) (*iam.GetOpenIDConnectProviderOutput, error) { return &iam.GetOpenIDConnectProviderOutput{ - Url: awssdk.String("oidc.cloud.formae.ai"), + Url: awssdk.String("issuer.test.example"), ClientIDList: []string{"sts.amazonaws.com"}, }, nil }, diff --git a/provx/aws/role_test.go b/provx/aws/role_test.go index 71aa9e4..99842c8 100644 --- a/provx/aws/role_test.go +++ b/provx/aws/role_test.go @@ -17,7 +17,7 @@ import ( // policy by unmarshal-and-compare, not byte equality. func assertSemanticTrustDoc(t *testing.T, doc string) { t.Helper() - want := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Federated":"arn:aws:iam::111122223333:oidc-provider/oidc.cloud.formae.ai"},"Action":"sts:AssumeRoleWithWebIdentity","Condition":{"StringEquals":{"oidc.cloud.formae.ai:aud":"sts.amazonaws.com","oidc.cloud.formae.ai:sub":"fai:t/i"}}}]}` + want := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Federated":"arn:aws:iam::111122223333:oidc-provider/issuer.test.example"},"Action":"sts:AssumeRoleWithWebIdentity","Condition":{"StringEquals":{"issuer.test.example:aud":"sts.amazonaws.com","issuer.test.example:sub":"fai:t/i"}}}]}` var gotV, wantV any if err := json.Unmarshal([]byte(doc), &gotV); err != nil { t.Fatalf("trust policy does not parse: %v\n%s", err, doc)