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

spec: Root CA in the signature manifest vs out of band root? #2143

Closed
asraa opened this issue Aug 9, 2022 · 11 comments · Fixed by #2152
Closed

spec: Root CA in the signature manifest vs out of band root? #2143

asraa opened this issue Aug 9, 2022 · 11 comments · Fixed by #2152
Labels
bug Something isn't working

Comments

@asraa
Copy link
Contributor

asraa commented Aug 9, 2022

Description

I'm curious if I'm being pedantic, or what the rationale is here.

The spec says:

chain string This OPTIONAL property contains a PEM-encoded, DER-formatted, ASN.1 x509 certificate chain. The certificate property MUST be present if this property is present. This chain MAY be used by implementations to verify the certificate property.

The example chain also includes a root certificate for Fulcio. I don't think that this chain "MAY" be used to verify the certificate property, because you MUST be provided your root CA certificate out of band.

This also appears in the proposal for #2131

This seems like a footgun.

I definitely want to open the conversation around this because other client workflows like sign-blob outputs should be aware.

@patflynn @haydentherapper @mnm678 @znewman01

Version

@asraa asraa added the bug Something isn't working label Aug 9, 2022
@asraa
Copy link
Contributor Author

asraa commented Aug 9, 2022

Note that cosign specifically excludes the root: https://cs.github.com/sigstore/cosign/blob/36e18875ce466bb85ac57f2d39b009624248e1da/pkg/cosign/verify.go?q=Chain%28%29+repo%3Asigstore%2Fcosign#L537

This is a really subtle detail and other client implementations really need to be aware not to include the root.

@dlorenc
Copy link
Member

dlorenc commented Aug 9, 2022

I think we should fix the example, the root should never be included.

@patflynn
Copy link

patflynn commented Aug 9, 2022

@loosebazooka @jvanzyl FYI.

@asraa
Copy link
Contributor Author

asraa commented Aug 9, 2022

I think we should fix the example, the root should never be included.

We include the root in the manifest right now, but don't include it for verification.

I can push a cosign fix.

@znewman01
Copy link
Contributor

znewman01 commented Aug 9, 2022 via email

@haydentherapper
Copy link
Contributor

I actually think it's fine to have the root in the annotation and would be wrong to not include it. It's a reflection of what the CA returned at that point in time. While we aren't currently doing this, we could also check that the root in the annotation matches a root in TUF, and fail otherwise. You could imagine where two different roots cross-signed an intermediate. Maybe someone wants to write their own verification logic to enforce an exact root is used.

@asraa
Copy link
Contributor Author

asraa commented Aug 9, 2022

You could imagine where two different roots cross-signed an intermediate. Maybe someone wants to write their own verification logic to enforce an exact root is used.

In this case the verification logic dictates which out of band root to use.

While we aren't currently doing this, we could also check that the root in the annotation matches a root in TUF, and fail otherwise.

Yes, but exposing the root in the signature manifest exposes footguns, and I think it's more severe to expose this option.

I could see an argument that PKCS7 optionally includes the certificate chain similar to this, although tools will NOT verify correctly unless the root is provided out of band. Either way, at minimum, I think wording needs to be avoided to use the embedded certificate chain for verification.

@haydentherapper
Copy link
Contributor

TLS does the same, returning a complete chain. I don't see this being a significant footgun, and FWIW I think there's many other ways clients can make mistakes - For example, maybe they just don't verify the chain at all, maybe they mess up the time verification, maybe they misunderstand X.509 and pull the root certificate from the CA Issuers extensions. We need to clearly articulate what is required for verification. Agreed we need better wording in the spec (the OCI annotation spec should be a part of the architecture docs for the client).

@asraa
Copy link
Contributor Author

asraa commented Aug 9, 2022

SGTM. I'll update the spec doc, clearer docs will help articulate the need for when you need to rely on out of band trust.

@znewman01
Copy link
Contributor

+1 to following TLS here, but also being very explicit in the client specification.

@asraa
Copy link
Contributor Author

asraa commented Aug 11, 2022

Cool, I found some details here: https://www.rfc-editor.org/rfc/rfc8446#section-4.4.2

Because
certificate validation requires that trust anchors be distributed
independently, a certificate that specifies a trust anchor MAY be
omitted from the chain, provided that supported peers are known to
possess any omitted certificates.

Certificates that are self-signed or
certificates that are expected to be trust anchors are not validated
as part of the chain and therefore MAY be signed with any algorithm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants