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

Expand CSR generation support #285

Open
lvkv opened this issue Jul 25, 2024 · 14 comments
Open

Expand CSR generation support #285

lvkv opened this issue Jul 25, 2024 · 14 comments

Comments

@lvkv
Copy link
Contributor

lvkv commented Jul 25, 2024

Hi! I'm looking for an alternative to rust-openssl's X509ReqBuilder for building and signing certificate signing requests. Ideally an alternative that supports:

  • Custom EKUs (supported by OpenSSL)
  • Custom certificate extensions (supported by OpenSSL)
  • Custom ASN.1/DER-encoded CSR attributes in the PKCS#9/10 sense (not supported in any sane way by OpenSSL AFAIK, but this would be amazing)

I've had a great experience with other Rustls projects, so I checked here first and found rcgen. Of course, I understand if a CSR builder is out of scope here.

Motivation: I'm implementing a subset of the MS-WCCE certificate enrollment protocol, which involves crafting CSRs with a many interesting extensions and attributes. I'm also open to suggestions for other crates that might be able to help out with this!

@lvkv lvkv changed the title Add a CSR builder Add a CSR builder? Jul 25, 2024
@djc
Copy link
Member

djc commented Jul 25, 2024

@lvkv we already have support for building CSRs, which currently works by building from a CertificateParams. Rather than supporting a random collection of custom stuff, I think we should probably go the way of defining some API that fits with the MS-WCCE standard. Given the scope here, it probably makes sense to make an exhaustive list of the exact bits you need and come up with a tentative API design that doesn't just expose a collection of customizations (extensions, EKUs and attributes) but tries to define comprehensive support for what MS-WCCE needs.

@lvkv
Copy link
Contributor Author

lvkv commented Jul 25, 2024

we already have support for building CSRs, which currently works by building from a CertificateParams.

Oh interesting! I didn't catch that the way to create a CertificateSigningRequest is to first create a CertificateParams and then to serialize_request. FYI this feels non-obvious, especially given that CertificateSigningRequestParams also exists, but isn't used for that purpose.

I tried creating a dummy CSR using this method, but ran into an UnsupportedInCsr error because I tried to add key usages to the request. Looks like EKU support was recently merged, so I assume this is all a work in progress still?

if serial_number.is_some()
|| *is_ca != IsCa::NoCa
|| !key_usages.is_empty()
|| name_constraints.is_some()
|| !crl_distribution_points.is_empty()
|| *use_authority_key_identifier_extension
{
return Err(Error::UnsupportedInCsr);
}

Given the scope here, it probably makes sense to make an exhaustive list of the exact bits you need and come up with a tentative API design that doesn't just expose a collection of customizations (extensions, EKUs and attributes) but tries to define comprehensive support for what MS-WCCE needs.

Agreed that thinking this out is a good idea. I'm also very much not against first-class support for these bits (it'd be quite nice, actually!). Comprehensive support might be a bit out of reach, given that the spec is a >250 page PDF, but I think it should be more than possible to implement enough of a skeleton so that people can easily add in whatever bits they need when they need to.

I think it'd be a good idea to figure out the boundaries of what rcgen would like to add support for. For example, adding MS-WCCE-specific bits for traditional PKCS#10 CSR generation feels like an achievable goal. However, MS-WCCE has other CSR types that encapsulate PKCS#10 CSRs with additional bundled information. E.g. 2.2.2.7 Certificate Request Attributes gives a handy list of all the different CSR flavors (truncated with added emphasis). The latter two bullets are what I'm curious to hear your thoughts on:

  • ... requests based on the PKCS#10 message format...
  • ... requests based on the CMS format... inner PKCS#10 certificate...
  • ... requests based on the CMC format... inner PKCS#10 certificate...

Personally what I'd like to support for is:

  • MS specific EKUs. Specifically XCN_OID_ENROLLMENT_AGENT. This one isn't MS-WCCE specific—it's more related to the issuing CA. In short, this denotes a certificate that allows you to request certificates be issued on others' behalf ("enroll on behalf of") (fun, I know).
  • Custom EKUs that represent user-generated (where "user" = server administrator) application policies
  • "Parameterized" protocol-specific certificate extensions, specifically szOID_CERTIFICATE_TEMPLATE. In this case "parameterized" means that the values of of some fields will vary based on what the user is requesting, so they must be supplied by the developer.
  • "Parameterized" protocol-specific attributes (in the PKCS#10 ATTRIBUTE) sense. Specifically szOID_RENEWAL_CERTIFICATE, which is basically just an ASN.1 SET containing a certificate you'd like to renew.

Lots of text! Let me know what you think. As always, I'm more than happy to work with you guys to create something useful!

@lvkv
Copy link
Contributor Author

lvkv commented Jul 25, 2024

The big picture motivation is to use AD CS as a CA on Linux. Unfortunately, that means learning how to speak the CSR equivalent of parseltongue. On the bright side, it's a great excuse to craft a super flexible CSR generation library... 🙂

@djc
Copy link
Member

djc commented Jul 26, 2024

Oh interesting! I didn't catch that the way to create a CertificateSigningRequest is to first create a CertificateParams and then to serialize_request. FYI this feels non-obvious, especially given that CertificateSigningRequestParams also exists, but isn't used for that purpose.

We're open to improving how that API works, if you have suggestions!

I tried creating a dummy CSR using this method, but ran into an UnsupportedInCsr error because I tried to add key usages to the request. Looks like EKU support was recently merged, so I assume this is all a work in progress still?

It's possible -- it's been a while, so it might make sense to try it against main to see what works there.

E.g. 2.2.2.7 Certificate Request Attributes gives a handy list of all the different CSR flavors (truncated with added emphasis). The latter two bullets are what I'm curious to hear your thoughts on:

Right now I think what we do for CSRs is the PKCS#10 message format, and I think deviating from that would sort of need a separate justification.

  • MS specific EKUs. Specifically XCN_OID_ENROLLMENT_AGENT. This one isn't MS-WCCE specific—it's more related to the issuing CA. In short, this denotes a certificate that allows you to request certificates be issued on others' behalf ("enroll on behalf of") (fun, I know).
  • Custom EKUs that represent user-generated (where "user" = server administrator) application policies

Adding XCN_OID_ENROLLMENT_AGENT to KeyUsagePurpose seems low-hanging fruit, though if we also do custom EKUs we might want to just support that.

  • "Parameterized" protocol-specific certificate extensions, specifically szOID_CERTIFICATE_TEMPLATE. In this case "parameterized" means that the values of of some fields will vary based on what the user is requesting, so they must be supplied by the developer.
  • "Parameterized" protocol-specific attributes (in the PKCS#10 ATTRIBUTE) sense. Specifically szOID_RENEWAL_CERTIFICATE, which is basically just an ASN.1 SET containing a certificate you'd like to renew.

For these I think it's all about coming up with a reasonably type-safe API.

(Note that these are my personal opinions, the other maintainers might feel differently.)

@lvkv
Copy link
Contributor Author

lvkv commented Jul 26, 2024

Right now I think what we do for CSRs is the PKCS#10 message format, and I think deviating from that would sort of need a separate justification.

I was thinking this too!

It's possible -- it's been a while, so it might make sense to try it against main to see what works there.

Yeah, using what's in main, it looks like EKUs are supported, but key usages aren't. How does the following sound as a high-level plan? These can more or less all be worked on in parallel:

  • Add missing support for key usages (e.g. digital encipherment) in CSRs
  • Add type-safe MS-WCCE certificate extension builders that would end up being added to CertificateParams::custom_extensions. Looking at § 2.2.2.7.7 szOID_CERT_EXTENSIONS, there appear to be only 3 (see the sidebar on the left):
    1. (Prioritize) CertificateTemplateOID (link):

       CertificateTemplateOID ::= SEQUENCE {
               templateID              OBJECT IDENTIFIER,
               templateMajorVersion    INTEGER (0..4294967295) OPTIONAL,
               templateMinorVersion    INTEGER (0..4294967295) OPTIONAL
           } --#public
    2. CertificateTemplateName (link):

      CertificateTemplateName ::= SEQUENCE {
          Name            UTF8String
      }
    3. OtherName (link):

       OtherName ::= SEQUENCE {
            type-id    szOID_NTDS_OBJECTSID,
            value      octet string
      }
  • Add type-safe MS-WCCE PKCS#10 attribute builders, starting with szOID_RENEWAL_CERTIFICATE. I haven't taken too close of a look yet, but this might require a little work to make rcgen's internal representations support arbitrary ATTRIBUTEs. Once that works, there are actually a fair number of attributes that we could add afterwards, with a comprehensive list over at § 2.2.2.7 Certificate Request Attributes—see the subsections listed in the sidebar on the left.

@lvkv lvkv changed the title Add a CSR builder? Expand CSR generation support, incl. MS-WCCE Jul 26, 2024
@cpu
Copy link
Member

cpu commented Jul 29, 2024

👋 I haven't gone deep on the MS-WCCE spec or the required PKCS#10 attribute changes, but here are some initial thoughts.

At one point I had ambitions of reworking the way extensions are handled (see #164) and one goal of that rework was to help bring parity between what's supported in certs, what's supported in CSRs, and how custom extensions are handled. Maybe there are some ideas to revive from that stale branch that could help here?

In general I'm a little bit wary about extending the existing API with new features while the underlying handling is messy/inconsistent but I also don't want to block helpful improvements on a big refactor I can't commit to seeing through myself. Maybe this concern would fall away after the diffs start to come up for review?

Rather than supporting a random collection of custom stuff, I think we should probably go the way of defining some API that fits with the MS-WCCE standard.

I'm not sure I'm convinced about this. I think in general I'd prefer to see Rcgen expose a helpful/usable API for custom EKUs, certificate extensions, and CSR attributes but not MS-WCCE specific bits (perhaps with the exception of defining well known OID constants).

The MS-WCCE standard feels pretty niche to me, and like something the majority of rcgen users wouldn't benefit from. I think it would be a better end-state if someone with the bandwidth to support the MS-WCCE use-case could do that external to Rcgen using primitives it exposes. WDYT?

@lvkv
Copy link
Contributor Author

lvkv commented Jul 29, 2024

I think it would be a better end-state if someone with the bandwidth to support the MS-WCCE use-case could do that external to Rcgen using primitives it exposes. WDYT?

Yeah, FWIW I'm totally fine with MS-WCCE ideas not getting first-class API support here 🙂 having the building blocks to express them myself is just as good if not better; there are undoubtedly many other weird standards put out by big tech companies, and choosing to support this one may set an exhausting precedent.

In general I'm a little bit wary about extending the existing API with new features while the underlying handling is messy/inconsistent but I also don't want to block helpful improvements on a big refactor I can't commit to seeing through myself. Maybe this concern would fall away after the diffs start to come up for review?

Yeah, how do you feel about this revised plan, then? I can take a stab at doing all 3 of these:

  1. Add missing support for key usages (e.g. digital encipherment) in CSRs, taking ideas and code, where applicable, from wip: refactoring extension handling #164
  2. Add support for custom PKCS#10 CSR attributes, taking ideas from wip: refactoring extension handling #164
    (Evaluate)
  3. Do whatever larger refactors/rewrites are deemed necessary, breaking them up into smaller chunks where possible

Like most users, I need this yesterday 🙂 so I'm naturally inclined to prioritize expanding the API. That being said, I'm more than happy to help you guys out with making the implementation more sustainable for the long run. Net-zero spaghetti initiative.

@lvkv lvkv changed the title Expand CSR generation support, incl. MS-WCCE Expand CSR generation support Jul 29, 2024
@cpu
Copy link
Member

cpu commented Jul 30, 2024

Yeah, how do you feel about this revised plan, then?

This sounds great to me.

@est31 Do you have any reservations here?

taking ideas and code, where applicable, from #164

I think we're on the same page but just to be super clear don't feel obligated to try and revive all of #164 or anything :-) There's been quite a bit of API changes since then and I think the scope was ambitious on its own. I trust your judgement on what might make sense to try and shim into your overall goal.

@est31
Copy link
Member

est31 commented Aug 1, 2024

@est31 Do you have any reservations here?

sounds fine to me.

@djc
Copy link
Member

djc commented Aug 1, 2024

I'm not sure I'm convinced about this. I think in general I'd prefer to see Rcgen expose a helpful/usable API for custom EKUs, certificate extensions, and CSR attributes but not MS-WCCE specific bits (perhaps with the exception of defining well known OID constants).

That's fair. I thought we might want to try making sure that people don't put together potentially dangerous combinations of custom EKUs, extensions and attributes, so I figured putting some guardrails in place might help here. But I do agree that this MS-WCCE stuff might be niche (it's hard to say how niche) so we don't want to put in place too much special stuff for it.

@lvkv
Copy link
Contributor Author

lvkv commented Nov 14, 2024

Now that #296 has been merged, I'd like to resurface #296 (comment) before either @cpu or I forget

Basically, there are aspects of the API here that can probably be updated or rethought—either to make the developer experience better, facilitate a more organized implementation, or both. Some things worth noting off the top of my head:

  • (Add PKCS#10 attributes to CSR serializer #296 (comment)) Consider splitting the API for generating certificates from the API for generating CSRs. The sets of inputs overlap quite a bit, but they're not the same. For example, CSRs can contain request attributes that don't make sense to have in CertificateParams
  • Consider updating the readme and GitHub repository description to highlight that this library can also build CSRs! I almost missed this project when I was looking around because the feature wasn't advertised. It looks like there are others out there too, e.g. https://www.reddit.com/r/rust/comments/haupzo/info_request_for_x509_csr_implementation_in_pure/, which was posted 4 years ago and has a follow-up comment as recent as 10 months ago. Side note: idk if aws-lc-sys would violate their "pure rust" requirement, but there's also ring!

@est31
Copy link
Member

est31 commented Nov 16, 2024

Consider updating the readme and GitHub repository description to highlight that this library can also build CSRs!

Description updated.

@lvkv
Copy link
Contributor Author

lvkv commented Nov 16, 2024

Generate X.509 certificates, CRLs

@est31 I think you meant to type "CSRs", not "CRLs"? FTR I think "Generate X.509 certificates and certificate signing requests" would also sound fine 🙂 but it's ultimately up to you

@est31
Copy link
Member

est31 commented Nov 17, 2024

Oops, lol, corrected. You are right.

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

No branches or pull requests

4 participants