-
Notifications
You must be signed in to change notification settings - Fork 272
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
Docs: Add repository tutorial based on metadata API #1685
Conversation
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 refreshment exercise :) I left some comments on the text itself.
I don't think the example is too broad, having everything at one place seems like a good idea. Maybe in time, more detailed examples can be added for some trickier cases (complex delegations, key rotations ...).
examples/repo_example/basic_repo.py
Outdated
|
||
# Common fields | ||
# ------------- | ||
# All roles have the same metadata container format. On the top-most level, |
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.
Maybe we can somehow point to the Metadata class here?
examples/repo_example/basic_repo.py
Outdated
# from which a client would download the target file. | ||
target_local_path = os.path.abspath(__file__) | ||
target_file_path = os.path.basename(__file__) | ||
target_file_info = TargetFile.from_file(target_file_path, target_local_path) |
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 have no good suggestion for improvement but these paths seem confusing. I admit I had to read it twice even though I was supposed to know what is going on 😁 Maybe an example path and/vs URL could help.
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 can see that how it confusing. I'll try to better explain/exemplify those two paths.
examples/repo_example/basic_repo.py
Outdated
# The timestamp role guarantees freshness of the repository metadata. It does | ||
# so by listing the latest snapshot (which in turn lists all the latest | ||
# targets) metadata. Choosing a short expiration interval, requires clients to | ||
# often re-download timestamp and thus immediately detect if new target files |
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.
Doesn't the client always try to download timestamp?
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.
Ha! So much for "fully internalized spec". Thanks for catching this. I'll reword.
# Just assume we do out-of-band signing for keys we don't have | ||
for key in [keys["root"], another_root_key] + new_root_keys: | ||
roles["root"].sign(SSlibSigner(key), append=True) | ||
|
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 the part "signing with a threshold of old keys AND a threshold of the new keys" needs an emphasis.
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 absolutely agree.
9b07a45
to
2fc5945
Compare
Pull Request Test Coverage Report for Build 1515912733
💛 - Coveralls |
Thanks for your review, @sechkova! I just finalised the example texts and squashed all commits into one. Your comments should be addressed in another commit on top of that for re-review convenience, i.e. 2fc5945. I'd appreciate another look. Also see the updated PR description, especially the "Questions for reviewers". |
30ab533
to
b3665f2
Compare
I just added very basic testing that runs the example script using Note that the test class is intended to be re-used for a similar example script that I have in the pipe. |
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.
Would be nice if more people provide feedback but you have my approval 😁
I also filled in the questionnaire.
Is this readable enough? Should this rather be a markdown document?
No strong opinion here, I'm ok with this one
Is it too broad? Should we move some usage scenarios to different files? E.g. hashed bin delegation will be in a separate file (see follow-up PR)
Nope, I like it,
How can we demonstrate validity? With a TrustedMetadata example?
Seems like the options are either a full Updater with a server or using TrustedMetadata by providing it with the metadata files as bytes. The second option sounds more interesting to me :)
How can we demonstrate usage? With a client example follow-up?
I have nothing better to propose.
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 this @lukpueh, beautiful work – very clear example and a testament to the Metadata API. 🎉
Is this readable enough? Should this rather be a markdown document?
To me this is very readable, appreciate the detail and care here.
Is it too broad? Should we move some usage scenarios to different files?
LGTM as is. Covers the common cases, I think.
How can we demonstrate validity? With a TrustedMetadata example?
Implicit in the below? And future work once the proposed repository API validation piece is implemented? i.e. I think this is a solid submission as-is.
How can we demonstrate usage? With a client example follow-up?
Could be a simple follow-on that runs a http server over the data generated here and uses ngclient to fetch the python file?
examples/repo_example/basic_repo.py
Outdated
# from within other metadata, and thus allows for repository consistency in | ||
# addition to protecting against rollback attacks. | ||
# | ||
# The expiry date, protects against freeze attacks and allows for implicit key |
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.
consistency:
# The expiry date, protects against freeze attacks and allows for implicit key | |
# The 'expiry' date protects against freeze attacks and allows for implicit key |
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.
The field name is 'expires'. Should we write "The 'expires' date... "? Does consistency beat grammar? :D
Regardless, I'll remove the stray comma.
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.
🤦 of course.
I think it is better to bind the comments to the variables, maybe:
"The date the metadata 'expires'..." ?
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.
LGTM, I think this is fine as the stop gap: it's demonstrating the basic elements that a repository needs to handle, without pretending to actually be a repository implementation.
My reaction to the "is it too broad" question is that we shouldn't put a lot of effort into expanding or splitting it (although the hashed bin example should be useful), if we can push the repository library and its example code (or a repository tool) forward instead...
The repository validation question is more interesting... but even there maybe the answer isn't to expand this tutorial code .
On the usage question: this could be a follow-up issue that is solved when initial versions of both this and the client example are present. Possible solution:
- document how to serve the generated directory over http
- add a "bootstrap from this root.json" option to the client so it can be used with the generated repository
93cdf24
to
83764c6
Compare
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.
👍
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.
LGTM, thanks!
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 just read it and liked the comments and comparing that to tuf/repository_tool.py
is such an improvement and simplification.
Great work!
As 'repository_tool' and 'repository_lib' are being deprecated, repository metadata must to be created and maintained manually using the low-level Metadata API. The added example code shall serve as temporary replacement until a new repository tool is available. The sample code contains the following repo workflows: - creation of top-level metadata - target file handling - consistent snapshots - key management - top-level delegation and signing thresholds - target delegation - in-band and out-of-band metadata signing - writing and reading metadata files - root key rotation Co-authored-by: Teodora Sechkova <[email protected]> Co-authored-by: Joshua Lock <[email protected]> Co-authored-by: Jussi Kukkonen <[email protected]> Signed-off-by: Lukas Puehringer <[email protected]>
Adds new test module that executes the basic repo example Python script and checks that it created certain (metadata) files. The test module is tailored for testing similar example scripts. Co-authored-by: Joshua Lock <[email protected]> Co-authored-by: Jussi Kukkonen <[email protected]> Signed-off-by: Lukas Puehringer <[email protected]>
83764c6
to
a1531d8
Compare
Thanks, everyone! |
[EDIT 11/23/2021: remove draft status and update PR description]
[EDIT 11/25/2021: remove reviewer question about test, the PR includes one]
Fixes #1673
Description
Add an excessively commented python script to demonstrate repository creation/operation using only the low-level metadata API.
Context
As 'repository_tool' and 'repository_lib' are being deprecated, repository metadata must to be created and maintained manually using the low-level Metadata API. The example code in these files shall serve as temporary replacement until a new repository tool is available.
Contents
Questions for reviewers
E.g. hashed bin delegation will be in a separate file (see follow-up PR)
Please verify and check that the pull request fulfills the following requirements: