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

wip: refactoring extension handling #164

Closed
wants to merge 29 commits into from

Conversation

cpu
Copy link
Member

@cpu cpu commented Sep 29, 2023

Not yet ready for review

Description

Work in progress refactor of extension handling (for certificates, CSRs and CRLs).

Goals:

Todo:

  • Support for reading more supported CSR exts into params
    • basic constraints
    • extended key usages
    • name constraints
    • key usage
    • crl dps
  • tidy up x509-parser CSR ext conversions
  • optional fn to allow filtering exts emitted from params into CSR
  • optional fn to allow filtering exts emitted into params from CSR
  • Test coverage for duplicate extension rejection
  • More interesting test coverage
  • Commit history tidy
  • Changelog updates for breaking changes
  • Verison bump commit

@cpu cpu mentioned this pull request Sep 29, 2023
10 tasks
@cpu
Copy link
Member Author

cpu commented Sep 29, 2023

This is the work I started in cpu#1, now rebased, updated to forbid duplicate extensions, and moved to a draft PR on this repo.

I've listed some TODO items I'd like to tackle before calling for broader review. Stay tuned. I have some other pressing work to handle first.

@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Attention: 163 lines in your changes are missing coverage. Please review.

Comparison is base (53a5232) 75.51% compared to head (9d2c80b) 70.72%.

❗ Current head 9d2c80b differs from pull request most recent head 0225a26. Consider uploading reports for the commit 0225a26 to get more accurate results

Files Patch % Lines
src/csr.rs 20.13% 115 Missing ⚠️
src/ext.rs 93.04% 24 Missing ⚠️
src/lib.rs 81.15% 13 Missing ⚠️
src/error.rs 0.00% 8 Missing ⚠️
src/crl.rs 92.30% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   75.51%   70.72%   -4.80%     
==========================================
  Files           7        8       +1     
  Lines        1981     2169     +188     
==========================================
+ Hits         1496     1534      +38     
- Misses        485      635     +150     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cpu cpu mentioned this pull request Dec 8, 2023
4 tasks
This commit creates a new crate-internal module, `ext`, for managing
X.509 extensions. In this commit we wire up emitting extensions managed
by this module, but do not yet convert any existing extensions to the
new arrangement. This will begin in subsequent commits.

This adds a dedicated `Extensions` struct and `Extension` trait that handle:

* tracking extensions maintaining insertion order.
* ensuring the invariant that we never add more than one instance of the
  same extension OID.
* writing the DER encoded SEQUENCE of extensions.
* writing each DER encoded extension SEQUENCE - including the OID,
  criticality, and value.

The `Extension` trait allows common operations across all extensions
like:

* getting the ext OID.
* getting the criticality (using a new `Criticality` enum).
* getting the raw DER value.
This commit lifts the authority key identifier extension into the `ext`
module.
This commit lifts the subject alternative name extension into the `ext`
module.

It additionally ensures we never write an empty SAN extension, if the
`CertificateParams` contain an empty vec of SAN names.

For the time being SAN extensions are always written as non-criticial,
but the required plumbing to handle the RFC5280 guidance on SAN ext
criticality is added for follow-up adjustment.
This commit lifts the key usage extension into the `ext` module.
TODO: Split out the non-eku related bits.

This commit lifts the extended key usage extension into the `ext`
module.
This commit lifts the name constraints extension into the `ext` module.
This commit lifts the CRL distribution points extension into the `ext` module.
This commit lifts the subject key identifier extension into the `ext`
module.

Diverging from the existing code we now adhere to the RFC 5280 advice
and always emit the SKI extension when generating a certificate.
Previously this was only done if the basic constraints specified
`IsCa::Ca` or `IsCa::ExplicitNoCa`, but not when using `IsCa::NoCa`.
This commit lifts the basic constraints extension into the `ext` module.
This commit lifts the custom extension handling into the `ext`
module.
This commit lifts the CRL number extension handling into the `ext`
module.
This commit lifts the CRL issuing distribution point extension handling
into the `ext` module.
This commit lifts the CRL entry reason code extension handling
into the `ext` module.
This commit lifts the CRL entry invalidity date extension into the `ext` module.

There are no longer any references to the lib.rs `write_x509_extension`
helper, so it is also removed.
Now that all of the CRL entry extensions have been migrated to
`Extensions` we can let that type write the `SEQUENCE` and extension
values.

There are no longer any callers to `Extensions.iter()` so we remove that
fn.
@cpu
Copy link
Member Author

cpu commented Jan 17, 2024

This needs a rebase. I'm going to close this for now and will revisit when I have time to follow through on finishing the remaining parts.

@cpu cpu closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant