sigstore / protobuf-specs

Protocol Buffer specifications
Apache License 2.0
23 stars 29 forks source link

Clarifying/reducing agility in a few more messages #38

Closed woodruffw closed 1 year ago

woodruffw commented 1 year ago

30 is getting a little long, so I'm filing a separate issue to track some other potential changes to the current messages 🙂


LogId

Here's how LogId is currently defined:

// LogId captures the identity of a transparency log.
message LogId {
        oneof id {
                // The unique id of the log, represented as the SHA-256 hash
                // of the log's public key, computed over the DER encoding.
                // https://www.rfc-editor.org/rfc/rfc6962#section-3.2
                bytes key_id = 1;
                // Currently not used but proposed by
                // https://datatracker.ietf.org/doc/rfc9162/
                ObjectIdentifier oid = 2;
        }
}

IMO, we probably don't want this agility: the message formats will probably have to change significantly anyways if and when Sigstore moves to CT 2.0, so we should probably limit LogId to just the SHA256(DER(pk)) format that CT 1.0 allows.

HashAlgorithm and SignatureAlgorithm

Here's how these are currently defined:

enum HashAlgorithm {
        HASH_ALGORITHM_UNSPECIFIED = 0;
        SHA2_256 = 1;
        SHA2_512 = 2;
}

// Subset of known signature algorithms.
enum SignatureAlgorithm {
        SIGNATURE_ALGORITHM_UNSPECIFIED = 0;
        ECDSA_P256_SHA_256 = 1; // See NIST FIPS 186-4
        ECDSA_P256_HMAC_SHA_256 = 2; // See RFC6979
        ED25519 = 3; // See RFC8032
        RSA_PKCS1V5 = 4; // See RFC8017
        RSA_PSS = 5; // See RFC8017
}

I might be wrong about this, but I believe we don't need SHA2-512 or RSA PSS: neither of these is mentioned in the CT RFCs. But it's possible these show up in certificates anyways; cc @haydentherapper for thoughts on these.

haydentherapper commented 1 year ago

re: LogId, I'm fine with removing OID because I don't think we'll move to CT 2.0 any time soon.

re: HashAlgorithm, one issue is we've hardcoded support for SHA256 all over the place. I think it'd be reasonable to keep the enum for HashAlgorithm in case we want to add additional algorithms in the future, but removing 512 now would be fine I think.

Is SignatureAlgorithm used anywhere currently? It doesn't seem like it, I think we can just delete the message.

woodruffw commented 1 year ago

Is SignatureAlgorithm used anywhere currently? It doesn't seem like it, I think we can just delete the message.

Not that I can see either 🙂 -- maybe there's somewhere that should have it that's currently missing, although the only place I can think of it being relevant is in the certificates themselves (and those are opaque DER blobs to the protobuf layer).

woodruffw commented 1 year ago

Opened #39 for this -- I left SignatureAlgorithm in because I wasn't sure, but I addressed the other two.

kommendorkapten commented 1 year ago

The SignatureAlgorithm is not used, so it can be removed. There was first a tuple <signature algorithm, key encoding> but it was flattened to a list of permutations to avoid scenarios where invalid combinations was used. It just seems that the SignatureAlgorithm was not removed.

woodruffw commented 1 year ago

Will remove now!