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

WIP: V1 of the trust root and the verification input. #5

Merged
merged 20 commits into from
Nov 18, 2022

Conversation

kommendorkapten
Copy link
Member

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

protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
protos/sigstore_trustroot.proto Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
@kommendorkapten kommendorkapten changed the title V1 of the trust root and the verification input. WIP: V1 of the trust root and the verification input. Nov 1, 2022
protos/sigstore_common.proto Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

This feels pretty close to me! Good work

protos/sigstore_verification.proto Outdated Show resolved Hide resolved
protos/sigstore_verification.proto Show resolved Hide resolved
protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
Copy link
Contributor

@asraa asraa 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 not blocked on any other comments from my end!)

protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
@kommendorkapten
Copy link
Member Author

@asraa @haydentherapper @znewman01 All discussions are resolved, please take a review with fresh eyes :)

Copy link
Contributor

@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.

The fact that I'm being this pedantic means we're really close, I swear :)

protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
protos/sigstore_common.proto Outdated Show resolved Hide resolved
znewman01
znewman01 previously approved these changes Nov 14, 2022
Copy link
Contributor

@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.

I think I'm happy at this point!

Please get approval from others too before merging of course :)

Copy link

@vaikas vaikas left a 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!

protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
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;
Copy link

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.

Copy link
Member Author

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.

protos/sigstore_trustroot.proto Outdated Show resolved Hide resolved
//
// 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
Copy link

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.

Copy link

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.

Copy link
Member Author

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.

Copy link

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 :)

Copy link
Member Author

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!

@kommendorkapten
Copy link
Member Author

@asraa @bobcallaway @haydentherapper Any final thoughts?

znewman01
znewman01 previously approved these changes Nov 15, 2022
@jku
Copy link
Member

jku commented Nov 15, 2022

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

@kommendorkapten
Copy link
Member Author

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...

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?

@vaikas
Copy link

vaikas commented Nov 16, 2022

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:
https://docs.google.com/document/d/1a-tV9YyXHvtoaZXm-eMcqP-zFTWg6Pqfjyl8W4uS9F4/edit#

@asraa
Copy link
Contributor

asraa commented Nov 16, 2022

@asraa wdyt?

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]>
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]>
Updated makefile to build all proto files.

Signed-off-by: Fredrik Skogman <[email protected]>
@kommendorkapten
Copy link
Member Author

Rebased to resolve conflicts with main. Ping @znewman01 @asraa @haydentherapper @bobcallaway

@kommendorkapten kommendorkapten merged commit 44211d4 into sigstore:main Nov 18, 2022
@kommendorkapten kommendorkapten deleted the trustroot branch November 18, 2022 07:32
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.

Define a Sigstore root of trust
6 participants