Skip to content
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

Merged
merged 8 commits into from Aug 26, 2022

Conversation

znewman01
Copy link
Contributor

Fixes #221

@znewman01 znewman01 force-pushed the interop branch 2 times, most recently from d31016c to d72fa29 Compare March 2, 2022 02:24
@znewman01
Copy link
Contributor Author

CC @jku to make sure the new python-tuf code is right; in particular:

  1. there's no built-in support for consistent snapshots, right?
  2. is there a better way to make sure that timestamp.json has correct hashes/length than what I'm doing?

CC @asraa @haydentherapper generally

My tests are passing, but some others are not passing due to the ErrWrongID change; would appreciate feedback. Core issue:

  1. e82b56b started ignoring ErrWrongID errors because of TAP-12
  2. TAP-12 seemingly got implemented in python-tuf v1.0.0
  3. however the change from (1) doesn't actually import the key! it just silently ignores it

Is the right thing to do:

  1. remove ErrWrongID generally?
  2. have a more-convoluted check, like "if keyid is provided check it but otherwise don't bother"?

Copy link
Member

@jku jku left a 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.

@znewman01
Copy link
Contributor Author

@jku:

Left some comments hopefully they are helpful

Yes, thank you for the quick review 😄 great context

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.

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.

@coveralls
Copy link

coveralls commented Mar 2, 2022

Pull Request Test Coverage Report for Build 1924696848

  • 8 of 13 (61.54%) changed or added relevant lines in 4 files are covered.
  • 6 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.02%) to 70.215%

Changes Missing Coverage Covered Lines Changed/Added Lines %
repo.go 0 1 0.0%
client/client.go 0 2 0.0%
verify/errors.go 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
client/client.go 1 81.41%
repo.go 1 73.37%
verify/errors.go 1 0%
verify/db.go 3 85.53%
Totals Coverage Status
Change from base Build 1831480097: -0.02%
Covered Lines: 2157
Relevant Lines: 3072

💛 - Coveralls

verify/db.go Outdated Show resolved Hide resolved
@mnm678
Copy link
Collaborator

mnm678 commented Mar 4, 2022

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.

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).

@znewman01
Copy link
Contributor Author

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.

@joshuagl
Copy link
Member

As #232 is merged can we rebase this and get it reviewed @znewman01?

@znewman01
Copy link
Contributor Author

Yup! Might be a week-ish until I get around to it.

@joshuagl
Copy link
Member

np, let us know when you're ready for review!

@asraa
Copy link
Contributor

asraa commented Aug 16, 2022

@znewman01 mind if I update this?

@znewman01
Copy link
Contributor Author

znewman01 commented Aug 16, 2022 via email

@asraa asraa force-pushed the interop branch 2 times, most recently from 59120cb to 000d999 Compare August 16, 2022 14:37
@asraa asraa changed the title Update Python interop tests to python-tuf v1.0.0 test: Update Python interop tests to python-tuf v1.0.0 Aug 16, 2022
Copy link
Contributor

@asraa asraa left a 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

@asraa
Copy link
Contributor

asraa commented Aug 16, 2022

@jku @mnm678 could you PTAL? TAP-12 key ID removal is done, and so is some minor go-tuf snapshot length/hashes presence bug is resolved too.

@asraa asraa requested a review from jku August 17, 2022 15:51
@znewman01
Copy link
Contributor Author

LGTM though I can't review it since it's my PR

@trishankatdatadog
Copy link
Member

I'll leave it to @jku since he's best suited to review this one

@jku
Copy link
Member

jku commented Aug 18, 2022

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.

znewman01 and others added 8 commits August 26, 2022 12:46
Signed-off-by: Zachary Newman <z@znewman.net>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Zachary Newman <z@znewman.net>
Signed-off-by: Asra Ali <asraa@google.com>
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 <z@znewman.net>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Zachary Newman <z@znewman.net>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
Signed-off-by: Asra Ali <asraa@google.com>
@trishankatdatadog
Copy link
Member

I made an exception for the DCO this time.

@trishankatdatadog trishankatdatadog merged commit d7ff71b into theupdateframework:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

interop tests use deprecated python-tuf code
7 participants