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

x509-cert: add Signed Certificate Timestamp (SCT) extension support #1134

Merged
merged 64 commits into from
Dec 19, 2023

Conversation

imor
Copy link
Contributor

@imor imor commented Jul 2, 2023

This PR adds support for Signed Certificate Timestamp extension to the x509-cert crate. Since the structures in SCT extension are TLS encoded we need to add a dependency on the tls_codec crate. This dependency has been made an optional dependency which is only enabled when the newly added sct feature is enabled.

imor added 2 commits July 2, 2023 13:42
The DeserializeBytes impl of TlsByteVecUX did not skip the length prefix
bytes.
@imor imor force-pushed the sct branch 2 times, most recently from e15c95e to 9f8b106 Compare July 2, 2023 15:44
@tnytown
Copy link
Contributor

tnytown commented Oct 20, 2023

@imor What's the status of the PR? We're using x509-cert in sigstore-rs and we'd like to parse Signed Certificate Timestamps.

By the way, thanks for this!

@imor
Copy link
Contributor Author

imor commented Oct 21, 2023

@tnytown the only thing pending is this TODO. Basically this PR is waiting for a new release of tls_codec which includes changes merged in #1132 and #1133. Once that is done, the path based dependency on tls_codec can be replaced with the new published version and then this PR will be ready to review.

@franziskuskiefer, @tarcieri when is the next planned release for tls_codec?

@tarcieri
Copy link
Member

I can potentially cut a release of tls_codec if need be but I'd prefer for @franziskuskiefer to do it

@franziskuskiefer
Copy link
Contributor

I plan to do one next week or so with couple changes. But if there's the need for an earlier one, I can do one before.

@tnytown
Copy link
Contributor

tnytown commented Nov 16, 2023

@franziskuskiefer gentle ping on the tls_codec release :)

@franziskuskiefer
Copy link
Contributor

@franziskuskiefer gentle ping on the tls_codec release :)

I prepared the release. There are a couple small things that need to get fixed but I think I can do the release today or tomorrow latest.

Copy link
Contributor

@franziskuskiefer franziskuskiefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I release tls_codec 0.4.0. There's one breaking change there. I commented on that here. With the rename here, this PR works as is.

x509-cert/src/ext/pkix/sct.rs Outdated Show resolved Hide resolved
@imor imor marked this pull request as ready for review November 22, 2023 11:53
Comment on lines +220 to +223
/// ED25519 signature algorithm.
Ed25519 = 7,
/// ED448 signature algorithm.
Ed448 = 8,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity: what RFC/doc do these enum members appear in? They aren't in the TLS 1.2 or 1.3 RFCs.

(The closest thing in 1.3 is SignatureScheme, which has 0x807 and 0x808 for Ed25519 and Ed448 respectively.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is mostly that: SignatureAndHashAlgorithm is SignatureScheme in TLS1.3. The first byte encodes for the hash alg, the second byte encodes for signature alg?

Copy link
Contributor

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a maintainer, but I did a review based on what downstreams like sigstore-rs need and this LGTM!

#[derive(Debug, PartialEq)]
pub struct SignedCertificateTimestampList(OctetString);

//TODO: Remove this and use const-oid's rfc6962::CT_PRECERT_SCTS once a const-oid version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO(baloo): make a backport of #1094 and publish a release on 0.9

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will leave this in for now but we can address it as a followup

x509-cert/Cargo.toml Outdated Show resolved Hide resolved
@tarcieri tarcieri changed the title Implement support for Signed Certificate Timestamp extension x509-cert: add Signed Certificate Timestamp (SCT) extension support Dec 19, 2023
@tarcieri tarcieri merged commit b579ad6 into RustCrypto:master Dec 19, 2023
172 checks passed
@imor imor deleted the sct branch December 20, 2023 08:50
@tarcieri tarcieri mentioned this pull request Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants