tough: condition tap-12 check with feature#886
tough: condition tap-12 check with feature#886glebiller wants to merge 1 commit intoawslabs:developfrom
Conversation
|
Thanks for the contribution! I'd like to unblock I'm unfamiliar with the TAP process, do you have a sense of how long it takes for such proposals to merge with the spec? It seems like the CI is failing due to an unrelated clippy lint. We can look to merge a fix for that separately. |
Thanks for your reply, unfortunately I am not really close to the TAP process too, however you can see from the previous TAP that it takes years to officially be added to the spec once approved. However, I checked the official implementation theupdateframework/python-tuf and they already implement TAP-12 as it do not enforce the keyid check, and the client can be used with the GitHub TUF repo without errors. |
| #[cfg(not(feature = "tap-12"))] | ||
| { | ||
| let calculated = key.key_id()?; | ||
| ensure!( | ||
| keyid == calculated, | ||
| error::InvalidKeyIdSnafu { | ||
| keyid: &keyid_hex, | ||
| calculated: hex::encode(&calculated), | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
My reading of TAP-12 suggests that it's not sufficient to just remove this check, because two different key IDs can now refer to the same key:
To ensure that a client does not use the same key to verify a signature more than once, they must additionally check that all keys applied to a signature threshold are unique. So, the specification should additionally require that "Clients MUST use each key only once during a given signature verification."
The existing code may not implement this requirement because before there was a 1:1 mapping between key IDs and keys, so the ensure! check here was sufficient.
There was a problem hiding this comment.
@bcressey In the interest of figuring out if a change similar to this could potentially be landed, would you consider this issue to be addressed properly if only unique key_id() values were to be counted towards the thresholds in the relevant places?
tough/tough/src/schema/verify.rs
Lines 37 to 48 in 0ad97ea
tough/tough/src/schema/verify.rs
Lines 80 to 90 in 0ad97ea
This would seem to be in the spirit of the TAP-12 proposal, as long as .key_id() qualifies as a "standardized representation for keys" (which it should, otherwise the previous logic already allows for colliding keys under different key IDs) , and furthermore should retain the previous guarantees.
|
@cbgbt We are also interested in solving the same problem. What alternative to a feature flag would you suggest? The way that other libraries went is to unconditionally remove the key ID checks, but you may not want to go to that extent |
Issue #, if available:
#766
Description of changes:
Conditionally disable the keyid with a feature
tap-12. The default behavior remain unchanged, but it can be disable easily.Since the TAP-12 is accepted, and hopefully should be merged in a near future, the goal is to unblock some TUF repos from being used with tough (like the GitHub TUF repo).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.