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

Signed Certificate Timestamp verification #326

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

tnytown
Copy link
Contributor

@tnytown tnytown commented Jan 22, 2024

Blocked on #311.

Summary

Adds Signed Certificate Timestamp verification and hooks it up to the bundle signing flow. SCT verification ensures that the signing certificate in a given operation has been submitted to the Certificate Transparency log, which aids in the detection of malicious certificates and keeps Certificate Authorities like Fulcio honest.

Release Note

  • Implemented Signed Certificate Timestamp validation for certificates used in Sigstore Bundle operations.

Documentation

No user-facing documentation needed, we automatically perform SCT validation when public sign and verify APIs are used.

Copy link
Contributor Author

@tnytown tnytown left a comment

Choose a reason for hiding this comment

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

I'll work on adding SCT to the verification flow once we get #311 merged. I think this is ready for a first pass :)

Cargo.toml Outdated Show resolved Hide resolved
src/crypto/keyring.rs Show resolved Hide resolved
Comment on lines +33 to +40
// TODO(tnytown): Migrate to const-oid's CT_PRECERT_SCTS when a new release is cut.
const CT_PRECERT_SCTS: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.3.6.1.4.1.11129.2.4.2");
const PRECERTIFICATE_SIGNING_CERTIFICATE: ObjectIdentifier =
ObjectIdentifier::new_unwrap("1.3.6.1.4.1.11129.2.4.4");
Copy link
Contributor Author

@tnytown tnytown Jan 22, 2024

Choose a reason for hiding this comment

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

const-oid 0.10 should include these OIDs, but isn't released yet

Cargo.toml Outdated Show resolved Hide resolved
};

match (algo, params) {
// TODO(tnytown): should we also accept ed25519, p384, ... ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only saw p256 SCTs while testing this changeset against the public-good prod and staging, do we need to support other key types?

@tnytown tnytown force-pushed the ap/wip-sct branch 3 times, most recently from 0501ffc to be5f3fc Compare February 15, 2024 21:44
@tnytown tnytown mentioned this pull request Feb 16, 2024
@tnytown tnytown force-pushed the ap/wip-sct branch 2 times, most recently from 773490d to 2c8a320 Compare February 28, 2024 00:19
@tnytown tnytown marked this pull request as ready for review February 28, 2024 00:21
@tnytown
Copy link
Contributor Author

tnytown commented Feb 28, 2024

This is ready for review, but we should get #311 in first.

@tnytown tnytown marked this pull request as draft February 28, 2024 03:01
@tnytown tnytown marked this pull request as ready for review February 28, 2024 21:15
@tnytown tnytown force-pushed the ap/wip-sct branch 3 times, most recently from 739b4b4 to 4180b1f Compare April 10, 2024 23:41
@tnytown
Copy link
Contributor Author

tnytown commented Apr 10, 2024

CC @flavio, please take a look at this one after #311; the pertinent changes are at c9ad592 :)

@flavio
Copy link
Member

flavio commented Apr 22, 2024

@tnytown is this one ready to be reviewed?

@tnytown tnytown force-pushed the ap/wip-sct branch 3 times, most recently from 310fff3 to 76992cd Compare April 22, 2024 23:21
Copy link
Contributor Author

@tnytown tnytown left a comment

Choose a reason for hiding this comment

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

flagging some expects for review!

),
fingerprint: {
let mut hasher = sha2::Sha256::new();
spki.encode(&mut hasher).expect("failed to hash key!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This expect should be safe: the only error that encode should return in this context is IO-related (and hasher shouldn't ever refuse a write.)

tbs_precert
.encode_to_vec(&mut tbs_precert_der)
.expect("failed to re-encode Precertificate!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This encode_to_vec shouldn't be fallible: all we've done to the tbsCertificate is removed an extension, and it previously decoded correctly.

@tnytown
Copy link
Contributor Author

tnytown commented Apr 23, 2024

is this one ready to be reviewed?

Yes, thanks for the ping @flavio!

Co-authored-by: Alex Cameron <alex.cameron@trailofbits.com>
Signed-off-by: Andrew Pan <andrew.pan@trailofbits.com>
@tnytown
Copy link
Contributor Author

tnytown commented Jun 17, 2024

@flavio gentle ping on this PR :)

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM, I left two minor suggestions.

src/bundle/sign.rs Show resolved Hide resolved
src/bundle/sign.rs Show resolved Hide resolved
Co-authored-by: Flavio Castelli <flavio@castelli.me>
Signed-off-by: Andrew Pan <3821575+tnytown@users.noreply.github.com>
@tnytown
Copy link
Contributor Author

tnytown commented Jun 20, 2024

LGTM, I left two minor suggestions.

@flavio I applied the changes, thanks! Let me know if there's anything else you need.

@flavio
Copy link
Member

flavio commented Jun 20, 2024

I think it’s all good. Can you file some issues to keep track of the few TODOs that are left?

@flavio flavio merged commit 1c2245e into sigstore:main Jun 20, 2024
7 checks passed
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.

3 participants