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

TAP for TUF developer key management #141

Merged
merged 33 commits into from
Feb 7, 2023

Conversation

mnm678
Copy link
Contributor

@mnm678 mnm678 commented Jul 27, 2021

This TAP proposes the use of Fulcio to improve developer key management in TUF.

Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Aug 12, 2021

cc @JustinCappos

@trishankatdatadog
Copy link
Member

As I've mentioned before:

One thing I don't see is how to bind the specific, ephemeral developer public key to a specific version of the message (i.e., TUF targets metadata). Recording it on Rekor is one way, but the TUF client may not always be able to efficiently verify that, especially in offline environments. Perhaps the message could also record the ephemeral pubkey itself, but it's something to think about.

Basically, how do I know which keys were used to sign which messages, even if all the keys are vouched by the Fulcio root?

@mnm678
Copy link
Contributor Author

mnm678 commented Aug 12, 2021

Basically, how do I know which keys were used to sign which messages, even if all the keys are vouched by the Fulcio root?

The Fulcio certificate chain would be included in the signature. But yes, the client would need to make sure that this particular certificate is allowed to sign the metadata either by checking the TL directly, or by trusting the repo to do so. May be the repo could additionally indicate the correct certificate?

@axelsimon
Copy link
Contributor

Rendered version here, for those interested.

Copy link
Contributor

@axelsimon axelsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more comments / suggestions on this. Thanks for turning it into a TAP @mnm678!

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Aug 19, 2021

Thanks for the review @axelsimon! I updated the document

Copy link
Contributor

@axelsimon axelsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
mnm678 and others added 2 commits August 31, 2021 15:07
Co-authored-by: axel simon <github@axelsimon.net>
Fulcio can use any OIDC identity. This changes the metadata format
to reflect this.

Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Sep 9, 2021

@trishankatdatadog @joshuagl Any blockers on having this merged in as a draft?

joshuagl
joshuagl previously approved these changes Sep 9, 2021
Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good basis to merge as draft. Do we want to make an explicit recommendation (in security analysis?) that systems using Fulcio for developer key management SHOULD set up an auditor?

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Sep 10, 2021

This looks like a good basis to merge as draft. Do we want to make an explicit recommendation (in security analysis?) that systems using Fulcio for developer key management SHOULD set up an auditor?

Added

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
Also clarify that auditors may use TAP 3 multi-role delegations

Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Nov 29, 2021

@trishankatdatadog @erickt I made edits and responded inline. Any objections to merging as a draft?

Copy link
Contributor

@axelsimon axelsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more suggestions, trying to resolve open conversations / comments.

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
mnm678 and others added 2 commits November 29, 2021 11:58
adds consistent capitalization and some clarifications

Co-authored-by: axel simon <git@axelsimon.net>
Signed-off-by: Marina Moore <mnm678@gmail.com>
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
mnm678 and others added 2 commits November 30, 2021 11:13
Co-authored-by: axel simon <git@axelsimon.net>
Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Jan 10, 2022

The previous reviews were dismissed by some recent changes. @joshuagl @trishankatdatadog @axelsimon can I get 2 reviews/approvals for this to be merged as draft?

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
mnm678 and others added 3 commits November 4, 2022 13:44
Co-authored-by: asraa <asraa@google.com>
Signed-off-by: Marina Moore <mnm678@users.noreply.github.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mikhailswift
Copy link

mikhailswift commented Jan 13, 2023

Apologies if any of this is a bit off base or not the correct avenue, I'm still familiarizing myself with TUF a bit so I very well may have some core misunderstandings. Please correct me if so.

I see some overlaps with this functionality and the some use cases we have for defining Roles using SPIFFE/SPIRE short-lived certificates. Where this is focused around verifying the OIDC identity using the Fulcio issued certificate we want to do similar by constraining on a specified SPIFFE ID on a SPIFFE issued certificate.

I'm wondering if they may be some way we can come to a common solution for both? Or if perhaps creating a base PKI key type that both the sigstore and SPIFFE implementations could extend for their specific requirements?

I've done some with with in-toto ITE7 for similar types of functionality with regards to defining functionaries with PKI issued keys, if that's relevant at all here.

@mnm678
Copy link
Contributor Author

mnm678 commented Jan 13, 2023

I see some overlaps with this functionality and the some use cases we have for defining Roles using SPIFFE/SPIRE short-lived certificates. Where this is focused around verifying the OIDC identity using the Fulcio issued certificate we want to do similar by constraining on a specified SPIFFE ID on a SPIFFE issued certificate.

I'm wondering if they may be some way we can come to a common solution for both? Or if perhaps creating a base PKI key type that both the sigstore and SPIFFE implementations could extend for their specific requirements?

I've done some with with in-toto ITE7 for similar types of functionality with regards to defining functionaries with PKI issued keys, if that's relevant at all here.

I think that we can certainly have a similar mechanism for using SPIFFE/SPIRE identities with TUF.

My main reason for keeping this proposal limited to Fulcio is to simplify the fields required in the delegation and signature, and ensure that each key type has all fields that are needed for a secure implementation. For example, the Sigstore bundle in the signature is vital for the security of Fulcio signatures.

Highlight that existing Sigstore tooling should be used

Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Jan 17, 2023

@JustinCappos @trishankatdatadog @joshuagl are we ready to merge this as a draft?

JustinCappos
JustinCappos previously approved these changes Jan 18, 2023
Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine merging as draft. I would like to have someone from the Sigstore side also review this before moving the TAP to accepted. Ideally we would have more clarity around monitoring TLs, etc. at that time too...

@znewman01
Copy link

someone from the Sigstore side also review this
Consider this comment an IOU. I'll also circulate it more generally.

Ideally we would have more clarity around monitoring TLs, etc. at that time too...
@SantiagoTorres is pushing hard on this on the Sigstore end so I bet he'll be a good source for updates. I think @mnm678 is also thinking about TUF<->TL interactions more generally as well.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great write-up, Marina! I think this will be fun to implement, e.g. for python-tuf using the new signer API.

I left a couple of minor comments inline, admitting that I don't know sigstore well enough for a detailed review of the signing and verification workflows.

I do have two high-level comments:

  1. Talking about "repository" and "developer" in this TAP requires some implicit assumptions about a particular TUF setup. Likely something like PEP 480. For the sake of generality and clarity, I wouldn't make those assumptions. Instead, I would limit the scope to the specification of the new key and signature metadata formats, and the signature creation and verification (online and offline), plus monitoring routines.

  2. A more vague and very high-level observation: We are conflating two PKIs here, which both seem to offer similar concepts like key hierarchies, expiry, revocation, etc. I can't pin-point a particular problem with that, I just want us to make extra sure that those concepts are not in conflict with each other, or add extra complexity to TUF without a practical security advantage.
    I mention this, because I just made a similar realization with gpg keys and signatures and redundant delegation and expiration checks.

Oh, and some more minor nit-picks (feel free to ignore):

  • There's a random mix of and " in this document. I suggest to s/”/"/g. (Same goes for and ').
  • Sentences in lists should end consistently with (or without) ..

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
Signed-off-by: Marina Moore <mnm678@gmail.com>
Especially this:
* generalized the "repository"
* generalized the "developer"
* fixes links

Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Jan 30, 2023

Thanks @lukpueh I updated the pr, and tried to generalize away from the "developer" and "repository" terms where possible. The main point is that someone is signing the metadata using an OIDC account, and some central entity is responsible for performing some Sigstore verification.

Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. I think this is good to be merged as draft. I left a few non-critical comments.

candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
candidate-fulcio-tap.md Outdated Show resolved Hide resolved
mnm678 and others added 2 commits February 2, 2023 10:41
Co-authored-by: Lukas Pühringer <luk.puehringer@gmail.com>
Signed-off-by: Marina Moore <mnm678@users.noreply.github.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
@mnm678
Copy link
Contributor Author

mnm678 commented Feb 2, 2023

Thanks @lukpueh!

If you think it's ready to be merged as a draft can you mark as approved?

Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ready for draft status...

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for continuing to drive this @mnm678 ! I am happy to merge this as a draft. I left a few minor comments, but we can easily handle those in a follow-on.

It would be good to get a 👍 from @znewman01 or another Sigstore expert before we do merge.

candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
Copy link

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With my Sigstore maintainer hat on: this LGTM as a draft.

candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
candidate-fulcio-tap.md Show resolved Hide resolved
@mnm678 mnm678 mentioned this pull request Feb 7, 2023
@mnm678
Copy link
Contributor Author

mnm678 commented Feb 7, 2023

I'm going to merge this, with #168 as a followup to address some minor comments, and #169 to document future changes, especially those that are pending documentation from the Sigstore side.

@mnm678 mnm678 merged commit 3b0d8b1 into theupdateframework:master Feb 7, 2023
@mnm678 mnm678 deleted the fulcio-tap branch March 1, 2023 15:15
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.

None yet