Add checkpoint key ID to trust root #284
Conversation
protos/sigstore_common.proto
Outdated
| // The checkpoint key ID, following the specification described here | ||
| // for ECDSA and Ed25519 signatures: | ||
| // https://github.com/C2SP/C2SP/blob/main/signed-note.md#signatures | ||
| // For RSA signatures, the key ID will match the ECDSA format of the hashed | ||
| // DER-encoded SPKI public key. Publicly witnessed logs MUST NOT use | ||
| // RSA-signed checkpoints, since witnesses do not support RSA signatures. | ||
| // This is provided for convenience. Clients can also calculate the checkpoint | ||
| // key ID given the log's public key. | ||
| message CheckpointKeyId { | ||
| // The key ID in a checkpoint, as a prefix to the signature. SHOULD be | ||
| // 4 bytes long, as a truncated hash. | ||
| bytes key_id = 1 [(google.api.field_behavior) = REQUIRED]; | ||
| } |
There was a problem hiding this comment.
This is a minor thing, but this will mark at least the third overlapping type for this in the protobuf-specs 😅
common.LogIdcommon.PublicKeyIdentifier- (new)
common.CheckpointKeyId
I have no objection to a new type in principle, but would either of the pre-existing ones work? PublicKeyIdentifier in particular might be ideal, since it isn't prescriptive in name like LogId and has an arbitrary string hint.
There was a problem hiding this comment.
PublicKeyIdentifier seems workable. What's your preference on string vs bytes? I assume we'd need to base64 or hex encode the content as a string. That was one motivation for bytes, so we didn't have to deal with encoding.
There was a problem hiding this comment.
My basic preference is for bytes (a la LogId), but it looks like PublicKeyIdentifier unfortunately already uses string 😅
Given that, my weak pref is for a hex-encoded byte string within that hint, since I believe that's what other hints/users of PublicKeyIdentifier are using. But I don't feel very strongly about that, and I could be convinced that the need to even make a decision here justifies adding a separate CheckpointKeyId type anyways 🙂
There was a problem hiding this comment.
I'd prefer bytes too. We could reuse LogId, but we'd need to move the comment on key_id up to the TransparencyLogInstance message. The motivation for a separate type would be to keep the spec comments alongside the message, but I think it can still be clean if moved out to a higher level. WDYT?
There was a problem hiding this comment.
We could reuse LogId, but we'd need to move the comment on key_id up to the TransparencyLogInstance message. The motivation for a separate type would be to keep the spec comments alongside the message, but I think it can still be clean if moved out to a higher level. WDYT?
That seems reasonable to me!
There was a problem hiding this comment.
I actually like this structure, because we can now use LogId to house both key_id and a future generic_log_id or something like that if we move away from logs represented by their keys.
haydentherapper
force-pushed
the
checkpoint-key-id
branch
from
9a895c0
to
ef98834
Compare
This adds a string to represent the checkpoint key ID for a log, which will differ for ed25519 logs. To simplify client implementation, we will provide this string so that clients don't have to compute the checkpoint key ID themselves using the public key. If it's not set, then a client should assume the log ID is equal to the checkpoint key ID, which is true for ecdsa and rsa logs. Ref: sigstore/rekor#2062 Signed-off-by: Hayden Blauzvern <[email protected]>
haydentherapper
force-pushed
the
checkpoint-key-id
branch
from
ef98834
to
43de29d
Compare
Signed-off-by: Hayden Blauzvern <[email protected]>
This adds a string to represent the checkpoint key ID for a log, which will differ for ed25519 logs. To simplify client implementation, we will provide this string so that clients don't have to compute the checkpoint key ID themselves using the public key. If it's not set, then a client should assume the log ID is equal to the checkpoint key ID, which is true for ecdsa and rsa logs.
Also cleaned up the text for the checkpoint.
Ref: sigstore/rekor#2062
Summary
Release Note
Documentation