Add Result message to communicate the result of a verification #520
Conversation
There was a problem hiding this comment.
This looks really good!
I think we should duplicate what you've created for (Unverified)TimestampResult for Fulcio and Rekor as well.
I don't think we need to include the cert chain for Fulcio or public key for Rekor, rather we should include some unique ID to correlate back to the trust root. For Fulcio, that could be the URI. For Rekor, that could be the log ID (URI will be unique in the future with Rekor v2).
For Rekor, we should specify what log proofs were unverifiable.
For Fulcio, we could also call this "CertificateAuthorityResult" to handle private PKI verification. We can include the list of cert subject key IDs.
| @@ -162,3 +162,164 @@ message Input { | |||
| // provided. | |||
| optional Artifact artifact = 4; | |||
| } | |||
|
|
|||
| // CertificateSummary contains verified metadata from the certificate | |||
| message CertificateSummary { | |||
There was a problem hiding this comment.
Do you think we should get rid of CertificateIdentity? I prefer this new message as clients shouldn't need to know OID values.
There was a problem hiding this comment.
They serve different different purposes, the CertificateIdentity is for verifying, as it allows regexps to be set for the SAN.
And it also allows for setting the extensions needed during verification. I agree that clients knowing with OIDs to use os not optimal, but clients can implement helper function here to configure the verification.
| message SignatureResult { | ||
| // If the bundle contains a public key, the public_key_id field will | ||
| // contain the public key ID | ||
| string public_key_id = 1; |
There was a problem hiding this comment.
The bundle should contain a public_key_id if a key is to be used to verify it, so is this repetitive? Though maybe this is useful when a keyset is provided and the bundle doesn't contain the ID (since it's an optional hint)?
There was a problem hiding this comment.
minor but this is called hint in the PublicKeyIdentifier message:
protobuf-specs/protos/sigstore_common.proto
Line 169 in e943ce1
There was a problem hiding this comment.
Should these be a oneof:
message SignatureResult {
oneof {
string public_key_hint = 1;
CertificateSummary certificate = 2;
}
}|
|
||
| message TimestampResult { | ||
| // The type of timestamp verification; may be `Tlog`, `TimestampAuthority`, or `CurrentTime` | ||
| string type = 1; |
There was a problem hiding this comment.
nit: should make it a enum
|
|
||
| // The OIDC issuer. Should match `iss` claim of ID token or, in the case of | ||
| // a federated login like Dex it should match the issuer URL of the | ||
| // upstream issuer. The issuer is not set the extensions are invalid and |
There was a problem hiding this comment.
| // upstream issuer. The issuer is not set the extensions are invalid and | |
| // upstream issuer. The issuer is not set if the extensions are invalid and |
?
| // upstream issuer. The issuer is not set the extensions are invalid and | ||
| // will fail to render. | ||
| // OID 1.3.6.1.4.1.57264.1.8 and 1.3.6.1.4.1.57264.1.1 (Deprecated) | ||
| string issuer = 3; |
There was a problem hiding this comment.
should we maybe align the field numbers with the last component of the OID?
| // MUST be application/vnd.dev.sigstore.verificationresult+json;version=0.1 | ||
| string media_type = 1; | ||
|
|
||
| // MUST be either "success" or "failure" |
| // MUST be either "success" or "failure" | ||
| string status = 2; | ||
|
|
||
| // SHOULD contain an error message if status == "failure" |
There was a problem hiding this comment.
also, MUST be empty if status is success?
|
|
||
| // If the bundle contains a DSSE, this result shonuld contain the | ||
| // deserialized and verified statement | ||
| string statement = 3; |
There was a problem hiding this comment.
I think the comments should be rephrased a bit.
// If the bundle contains DSSE wrapped in-toto statement , this result contains the
// deserialized in-toto statement
As an in-toto statement is a JSON encoded object, i.e. a string.
| // OID 1.3.6.1.4.1.57264.1.8 and 1.3.6.1.4.1.57264.1.1 (Deprecated) | ||
| string issuer = 3; | ||
|
|
||
| // Deprecated |
There was a problem hiding this comment.
The deprecated OIDs, can we ignore them here? They were marked as deprecated two years go (almost to the date) sigstore/fulcio#945.
If a specific client want's to support them, that's fine. But from a standard perspective, I think we should only focus on the current extensions.
Summary
This PR is an attempt at formalizing the output of a verifier using a new message:
dev.sigstore.verification.v1.Result.This initial form was extracted from sigstore-go, from the type
VerificationResult, with the following changes:VerifiedIdentityas it seems to echo back the verification inputs and the matching values can all be found inCertificateSummary.statusanderrorfields which may be used to communicate a verification failure.UnverifiedDatawhich may be used to communicate if there were unverified TSA/Tlog signatures, as described in Include skipped signatures in VerificationResult sigstore-go#48Things to consider:
I haven't generated docs/code yet to keep the review simple.
Fixes #66
Release Note
Documentation