Open bdehamer opened 1 year ago
Another thing which occurs to me is that it might be helpful for the VerificationResult to surface the Fulcio certificate extensions (in the case where the verification is successful).
+1 @vaikas, any other things useful or interesting?
My own thoughts about the error struct:
But +1 on defining step related verification results - I image the message contains info like "the ith tlog entry was unable to verify"
We can always extend for the surfaced info when there is a success.
Maybe we do something like this:
message VerificationResult {
oneof result {
Success success = 1;
MalformedContent malformed_content = 2;
InternalError internal_error = 3;
VerificationError verification_error = 4;
}
}
message Success {
// leave for extending with more info
}
message VerificationError {
optional VerificationStep failed_step = 1;
optional string message = 2;
}
message InternalError {
optional string message = 1;
}
message ValidationError {
// Didn't want to use the same "ErrorWithMessage" type in case there's extensions.
optional string message = 1;
}
Another thing which occurs to me is that it might be helpful for the VerificationResult to surface the Fulcio certificate extensions (in the case where the verification is successful).
One concern for this is that if the verification result with parsed extensions gets persisted and later used for verification, it's susceptible to compromise/mutation. Can we be very clear that the verification result is meant to only be used at the time of verification?
+1 on having error classes.
One small question is do we want to distinguish between whether a key was used vs a certificate? Or is it sufficient to derive that from what was provided for verification?
One concern for this is that if the verification result with parsed extensions gets persisted and later used for verification, it's susceptible to compromise/mutation.
I would find it surprising that people would persist this and use it in an untrusted process. But yeah, I agree, it shouldn't be passed to other processes.
One small question is do we want to distinguish between whether a key was used vs a certificate? Or is it sufficient to derive that from what was provided for verification?
Another way to put it is that some VerificationSteps may only be relevant for some inputs (like a key being used will never trigger a CERTIFICATE_CHAIN
error).
If we add verification output, then I image that the certificate OID extensions would be in an optional field that is only present when the input was a cert.
Big +1 on the error classes -- much more expressive than my initial proposal.
If we add verification output, then I image that the certificate OID extensions would be in an optional field that is only present when the input was a cert.
Maybe the OID extensions become an optional field in Success
message (would we ever want to return these if the verification wasn't 100% successful)? Presumably, the caller will know whether a key or certificate is being used since they'll have to provide it -- so it should be obvious whether or not to look for the OID extensions in the verification output.
(would we ever want to return these if the verification wasn't 100% successful)?
No :D because no one should use it. But actually I'm just kidding:
I believe ArtifactVerificationProperties did have properties on identity / extension stuff? If so, maybe you fail on policy but have a cryptographically valid sig. In which case the error class that describes cert invalidity can have an error that allows describing expected/retrieved extensions. I'd say that's somewhat different than a sig verification error, and more of a policy error?
I believe ArtifactVerificationProperties did have properties on identity / extension stuff? If so, maybe you fail on policy but have a cryptographically valid sig
Correct, and that is an important info to be returned back to the client too.
FWIW, we currently do a similar thing in sigstore-python
:
So I'm a big fan of more formally enumerating the error states here 🙂
Inspiration from the sigstore-go client: https://github.com/sigstore/sigstore-go/blob/main/pkg/verify/signed_entity.go#L156
We already have message types defined that describe the inputs to the verification process (
Bundle
,TrustedRoot
,ArtifactVerificationOptions
) so it seems reasonable to also define a standardized verification response. This will give clients a standard way to convey more specific verification errors (and make it easier to compare the verification results across the different client implementations).I'm not married to this particular design, but offering it as a starting point for refinement:
If we go with this idea of having some sort of enum to identity the specific verification step which failed we could go even more granular (e.g.
CERTIFICATE_CHAIN_SCT
,TRANSPARENCY_LOG_INCLUSION_PROOF
, etc) . . . not clear to me what the most useful level of detail is gonna be.Another thing which occurs to me is that it might be helpful for the
VerificationResult
to surface the Fulcio certificate extensions (in the case where the verification is successful). The extensions are probably the next things any client is gonna read after verifying the bundle and this could save them the trouble of fishing them out of the certificate.CC @kommendorkapten @asraa