Repository navigation
Address the follow-ups of the provenance reviews - #4594
kubernetes-prow[bot] merged 1 commit into
Conversation
kubernetes-prow
Bot
added
release-note
kubernetes-prow
Bot
added
approved
29cca2a to
d18b772
Compare
Looking for a bundle of the same statement on an image read every layer of every referrer with the predicate type annotation in full, so a large layer planted on a staged image could slow down or exhaust the signing step. Only layers which the referrer manifest names as sigstore bundles of a known size of at most 4 MiB are read now, as stored and without decompressing them, since cosign writes bundles uncompressed. A layer that can't be read is skipped with a warning instead of failing the step. krel release takes the account to sign the final provenance as from the new --signing-account flag, which defaults to GOOGLE_SERVICE_ACCOUNT_NAME, instead of reading the environment while signing, so that the account is part of the release options. It is only needed when there is something to sign. Submitted jobs sign as the account of the release job, so the flag warns together with --submit. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
d18b772 to
2565b81
Compare
|
/retest |
| } | ||
| defer reader.Close() | ||
|
|
||
| return io.ReadAll(io.LimitReader(reader, maxBundleSize)) |
There was a problem hiding this comment.
nit: The limited reader doesn't only back things up. go-containerregistry checks the digest only when its reader returns EOF (internal/verify). A layer of exactly maxBundleSize makes our LimitReader stop first, so its digest is never checked. Could we use maxBundleSize+1 here, or drop the extra limit, since fetchBlobURL already limits the read to the size in the manifest?
There was a problem hiding this comment.
Done in #4601: the limited reader allows maxBundleSize+1 now, so the end of the blob is always read. In practice the digest was checked already, since Go's HTTP client returns EOF with the last bytes of a response with a Content-Length; a test against a registry serving a swapped blob of exactly maxBundleSize confirms it is rejected.
| // doesn't check the blob against, doesn't belong to a bundle either. | ||
| desc := &manifest.Layers[i] | ||
| if !strings.HasPrefix(string(desc.MediaType), bundleMediaTypePrefix) || | ||
| desc.Size <= 0 || desc.Size > maxBundleSize { |
There was a problem hiding this comment.
When the blob can't be read from the registry, remoteImageLayer.Compressed() falls back to the urls of the descriptor. So a planted referrer can make the signing step fetch from any HTTPS host. Size and digest are still checked, and this was already the case before, but should we also skip descriptors with URLs, since cosign never sets them for bundles? Fine as a follow-up.
There was a problem hiding this comment.
Done in #4601: bundle layers with urls are skipped now. go-containerregistry already refuses private and link-local hosts, so a local test can't observe the fetch; the layer checks moved into isBundleLayer with a table test instead.
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert, xmudrii 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The two follow-ups from the reviews of #4551 and #4550:
application/vnd.dev.sigstore.bundle…) of a known size of at most 4 MiB are read now, before anything is downloaded. They are read as stored, without decompressing them, since cosign writes bundles uncompressed: a small gzip layer can't expand past the limit, and the registry client checks the blob against the size and digest of the manifest. A limited reader backs that up. A bundle layer that can't be read, like one planted with a blob that doesn't match, is skipped with a warning instead of failing the step; at worst the bundle gets attached once more.krel releasetakes the account to sign the final provenance as from the new--signing-accountflag, which defaults toGOOGLE_SERVICE_ACCOUNT_NAME, instead ofSignProvenancereading the environment. The account is part ofReleaseOptionsnow, so the release job, which sets the variable, behaves the same. The account is only needed when there is something to sign, so local mock runs without final attestations don't need it. The flag only applies to local runs (--submit=false); submitted jobs sign as the account of the release job, so setting it together with--submitlogs a warning.Which issue(s) this PR fixes:
Part of #2616
Special notes for your reviewer:
The registry test plants our statement in a layer of another media type, in a bundle layer above the limit and in a gzipped bundle layer; none counts. A referrer whose bundle layers have an unknown size or a blob that was never pushed is skipped. Without the size check, or with the layers decompressed again, that test fails. The release tests pass the account through the options. The regenerated fake uses counterfeiter v6.12.2 like the existing one, so it only changes
SignProvenance.Does this PR introduce a user-facing change?