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

refactor key test #86

Closed

Conversation

AdamKorcz
Copy link
Contributor

@AdamKorcz AdamKorcz commented Jul 27, 2024

  1. Add comments
  2. Make it explicit that two identical public keys break the update.

Signed-off-by: Adam Korczynski <[email protected]>
Signed-off-by: Adam Korczynski <[email protected]>
@AdamKorcz AdamKorcz force-pushed the refactor-keys-test branch from cca79e6 to 9a38773 Compare July 27, 2024 19:10
@AdamKorcz
Copy link
Contributor Author

@jku Could you take a look at this? Python-tuf succeeds and go-tuf fails because a key that root MD has for snapshot does not exist in the snapshot md.

@AdamKorcz AdamKorcz mentioned this pull request Jul 27, 2024
@jku
Copy link
Member

jku commented Jul 30, 2024

I think the the practical issue is this (but would have to check the repository metadata dumps to verify):

  • the keyids are added to root.roles[role].keyids, but the corresponding public key is not in root.keys[keyids] -- I'm not sure if spec defines the correct action in this case.
  • go-tuf decides this is an issue: key with ID 41898f69a6e541a5696793230a1036c76acd0b83e48405821a5c0e061b263c28 not found in snapshot keyids (it says "keyids" but I think it means "keys")

The more higher level question is how should duplicate keys be tested:

  1. the really important thing is that a single keyid should never be able to successfully reach threshold 2
  2. possibly clients should immediately fail if keyids list contains duplicates (but would have to re-read the spec to verify the details)

the second option is easier to test but first option would be correct if spec is not super clear about the correct action.

@jku
Copy link
Member

jku commented Jul 31, 2024

the keyids are added to root.roles[role].keyids, but the corresponding public key is not in root.keys[keyids] -- I'm not sure if spec defines the correct action in this case.

Spec does not seem to define the correct action here. I think this is an interesting case to test but I believe it is not the case described in the test description .

To fix this test to be as described, instead of generating a new signer and adding the keyid to the keyids list you could:

  • starting with default repository, find a current snapshot key keyid. Add that keyid to snapshot keyids list (the keyid should now be duplicated in the list ) and bump root
  • test if client accepts the root and snapshot: I think it should but would not be surprised if some client considers duplicate keyids an error
  • now increase snapshot threshold, bump root
  • test if client accepts the root and snapshot: snapshot should not be accepted as it's only signed by a single key
  • bonus test: now add a duplicate signature into snapshot: just take the existing sig and append another copy of it
  • test if client accepts the root and snapshot: snapshot should not be accepted as it's only signed by a single key

@jku
Copy link
Member

jku commented Aug 14, 2024

I've filed #144 for the keyid-without-key case: this test can now be fixed I think

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.

marking request changes, see previous comments

@jku
Copy link
Member

jku commented Sep 27, 2024

I believe we have tests that cover what I suggested in last comment (see e.g. test_duplicate_keys and many subtests in test_updater_key_rotations.py.

I'm closing this.

@jku jku closed this Sep 27, 2024
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.

2 participants