Migrate provenance attestation to SLSAv1 - #4502
Conversation
saschagrunert
left a comment
There was a problem hiding this comment.
Nice migration. The move from the custom SLSA v0.2 predicate to upstream v1 types, the protobuf buildType definition, and the per-tag source tracking are all solid improvements. A few comments inline.
| checked = true | ||
| } | ||
|
|
||
| if !checked && sub.GetDigest() != nil { |
There was a problem hiding this comment.
A subject with a nil digest (field never set) silently passes verification here. GetDigest() returns nil for an unset map, so !checked && nil != nil is false and no error is reported. A subject with an empty map[string]string{} correctly errors (non-nil, len 0), but one with a completely absent digest is accepted without any hash check.
| if !checked && sub.GetDigest() != nil { | |
| if !checked { |
Separately, the digest values themselves are compared as raw strings with no format validation (hex length, charset). A malformed entry like {"sha256": "not-a-hash"} with a recognized algorithm key would be compared verbatim and silently fail. Validating the digest format before matching (algorithm name, expected hex length, hex charset) would make this more robust.
There was a problem hiding this comment.
OK, I've added another commit making the digest checks a little more robust/
| ) ([]intoto.Subject, error) { | ||
| ) ([]*intoto.ResourceDescriptor, error) { | ||
| // Create the dummy statement to read artifacts | ||
| dummy := provenance.NewSLSAStatement() |
There was a problem hiding this comment.
GetStagingSubjects and GetBuildSubjects (line 362) still create a provenance.NewSLSAStatement() from the old bom package internally, call its ReadSubjectsFromDir/AddSubjectFromFile methods, and return dummy.Subject. This works because the pre-release bom also switched its Subject field to []*intoto.ResourceDescriptor, but it keeps the sigs.k8s.io/bom/pkg/provenance dependency around. Worth a TODO to migrate these to drop that dependency once the bom release lands?
There was a problem hiding this comment.
So as part of its validation functions, bom has these functions to compute provenance subjects from paths and files. The goal is to keep them and use still them here. The dep should not be a concern as we are using bom for the SBOMs anyway.
| google.golang.org/protobuf v1.36.12 | ||
| k8s.io/apimachinery v0.36.4 | ||
| sigs.k8s.io/bom v0.7.1 | ||
| sigs.k8s.io/bom v0.7.2-0.20260822012113-f3fd7c6e0240 |
There was a problem hiding this comment.
Pre-release bom commit pin. Is there a timeline or tracking issue for the v0.7.2 bom release? If that commit gets rebased this breaks.
There was a problem hiding this comment.
OK, so the plan is that I will be updating k/release with bom prereleases as we satisfy any bugs or issues that arise as we modernize the SBOM and attestations generated by krel.
Once the work here is ready I will cut a bom release, pin it here and we cut a new krel tag (after the v1.37 release)
|
Ah this needs a rebase now. |
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
We now record the real repo, even if $K8S_ORG/$K8S_REPO are set. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
This adds the krel buildType definition for the SLSA build attestations Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
This commit modifies the attester to record the actual build point (the tagged commit) instead of blindly using HEAD. Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Since we are updating the provenance, the current verification breaks. The current verify process is not very valuable because it is not fatal, the attestation is not signed and we don't actually verify the predicate. This change modifies the attestation verification to do a simple parse of the subjects and check the hashes (regardless of the attestation type). Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: puerco, saschagrunert 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 |
|
ok, I rebased the branch and pushed again. PTAL. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
In preparation to finishing the provenance work, this PR moves the provenance generation to SLSA v1.
I've scoped this PR is to only move the predicate type to v1. I added a fix in 49dea93 to attest the correct commit but I realized the PR is large as it is so once we get this in, I will start sending the rest of the fixes to the attestation contents and signing.
Aside from the code changes required to move our provenance to v1, this commit includes the foundations for the krel buildType definition (see the SLSA spec for more info). The build definition captures the krel-specific data of our builds. This is a full protobuf definition as we will check it into the upcoming slsa verifier so that it recognizes our build automatically.
Which issue(s) this PR fixes:
None
Part of kubernetes/enhancements#3027
Special notes for your reviewer:
Instead of using our custom slsa predicate, we now use the upstream types from intoto (we've moved other tools to the same a while ago).
I've migrated the simple check we do on the attestation during
releaseto do the same in the new provenance format. This is not the final verification. this merely emulates the non-fatal verification we do today, as part of the upcoming fixes I will implement the full slsa check as defined in the spec (all of this in upcoming PRs). The attestation check is still non-fatal as we have it now.I've run full stage and release and things seem to be working as expected.
I've included the resulting attestation in this PR if needed: provenance.json
Does this PR introduce a user-facing change?