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

rust: add crate skeleton for X.509 path validation #8873

Merged
merged 193 commits into from
Dec 22, 2023

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented May 5, 2023

This PR adds the core path validation algorithm and policy components, as well as the Python-side verification API.

I'll use this PR to add some basic types as well (and remove the default code), as well as hook this up to the CI's tests and coverage.

See #2381.

@woodruffw
Copy link
Contributor Author

Design points:

  1. I named the "top" type here Validator, which is a little generic. Do you have preferences here?
  2. Should the trust store (currently Store) hold both trusted and untrusted parts, or just the trusted parts? My plan was to have it hold both in separate collections, but it could also be just the trusted certificates (with the top-level validation step taking in the untrusted certs separately).

@alex
Copy link
Member

alex commented May 5, 2023 via email

@woodruffw
Copy link
Contributor Author

Related: WDYT about a Policy or Constraints type for the constraints associated with a particular EE cert validation step?

i.e., the top-level API use would be something like:

// Fallibility reflects construction invariants: must be nonempty, must be only CA:true, etc.
let store = Store::try_from(trusted_certs)?;

// Validator defaults to `SystemTime::now`, `Validator::with_time(...)` for control of the validation time.
let validator = Validator::new(store);

let built_chain = validator.validate(leaf, policy)?;

where policy would be something like:

// Fluent-style, or something else?
let policy = Policy::new()
  .untrusted_intermediates(...)
  .key_usages(...)
  .extended_key_usages(...);

@alex
Copy link
Member

alex commented May 5, 2023 via email

@woodruffw
Copy link
Contributor Author

I don't think I understand why time goes on Validator and not policy.

No particular reason, it makes more sense on Policy!

(My original rationale was based on 5280 specifying that the time is more "intrinsic" to verification than additional user-introduced constraints, but I see no reason to expose that kind of distinction in a public API, given that the user-facing behavior is the same.)

@woodruffw woodruffw force-pushed the tob-x509-cv-skeleton branch 3 times, most recently from 60f312e to 212a651 Compare May 10, 2023 16:03
@woodruffw
Copy link
Contributor Author

There's a lot more work to be done, but the end-to-end MVP is now in place:

    policy = PolicyBuilder(profile=Profile.RFC5280).build()
    store = Store([root])
    chain = verify(ee, policy, [intermediate], store)

    assert chain == [ee, intermediate, root]

(I added a small testcase that demonstrates this; I'll rewrite it to be more idiomatic after some rounds of review.)

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

Some initial thoughts, I tried to focus on API/design.

There's a lot of code here, and as a general rule we don't land more than a few hundred LoC at a time, so one thing to start thinking about is how you're going to want to split it up.

src/cryptography/x509/__init__.py Outdated Show resolved Hide resolved
src/cryptography/x509/base.py Outdated Show resolved Hide resolved
src/cryptography/x509/base.py Outdated Show resolved Hide resolved
src/cryptography/x509/base.py Outdated Show resolved Hide resolved
src/cryptography/x509/base.py Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/Cargo.toml Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/backend.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/backend.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/certificate.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/certificate.rs Outdated Show resolved Hide resolved
@woodruffw
Copy link
Contributor Author

There's a lot of code here, and as a general rule we don't land more than a few hundred LoC at a time, so one thing to start thinking about is how you're going to want to split it up.

Yep -- I'm thinking that the Certificate and Extension API changes can probably go in first, followed by the core path validation logic, followed by the Python bits.

src/rust/cryptography-x509-validation/src/lib.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/lib.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/lib.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/lib.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/lib.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/policy/mod.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/policy/mod.rs Outdated Show resolved Hide resolved
src/rust/cryptography-x509-validation/src/policy/mod.rs Outdated Show resolved Hide resolved
tests/x509/verification/test_limbo.py Show resolved Hide resolved
@alex alex linked an issue Dec 21, 2023 that may be closed by this pull request
if let Time::GeneralizedTime(_) = validity_date {
// NOTE: This is technically wrong for certificates issued before 1950,
// but this does not matter in practice.
if validity_date.as_datetime().year() < GENERALIZED_DATE_CUTOFF_YEAR {
Copy link
Member

Choose a reason for hiding this comment

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

This didn't get fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced with a range check.

}

/// Checks whether the given EE certificate is compatible with this policy.
pub(crate) fn permits_ee(
Copy link
Member

Choose a reason for hiding this comment

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

This can just be merged into permits_leaf now (or vice-versa)

Just call permits_ee directly.

Signed-off-by: William Woodruff <[email protected]>
@alex alex merged commit 3763aa7 into pyca:main Dec 22, 2023
58 checks passed
woodruffw added a commit to woodruffw-forks/cryptography that referenced this pull request Dec 22, 2023
Signed-off-by: William Woodruff <[email protected]>
alex pushed a commit that referenced this pull request Dec 22, 2023
* CHANGELOG: record #8873

Signed-off-by: William Woodruff <[email protected]>

* docs/x509/verification: clean up, update note

Signed-off-by: William Woodruff <[email protected]>

* add module ref

Signed-off-by: William Woodruff <[email protected]>

* CHANGELOG: Cryptograpy's -> our

Signed-off-by: William Woodruff <[email protected]>

* CHANGELOG: reflow, better linkage

Signed-off-by: William Woodruff <[email protected]>

---------

Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw deleted the tob-x509-cv-skeleton branch December 22, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add x509 Certificate Validation
5 participants