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

Add KeyUsage support to CSR generation #287

Merged
merged 5 commits into from
Aug 23, 2024
Merged

Conversation

lvkv
Copy link
Contributor

@lvkv lvkv commented Aug 7, 2024

I've added KeyUsage support to CSR generation, as well as a number of improvements to the parsing and writing of DER-encoded key usages.

These commits can be reviewed in order!

(Coming from #285)

@lvkv
Copy link
Contributor Author

lvkv commented Aug 7, 2024

I took a stab at simplifying the existing KeyUsage serialization logic. AFAICT, there seems to be no downside to unconditionally encoding KeyUsage with 9 bits—this significantly cuts down the required bit twiddling. I also took the opportunity to use the same logic for both certificates as well as CSRs. Either way—LMK your thoughts on these!

@lvkv lvkv changed the title Lvkv csr key usage Add KeyUsage support to CSR generation Aug 7, 2024
@lvkv
Copy link
Contributor Author

lvkv commented Aug 7, 2024

I'll also add tests for this...

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

I'd suggest reorienting the commit history as such:

  • Introduce the separate KeyUsagePurpose to u16 conversion method, and start using it in the same commit in place (and also other changes to always use 16 bits)
  • Extract the write_key_usage() method from its current context
  • Add writing key usages in CSR serialization

rcgen/src/lib.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
@lvkv lvkv force-pushed the lvkv-csr-key-usage branch 2 times, most recently from 55ceece to 1f188c5 Compare August 7, 2024 18:56
@lvkv
Copy link
Contributor Author

lvkv commented Aug 7, 2024

A few notes:

  • I've simplified the parsing of KeyUsagePurposes with a from_u16 function (complementing to_u16) that parses the BIT STRING value into a vector of key usages. It seems that the previous implementation was written so as to not depend on the bit order of x509-parser's flags, though it's a bit... if-fy (ha). What I've written is aware of the bit order, but is also more elegant (IMO) and has tests added so that if x509 parser changes its bit order we'll know. I think it's better, but I'm fine with changing it if anyone feels strongly.
  • Looks like the build CI steps (macOs, ubuntu, etc.) don't run with the x509-feature enabled. Should they?

@lvkv lvkv requested a review from djc August 7, 2024 19:07
@lvkv lvkv marked this pull request as ready for review August 7, 2024 19:07
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks!

rcgen/src/csr.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Aug 20, 2024

  • Looks like the build CI steps (macOs, ubuntu, etc.) don't run with the x509-feature enabled. Should they?

It looks like the coverage job enables --all-features. I guess that might be enough?

@djc djc added this pull request to the merge queue Aug 23, 2024
Merged via the queue into rustls:main with commit 964f9c6 Aug 23, 2024
15 checks passed
@cpu
Copy link
Member

cpu commented Aug 26, 2024

Thanks! Sorry I didn't get a chance to take a look at this before it merged. I reviewed the diff that landed and it looks good to me.

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.

4 participants