Add KeyUsage support to CSR generation

rustls / rcgen

Generate X.509 certificates, CSRs

Other

346 stars 108 forks source link

Add KeyUsage support to CSR generation #287

Closed lvkv closed 3 months ago

lvkv commented 3 months ago

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 https://github.com/rustls/rcgen/issues/285)

lvkv commented 3 months ago

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 commented 3 months ago

I'll also add tests for this...

lvkv commented 3 months ago

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?

djc commented 3 months ago

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?

cpu commented 2 months ago

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.