Open
Conversation
loosebazooka
force-pushed
the
tuf-delegated-targets
branch
6 times, most recently
from
58d5ebf to
cd71b05
Compare
jku
approved these changes
Mar 6, 2026
Member
jku
left a comment
jku
left a comment
There was a problem hiding this comment.
Looks good to me:
- I would have raised the question of "do you really want more code in sigstore-java" but if there are any plans of extracting the tuf part into a library, then I think this is valid as is it makes the implementation spec complete
- I've read the code, left one note about reading local metadata
- I would not trust my ability to verify this code by just reading (since I've overlooked so many bugs in the corresponding python implementation) but I am much more confident about the conformance testing: it really should cover a lot of potential mistakes in this area
| if (localTargets.get().getSignedMeta().getVersion() > snapshotTarget.getVersion()) { | ||
| throw new SnapshotTargetVersionException( | ||
| roleName, snapshotTarget.getVersion(), localTargets.get().getSignedMeta().getVersion()); | ||
| } |
Member
There was a problem hiding this comment.
two possible issues here:
- if a local targets file is present... it should still be verified (hashes match, threshold of signatures is correct), right?
- If these checks are not done for other local file reads either, then no need to make this specific one more complicated in this PR
- If you're wondering why verify local "trusted" files: It potentially makes the client a bit more secure (even if local attack is tricky to defend against) but it also makes client safer against cache corruption and the rare case mentioned below (where local targets may no longer be signed by the correct keys and version numbers may have restarted)
- The conformance test suite does not try to test local cache corruption issues since it would require fully enforcing a specific storage format which feels wrong -- if someone wants to use sqlite they should be able to...
- targets version could in a very rare case go down (see the note in 5.3.11 in TUF spec: if the "snapshot memory" is wiped, repository is free to e.g. restart targets versions from 1 -- this seems like a case your code supports elsewhere) so the second if-clause seems unnecessary
Member
There was a problem hiding this comment.
the comment looks a bit dumb, I think I failed to select correct context: it's supposed to reference both if-clauses at the top of the method
- No support for succinct delegations - Removed eager download of all targets - SigstoreTufClient must now download the targets it wants explicitly - Rename update() to refresh() to match naming of other clients - Correctly handle url escaping in names - Delegated target meta is cached per session NOTE: this was produced with the help of AI coding tools Signed-off-by: Appu Goundan <appu@google.com>
loosebazooka
force-pushed
the
tuf-delegated-targets
branch
from
cd71b05 to
0a91cbb
Compare
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
2 participants
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.
NOTE: this was produced with the help of AI coding tools
Summary
Release Note
Documentation