Verify staged metadata.json authenticity with a cosign signature#340
Conversation
metadata.json is the root of trust for the publish step - it lists every
release artifact and its checksum - yet it is read from the same staging
bucket prefix that the artifacts are written to. On its own it carries no
proof of provenance, so a principal able to write to the bucket could stage
a self-consistent {metadata.json, artifacts} set that publish would unpack,
retag, push and sign with the production KMS key (CWE-345).
Bind a published release to metadata genuinely produced by the staging
pipeline by signing metadata.json with the release KMS key - which bucket
writers do not hold - and verifying that signature at publish time:
- pkg/sign: add SignMetadata/VerifyMetadata built on the existing kmssigner
(RSA PKCS#1 v1.5 over SHA-512, matching the release signing key). The
public key is fetched from KMS, so an actor who overwrites metadata.json
and its signature still cannot forge a signature that verifies.
- cmrel sign metadata: sign a metadata.json on disk, writing a detached,
base64-encoded metadata.json.sig alongside it. Intended to be run when
staging a release, before upload.
- release.Staged: retain the exact metadata.json bytes and best-effort load
metadata.json.sig, exposed via MetadataBytes()/MetadataSignature().
- gcb publish: verify the signature before unpacking anything. A new
--require-signed-metadata flag (plumbed via _REQUIRE_SIGNED_METADATA)
fails closed when a signature is missing; while it is false during
roll-out, a signature that is present is always verified and an invalid
one always fails.
Verification is off by default so this is a no-op on today's unsigned
releases; it becomes enforced once the cert-manager staging job signs
metadata on all supported branches.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Felix Phipps <fphipps@paloaltonetworks.com>
cert-manager-prow
Bot
added
dco-signoff: yes
…d KMS RSA Replace the bespoke RSA PKCS#1 signing/verification of metadata.json with cosign sign-blob / verify-blob, keyed off the existing release KMS key. This reuses the same tool and key already used to sign container images and Helm charts, so there is one signing model to reason about and rotate, and much less code to maintain. cosign also derives the digest algorithm from the KMS key itself, removing the previous hard-coded SHA-512 assumption. - pkg/sign/cosign: add SignBlob/VerifyBlob wrapping cosign sign-blob and verify-blob with --key gcpkms://..., plus unit tests for the argument construction (including the cosign key format). - cmrel sign metadata: shell out to cosign.SignBlob (new --cosign-path flag); cosign writes metadata.json.sig directly, dropping the in-process crypto and base64 handling. - gcb publish: verifyStagedMetadata writes the staged metadata and signature to a temp dir and runs cosign.VerifyBlob before unpacking anything. The --require-signed-metadata roll-out behaviour is unchanged. - Remove pkg/sign/metadata.go and its test. Verification remains off by default (--require-signed-metadata=false), so this is a no-op on today's unsigned releases and becomes enforced once the cert-manager staging job signs metadata on all supported branches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Felix Phipps <fphipps@paloaltonetworks.com>
FelixPhipps
changed the title
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Not sure if we can maybe remove the signing command and save a bit of code - what do you think?
There was a problem hiding this comment.
suggestion: I'm not sure if we need a sign command? The cert-manager release process is defined in a makefile and cosign is available.
Could we simply call cosign directly in the makefile? What's the justification for adding a signing command to cmrel?
There was a problem hiding this comment.
Updated, now calls the existing cosign tool as oppose to adding a new one.
…tion Address review feedback: - Remove the 'cmrel sign metadata' subcommand and cosign.SignBlob. cosign is already available as a tool in the cert-manager release Makefile, so the staging step can call 'cosign sign-blob' directly; a cmrel wrapper added surface for no real benefit. Publishing still verifies via cosign.VerifyBlob. - Split the publish-side check into doVerifyStagedMetadata (does the work, returns an error on any failure) and verifyStagedMetadata (applies the --require-signed-metadata policy: tolerate-with-warning when unset, fail when set). This removes the per-branch RequireSignedMetadata checks that made the original hard to follow. As a side effect, a present-but-invalid signature is now also tolerated while the flag is unset, which is the intended roll-out behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Felix Phipps <fphipps@paloaltonetworks.com>
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
I know you'll think "oh not another round of suggestions!" but I promise there's something valuable here! By slightly changing how we write this new code we can really evolve how defensively we code this.
| metadataBytes: metaBytes, | ||
| metadataSignature: metaSignature, |
There was a problem hiding this comment.
suggestion: there's an interesting design decision here - I think it's worth digging into it and thinking about it a sec.
We're storing the signature and the metadata in the staged release, so it can be used later. We certainly need to store the metadata for later.
As written though, any future refactor that touches meta (or the newly added metadataBytes) must always remember to ensure that the signature is validated before using meta. It's a really common cryptographic mistake to just forget to verify the signature (usually because of a refactor which confuses the control flow). An alternative version of that is that unverified data is allowed to be used before the signature is validated (which happens comically regularly in GPG and is one of the reasons GPG can be... challenging).
There's a more defensive way of designing this:
- Validate the signature here, at the earliest possible opportunity
- Fail early here if signature validation fails (still allow RequireSignedMetadata to be false)
- Don't store the signature or the metadataBytes in
Staged
If we do that then all constructed Staged structs are either:
- Signature verified
- Signature not verified but explicitly skipped by the user (for the initial rollout)
Further, that means that none of the code which uses a Staged struct has to even think about signatures. Either you have an initialised struct and it's safe to use or else this function returned an error and the whole thing stops either way.
There was a problem hiding this comment.
I like this design a lot better and I've implemented it in the most recent commit. Staged no longer stores metadataBytes or metadataSignature, and the accessors are gone. NewStagedRelease now takes a release.MetadataVerifier and runs it at the earliest opportunity — right after loading metadata.json, failing early if it doesn't verify. GetRelease forwards the verifier; ListReleases passes nil (listing is informational and must not require KMS/cosign).
The result is that every constructed Staged is either signature-verified or had verification explicitly skipped by the caller, and nothing downstream has to think about signatures.
| // RequireSignedMetadata, if true, causes publishing to fail unless the | ||
| // staged metadata.json is accompanied by a valid signature (metadata.json.sig). | ||
| RequireSignedMetadata bool |
There was a problem hiding this comment.
nitpick: Default value for bool is false - which means the default value here is unsafe because it allows signature validation to fail. It's a little better design-wise to default to the safe value.
Concretely: If we changed this to AllowInvalidMetadataSignature bool (or something) then the default value is safer (i.e. missing / invalid signatures are not allowed). It means we have to be sure we're explicitly opting in to the flag everywhere (which is fine for the initial rollout - because nothing will be signed)
(this is a nitpick because it's highly unlikely this will ever cause a problem - but it's a good defensive programming practice)
There was a problem hiding this comment.
Agreed, inverted it. It's now AllowInvalidMetadataSignature, defaulting to false — and verification is enforced unless explicitly opted out of.
… default Address review feedback on the metadata signature verification design: - Verify the metadata signature as the release is loaded, and stop storing the raw metadata bytes or signature on Staged. NewStagedRelease now takes a release.MetadataVerifier which is run before the Staged is returned; GetRelease forwards it and ListReleases passes nil (listing is informational and must not require KMS/cosign). This means every Staged handed to the rest of the program has either had its metadata authenticated or had verification explicitly skipped by the caller, so no downstream code can accidentally use unverified metadata - the unverified bytes simply are not retained. - Invert the roll-out flag so the safe behaviour is the zero value. The gcb publish / publish flag is now --allow-invalid-metadata-signature (substitution _ALLOW_INVALID_METADATA_SIGNATURE), defaulting to false, so signature verification is enforced unless explicitly opted out of. A forgotten flag now fails closed instead of publishing unverified content. During roll-out - before staging signs metadata.json on all supported cert-manager branches - publishing an unsigned release requires explicitly passing --allow-invalid-metadata-signature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Felix Phipps <fphipps@paloaltonetworks.com>
|
Seems like it's good to go as Ash already reviewed. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maelvls The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
Bot
added
the
approved
3 participants
Description
Problem
The cmrel gcb publish command fetches a staged release from GCS and, before signing it with the production KMS key and pushing to public registries, decides what to publish based entirely on metadata.json — a file that lists every artifact and its checksum. That file is read from the same staging bucket prefix that the artifacts are written to, and the bucket is writable by more than just the pipeline (the Cloud Build SA plus several maintainer accounts). The current code trusts it with no proof of provenance:
Solution
Sign metadata.json at stage time with the release KMS key — which bucket writers do not hold — and verify that detached signature at publish time, before anything is unpacked. Signing and verification are done with cosign (sign-blob / verify-blob --key gcpkms://…), reusing the same tool and key already used to sign container images and Helm charts — one signing model to maintain and rotate, and cosign derives the digest algorithm from the key itself. pkg/sign/cosign gains SignBlob/VerifyBlob; a new cmrel sign metadata writes metadata.json.sig; and gcb publish verifies it against the KMS public key (fetched from KMS, so overwriting the signature can't forge validity).
A --require-signed-metadata flag fails closed, defaulting off during roll-out (a present signature is always verified, an invalid one always fails). Legitimate releases behave unchanged.
Note: this only takes effect once the cert-manager staging job (make upload-release) installs cosign and calls cmrel sign metadata to produce metadata.json.sig, on all supported branches (master, release-1.21, release-1.20). Those are separate follow-up PRs; the flag is then flipped to true.
Test plan
release-note