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

RepositorySimulator._compute_hashes_and_length is broken #128

Closed
jku opened this issue Aug 12, 2024 · 5 comments · Fixed by #135 or #154
Closed

RepositorySimulator._compute_hashes_and_length is broken #128

jku opened this issue Aug 12, 2024 · 5 comments · Fixed by #135 or #154
Labels
bug Something isn't working

Comments

@jku
Copy link
Member

jku commented Aug 12, 2024

EDIT: Every test that uses RepositorySimulator._compute_hashes_and_length = True is currently broken because :

  • the simulator signs at request time
  • ecdsa signatures are not deterministic
  • hashes/length calculation is done over the whole file (so it includes these changing signatures)

This should get fixed if any option of #130 is implemented

Original report was :


test_snapshot_rollback_with_local_snapshot_hash_mismatch() does not seem right:

  • the metadata changes in the test seem valid, yet client fails refresh
  • test claims in comments to do a target version rollback but does not?
  • client actually fails because length and hash are incorrect?

I can't quite see what the issue is.

We should

  • find the root cause here
  • fix the test
  • add a simple test that just uses hashes and length in metafiles without any fancy attacks
@jku jku added the bug Something isn't working label Aug 12, 2024
@jku
Copy link
Member Author

jku commented Aug 12, 2024

find the root cause here

oh, it's likely the combination of:

  • RepositorySimulator signing at request time
  • using ecdsa keys that are not "deterministic"

the hashes/length are over the whole file so include signatures.

This (the fact that making a mistake like this is so easy) is certainly a major downside to signing at request time so we may have to reconsider that decision... Still, maybe switching to a different default key type would also work. I originally only chose ecdsa because they are fast to generate (unlike rsa).

@jku
Copy link
Member Author

jku commented Aug 13, 2024

I filed an issue for switching the signing keytype (or otherwise solving the underlying issue).

That said, the test is still completely wrong: it claims to test a targets rollback but does not actually try to do one.

@AdamKorcz
Copy link
Contributor

Should this remain open, @jku?

@jku
Copy link
Member Author

jku commented Aug 13, 2024

yeah, that makes sense: currently client fails as expected but for wrong reasons.

Once #131 is implemented (and #130 is fixed) we have more confidence that the failure happens for right reason

@jku jku reopened this Aug 13, 2024
@jku
Copy link
Member Author

jku commented Aug 14, 2024

test_new_targets_hash_mismatch() is broken because of this as well: If any return values had been checked that would have been visible

@jku jku changed the title something is wrong with compute_metafile_hashes_length() RepositorySimulator.compute_metafile_hashes_length() is broken Aug 16, 2024
@jku jku changed the title RepositorySimulator.compute_metafile_hashes_length() is broken RepositorySimulator._compute_hashes_and_length is broken Aug 16, 2024
jku added a commit to jku/tuf-conformance that referenced this issue Aug 16, 2024
* Add a test for simple targets rollback
* Fix the test for targets rollback when metafile hashes are used

This now leads to the latter test failing because of theupdateframework#128: This PR can
be used to test the fix for theupdateframework#128. This PR should not be merged before
that.

Signed-off-by: Jussi Kukkonen <[email protected]>
jku added a commit to jku/tuf-conformance that referenced this issue Aug 16, 2024
* Add a test for simple targets rollback
* Fix the test for targets rollback when metafile hashes are used

This now leads to the latter test failing because of theupdateframework#128: This PR can
be used to test the fix for theupdateframework#128. This PR should not be merged before
that.

Signed-off-by: Jussi Kukkonen <[email protected]>
jku added a commit to jku/tuf-conformance that referenced this issue Aug 19, 2024
* Add a test for simple targets rollback
* Fix the test for targets rollback when metafile hashes are used

This now leads to the latter test failing because of theupdateframework#128: This PR can
be used to test the fix for theupdateframework#128. This PR should not be merged before
that.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku linked a pull request Aug 19, 2024 that will close this issue
@jku jku closed this as completed in #154 Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants