-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
editorial: clarify requirements around cache use by the build platform #901
Conversation
✅ Deploy Preview for slsa ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b984852
to
88c070e
Compare
ae2cac9
to
9ea5416
Compare
9ea5416
to
f754e88
Compare
I'm kind of losing the rationale for this change. Could you update the PR description to explain? I'm no longer clear on the problem in the original text, so it's hard for me to say whether this updated text is an improvement. |
The PR description is updated to restate the problem that needs clarification. |
b53211c
to
88eb436
Compare
Thank you for updating the PR description. That really helps define the problem. I'm still not sure about the proposed changes. I find the new wording even more confusing than before, without resolving any of the original problems. In particular, I don't understand the bits about external communication and the provenance being identical. I feel like there are two main problems:
|
I feel like the following description to me was most clear in the desired outcome (matching cache vs no cache)
|
Agreed, while clarifying that the subject (which exists outside the provenance but within the ite6 statement) doesn't have to be identical unless the builds are bit for bit reproducible. |
I resolved all open threads in this issue as they affected the readability of this PR. @adityasaky, I think most of the open discussion was between us, so if you want to re-state any of it, then can you open a new thread? The part referenced above is the core part of this PR. This would involve removing the change to the provenance specification as well as lines 325-326. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the attention to detail in this thread all and to @arewm for continuing to drive this change forward.
The part referenced above is the core part of this PR. This would involve removing the change to the provenance specification as well as lines 325-326.
I think removing these hunks helps clarify the change.
Addresses slsa-framework#894 Signed-off-by: arewm <arewm@users.noreply.github.com>
Signed-off-by: arewm <arewm@users.noreply.github.com>
Signed-off-by: arewm <arewm@users.noreply.github.com>
Signed-off-by: arewm <arewm@users.noreply.github.com>
Signed-off-by: arewm <arewm@users.noreply.github.com>
88eb436
to
7fc41a4
Compare
The PR has been distilled to the core change as previously indicated. |
7fc41a4
to
b6c7401
Compare
Signed-off-by: arewm <arewm@users.noreply.github.com>
b6c7401
to
a57d635
Compare
- If the build platform is capable of providing the provenance information for | ||
an external resource when a cache is not in use, then the provenance | ||
information MUST remain unchanged if a cache is used. In other words, the | ||
information in the provenance MUST be identical whether or not the cache is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this + @mlieberman85's suggestion re the artifact hash addresses the reproducibility issue. There may be a separate question about whether intermediate artifacts that can impact the target artifact should be recorded for completeness but that's out of scope here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlieberman85 , do you have a suggestion to the content of this PR related to your earlier comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this makes sense to me, but I do wonder if it's not clear enough to someone who isn't familiar with what we're implying? Maybe just a clarification that unique identifiers such as checksums are what we're talking about here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unique identifiers of what? The produced artifact or the cached entries used?
If the former, are you suggesting that we indicate that the hash of the artifact can differ if the cache is used because we are not claiming anything about reproducibility of the artifact?
If the latter, that seems like it might fit better in the previous bullet like so:
- It MUST NOT be possible for one build to inject false entries into a build
cache used by another build, also known as "cache poisoning". In other
words, the output of the build MUST be identical whether or not the cache is
used. This SHOULD be achieved using unique identifiers in the cache such
as checksums
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, the output of the build MUST be identical whether or not the cache is used.
This implicitly requires reproducibility, no? Unless this only applies to the provenance predicate, allowing the hash of the produced artifact to still change.
I don't think that we definitively agreed on a strict improvement on the wording. I am going to close the PR and discussion can continue in the issue before another attempt of clarification is made. |
There was an inconsistency between the provenance model and build L3 requirements for how build caches might affect a build:
resolvedDependencies
).Clarification includes:
byproducts
which should not be present in the provenance.Clarification does NOT include:
Addresses #894
Signed-off-by: arewm arewm@users.noreply.github.com