-
Notifications
You must be signed in to change notification settings - Fork 41
Add Result message to communicate the result of a verification #520
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -162,3 +162,164 @@ message Input { | |||||
// provided. | ||||||
optional Artifact artifact = 4; | ||||||
} | ||||||
|
||||||
// CertificateSummary contains verified metadata from the certificate | ||||||
message CertificateSummary { | ||||||
// The issuer of the certificate | ||||||
string certificate_issuer = 1; | ||||||
|
||||||
// The Subject Alternative Name, or SAN, from the certificate | ||||||
string subject_alternative_name = 2; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might not be an issue in practice, but flagging: X.509 technically has multiple SANs, and I don't think any of the Sigstore specifications/documentation explicitly say that Sigstore leaf certificates only have a single SAN. In practice this is true, but we might want to flag some documentation improvements as a follow-up to this to emphasize that, as a matter of X.509 profile, Sigstore-compatible CAs only issue EE certs with singular SANs 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should add the type for the SAN too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The certificate spec already mentions this: https://github.com/sigstore/fulcio/blob/main/docs/certificate-specification.md#issued-certificate
|
||||||
|
||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? |
||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we maybe align the field numbers with the last component of the OID? |
||||||
|
||||||
// Deprecated | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
// Triggering event of the Github Workflow. Matches the `event_name` claim of ID | ||||||
// tokens from Github Actions | ||||||
// OID 1.3.6.1.4.1.57264.1.2 | ||||||
string githubWorkflowTrigger = 4; | ||||||
|
||||||
// Deprecated | ||||||
// SHA of git commit being built in Github Actions. Matches the `sha` claim of ID | ||||||
// tokens from Github Actions | ||||||
// OID 1.3.6.1.4.1.57264.1.3 | ||||||
string githubWorkflowSHA = 5; | ||||||
|
||||||
// Deprecated | ||||||
// Name of Github Actions Workflow. Matches the `workflow` claim of the ID | ||||||
// tokens from Github Actions | ||||||
// OID 1.3.6.1.4.1.57264.1.4 | ||||||
string githubWorkflowName = 6; | ||||||
|
||||||
// Deprecated | ||||||
// Repository of the Github Actions Workflow. Matches the `repository` claim of the ID | ||||||
// tokens from Github Actions | ||||||
// OID 1.3.6.1.4.1.57264.1.5 | ||||||
string githubWorkflowRepository = 7; | ||||||
|
||||||
// Deprecated | ||||||
// Git Ref of the Github Actions Workflow. Matches the `ref` claim of the ID tokens | ||||||
// from Github Actions | ||||||
// OID 1.3.6.1.4.1.57264.1.6 | ||||||
string githubWorkflowRef = 8; | ||||||
|
||||||
// Reference to specific build instructions that are responsible for signing. | ||||||
// OID 1.3.6.1.4.1.57264.1.9 | ||||||
string buildSignerURI = 9; | ||||||
|
||||||
// Immutable reference to the specific version of the build instructions that is responsible for signing. | ||||||
// OID 1.3.6.1.4.1.57264.1.10 | ||||||
string buildSignerDigest = 10; | ||||||
|
||||||
// Specifies whether the build took place in platform-hosted cloud infrastructure or customer/self-hosted infrastructure. | ||||||
// OID 1.3.6.1.4.1.57264.1.11 | ||||||
string runnerEnvironment = 11; | ||||||
|
||||||
// Source repository URL that the build was based on. | ||||||
// OID 1.3.6.1.4.1.57264.1.12 | ||||||
string sourceRepositoryURI = 12; | ||||||
|
||||||
// Immutable reference to a specific version of the source code that the build was based upon. | ||||||
// OID 1.3.6.1.4.1.57264.1.13 | ||||||
string sourceRepositoryDigest = 13; | ||||||
|
||||||
// Source Repository Ref that the build run was based upon. | ||||||
// OID 1.3.6.1.4.1.57264.1.14 | ||||||
string sourceRepositoryRef = 14; | ||||||
|
||||||
// Immutable identifier for the source repository the workflow was based upon. | ||||||
// OID 1.3.6.1.4.1.57264.1.15 | ||||||
string sourceRepositoryIdentifier = 15; | ||||||
|
||||||
// Source repository owner URL of the owner of the source repository that the build was based on. | ||||||
// OID 1.3.6.1.4.1.57264.1.16 | ||||||
string sourceRepositoryOwnerURI = 16; | ||||||
|
||||||
// Immutable identifier for the owner of the source repository that the workflow was based upon. | ||||||
// OID 1.3.6.1.4.1.57264.1.17 | ||||||
string sourceRepositoryOwnerIdentifier = 17; | ||||||
|
||||||
// Build Config URL to the top-level/initiating build instructions. | ||||||
// OID 1.3.6.1.4.1.57264.1.18 | ||||||
string buildConfigURI = 18; | ||||||
|
||||||
// Immutable reference to the specific version of the top-level/initiating build instructions. | ||||||
// OID 1.3.6.1.4.1.57264.1.19 | ||||||
string buildConfigDigest = 19; | ||||||
|
||||||
// Event or action that initiated the build. | ||||||
// OID 1.3.6.1.4.1.57264.1.20 | ||||||
string buildTrigger = 20; | ||||||
|
||||||
// Run Invocation URL to uniquely identify the build execution. | ||||||
// OID 1.3.6.1.4.1.57264.1.21 | ||||||
string runInvocationURI = 21; | ||||||
|
||||||
// Source repository visibility at the time of signing the certificate. | ||||||
// OID 1.3.6.1.4.1.57264.1.22 | ||||||
string sourceRepositoryVisibilityAtSigning = 22; | ||||||
} | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor but this is called hint in the protobuf-specs/protos/sigstore_common.proto Line 169 in e943ce1
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these be a oneof: message SignatureResult {
oneof {
string public_key_hint = 1;
CertificateSummary certificate = 2;
}
} |
||||||
|
||||||
// If the bundle contains a certificate, the certificate field will | ||||||
// contain metadata about the certificate | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: should make it a enum |
||||||
|
||||||
// The URI of the timestamp provider; may contain the base URL of the | ||||||
// TSA or Tlog that was used to establish a timestamp | ||||||
string uri = 2; | ||||||
|
||||||
// The actual timestamp time that was verified | ||||||
google.protobuf.Timestamp timestamp = 3; | ||||||
} | ||||||
|
||||||
message UnverifiedTimestampResult { | ||||||
// Data about the timestamp that was unverified | ||||||
TimestampResult result = 1; | ||||||
|
||||||
// Reason or error text describing why this timestamp was not verified | ||||||
string reason = 2; | ||||||
} | ||||||
|
||||||
message UnverifiedData { | ||||||
repeated UnverifiedTimestampResult timestamps = 1; | ||||||
} | ||||||
|
||||||
// Result captures the result of the verification | ||||||
message Result { | ||||||
// MUST be application/vnd.dev.sigstore.verificationresult+json;version=0.1 | ||||||
string media_type = 1; | ||||||
|
||||||
// MUST be either "success" or "failure" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be an enum? |
||||||
string status = 2; | ||||||
|
||||||
// SHOULD contain an error message if status == "failure" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, MUST be empty if status is success? |
||||||
string error = 3; | ||||||
|
||||||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bytes instead of string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the comments should be rephrased a bit.
As an in-toto statement is a JSON encoded object, i.e. a string. |
||||||
|
||||||
// Data about the public key or certificate associated with the verified signature | ||||||
SignatureResult signature = 4; | ||||||
|
||||||
// Data about verified timestamps | ||||||
repeated TimestampResult verified_timestamps = 5; | ||||||
|
||||||
// Any data which was unable to be verified. | ||||||
UnverifiedData unverified_data = 6; | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.