rcgen icon indicating copy to clipboard operation
rcgen copied to clipboard

Add KeyUsage support to CSR generation

Open lvkv opened this issue 1 year ago • 3 comments

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 avatar Aug 07 '24 01:08 lvkv

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 avatar Aug 07 '24 01:08 lvkv

I'll also add tests for this...

lvkv avatar Aug 07 '24 01:08 lvkv

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 avatar Aug 07 '24 19:08 lvkv

  • 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 avatar Aug 20 '24 10:08 djc

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.

cpu avatar Aug 26 '24 14:08 cpu