-
Notifications
You must be signed in to change notification settings - Fork 52
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
Use right fulcio certificate when verifying bundles #32
Comments
I found the root cause, to solve that we need to verify the certificate against a set of root CAs. After a lot of searching, I found out that |
Yep!
Also -- Hayden and I scoped how this would look like in the future as more Fulcio and rekor logs are rotated. It will end up being that we add all targets that have an annotation `"usage": Fulcio" attached to them. Hayden made a PR here sigstore/cosign#1423 I wonder if awslabs has support for parsing/reading custom metadata attached to targets? I can try to look into that |
Thanks for pointing this out! In the meantime my plan is to just load all the files that are matching a certain pattern.
Yes, it seems this is handled: https://docs.rs/tough/latest/tough/schema/struct.Target.html - I guess they will show up inside of the There's another question I have. What is your plan regarding rekor's public key? What do you plan to do if this key is suddenly changed? |
We might have a "problem". We need to create a certificate pool composed of all the Fulcio certificates found inside of the TUF repository, and then use it to validate the certificates issued by Fulcio. Currently there are two Rust libraries that allow that: Unfortunately there isn't a clear winner and, most important of all, there's nothing that works out of the box. Let's take a closer look at both libraries. webpkiPros:
Cons:
pickyPros:
Cons:
Proposed solutionsThese are the possible solutions I see. I would personally go with the first one, the one that leverages webpki. Leverage webpkiI propose we build a solution based on webpki, using the patch that hasn't been accepted upstream. The patch is really small and not invasive. Hopefully we will find an agreement with upstream about that, and sort things out for their next release. We however have an issue. Given we want to release A common solution is to vendor the whole patched Perform a small change to the current codeCurrently, the code is verifying the validity of the certificates issued by Fulcio in this way:
We could change the code to:
Pros:
Cons:
(*) Yesterday a new version of |
Is this likely to always be the case or temporary, e.g. is it possible upstream code changes provide a method of handling intermediates?
How certain are we it will get merged (do you have a link to the PR?) |
Both webpki and picky handle intermediate certificates. It's our current naive implementation that doesn't.
To be honest, I'm not certain at all. There are lots of pending PRs. Mine is briansmith/webpki#254, which is pretty small and building on top of already existing code. That should give it higher chances to be merged 🤞 Another option would be to go ahead and implement EC support inside of picky. I've already worked with other EC libraries that are not ring, that wouldn't be a problem for me. But I'm not sure this is worth the efforts due to the limited popularity (compared to webpki) of picky (no offense for the maintainers intended!). |
I've worked on this PR Devolutions/picky-rs#132 that adds EC support to picky. |
Good news, the PR has been accepted and merged. A new version of picky which includes this fix is going to be released in the next days. I'll start working on the code that consumes the library, so that everything is going to be ready by the time a picky release is available. |
@asraa I just checked, it looks like this extra information is not yet added to the TUF repository (unless tough is not parsing it - which would be a bug - or I'm doing something wrong). Do you know if this additional metadata is already added to the targets definition? Thanks! |
great, they were evidently the good pick of the two if they reviewed that quickly. |
Create a struct that helps manage a pool of trusted root certificates. The certificate verification is then done using the `picky` crate. For more information about why the `picky` crate has been chosen, please refer to this conversation: sigstore#32 (comment) Signed-off-by: Flavio Castelli <[email protected]>
Fulcio has recently introduced a new root CA to issue certificates. Prior to this commit, the code was making use only of the initial root CA. Because of that, recent keyless signatures were considered not valid. Now the code is taking into account of the root CAs that Fulcio used over the time. This fixes sigstore#32 Signed-off-by: Flavio Castelli <[email protected]>
Fulcio has recently introduced a new root CA to issue certificates. Prior to this commit, the code was making use only of the initial root CA. Because of that, recent keyless signatures were considered not valid. Now the code is taking into account of the root CAs that Fulcio used over the time. This fixes sigstore#32 Signed-off-by: Flavio Castelli <[email protected]>
Create a struct that helps manage a pool of trusted root certificates. The certificate verification is then done using the `picky` crate. For more information about why the `picky` crate has been chosen, please refer to this conversation: sigstore#32 (comment) Signed-off-by: Flavio Castelli <[email protected]>
Fulcio has recently introduced a new root CA to issue certificates. Prior to this commit, the code was making use only of the initial root CA. Because of that, recent keyless signatures were considered not valid. Now the code is taking into account of the root CAs that Fulcio used over the time. This fixes sigstore#32 Signed-off-by: Flavio Castelli <[email protected]>
Hey! I just saw this. That's correct, we're waiting for version 3 root signing. The targets will looks like sigstore/root-signing#139 (comment) but we fallback on the hardcoded names |
@asraa thanks, would you be so kind to ping me when this change is finally in place (meaning the TUF metadata is updated) 🙏 |
Description
Fulcio has updated its certificate. Right now, inside of Sigstore's TUF repository there are 2 certificates:
fulcio.crt.pem
fulcio_v1.crt.pem
Keyless signatures produced some months ago have been signed with
fulcio.crt.pem
, while more recent ones have been signed withfulcio_v1.crt.pem
.Right now the
cosign::Client
uses just one certificate, which causes some signatures to fail verification.Apparently the
cosign
client isn't affected by this issue. We should understand how the upstream client handles that, and replicate the behaviour here.The text was updated successfully, but these errors were encountered: