go-tuf
go-tuf copied to clipboard
theupdateframework
Update Python interop tests to python-tuf v1.0.0
CC @jku to make sure the new python-tuf code is right; in particular:
- there's no built-in support for consistent snapshots, right?
- is there a better way to make sure that
timestamp.jsonhas 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:
-
e82b56b started ignoring
ErrWrongIDerrors because of TAP-12 - TAP-12 seemingly got implemented in python-tuf v1.0.0
- however the change from (1) doesn't actually import the key! it just silently ignores it
Is the right thing to do:
- remove
ErrWrongIDgenerally? - have a more-convoluted check, like "if keyid is provided check it but otherwise don't bother"?
@jku:
Left some comments hopefully they are helpful
Yes, thank you for the quick review :smile: 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.
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% |
| <!-- | Total: | 8 | 13 |
| 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% |
| <!-- | Total: | 6 |
| Totals | |
|---|---|
| Change from base Build 1831480097: | -0.02% |
| Covered Lines: | 2157 |
| Relevant Lines: | 3072 |
💛 - Coveralls
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).
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.
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.
@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.
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.
