Skip to content
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

Clarifying/reducing agility in a few more messages #38

Closed
woodruffw opened this issue Dec 13, 2022 · 5 comments · Fixed by #39
Closed

Clarifying/reducing agility in a few more messages #38

woodruffw opened this issue Dec 13, 2022 · 5 comments · Fixed by #39
Labels
enhancement New feature or request

Comments

@woodruffw
Copy link
Member

#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.

@woodruffw woodruffw added the enhancement New feature or request label Dec 13, 2022
@haydentherapper
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member Author

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

@kommendorkapten
Copy link
Member

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
Copy link
Member Author

Will remove now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants