-
Notifications
You must be signed in to change notification settings - Fork 110
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
test: Update Python interop tests to python-tuf v1.0.0 #228
test: Update Python interop tests to python-tuf v1.0.0 #228
Conversation
d31016c
to
d72fa29
Compare
CC @jku to make sure the new python-tuf code is right; in particular:
CC @asraa @haydentherapper generally My tests are passing, but some others are not passing due to the
Is the right thing to do:
|
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.
Left some comments hopefully they are helpful
The metadata generation is quite verbose as is visible here: I hope we can improve on that soon, I'll try to remember to send a patch here when that happens.
there's no built-in support for consistent snapshots, right?
I think the metadata you now produce is completely valid consistent_snapshot (the non-consistent version seems a bit incorrect as root is not versioned but I commented on that in code). Let me know if there's anything that seems missing.
It's true we don't currently handle target files themselves in any way so you will need to produce the hash prefixed filename yourself when saving the file, that is a bit annoying: I left a comment in the code.
is there a better way to make sure that timestamp.json has correct hashes/length than what I'm doing?
this is an oversight in python-tuf: we've not been using the optional hashes in meta dicts
TAP-12 seemingly got implemented in python-tuf v1.0.0
Maybe more accurate to say Metadata API does not enforce or test that the id actually is the hex digest of the canonical form: it's possible that as an accidental result of that we are producing metadata that is not strictly conformant with that current requirement.
@jku:
Yes, thank you for the quick review 😄 great context
ACK, makes sense. I will consider this a python-tuf bug and track python-tuf#1894. However, it seems like it should be okay to ignore the keyIDs as a client and barring objections I will update the PR to allow this. |
Pull Request Test Coverage Report for Build 1924696848
💛 - Coveralls |
With the python-tuf implementation, I think that we can move TAP 12 to accepted pretty quickly. Unless there are strong objections to the TAP that will lead to major changes (which I haven't seen), I'd argue for future-proofing go-tuf by implementing TAP 12 now. It may make the implementation temporarily out of sync with the specification, but not in a major way, and not in a way that affects security (as described in more detail in the TAP). |
Filed #232 for TAP-12. I think we should probably block this PR on that issue. That would make sure we don't half-implement TAP-12 and accidentally fail-open. |
As #232 is merged can we rebase this and get it reviewed @znewman01? |
Yup! Might be a week-ish until I get around to it. |
np, let us know when you're ready for review! |
@znewman01 mind if I update this? |
Go for it! Let me know if you don’t find time,it had finally popped to the top of my TODO list this week but happy to have you take it off my hands
… On Aug 16, 2022, at 9:48 AM, asraa ***@***.***> wrote:
@znewman01 mind if I update this?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
59120cb
to
000d999
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.
@znewman01 rebased, tests passing
LGTM though I can't review it since it's my PR |
I'll leave it to @jku since he's best suited to review this one |
Sorry, I'm unlikely to have a real look in the next weeks. |
Signed-off-by: Zachary Newman <[email protected]> Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Zachary Newman <[email protected]> Signed-off-by: Asra Ali <[email protected]>
A few changes, besides pointing at the new directory: - the python-tuf repo says "High-level support for implementing repository operations is planned but not yet provided;" this includes consistent snapshot, so we'll add a consistent snapshot test at that point. - make a new tmp dir for each Python client test (it gets confused otherwise) - the new python-tuf client downloads `foo/bar.txt` to `targets/foo%2Fbar.txt` so check for it there Signed-off-by: Zachary Newman <[email protected]> Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Zachary Newman <[email protected]> Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
fa71b4d
to
674204e
Compare
I made an exception for the DCO this time. |
Fixes #221