policy.json BYOPKI signature verification API #2579
Conversation
QiWang19
changed the title
|
Hi @mtrmac , I’ve created this draft PR for the policy.json API change, could you take a look and provide your initial thoughts on the structure of the API update? |
There was a problem hiding this comment.
Thanks! Looks good overall.
Earlier discussion openshift/enhancements#1658 .
Doing both the API and implementation in one PR works for me, having the API available but broken is not really helping anything.
QiWang19
force-pushed
the
byo-pki-verify
branch
2 times, most recently
from
b3a0606 to
f8585cd
Compare
QiWang19
changed the title
|
@mtrmac Could you please take a look? It's ready for review. I am unsure about how I’m handling the intermediate certificates. |
There was a problem hiding this comment.
Just a few-minute first pass — this looks very good. I didn’t read this carefully, and I skipped the tests completely, for now.
Combining the policy-provided and signature-provided intermediate certificates makes sense to me, but I’ll at least check what Cosign does.
signature/pki_cert.go
Outdated
| } | ||
|
|
||
| // isIntermediateCA checks if the certificate is an intermediate CA | ||
| func isIntermediateCA(cert *x509.Certificate) bool { |
There was a problem hiding this comment.
It’s not immediately clear to me that this is necessary, I’d expect the crypto/x509 code to validate the intermediates are usable. Is there a specific reason to filter them this way? If so, it would be nice to document that.
There was a problem hiding this comment.
I added this because I’m not sure if multiple root certificates will be included in the untrustedIntermediateChainBytes from dev.sigstore.cosign/chain. So, I’ve been filtering out intermediates from the data. Do you think this is necessary?
There was a problem hiding this comment.
By the principle of the thing, and reading crypto/x509.Certificate.buildChains, an intermediate entry is only trusted if it is signed by some other CA. So including any extra certificates should not hurt — after all, an attacker can include arbitrary intermediate certificates in the signature.
(Also — probably not for sigstore, but in general — there are transition scenarios, where a new CA might get its to-be-root certificate signed by some other CA; this to-be-root certificate can then be accepted as an intermediate. That’s a valid situation (again, in general, probably not now for sigstore, and not for the BYOPKI users).
The certificate/issuer link is nowadays primarily happening via SubjectKeyId/AuthorityKeyId fields, so it seems to me it is possible to for an intermediate to have cert.Issuer == cert.Subject.)
|
Cc: @Honny1 as well — c/image/signature is a somewhat separate part of the codebase, and as security-critical with a lot of paranoia. Worth understanding the structure and idioms. |
QiWang19
force-pushed
the
byo-pki-verify
branch
from
f8585cd to
f70f17a
Compare
|
@mtrmac Do you think the PKI code needs a file like fulcio_cert_stub.go and should include build tags? I'm not fully clear on the purpose of this compile configuration. |
|
The Here, the only similar dependency seems to be |
I checked some cosign code, maybe I got lost in the code path, I did see it process the signature-provided intermediate certificates 🤔
|
|
@mtrmac could you review? |
signature/pki_cert.go
Outdated
|
|
||
| untrustedIntermediatePool := x509.NewCertPool() | ||
| if pkiTrustRoot.caIntermediatesCertificates != nil { | ||
| untrustedIntermediatePool = pkiTrustRoot.caIntermediatesCertificates |
signature/pki_cert.go
Outdated
| } | ||
| if len(untrustedIntermediateChain) > 1 { | ||
| for _, untrustedIntermediateCert := range untrustedIntermediateChain { | ||
| untrustedIntermediatePool.AddCert(untrustedIntermediateCert) |
There was a problem hiding this comment.
(A) … and this, together, means, that pkiTrustRoot.caIntermediatesCertificates will grow over time as validations happen.
Right now that doesn’t really matter, but see the pending // FIXME: move this to per-context initialization — the idea is that “the trust root” is computed once and ~immutable, maintained over the lifetime of a PolicyContext for possibly many signature validations.
There was a problem hiding this comment.
I also added the same comment before these lines of code.
There was a problem hiding this comment.
It’s not just the comment, it’s that the “trust root” is supposed to represent a trust root, and now it accumulates unrelated untrusted data. That’s confusing and it shouldn’t be happening.
(I didn’t check whether it is possible to easily clone a certificate pool. If it weren’t possible, I’d prefer going back to the situation where the trust root contained unparsed file contents to the current state.)
There was a problem hiding this comment.
There is a clone function https://pkg.go.dev/crypto/x509#CertPool.Clone
I updated to clone the trusted certs pool to a new pool and append untrusted certs to it. Could you take a look?
QiWang19
force-pushed
the
byo-pki-verify
branch
from
f70f17a to
897b425
Compare
Signed-off-by: Qi Wang <qiwan@redhat.com>
QiWang19
force-pushed
the
byo-pki-verify
branch
from
897b425 to
f020a87
Compare
|
@containers/image-maintainers can we get this merged? |
OCPNODE-2338