Removing the root cert from the bundle's cert chain? #75
Comments
|
sigstore/cosign#2143 (comment) - my thoughts on this previously I view it as a verification implementation error to trust the last certificate in the chain without comparing it to a root provided out of band. Happy to revisit this if folks think otherwise! |
|
Yep -- I agree that it's an implementation error to trust the last certificate in the chain, but I also think we can nudge people away from that implementation error by defining the problem away 🙂 From your comment:
I might be wrong, but I don't think this is true: here's what I get from the TLS handshake for (via i.e. it sends the leaf, then an intermediate, and then a cross-sign for the intermediate, but no actual root CA. Chrome shows the root CA in its cert viewer, but that's only because of a successful out-of-band chain construction 🙂 |
|
I also talked a bit with @reaperhulk about the RFC language referenced in sigstore/cosign#2143 (comment), and we think that this part:
Refers specifically to intermediates and possible client-side intermediate caching, given that the first part refers explicitly to the trust anchors (i.e. root CAs) being distributed independently. And that would comport with that |
|
Couple thoughts: is this a breaking change? Possibly, because clients would expect a root cert in the final spot? But if the clients are just python and JavaScript now, and I think you both load in presented certs into cert pools, maybe we’re good. do we need an identifier, the subject key ID of the root, if the root is omitted? The intermediate should contain the authority key ID for chain building, but in byo pki cases, it may not. Either we consider that a hard requirement or provide the SKID of the root, like how we provide the log ID for the tlogs. |
|
Just for some context. We are aware that shipping the entire cert chain can be seen as problematic. The client spec clarifies how to verify an artifact via the bundle:
But if we feel that there is a risk of creating faulty implementations, we may revisit the bundle format. We decided to not allow any public keys to be stored in the bundle for exactly this reason, it could lead to footguns. As the spec mandates that anchoring trust to an OOB delivered root, it was deemed safe to ship the entire chain. edit: Added link to the Sigstore client spec |
I think this is arguably a breaking change, but I think it's also a safe change to make for the reasons you've mentioned: both the Python and JS clients (thankfully) do the correct thing already and don't need/access the other chain members at all. We're also technically below 1.0 for the protobuf specs, so this kind of "breaking only in spirit, not in practice" change is arguably okay 😉
I might be underthinking it, but I think we'd be okay without any additional key ID -- the intermediate (or even just the leaf really, since for the public deployments the intermediates are also shipped via TUF) contains sufficient context for chain building, since it tells us the next step in the chain via its AKI extension. (This is actually an argument IMO for dropping the entire chain, and stipulating that even the intermediate(s) must be delivered over TUF or a similar trusted mechanism. But I think that's completely separate from this, and doesn't represent a footgun the way this does.)
This reasoning makes sense to me, but I personally fall on the side of thinking that this is a footgun! I had to double (and then triple-check) sigstore-python to make sure I wasn't accidentally doing this, and I think the only reason I didn't do it inadvertently is because I shoehorned the bundle's deserialization into our pre-existing data models (which only take the leaf, and not any other parts of the chain). That's purely coincidental though; it I had written the bundle support from scratch I would likely have skipped that part of the client spec and accidentally shoved everything into the chain building context! |
I'm not gonna say this is a solution, as it's not (you can't get high quality or security via only testing). But when we get more traction on the conformance testing, I would envision that we have test-cases for scenarios like this -- bundle with roots that does not exist in the trust root and other negative test cases. Shipping only the leaf certificate makes the most sense from ease of client implementation, and keeps the bundle smaller. I'm wondering if we know the reasoning for the inital Sigstore design to ship the entire chain? As we did replicate that behaviour when creating the bundle. @haydentherapper @znewman01 maybe have more historical context? |
Agreed! This is a another strong argument for us pushing forward on bundle test cases in I'd also be curious to understand the context/historical reasons for including anything besides the leaf -- I can imagine in making some BYO PKI setups a little easier, but I'm a little hazy there 🙂 |
|
Correct, the context was BYO PKI, for those that only provide their root out of band and view intermediates as just chain builders. FWIW, this has been a little messy in Cosign, because we have logic where we first check the TUF root for the intermediates and if it's not present, then we check the bundle/OCI manifest. Sigstore's TUF root does ship the entire chain, and it would be my preference to mandate this because TUF becomes the revocation mechanism rather than deal with CRLs. However we did get some feedback for Cosign where someone said they wanted to use CRLs - While I'd prefer to not support that, if there's enough interest for BYO PKI, we might do so. One way we could do it is having a required field for the leaf and an optional field for the intermediate chain. We could also do it is a one-off, either setting just the leaf cert or the chain (leaf + intermediates). For the public deployment of sigstore, we'll set only the leaf cert in the bundle, and rely on the TUF trusted root for providing the chain. This shouldn't complicate client logic any more than it already is. So it becomes:
Thoughts @woodruffw @kommendorkapten ?
Not necessarily for BYO PKI cases, unless we mandate that intermediates contain the AKI. They should, and something like RFC5280 mandates it iirc. I'm fine saying that chain building isn't possible without the AKI so it should be mandated. |
Making sure I understand: does this mean that in one scenario the bundle would still include the full chain, including the root, or that the chain would only be the leaf + intermediate(s)? If it's the latter, that sounds great to me!
Yeah, I think it would be nice for us to ratchet down on RFC 5280 here and mandate the AKI. @reaperhulk pointed out to me that AKIs aren't technically necessary for chain building (OpenSSL will also compare DNs, apparently), but we have the opportunity to do everything right from scratch 🙂 |
My preference is if we could make this via a patch update, i.e only update the comment to state:
This would not break the message format, and clients that ignores edit: s/leaf/intermediate/ |
|
Requiring SKI and AKI sounds like a good plan 👍 |
Oh, that's a good point -- yeah, preserving the wire format would be nice here. |
|
+1, I'm good with that as a plan (That's effectively what Cosign now does anyways, if the intermediates exist in the trust root) |
|
Just realized that this is reused in the trustroot proto too, the chains will contain a root and optionally contain a leaf (the leaf is included for the TSA, but not for Fulcio obviously). So we might need to rethink this. Maybe just update the comment in https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_bundle.proto#L54? |
|
WDYT about two identical (in terms of wire layout) types, but with different names to distinguish their purposes? The trust root could continue to use (If you think that's too complex or not worth the additional codegen, then I'm good with just updating the comment + adding additional client checks during signing and verification that ensure a root isn't present!) |
|
I would vote on what @haydentherapper proposes, update the comment in the bundle that the root must not be present. This is a non-breaking change. My first gut feeling was to require a client to abort if the bundle contains a root cert, but that could cause problems if a new client verifies an old bundle, so that would be a breaking change. |
|
Do we then need to instruct clients to inspect all certificates in the bundle and ignore any roots if present? So then the comment becomes "The bundle certificate chain SHOULD contain a leaf and MAY contain a set of intermediates. The bundle SHOULD NOT contain the root, but if present, the client MUST ignore the root for verification purposes. The client MUST confirm if the last certificate in the chain is a root or not, by comparing the certificate's Subject and Issuer fields" Thoughts? |
|
FWIW, I would prefer to keep the full cert chain in the bundle. The java logic we have now expects a full chain, and it verifies that the chain concludes in a known trust anchor. But maybe the Java logic just worked out conveniently that way and I'm being lazy asking for no changes 🤷 Removing the chain seems like I now have a more complex lookup for the intermediate/root, and do a reconstruction. Is there enough information in the bundle to make that easy? We already should be doing a few checks here:
I think we can still check that clients are behaving via conformance? |
I think we can have our cake and eat it too: if we set an "epoch" after which clients must not include the root in their bundles, then any previously generated bundle (i.e., based on leaf issuance time) can be given a warning and any subsequently generated bundle can produce a hard error. (This will also make inclusion in the conformance suite easier, since we'll be able to confirm that verification actually fails with a "dangerous" bundle.) The other option there (which is also reasonable, IMO) is to specify that the bundle
Yeah, I think it's purely convenience (and a little dangerous) that it worked out that way 🙂. TLS and other ecosystems that I'm aware of all discourage sending full chains, because "accept the chain and check" is much easier to accidentally mess up than "sort into trusted and untrusted and build the chain."
Yep! Depending on what you're doing (I know very little about Java cryptography), there are two routes I can think of:
|
This is true for the sigstore go library too (go has a method for chain building). Sigstore-js implements this) |
|
Sorry, just catching up on this. Overall I'm a big +1 on taking the root out of the bundle, very carefully. I think it removes a big footgun. Given that:
I don't actually think a "flag day" is necessary: in terms of backwards compatibility, I don't see a big deal with continuing to accept chains that have the root in them indefinitely. The footgun that we're looking to avoid is depending on this root and accidentally accepting it even if it doesn't come from a trusted source. IIRC a lot of "store-style" APIs will handle this nicely out of the box—does that make sense, esp. @woodruffw ? |
so only leafs? or everything but root? |
|
I think @znewman01 proposes makes sense (which is very similar to what @woodruffw proposed). It follows the robust principle, be liberate in what you accept (i.e the root may be present but ignored), and strict in what you produce (no roots in the bundle). As we don't have any mass adoption yet, and the clients I'm aware of would not break if we start to remove the root certificates from the bundle we can roll this out "easily". (sigstore-js would not break, and my understanding is that the the Python client would still continue to work.) For intermediates, we would need to specify the expectations. There is a desire to support the BYO PKI scenario, where the intermediates may not be available via the OOB method. As this is mostly for the non public-good instance, I think we should support it to make it easier to adopt the bundle. |
|
Yeah I'm okay with anything as long as we have very strong guidance around expectations. |
This is getting complicated enough that it warrants discussion at the next sig-clients meeting, so the below should just be considered my opinion.
+1, we'll add this in both the client spec and the protobuf-spec docs. tl;dr: I think we want to recommend that signers only include things below the intermediate (given that the public good instance sets This is a little wonky because we distribute the intermediates using TUF as well (CC @asraa, @haydentherapper). I guess strictly speaking, the verification procedure for the public good instance should be:
|
I'm not sure about Java and other ecosystems, but this unfortunately isn't true of OpenSSL (or other cryptographic libraries that have inherited OpenSSL's baggage): store construction isn't divided into trusted and untrusted components, because the act of adding a cert to the store is considered equivalent to trusting it. That's one of the footguns I'd like us to define away: clients who do something naive like using an OpenSSL (or LibreSSL, etc.)-style To that end: what do you think about my other proposal (i.e., the non-"flag day" one)? I think making this into a Edit: Also, 👍 to discussing this at the clients meeting on May 2nd! |
I guess my concern is that a syntactically-correct but logically-incorrect Sigstore bundle can exploit this as well: we can't rely on clients not to do something. So then we have to add the check anyway. I think this still mostly works with openssl-style APIs: you should add only the root as a trusted element of the store. Then:
|
|
Some of the questions overlaps quite much with the client spec update I worked on end of last year, related to verification against the trusted root. I feel like it's time to revive that content and add it to the client spec for discussion, the larger topic at that time not being resolved was related to polices (which I think we can punt on for now). The major topic discussed here (certificate chain building) was not covered in detail (it relied on the existing way cosign does it, i.e a but underspecified 😄) . But as we are seeing more interest in the trust bundle now, and the bundle is also in production TUF root I think it's time to hash out the details. To make sure we have enough written documentation before the next client spec meeting, I can extract the previous details and the primary topic in this PR and suggest it into the client spec. |
|
The split seems to be between verification with the public good instance (where intermediates will always be present in TUF) and private instances. Would it make sense that environment be encoded in the bundle? At the very least, we can mark if we're verifying something from PGI vs elsewhere and have stricter verification logic for PGI. |
|
In terms of the bundle, we expect the inputs to a verification to match the protobuf-specs/protos/sigstore_verification.proto Lines 119 to 136 in b6d2576 This includes the environment, encoded in a protobuf-specs/protos/sigstore_trustroot.proto Lines 59 to 88 in b6d2576 as well as general verification options: protobuf-specs/protos/sigstore_verification.proto Lines 46 to 48 in b6d2576 It would make sense to me to make this a property of the protobuf-specs/protos/sigstore_trustroot.proto Lines 43 to 57 in b6d2576 |
I'm not sure 🤔 This value is unauthenticated and so could be misused. The verifier would still require to make a decision based on a policy on how to verify, so I think @znewman01 's proposal on adding a parameter to the edit: s/your/znewman01/ |
Yep, agreed that the check needs to happen anyways. I see this more as a way to introduce additional test cases, with explicit success and failure states -- with a
I might have misled you here: there are no "trusted" or "untrusted" members of an OpenSSL-style store. All members are considered trusted, since the act of adding a member is considered equivalent to trusting it. That's why the "leaf + intermediates + root" case is potentially dangerous with OpenSSL/LibreSSL/BoringSSL/etc.: a client might think that they're only adding untrusted chain-building components, when what they're really telling the API to do is to trust everything 🙃 |
What sigstore-js does is that it verifies each certificate in the created chain, that it is included in the trusted store provided before using the chain to do path verification. Approx algorithm is:
So I feel like a similar algorithm should be fairly easy to implement with an "OpenSSL-style store". |
ACK, makes sense.
No, I think I was just a little unclear. I still think this works, it's just a little awkward. We should probably add an "implementation note" in the client spec.
If I'm reading you correctly, this means that a CA could use an intermediate that's not from the trust root, which I think is the whole point of embedded intermediates in the trust root. This point could definitely use clarification in the spec. We can also consider adding "implementation notes" or so. |
Yes, that may be the case. I'll have to dig more into this, I'm not very used to JS so I might have misunderstood something. But agreed, the entire point of having the intermediates in the trusted root is to make sure that we can revoke them if needed, and so we should not accept unknowns intermediates in the bundle. |
This sounds like another good (I believe sigstore-python doesn't use any intermediates from the bundle currently, so we shouldn't have any incorrect potential chains here.) |
|
Taking a step back for a second: the lack of clarity (including on my part!) about what each client currently does indicates that we need to start with tests here -- this is exactly the kind of thing the conformance suite is intended for, and we should take advantage of it. So, as an actionable proposal: we should create some test vectors ensuring that current implementations do not trust random intermediates or roots that don't also appear in the trust root, and that they can gracefully handle bundles that only contain leaves. CC @tetsuo-cpp, since I know you were working on bundle support in the test suite. |
|
Okay, I've been thinking about this a bit more. I think it's possibly clearer to view verification as a two-step process. Imagine the following structure: To verify:
Then, the guidance for clients: clients SHOULD include the leaf up to (but not including) the Fulcio certificate in the bundle. So: This has a few advantages. First, the client spec becomes much simpler: just use the provided Fulcio certs. We avoid departing from RFC 5280 by requiring any extra checks. We don't need to filter the certificates from the chain or anything (I worry a client might forget). Second, all of the complexities about which intermediates to trust can get moved into the "policy for distributing roots." This makes sense, as that's a peculiarity of our deployment, not a fundamental part of the model. If you want to avoid surprise intermediates, you should set EDIT: there is one potential issue. If the root CA expiration is before the intermediate's expiration, we need to propagate that information. |
I think this makes sense, but that would be for public good Sigstore, right? As in the BYO PKI scenario Fulcio may not even be used. Will this be clear enough in the specification? But comparing this with the other alternative proposed in this discussion. Having a parameter (either on the CertificateAuthority or on the ArtifactVerificationOptions ) that dictates how chain building works (if we allow any intermediates to be used from the bundle or not). My understanding is that this would still be RFC 5280 style chain building, it's a question based on which material we build the chain. This is what I think you refer to here:
So that I understand, your comment is mostly around how the spec clarifies what certificates goes into the bundle? For public good instance we can for sure describe what you propose, but I'm afraid that we may end up underspecified for the BYO PKI. |
|
I wrote a bunch below but it's pretty rambly, hopefully this clears some things up but if not we can discuss.
I don't understand why you'd need the Sigstore specification for BYO PKI. It's up to you in that circumstance to figure out how to validate. If you have BYO PKI, are your certs even short-lived? We don't know that, and can't write a one-size fits all spec for you. I'm generally opposed to over-optimizing for this use case in the specifications/bundle format.
I guess my point is that, for the "Sigstore Client Spec," describing it as "get some roots and some intermediates, then do some extra checks that are deployment-specific to build a path from a leaf up to the roots" is really complicated both to explain and implement. Whereas "(1) figure out which Fulcio certs you trust, and use those in the trusted certificate store; (2) you MUST validate like you'd validate any other certificate" is friendlier to openssl-style APIs (which are quite common) and moves all the complexity about figuring out what to trust over to the deployment ("Public Deployment Spec"), which is good because it's a deployment-dependent process. Then we don't need a configuration option for how to validate, we just have to construct the trust root correctly.
My comment is mostly about how the "Client Spec" describes the certificate validation process (section Verification -> Certificates). The concern is that if we say "use the roots and the intermediates from the trust root, and also you can take some intermediates from the certificate chain but how you do that is totally up to you but you do have to be careful about it" is confusing (I think @woodruffw points out that this is really confusing for openssl-style verification APIs). BUT it's pretty easy for us to say in the client spec "just validate like normal, chaining up to the Fulcio cert that you got securely out of band." Then, if we want to enforce the rule "only use intermediates that come from TUF," we do that when describing certificate distribution (which is a property of the Public Good Deployment):
That's why a simple rule like "always chain up to the Fulcio cert, ignore everything above that" is nice. That should always be true. Then "how signatures are being verified" is part of "get the keys securely, how you do that is out of scope." I don't know if we're disagreeing really, I just really want the "Client Spec" to be simple and prescriptive about this question and the quirks of how this actually works in Sigstore can go in the "Public Deployment Spec." |
No, I believe we are trying to achieve the same thing :) It's the BYO PKI adds confusion IMHO. And I think that can be left outside of the "Client Spec", i.e if you have BYO PKI you are on your own. |
👍
IMO, we should prioritize the full Sigstore flow in this repo. Then, we can have guidance about BYO PKI which basically says "hey, use this Client Spec flow with modifications X and Y and you might want to use the protobuf-specs bundle, disregarding Z." We don't want to leave BYO PKI folks in the dark, but at the same time I don't think we can anticipate every need they might have. |
💯 |
|
LGTM on that, let’s make that explicit in the documentation. |
|
Okay, so to summarize the expected doc changes here:
If that sounds good, then I can create a PR and close this out! |
|
LGTM, (3) seems necessary to avoid breaking backwards compatibility. Should it be |
Yes, good point! |
I was thinking about this some more:
protobuf-specs/protos/sigstore_common.proto
Lines 167 to 175 in 4dbf10b
...and I think, as specified, it presents a sharp edge towards Sigstore clients: the naive thing to do would be to shove the entire cert chain into the verifying context (i.e. a thing that looks roughly like an OpenSSL
X509_STORE). However, if a client does this, then Sigstore signatures effectively become self-signed: any malicious user can insert a cert chain that includes a root CA that isn't one of the trusted ones, and chain validation will resolve against that CA instead.Clients still need to handle this their own (they should verify the expected contents of the bundle's cert chain), but as a risk reduction strategy: what does everyone think about specifying the root certificate out of the bundle's cert chain? That would turn this from a silent "gotcha" into a spec violation, one that we could write conformance tests against.
The text was updated successfully, but these errors were encountered: