added public key check for SCTs #428
Conversation
pkg/verify/signed_entity.go
Outdated
| @@ -515,6 +515,18 @@ func (v *SignedEntityVerifier) Verify(entity SignedEntity, pb PolicyBuilder) (*V | |||
| return nil, fmt.Errorf("failed to build policy: %w", err) | |||
| } | |||
|
|
|||
| // Early check: If SCTs are required, ensure the bundle is certificate-signed not public key-signed | |||
| if v.config.requireSCTs { | |||
There was a problem hiding this comment.
Could we do this in the construction of SignedEntityVerifier?
There was a problem hiding this comment.
I don't think we want this check in SignedEntityVerifier - see #428 (review)
There was a problem hiding this comment.
I agree that this implementation of binding SignedEntity and SignedEntityVerifier together isn't what we want to do. I was thinking we could implement this check by checking for a public key in the trust root material, specifically if there is a TrustedPublicKeyMaterial verifier. Though I recognize now this might not be straightforward - if there are multiple verifiers for both keys and certificates, it's only at verification time for the SignedEntity we will know if it is incompatible with the verifier policy.
so tldr - let's move it to Verify() like you suggested
Horiodino
force-pushed
the
Verifier_does_not_enforce_WithSignedCertificateTimestamps
branch
from
1ba2edb to
361cc17
Compare
There was a problem hiding this comment.
We want to have the concept of a SignedEntityVerifier be separate from a SignedEntity.
Imagine you're running sigstore-go on a server. You have your policy for what you consider valid or not, which is what you use to construct your SignedEntityVerifier. But at that time, you don't yet have a bundle you're verifying. Then you (re)use that verifier many, many times to verify various user-submitted SignedEntitys. This isn't hypothetical - this is exactly how npm provenance works on the server side.
That said, it does seem like requireSCTs is ignored if your bundle refers to a public key instead of having a signing certificate. Instead of this approach, I think we could have an if statement in Verify that checks if you have a public key but said you require SCTs?
Signed-off-by: Horiodino <[email protected]>
Horiodino
force-pushed
the
Verifier_does_not_enforce_WithSignedCertificateTimestamps
branch
from
1dcd863 to
56c128d
Compare
Signed-off-by: Horiodino <[email protected]>
Summary
return err when the bundle is signed with a public key with Scts
fixes: #403
Release Note
Documentation