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

Discussion of TAP 8: Key rotation and explicit self-revocation #37

Open
vladimir-v-diaz opened this issue May 31, 2017 · 51 comments
Open
Milestone

Comments

@vladimir-v-diaz
Copy link
Contributor

TAP 8 proposal is available at https://github.com/theupdateframework/taps/blob/master/tap8.md

Pull requests related to TAP 8:
#20

Discussion of TAP 8 on the TUF mailing list:
https://groups.google.com/d/msg/theupdateframework/Yj1z3PkMRZc/n8wXM4VZBgAJ

@vladimir-v-diaz vladimir-v-diaz changed the title Acceptance of TAP 8: Key rotation and explicit self-revocation Discussion of TAP 8: Key rotation and explicit self-revocation Jul 24, 2017
@vladimir-v-diaz
Copy link
Contributor Author

Hi, @hannesm.

I came across an old comment of yours on the issue discussion for TAP 3 (multi-role delegations).
In the comment you say that a naming mechanism is still needed for TAP 8 (key rotation). Have you found a solution yet? Is there anything else blocking TAP 8?

(I'm just checking in on the status of TAP 8)

@hannesm
Copy link
Contributor

hannesm commented Sep 20, 2017

It looks like GitHub ate my email; yes, I'm working on a revision of TAP8 - unfortunately not yet done, but hopefully by the end of this week. I'll open a PR then and let's discuss in that how to move forward. Sorry for the delay.

@vladimir-v-diaz
Copy link
Contributor Author

No problem!

Thanks, and we're looking forward to TAP 8 revision.

@vladimir-v-diaz
Copy link
Contributor Author

@hannesm

@JustinCappos asked about TAP 8 during a recent meeting. He was wondering if you've come across any potential issues with TAP 8 while implementing it.

@hannesm
Copy link
Contributor

hannesm commented May 30, 2018

@vladimir-v-diaz due to other projects, my implementation of tuf + tap8 is only progressing slowly (only doing this in my spare time, working on other projects for funding). so far I have basic tuf nearly finished (in OCaml), and will work over the summer on finalising tuf+tap8. I'll get back to tap8 once implemented, sorry for the delay.

@awwad
Copy link
Contributor

awwad commented Sep 7, 2018

I think this is a good and wise feature add. I was reviewing @mnm678's TUF PR draft implementing TAP 8 and combed through TAP 8 and chatted with Marina a bit. I think that there are some conceptual issues and a few clarifications that should be made. I'm going to stick to only the two bigger conceptual points for this post:

Root Key Rotation Efficiency Improvement

In the course of the client's traversing the path from the currently trusted root file to the most recent root file, I'm not yet convinced that it's okay to only fetch root files that include key/threshold changes for the root keys:
There are settings that change in the root file, consistent-snapshots and spec-version, that might affect the way future root files are interpreted and therefore their validity or the impact of their production. (Consistent snapshots probably doesn't? Haven't thought through it. Spec version certainly could.)
The fact that you always validate root version 10 using root version 9 could matter; even if the keys and threshold are the same, some change to the spec version could change whether or not root version 10 looks valid from root version 8 vs root version 9. In one direction (10 looks invalid from 8, 10 looks valid from 9), this can be resolved by, if an error is encountered, grabbing an intervening root file. In the other direction, (10 looks valid from 8, looks invalid from 9), this cannot be resolved and is a security issue. (You could say that security-impacting new root versions should always include a key change, but that seems like a backward sort of policy.)
I could be off here. Perhaps it's wrong to be concerned about behavioral changes when the spec shifts? (What do we do with old root versions and the clients still using them?) Still, a complication to consider, and the efficiency gain is small.

TAP 3 Interoperability

I think that the particular (additional) TAP 3 interoperability advertised is a little too ambitious -- too complex in code and in concept -- and so probably inadvisable. Imagine that role X delegates to two roles A and B (with two thresholds and two key sets). Which of the following behaviors make sense?

  • (IMO Yes) -- A threshold of the keys that X delegates to (associated with role A) could rotate the keys currently expected of role A in the delegation from X. (i.e. TAP 8 deals with individual delegations)
  • (IMO No) -- A threshold of the keys A expects of X plus a threshold of the keys A expects of Y could together alter the keys expected / structure of the whole multi-role delegation, using a rotate file whose name identifies these key relationships. (Note that among the issues is the fact that the semantics of the existing rotate file ID formula (hash(k1.k2.k3)) become ambiguous, and collisions would be a problem -- [1 in [k1] + 2 in [k2, k3] vs 2 in [k1, k2, k3], etc.)

Related to the conceptual complexity, it's worth noting that the code to do the second bullet point would mangle the natural structure that exists currently, I think (process a role, move to its delegations). Just knowing what rotate filename to look for in the multi-role case is complicated here, because the keyids in a multi-role delegation to roles B+C may be changed by changes to the keyids for B.

(That said, abstractly: this extra type of multi-role rotation is a feature that hints at the possible value of further reducing the significance of roles in favor of key sets. It's possible that that feature (and possibly TAP 3 and other properties of TUF) would make more sense with a further diminishment of the role of "roles" vs delegations. If there's room to consider something like that, maybe the proposed TAP 3 multi-role rotation would make more sense. Food for thought for someday.)

@mnm678
Copy link
Contributor

mnm678 commented Sep 12, 2018

After talking with @JustinCappos and @awwad , we found that there are two categories of use cases that TAP 8 is trying to address.

The first (role rotation) involves a threshold of keys from a role rotating the set of keys for that role. This includes the role adding new keys (think adding a new developer to the team) as well as removing a key, changing a key, or changing the threshold.

The second (individual key rotation) involves rotating an individual key (as for key revocation) everywhere that that key is used.

Unfortunately, due to issues with ordering and naming, I do not think it is possible to include both of these features without some major spec changes. Because rotate files are named using keyids, changing keyids with either of the above methods would make it impossible to determine if the other type of rotation is happening elsewhere.

Either of these options would provide some useful features, but I think that the first covers more of the use cases discussed in the TAP. The second case could be achieved using role rotation if the key owner coordinated with all of their associated roles (or could be done individually for any roles with a threshold of 1).

However, a case could be made for expediting individual key rotation in the case of a compromised key. This process would be faster with the second option.

@hannesm Any thoughts about which option we should implement?

@JustinCappos
Copy link
Member

JustinCappos commented Sep 12, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Sep 12, 2018

Sure, here are some example scenarios for role vs key rotation.

For role rotation, say there's a developer role that has 3 developers who each have a key (A, B, and C) associated with the role with a threshold of 2 keys. A and B would be able to sign a rotate file that adds D to the role, leaving the threshold at 2. The role would then have 4 keys (A, B, C, D) and a threshold of 2. Using this method, a threshold of keys in the role can make any changes to the key/threshold makeup of the role without the delegator needing to sign a new file. This method could also be used for key rotation/revocation. If B wanted to change their key to B2, they could get together with A to sign a rotate file that includes keys A, B2, C, and D. However, if B's key was also used as part of the testing role, B would remain a valid key in that role unless an additional rotate file was created for the testing role.

For key rotation, if C wanted to change their key (possible compromise, etc) they would sign a rotate file with the new key C2. This would then have any role that formerly trusted C trust C2 instead. In this method, a key can make changes to itself (or self revoke by creating a cycle) but any changes to roles would have to come from the delegator.

These are both useful features, but a problem arises when trying to implement both. If C rotated its key to C2 using key rotation, a file specifying that A, B, C should be changed to A, B, C, D using role rotation would no longer be found as the system would look for a file changing A, B, C2 (rotate filename are role.rotate.ID where ID=sha256(keys.threshold)). Removing the keys from the filename would create some confusion as to when various rotate files are valid and make cycle detection more challenging. So as far as I can tell this leaves us with picking either role rotation or key rotation.

@awwad
Copy link
Contributor

awwad commented Sep 17, 2018

As a further note: the TAP 3 Interoperability comment I made above pertains to the role/keyset-rotation feature, which is implicit in the TAP as it exists currently, and the current "Interoperability with TAP 3" section.

@hannesm
Copy link
Contributor

hannesm commented Sep 19, 2018

thanks for your comments.

  • root file rotations - spec-version / consistent snapshots: I wasn't able to find any documentation how TUF clients should behave if spec versions are modified; similarly I couldn't find any required behaviour when consistent snapshots changes in the root file. Could you point me to documentation? the only snippet I could find in tuf-spec is that spec-version should be compared to match either directly or same major version. I couldn't even find any monotonicity requirement for spec-version - can a root file version 8 with spec-version 1.3 be succeeded by a root file version 0 with spec-version 1.2? how do existing clients deal with this situation?
    I agree that if these two values are expected to be modified during the lifetime of a repository, they should be handled well. An idea in my mind would be to include these in the hash of the rotate root filename, i.e. there'll be the demand of a rotation when you modify these variables.

  • TAP3 / multi-role delegations
    this is tough, I also ran into some (not yet finally resolved) issues where to store the signatures, esp. when I want to allow a rotation of C to threshold of 2 [C1;C2;C3] (and how multi-role delegations work afterwards). but what is the actual trouble here for python-TUF? shouldn't the verification algorithm see the above mentioned rotation from 2 [A;B;C] to 2 [A;B;C;D], and then look for signatures (assuming that B and C2 signed): first check A, no signature, then B, valid signature, then C, find the rotation, use the public key C2 instead, and consider that valid!?

@mnm678
Copy link
Contributor

mnm678 commented Sep 20, 2018

A couple more thoughts:

root file rotation - An option would be to include the spec-version in the root rotate filename, but I’m not sure that is needed. I think a change in the root file’s spec-version or consistent-snapshots is a separate issue that goes beyond this tap. These values could also be changed in a normal root file update and cause inconsistencies. However, TAP8 will introduce a non backwards compatible spec change, so I’ll look more into this issue and how people could update to using TAP8 for root file rotation.

TAP 3 - I think the tricky part would be how to find the rotate file. If there’s a delegation to roles [A, B, C] which have keys [A1, A2], [B1, B2], [C1, C2]. Using a combination of keyids to find the rotate file wouldn’t work if A, B, or C did an individual rotation, so we would need to define a different naming scheme for the rotate file for the multi-role delegation (it would need to include at least the role threshold and role names). This would rely on role names being used as identifiers for the rotate file which brings up some issues with overlapping role names.

Additionally, I made a couple of changes to TAP8 (here) that clarify roles and keys as well as making a slight change to the timing of a rotate file check to ensure the file is always found.

@mnm678
Copy link
Contributor

mnm678 commented Oct 2, 2018

@hannesm @JustinCappos Any thoughts about the role vs key rotation mentioned above? I was planning to go ahead with role rotation unless there's a reason to do the key rotation instead.

@JustinCappos
Copy link
Member

JustinCappos commented Oct 4, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Oct 4, 2018

Root rotation

I agree that we should leave the root rotation as is. There doesn’t seem to be a good way to ensure rotate files are found for root, and additionally the spec-version, etc in root should be read for each version of the root file, which makes more sense with the current mechanism.

Location of rotate files

I think putting them in their own directory makes sense. The main issue here is making sure that the client sees any available rotate files.

Spec-version update

While looking at root key rotation, it seems like what to do on a spec-version update is not made clear. I’ve come up with a couple of ideas here, and I think the second makes more sense for most uses.

1: Require all changes to be backwards compatible. This allows clients and servers to update independently, but I think it creates more problems than it solves because it limits the potential of TUF going forward. TAP 8, along with other proposed changes would break on older versions.

2: Require clients to update. Specify that as soon as they see an increase in the spec-version, they must update their client to finish the update process. This might cause problems if the client for some reason can’t/doesn’t want to update, but it allows for more flexibility and potential security fixes in TUF.

Pooling Rotate Files

I think that they should be dependent on the role. The use case of a person entering/leaving a role makes the most sense if the rotate file only applies to that role in that repository. That way if person A left a role, they would still have access to their other roles. However, it might be nice if the self-revocation worked across repos.

@JustinCappos
Copy link
Member

JustinCappos commented Oct 4, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Oct 5, 2018

@JustinCappos Here’s some additional explanation of the pros and cons of pooling rotate files across repositories:

A rotate file is named using the role name, keys, and threshold. In a situation with pooled rotate files, any roles that share a name, keys, and threshold would be rotated together. In the case of a self-revocation, this is probably the best behavior. If a key was compromised, that same key will be compromised elsewhere. However, if there were two roles in separate repos with the same name (for example a developer role) this could cause an unwanted rotation. For example: people A, B, and C are all developers on projects foo and bar. Person C decides to leave foo to focus on bar. A and B sign a rotate files that takes C off of the project foo. This rotate file will be seen by anyone using bar as well and C will effectively be removed from both projects instead of just foo.

Essentially, this is a tradeoff between allowing a revocation to be seen by more people at once and allowing the rotate mechanism to be independent. However, I think it is unlikely that all uses of a key would fit under the same rotate file (it might be used for different role names, with different other keys, etc), so I feel it makes more sense to have the rotate files only work within the same repository to prevent unwanted rotations.

@JustinCappos
Copy link
Member

JustinCappos commented Oct 9, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Oct 11, 2018

@JustinCappos I think it would be possible to prevent all collisions by adding fields to the filename, but I'm not sure how many use cases there would be for the pooled rotate files considering how much is specified in the rotate filename. It would only be useful if repositories shared role names, keys, and thresholds as well as project name if that was added to the filename. In most cases, revoking a key would still require a rotate file for each use of the key.

@JustinCappos
Copy link
Member

JustinCappos commented Oct 11, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Oct 15, 2018

I made some proposed changes to the spec based on this discussion. Specifically, I made changes to root rotation, location of rotate files, getting rid of unintended cycles, and what to do with conflicting rotate files. More details are in the PR.

@mnm678
Copy link
Contributor

mnm678 commented Oct 17, 2018

While writing a revision to the tap, I found a couple of additional interesting points.

First: What should the rotate file be named? The filename was previously defined as role.rotate.ID, with ID as the sha256 hash of the keys (in lexicographical order) and the threshold (in ascii). Now that rotation cycles are allowed (through adding the hash of the previous rotate file), these filenames could collide. I propose adding the hash of the previous file to the filename to prevent this ambiguity. The ID would still have to be in the filename as the first rotate file would always have a previous file of null, so a collision could occur in the first rotate file after a re-delegation (ex: role A is delegated to, creates a rotate file, is delegated to with a new key, creates another rotate file that will have the same previous hash as the first). One potential downside of including both the ID and previous file hash is a much longer filename.

Second: Does the rotate file need to contain both the new keys and a list of new keyids? From my understanding the key structure contains the keyids, so this seems like redundant information to include in the rotate file.

Third: Should clients work backward from the most recent rotate file, or forward from the delegation? Now that rotate files point backward, would it make sense to look for the most recent rotate file? I think that it would be better to still start from the delegation to ensure that the chain of trust is maintained (so stay with the current mechanism of getting the delegation, then repeatedly looking for rotate files with the current keys).

@JustinCappos @hannesm I’d appreciate any input/insight into any of these issues, or any other comments on the changes to the TAP8 spec (found here).

@JustinCappos
Copy link
Member

JustinCappos commented Oct 17, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Oct 17, 2018

Role names:
The TUF spec does not specifically forbid roles with the same role name, but in practice it would not work as the role's metadata file is named rolename.json.

I think another question is whether we want the rotate mechanism to replace delegation here. If the delegator is allowed to continue handling key rotation, they have to be allowed to re-delegate to the same rolename (they delegate to role A with key a, then later change this to role A with key b). If we want to eliminate this re-delegation use case, we could prevent role name reuse by delegators. I think taking this power away from delegators will lead to a lot of confusion (for example, they delegate to a developer role, then to developer_1 role, etc until they are delegating to developer_42 which I think makes the role's purpose less clear).

Working Backwards:
I agree, I think working backwards would create a lot of unnecessary issues.

@JustinCappos
Copy link
Member

JustinCappos commented Oct 17, 2018 via email

@hannesm
Copy link
Contributor

hannesm commented Oct 27, 2018

@mnm678 thanks for your work:

  • First (naming of rotate files): if we leave the previous field of the initial rotate file empty (instead of the null key), I don't think we need to include the hash of the old rotate file into the new rotate filename. At least I don't know how to construct a collision.

(ex: role A is delegated to, creates a rotate file, is delegated to with a new key, creates another rotate file that will have the same previous hash as the first)

not sure I can follow here, since a rotate file contains the old keys, and also the previous hash, and its filename is constructed by a hash over the content. I may need a more complete example.

delegating if you have the same name w/ multiple keys (some revoked)

my intuition is that you can always delegate to the rotated key, and as outlined below the client workflow handles this well.

  • Second: right, since the key includes the keyid, there's no need to explicitly contain the keyid.

  • Third: I'm not entirely sure about the client implementation, but a possible workflow would be (consider a foo with key A being rotated to key B, i.e. foo.rotate.A exists; and there's a delegation to foo with key A):

    1. read the file foo
    2. signature verification fails (key B used, key A expected)
    3. read foo.rotate.A, verifies correctly and establishes key B
    4. file foo verifies successfully with key B

@mnm678
Copy link
Contributor

mnm678 commented Nov 1, 2018

@hannesm @JustinCappos
Clarifying key revocation:
I have a few thoughts about the role name discussion. Specifically how to ensure a key revocation is always seen by the client, and how to use delegations to ensure they don’t contradict rotate files.

First, the hash of the previous filename is included in the next filename to allow for cycles in rotation, to be used if a person/key leaves a team and then later rejoins the team. In this case a role foo with initial key A could create foo.rotate.A.null to rotate to key B, then foo.rotate.B.prev to rotate back to A, then foo.rotate.A.prev to rotate to key C. This ensures that foo.rotate.A.null and foo.rotate.A.prev have different names no matter when this cycle occurs.

Currently, a new delegation breaks any existing rotate chain for a role and can lead to confusion about which keys to trust, especially if a previously revoked key is later delegated to. For example, if key A is delegated to for role foo, then A rotates to key B (creating foo.rotate.B.null), then B to key C (creating B.rotate.C.prev), then the delegator changes the delegation to key B, should the client use key B or key C? I think that it makes sense for the client to use key C, but to do so it would need to find B.rotate.C.prev instead of B.rotate.C.null. This especially applies if key C was a null key (creating a revocation). The following is a system to make that happen.

I propose treating all rotate files as a chain of trust associated with a role name. The chain of trust starts with the initial delegated key, and goes through any rotate files to the current trusted key. The delegator can point to any key in this chain, and the client can check if this is consistent by using the filenames and previous fields to treat rotate files as a doubly linked list. If this chain is broken for any reason (a rotate to null of any key in the chain, or a delegation to a key not in the chain), then the role name is no longer valid, and trust must be delegated to a new role name to continue.

The chain of trust ensures that it is clear to both the client and delegator which key is trusted at any given time. All rotate files associated with a role name will be a part of the same chain, and once this chain is no longer valid a new chain with a new name is delegated to. This has the added benefit of allowing the server to delete unused rotate files after trust has been delegated away from that chain.

One downside of this proposed method is that existing delegators might have to change their delegation process to comply with the new spec. To change keys without the use of rotate files, the delegator would have to delegate to a new role name with the new key. TAP 8 is already a non-backwards compatible change, so I think that this is not a major concern.

Client workflow:
If the client only checks rotate files after signature verification fails, they might miss a revocation. I propose the following workflow:

1 look for rotate files and find foo.rotate.A.null
2 verify that foo.rotate.A.null verifies and establishes key B
3 read the file foo
4 file foo successfully verifies with key B

This ensures that if foo.rotate.A.null contained a revocation, the client would see this before verifying foo.

@JustinCappos
Copy link
Member

JustinCappos commented Nov 1, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Nov 5, 2018

@JustinCappos
One potential problem with this proposal is the roles defined in the TUF spec (timestamp, snapshot, etc). These roles can't be delegated to new rolenames in the case of a revocation as they are hardcoded in the spec (the client will always look for timestamp.json for example).

One option would be to require delegation to a new key after a revocation. This requirement would allow the chain of rotate files to start again from that new key. The old rotate files would still be around, but the new chain would not see them as long as the new key and any future keys are not part of the old chain. For example, if the timestamp role has an initial key of A, which is rotated to B, then self-revoked, the root can then delegate to a key C (but not to A or B) to start a new chain of trust. A and B would not be allowed in the new chain of trust.

This change could apply to all roles or be different for the TUF-defined roles (with other roles following the original proposal).

Any other ideas about how to handle this?

@JustinCappos
Copy link
Member

JustinCappos commented Nov 5, 2018 via email

@mnm678
Copy link
Contributor

mnm678 commented Nov 15, 2018

@JustinCappos @hannesm
I made some changes in the pull request relating to the single chain of trust issue discussed here, as well as some clarifications about null keys.

For TAP 5 I think it makes sense to keep the rotate files on the repository. The client would determine the current valid key using the rotate mechanism, then download the file from the provided url and check against the key. Is there any reason the rotate files should go on the mirror instead?

@mnm678 mnm678 added this to the TUF 2.0.0 milestone Apr 29, 2021
@mnm678
Copy link
Contributor

mnm678 commented Jan 24, 2023

I've been working on a POC for this TAP, and have a couple of simplifications that I'd like to make, but want to get some feedback first.

  • The filename for rotate files is listed in the TAP as requiring a hash of all currently trusted keys, and a hash of the previous rotate file (if any). My understanding is that the goal is to discover the next rotate file in the chain, which could also be done with a simple counter (which also makes the client logic much more simple), which re-sets whenever a new delegation is created (and all old rotate files for that role are deleted). Are there any other properties from the current filename setup that a counter is missing?
  • Similarly, rotate files in the TAP contain a "previous" field that lists the previous filename. This field was added to prevent cycles using the above filename scheme. As rotate files are now added to snapshot metadata, a simple version number (matching the counter in the filename) should be sufficient here to prevent replays.

cc @joshuagl @JustinCappos

@JustinCappos
Copy link
Member

I'm assuming that you also list the counter in the file so that the repo, etc. cannot change it. Perhaps expressly listing the filename in the file is the easiest way to do this.

If so, I don't spot a flaw with moving to the model you describe.

Are these counter numbers required to be sequential? What about a client that wishes to rotate their key on different systems? Suppose Alice rotated to Alice_key2 on repo 1. On repo 2 Alice only uses Alice_key2. Alice now wants to rotate to Alice_key3. What does the counter look like for this file? Can it be uploaded to both repositories?

I'm not sure this is changed by what you propose, but can you refresh my memory about whether all clients retrieve / analyze all rotate files?

@mnm678
Copy link
Contributor

mnm678 commented Jan 26, 2023

Are these counter numbers required to be sequential? What about a client that wishes to rotate their key on different systems? Suppose Alice rotated to Alice_key2 on repo 1. On repo 2 Alice only uses Alice_key2. Alice now wants to rotate to Alice_key3. What does the counter look like for this file? Can it be uploaded to both repositories?

If the same set of rotations are done on both repositories, the same rotate files can be used. If one of the rotate files in the sequence was not included on one repository (as in your example), then the counter would be out of sync and Alice would need to generate two rotate files (alice.rotate.0 and alice.rotate.1). With proper tooling this should not be a big issue.

I'm not sure this is changed by what you propose, but can you refresh my memory about whether all clients retrieve / analyze all rotate files?

Yes, they download the entire rotate chain. This ensures that any rotates to null are detected. This is the same in my proposal, except that the counter makes the chain more human-readable.

@JustinCappos
Copy link
Member

JustinCappos commented Jan 26, 2023 via email

@mnm678
Copy link
Contributor

mnm678 commented Jan 26, 2023

This ensures that the rotate files are processed in the correct order. Each file will be signed by a threshold of previous keys, and so the client needs to get them in order to ensure they end up with the correct key.

@trishankatdatadog
Copy link
Member

I'm still worried about the complexity of this feature (while a nice one for decentralised control) vs the benefits. I haven't heard of a use case for this beyond Haskell and Fulcio(?). It will make at least the TUF client that much more complicated to implement and test for correctness.

@mnm678
Copy link
Contributor

mnm678 commented Feb 22, 2023

In the case of large TUF deployments (PyPI, etc), having the top-level targets responsible for all key changes is not sustainable. Every time a maintainer is added to a project a repository admin has to be notified, find the offline targets key, and use this to sign a new delegation for the project. In many cases these are volunteer administrators, and this is a lot of work to put on them. TAP 8 solves this problem.

I have a WIP implementation that only adds or changes about 200 lines of code. This can likely be further simplified. Every feature adds some complexity, but I argue that we have worked to make this particular feature as simple as possible (with changes like #167), and it provides a much-needed improvement to TUF's ease-of-use.

@trishankatdatadog
Copy link
Member

I see, thanks for the explanation! I wasn't aware that community software repos were a major use case. I also glanced at your WIP implementation, and the changes to the client do look fairly small. I strongly recommend adding these use cases (e.g., community repos, and Fulcio) to the TAP in order to more clearly motivate the TAP. I also think a small benefits vs drawbacks analysis might be helpful.

@mnm678
Copy link
Contributor

mnm678 commented Feb 23, 2023

Thanks for the feedback! I'll update the TAP.

@jku
Copy link
Member

jku commented Feb 27, 2023

+1 to trishanks concerns: the implementation is not simple and it's also not complete (no client caching for example).

I don't want to derail reviewing the implementation but I'll leave these comments on the core idea:

  1. it seems to me that rotate files are more or less a temporary clutch: something that is needed until the delegating offline keys are used to "promote" the changed keys from the rotate file to the actual delegation. Likely repositories will not want to leave the rotate files in place for long as they make everything slower

  2. if rotate files are temporary, the question then becomes why not allow a single rotate file only? Delegated keyowners can modify the rotate file multiple times, it just has to always be signed by the original delegated keyowners

  3. Assuming that there is only a single rotate file, this is essentially the same as just another level of targets delegation -- rotate files are not needed at all:

    • toplevel targets delegates to foo-promoted
    • foo-promoted delegates to foo
    • foo contains the actual targets

    when role foo is initially created, the keyowners for foo and foo-promoted are same. Now the keyowners of foo-promoted can change the keyowners of foo whenever they want (modifying the practical targets maintainer list whenever they want). When these keyowner changes do happen, top-level targets keyowners can copy foo keyowners to foo-promoted keyowners at their own leisure. This requires no new roles or spec changes: it's something currently operating repositories can do today.

The above suggestion is not 100% equivalent to the TAP (as it temporarily creates a situation where there are two levels of foo maintainers) but I would like this TAP to describe why the complexity is warranted over the above solution that seems to require no spec changes.

@trishankatdatadog
Copy link
Member

Wanted to note @dstufft observation that perhaps a rotate file should also be signed by the new threshold T_2 of signers S_2. Otherwise, you could run into potential cases of abuse where an old threshold of T_1 of signers S_1 could make T_2 of S_2 responsible whether they like it or not.

@dstufft
Copy link

dstufft commented Jun 7, 2023

For PyPI explicitly we will likely want every new key to have to sign the rotation file, not just a threshold, because the goal for us would be to disallow someone to "force" another person to be associated with their project, without that person's consent.

We had this happen in the past when PyPI would allow you to add another person to a project (without invitation or confirmation, just blindly add people), to "steal" their reputation. Sometimes they would also leave the project themselves to make it appear like it came wholly from that person. We've prevented this at the PyPI level, but implementing this TAP for PyPI would mean reintroducing a form of that currently, through the key identities.

Of course PyPI could apply it's own logic upon receiving a rotate file, and independently confirm that the key holders are OK with it, but that has to be done entirely out of band and we would have to delay using the rotation file until that happens (which has consequences for what keys the project should be signing with in the future). So it's convenient to implement it by having the new keys sign the rotation file which proves that the key owners agree with the rotation.

One other note about having all of the keys sign the rotation file is it makes accidental key inclusions harder, since if you typo the name or something you won't be able to get a signature from that key unless you happen to own that key too. This could possibly cause people to be unable to reach their threshold or to have less of a margin for key compromise than expected. This makes a TUF repository more resilient to misuse.

@trishankatdatadog
Copy link
Member

One other note about having all of the keys sign the rotation file is it makes accidental key inclusions harder, since if you typo the name or something you won't be able to get a signature from that key unless you happen to own that key too. This could possibly cause people to be unable to reach their threshold or to have less of a margin for key compromise than expected. This makes a TUF repository more resilient to misuse.

Just wanted to note that one way to make this happen is to: (1) specify the threshold such that all signers must sign, and (2) enforcing this check in the package manager that integrates with TUF.

@trishankatdatadog
Copy link
Member

I should also add that I'm not a fan of fixing things like key IDs are assumed to be SHA2-256 hashes. Not only does it violate the principle of crypto agility in TUF (hence making it difficult to update metadata w/o breaking), but it also violates TAP 12.

@mnm678
Copy link
Contributor

mnm678 commented Jun 13, 2023

I should also add that I'm not a fan of fixing things like key IDs are assumed to be SHA2-256 hashes. Not only does it violate the principle of crypto agility in TUF (hence making it difficult to update metadata w/o breaking), but it also violates TAP 12.

This assumption is removed in this pr: #167 that simplifies the naming of rotate files.

@mnm678
Copy link
Contributor

mnm678 commented Jan 17, 2024

I did some clean-up on the existing TAP 8 prs (#165, #167, and #172), and added some minor clarifications in #181.

Once this is cleaned up, I plan to separate the self-revocation from the self-rotation into separate TAPs (with a new TAP for revocation. This new TAP should be much simpler, and will allow us to include these changes incrementally.

@mnm678 mnm678 mentioned this issue Jan 30, 2024
@kipz
Copy link

kipz commented Feb 5, 2024

I probably missed something, but wouldn't it be possible for the owner of a key (or an attacker) to self-rotate an arbitrarily large number of times and effectively DoS the repo role by putting a unwieldy number of rotate files in the chain?

@mnm678
Copy link
Contributor

mnm678 commented Feb 5, 2024

I probably missed something, but wouldn't it be possible for the owner of a key (or an attacker) to self-rotate an arbitrarily large number of times and effectively DoS the repo role by putting a unwieldy number of rotate files in the chain?

This would be a good one to discuss in the TAP. I think this is similar to an existing attack any key holder can do where they upload several different versions of the same metadata file. The mitigation would be for the repository to set rate limits after 1-2 of these have been uploaded (similar to endless data attack, etc).

@kipz
Copy link

kipz commented Feb 5, 2024 via email

@mnm678
Copy link
Contributor

mnm678 commented Feb 7, 2024

I added a brief discussion of this in #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants