-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add TAP for keyid flexibility #112
Add TAP for keyid flexibility #112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, Marina! This will only make TUF implementations stronger and more flexible
|
||
# Augmented Reference Implementation | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be finished, ofc 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as Draft, great job, Marina!
I don't have write access to a few TUF repos. Can someone help me fix this? @lukpueh @JustinCappos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up! I'm thrilled this issue is being addressed.
metadata file remain unique. Metadata owners should only publish metadata that | ||
contains a unique keyid to key mapping. | ||
|
||
## Implications for complex delegation trees |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You touch upon this in the specification, but it might be helpful to also include an example here for the case where the targets metadata file A delegates to C using the key D and the keyid K, as well as another targets metadata B delegates to C using the key E and the keyid K. From what I understand, this should result in C being signed with:
{
"sigs": [
{
"keyid": "K",
"sig": "1234..."
},
{
"keyid": "K",
"sig": "abcd..."
},
...
],
...
}
Both signatures will be checked if the delegation search ever ends up at A or B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, thanks for the suggestion!
candidate-keyid-tap.md
Outdated
file. This means replacing requirements 1 and 2 above with a description of | ||
required keyid properties, ie “The KEYID is an identifier for the key that is | ||
determined by the metadata owner and MUST be unique within the root or | ||
delegating targets metadata file.” Once this keyid is determined by the metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The KEYID is an identifier for the key that is determined by the metadata owner and MUST be unique within the root or delegating targets metadata file.
Now that there wouldn't be a cryptographically derived keyid for each key, it's now possible to list the same key multiple times, each with a different keyid. Can we instead impose another rule that the key must also be unique? Without this rule, an attacker could duplicate a signature to reduce the effective threshold of keys needed to validate the metadata. We do this in go-tuf and rust-tuf to avoid double counting keys due to our (temporary) support of python-tuf's keyid_hash_algorithms
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. If there are multiple delegations to the same KEYID as part of a threshold, is this allowed? If I delegate to both Alice and Bob, who both delegate to Charlie, is Charlie's approval enough? (I think I know the answer in both cases and this is mostly tangental to this issue, but think we should discuss if we think TUF is doing the right thing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a description of this in the scenarios below. As currently written, in this situation Charlie's approval would be enough. I think that this should be sufficient to trust the metadata as a threshold of signatures using unique keys must be reached to obtain Charlie's approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have less practical experience with tuf than the other folks in this discussion, but I would be surprised that Charlie's approval would be enough. My understanding of the specification is that thresholds are a defence against key compromise. In the scenario described we only have to compromise a single key in order to meet a threshold that is > 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like the "Deadly Diamond of TUF" 😜
Jokes aside, I also think that Charlie's approval is enough. But in reality it required Justin's, Alice's and Bob's approval to even end up in a situation, where it only requires Charlie's approval.
I agree that it doesn't sound ideal. So either:
- Justin reconsiders delegating trust to Alice and Bob, seeing them void the threshold,
- or he at least does not allow them to further delegate trust (IIUC, this is what the
"terminating"
flag in delegations is for). - Alternatively, we could add a feature to the TUF specification, to disable such signature threshold regression in delegation graphs. But I think above tools are enough.
Do others agree? @JustinCappos, you really got me curious about your answers to these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirement: Justin requires two roles in agreement about a given target branch search a): Justin asks Alice, Alice asks Charlie, Charlie has target, back up to Justin branch search b): Justin asks Bob, Bob asks Charlie, Charlie has target, back up to Justin success: Justin has two roles in agreement and releases the target
Shouldn't it be enough to, along with Charlies knowledge about the target, also pass up the public key and let Justin only release the target if two roles with different keys were in agreement?
The current algorithm returns with the first match. So suppose that Alice (or Bob) also delegated trust to Daniela, who also approved the target. You should approve it in this case, but the current algorithm would not do so. You'd need to match all parties that agree. Then you'd need to be sure that Alice and Bob each have at least one person that was not yet chosen who they could select to fulfill this need. Also, what if Alice and Bob have threshold delegations to Charlie and Daniela in this case? It's just a lot messier to deal with, in my view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this issue pertains more to the existing delegation resolution algorithm than to the keyid calculation, so I propose we open a new issue to discuss this issue further. As @lukpueh mentioned, TAP 3 is not currently included in the specification, so this issue does not exist with the current specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a closer look at this issue, it appears that the visited
flag ensures that Charlie's role will not be visited more than once in the DFS as described in 4.4.1 of the client workflow. Does this solve the issue @JustinCappos @lukpueh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this solves it. I don't think it would handle the case above with Daniela, for example.
I do think it would be fine to move this elsewhere, but I think the most important question is what should the system actually do? I think we could code it to work however, but I think we should definitely strive to do what a user would expect first and foremost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I created a new issue to continue this discussion.
candidate-keyid-tap.md
Outdated
file. This means replacing requirements 1 and 2 above with a description of | ||
required keyid properties, ie “The KEYID is an identifier for the key that is | ||
determined by the metadata owner and MUST be unique within the root or | ||
delegating targets metadata file.” Once this keyid is determined by the metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. If there are multiple delegations to the same KEYID as part of a threshold, is this allowed? If I delegate to both Alice and Bob, who both delegate to Charlie, is Charlie's approval enough? (I think I know the answer in both cases and this is mostly tangental to this issue, but think we should discuss if we think TUF is doing the right thing.)
candidate-keyid-tap.md
Outdated
only that stage of the depth-first search. | ||
|
||
These changes to the specification would allow the repository to use any scheme | ||
to determine keyids without needing to communicate it to clients. By making this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how or why the repository is determining KEYIDs. Can you clarify?
at any time to deprecate a hash algorithm or change to a different keyid | ||
calculation method. | ||
|
||
## Keyid Deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why this is needed anymore. Just generate a new file with whatever new KEYID scheme you want. All consumers didn't understand how you got the KEYIDs anyways, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process is pretty straightforward, but the metadata owner should ensure that the keyids remain unique throughout any transition. This might not need a whole section, but I think that it's important to note.
candidate-keyid-tap.md
Outdated
algorithm that is already in use by the system. | ||
|
||
The specification sets the following requirements for keyid calculation: | ||
1. The KEYID of a key is the hexdigest of the SHA2-256 hash of the canonical JSON form of the key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the biggest thing, but we likely should be consistent with keyid vs KEYID capitalization. If you're quoting, that's fine, perhaps be more explicit about that in your formatting though...
…me keyid used in different metadata files
…many-to-one keyid mapping
candidate-keyid-tap.md
Outdated
file. This means replacing requirements 1 and 2 above with a description of | ||
required keyid properties, ie “The KEYID is an identifier for the key that is | ||
determined by the metadata owner and MUST be unique within the root or | ||
delegating targets metadata file.” Once this keyid is determined by the metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have less practical experience with tuf than the other folks in this discussion, but I would be surprised that Charlie's approval would be enough. My understanding of the specification is that thresholds are a defence against key compromise. In the scenario described we only have to compromise a single key in order to meet a threshold that is > 1.
Co-Authored-By: Joshua Lock <jlock@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this very good write-up, @mnm678! I left a few comments and I'm interested to hear what others say in particular in regards to client-side key/keyid uniqueness check.
candidate-keyid-tap.md
Outdated
for some implementations, such as: | ||
* Lack of consistency in implementations that use other hash algorithms for | ||
calculating file hashes and would prefer not to introduce SHA2-256 for this one | ||
instance. For example, the PEP 458 implementation (https://python.zulipchat.com/#narrow/stream/223926-pep458-implementation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: I'd either point directly to that conversation https://python.zulipchat.com/#narrow/stream/223926-pep458-implementation/topic/Timeline/near/188666946 (given that the link is permanent-ish, which I don't know), or leave the link out altogether.
candidate-keyid-tap.md
Outdated
file. This means replacing requirements 1 and 2 above with a description of | ||
required keyid properties, ie “The KEYID is an identifier for the key that is | ||
determined by the metadata owner and MUST be unique within the root or | ||
delegating targets metadata file.” Once this keyid is determined by the metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
within the root or delegating targets metadata file
Just for clarity, can we generalize this early on?
A role that authorizes the signing keys (by keyid) for another role and has its public keys == a role delegating trust to another role == a delegating role == root or delegating targets role (including top-level targets).
You do generalize it in the next line ...
will be listed in the delegating metadata file
where, I think, you are referring to root and delegating targets, right?
Anyway, it's not a big thing. I just want to make sure everyone is on the same page.
candidate-keyid-tap.md
Outdated
file. This means replacing requirements 1 and 2 above with a description of | ||
required keyid properties, ie “The KEYID is an identifier for the key that is | ||
determined by the metadata owner and MUST be unique within the root or | ||
delegating targets metadata file.” Once this keyid is determined by the metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like the "Deadly Diamond of TUF" 😜
Jokes aside, I also think that Charlie's approval is enough. But in reality it required Justin's, Alice's and Bob's approval to even end up in a situation, where it only requires Charlie's approval.
I agree that it doesn't sound ideal. So either:
- Justin reconsiders delegating trust to Alice and Bob, seeing them void the threshold,
- or he at least does not allow them to further delegate trust (IIUC, this is what the
"terminating"
flag in delegations is for). - Alternatively, we could add a feature to the TUF specification, to disable such signature threshold regression in delegation graphs. But I think above tools are enough.
Do others agree? @JustinCappos, you really got me curious about your answers to these cases.
candidate-keyid-tap.md
Outdated
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." All metadata definitions would remain the same, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @mnm678 and I discussed this quite a bit, and I seem to remember that we agreed that a client-side key or keyid uniqueness check within a delegation only prevents scenarios, where the repository owner already screwed up, i.e. uses the same keyid for different keys, or different keyids for one key.
Or am I missing something?
If not, I wonder if we really want to make client verification more complex, because the client can't trust repositories to do their job right. I mean, the client also doesn't check for faux-pas like the delegation scenario, Justin described above, or for a repository using the same key to sign root and timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of different uniqueness requirements that the keyid/key systems needs. I agree that where possible we want to simplify the client workflow by trusting information found in signed metadata files. The situations in which uniqueness is needed include:
- keyids provide a mapping that can be used to determine which key is trusted. As @lukpueh and I discussed in the above link, this mapping is provided in signed metadata, and so can be trusted by the client without additional verification.
- Duplicate keys cannot be applied to the same threshold. If an attacker is able to append a duplicate signature, the client should only apply this signature to the threshold once. In this case, I think that the client can do a O(1) check (eg by adding each key to a set) to ensure uniqueness. Previously this was done by ensuring the keyid was only used once, but using the key instead seems like a less error-prone way to make this check (that solves some current issues).
- In a multi-role delegation each role's approval is only used once. This is the situation discussed in the comments above. I think that like the keyid/key mapping, this situation can only come up if the delegation tree allows two roles to delegate to the same role. Afaik there is not a way for an attacker without a threshold of keys to create this situation, so it should be left up to the repository manager to occasionally audit to ensure the delegation tree makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra clarification, @mnm678! This makes sense to me. Regarding the third item, I left some thoughts in the discussion above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we keep track of duplicate public keys? I mean, what if there are the same public keys, but listed in different formats? (I know we use only PEM right now, but humor me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would guess that if multiple formats are supported, they would need to be converted to a consistent format for signature verification, so the uniqueness check would take place after any conversion.
Are there any use cases in which multiple formats would be used but not be able to be converted? In this case we might need to have some more discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But once again (and thanks for pointing this out, @SantiagoTorres), if the attacker does not control the metadata that lists the public keys, then they cannot make different representations of the same key each count towards the threshold.
And if the attacker does control the metadata, they can add any key they want and uniqueness checks on the client are moot.
This means that we really only need to de-duplicate on the one key representation in the delegating metadata. Unless, and this brings me back to my initial comment above, we don't trust the delegating role.
Sorry for adding to the confusion with my comment above, I was briefly confused too. Maybe we should re-hash the threat model for this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was more interested in someone (e.g., the delegating role) trying to deliberately fool python-tuf
into accepting what are actually duplicate keys by using different public key formats which actually describe the same public key parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but this delegating role could also just change the threshold? My impression is that this is necessary because we want to avoid footguns, but it's not a very exploitable attack vector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it's still something to think about, I think, especially if the client-side checks are not expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unique key check already exists to ensure that an attacker cannot append duplicate signatures (although this requirement is not explicitly stated in the specification). The main difference here is requiring that the client do this uniqueness check on the key instead of the keyid. If all goes correctly, using the key does not make any difference, but if the metadata accidentally has duplicate keys with different keyids, using keyids to verify uniqueness of keys could lead to a key being applied to a threshold more than once.
Co-Authored-By: Trishank Karthik Kuppusamy <33133073+trishankatdatadog@users.noreply.github.com>
@JustinCappos @trishankatdatadog Are there any blockers to getting this merged as a draft? |
As a draft? No, I don't think so. Justin, what about you? |
Not at all. Feel free. |
It looks like I don't have write access to this repository. |
This TAP proposes a specification change to allow for increased keyid flexibility. The need for this TAP arose from discussion about keyid use cases (theupdateframework/python-tuf#848, theupdateframework/python-tuf#660).
A previous draft of this document was available here.