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/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) 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.