fix(provx/gcp): trust the issuer the caller names, not the one compiled in - #5
Merged
Conversation
…ed 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.
…e 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits: the fix, and the test weakness that let it exist.
1.
fix(provx/gcp): trust the issuer the caller namesprovx/gcpresolved its issuer from theprovx.Endpointconstant, so every workload identity provider it created trustedhttps://oidc.cloud.formae.airegardless of the issuer the caller had been given.gcp.Newnow takes the issuer and validates it withprovx.ParseIssuer— the same canonical-origin rule the AWS provisioner has always held it to.Why it matters. The caller was already carrying an issuer and had nowhere to put it. formae's
internal/cli/connect/gcpprovision.gotakes anissuerargument,openSessionvalidates it against what the control plane reports, and then the call togcp.Newdrops it.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, and the failure surfaces later as an opaque exchange rejection rather than as the wrong issuer it is.
This is the AWS path's existing contract:
provxaws.Newtakes the issuer, and the issuer travels verbatim from the control plane into the artifacts. This makes GCP match.Testing. The gap that let this stand was that no test asserted the created provider's
issuerUri. The fake-backed tests now trust a deliberately non-production issuer, so a value compiled in rather than passed through fails them:TestProviderTrustsTheIssuerItWasGiven— the created provider carries the caller's issuer.TestRefusesAnIssuerThatIsNotACanonicalOrigin— empty, http, trailing slash and ported issuers are refused at construction, not at provisioning.2.
test(provx/aws): stop testing the issuer against the value it would be hardcoded toThe same weakness, on the side that happens to be correct.
provx/awsdoes take its issuer as a parameter and does thread it into the provider URL and the trust policy, and the tests pin both — theUrlcheck inconnect_provider_test.goand the whole trust-policy document inrole_test.go, condition keys included. ButtestIssuerwas the production issuer, so all of it would pass equally against an issuer resolved fromprovx.Endpoint. Those assertions pinned the issuer's shape and never its provenance, which is precisely how the GCP bug above went unnoticed.testIssuernow points at a non-production origin. No production code changes and the suite still passes — which is the useful outcome: it confirms the parameter really is what reaches the artifacts, instead of leaving that true by luck and unproven.Scope
go test ./...passes acrossprovx,provx/awsandprovx/gcp.cmd/ooxbuilds; its two GCP call sites now passprovx.Endpointexplicitly, so its behaviour is unchanged.Follow-up
formae must re-pin
oox/provxand pass its issuer throughnewGCPProvisionerfor the fix to reachformae connect gcp. That unblocks the GCP half of platform-engineering-labs/formae#696, whose test currently skips for exactly this reason.