-
Notifications
You must be signed in to change notification settings - Fork 31
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
WIP: V1 of the trust root and the verification input. #5
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.
This feels pretty close to me! Good work
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'm not blocked on any other comments from my end!)
@asraa @haydentherapper @znewman01 All discussions are resolved, please take a review with fresh eyes :) |
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 fact that I'm being this pedantic means we're really close, I swear :)
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 I'm happy at this point!
Please get approval from others too before merging of course :)
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.
Thank you so much for continuing to make headway on this, super exciting!
dev.sigstore.common.v1.HashAlgorithm hash_algorithm = 2; | ||
// The public key used to verify signatures generated by the log. | ||
// This attribute contains the signature algorithm used by the log. | ||
dev.sigstore.common.v1.PublicKey public_key = 3; |
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.
Do we expect more than public_key being embedded in the TUF target? Today we only have the public_key in the TUF root, so just wondering if we're going to bake some of this information into our TUF targets? For example, we could serialize this into the TUF target for a given Rekor instance, so just wondering out loud. Jotted it down here, but applicable to CA below as well.
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 can't say for sure, but I think so. As an example the base_url
corresponds to the metadata field uri
in the TUF target. We are talking about adding a time range for the log too, which would then populate the same fields in the linked public_key
. The same reasoning goes for CA too of course.
This is not the complete set of attributes, some are missing still like the hash and signature algorithm for the log, or the distinguished name for a CA. As they are relevant from a verification perspective, we should think how such values are communicated. The distinguished may be part of the policy decision though.
// | ||
// The TrustedRoot is not meant to be used for any artifact verification, only | ||
// to capture the complete/global set of trusted verification materials. | ||
// When verifying an artifact, based on the artifact and policies, a selection |
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.
Just thinking out loud if CA & TransparencyInstance has enough information in there to write policies against. Both of them have a URI / base_url and CA has DistinguishedName, but just curious if that's enough and that's what one would use to filter out the instances that should be used for verification.
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 guess thinking that the one (only?) reason for having the TrustedRoot as a collection of all the everythings is that you can then pass it through some sort of policy machine which will distill it down to the verification materials before calling the actual verification with a subset of keys/authorities.
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 guess thinking that the one (only?) reason for having the TrustedRoot as a collection of all the everythings is that you can then pass it through some sort of policy machine which will distill it down to the verification materials before calling the actual verification with a subset of keys/authorities.
Yes, that's the exact reasoning.
Based on the scenarios I'm aware of, we should have enough data to confidently distill what exact "instance" of a CA or log to use:
- The name (log_id or distinguished name)
- URI
- Time range the service was active for
What could possibly cause some confusion is that if a client is relying on a TUF multi root setup, where multiple TUF roots exposes targets for the log or CA with identical data. But this is an explicit confusion as I see it, and so the precondition that the client trusts the TUF root may not hold any more, then all bets are off, as the TUF repository may serve arbitrary data. But I don't think of this as a real problem.
What the policy will protect against is e.g. where an artifact was signed with a certificate from the wrong CA given a policy, and for that I'd say we are good.
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.
Fantastic. Again just thinking of how the layer above this (policy-controller) will sort out the details. But, I guess that's my problem :)
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'm working a bit on that too, I hope that I can share some details on this later today!
@asraa @bobcallaway @haydentherapper Any final thoughts? |
8068a74
to
357487f
Compare
This looks good to me (with the caveat I mentioned in the issue: hard to tell if it's complete when it's not entirely clear how clients use the data -- this may be more clear to folks more familiar with the implementations). This approach could also simplify TUF repository design (as I think @vaikas may be implying in a comment): If the sigstore clients are expected to support the protobuf in any case, using the protobuf as the TUF target would make a lot of sense. In fact using anything else as tuf targets would require some good reasons... This would also solve the TUF target discovery issues* as we will have a single target (per sigstore instance) with a well-known targetpath -- the complexity of dealing with multiple certificates etc is no longer a TUF repository problem, but correctly a dev.sigstore.trustroot protobuf handling problem. *) TUF target discovery issues: using attributes in TUF metadata to figure out which targets should be used has safety issues if combined with delegations |
That's a very interesting idea, as it would probably remove the need for the custom metadata on the targets, and overall become a huge simplification! @asraa wdyt? |
Yes @jku that's what I was after, so thanks for being able to decode the thought :) Part of the reason for my wonderings is because of the work that I'm doing in policy-controller to be able to add support for multiple TUF roots, as well as 'bring your own keys/certs' so thinking through how this plumbing would look like from that end. If the specification for identifying CA or TLog can all be captured in a single proto (well, one for each of them) seems like that's the unit that we care about. I was not saying that we should combine the two protos necessarily, but using serialized protos in our TUF root seemed like a nice thing to have. If you want more musings about the policy-controller work, it's here: |
Yep! @jku and I discussed, and I'm going to track the rest of it on: sigstore/root-signing#539 I'd be happy to target the next root-signing update to add this in for optional consumption. Right now there's no target (hahaha) date on that, but we can make one :) |
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signer identities are moved from trust root to verification options. Key encoding are grouped to key format to include signature algorithm. Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
Signed-off-by: Fredrik Skogman <[email protected]>
…tains. Signed-off-by: Fredrik Skogman <[email protected]>
Updated makefile to build all proto files. Signed-off-by: Fredrik Skogman <[email protected]>
038e939
to
ad5ffe1
Compare
Rebased to resolve conflicts with main. Ping @znewman01 @asraa @haydentherapper @bobcallaway |
Signed-off-by: Fredrik Skogman [email protected]
Summary
This PR introduces the structure to capture the resolved trust root, and the data-structures needed for performing a sigstore bundle verification. This work would simplify the design of cross languages verification interfaces and implementation. The work is relevant for future additions of the policy controller and the RFC for linking source to npm packages.
Closes #4
Release Note
NONE
Documentation
NONE