Skip to content

Add support custom CSR extensions when parsing #337

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jean-airoldie
Copy link
Contributor

This PR adds CertificateSigningRequestParams::from_pem_validated & from_der_validated methods, which allow the user to provide a custom validation closure to handle otherwise unsupported extensions found in the OID_PKCS_9_AT_EXTENSION_REQUEST CRL attribute. In other words, this allow CSR to correctly handle CustomExtension found into the custom_extensions field when parsing from DER or PEM.

This depends on this PR being merged.

This closes #150.

@djc
Copy link
Member

djc commented May 5, 2025

What are you trying to achieve? Which extension do you want to support?

@jean-airoldie
Copy link
Contributor Author

jean-airoldie commented May 5, 2025

Proprietary extension, such as storing a user ID directly in the certificate. The idea is that since the certificate is signed, this metadata is guaranteed to have been validated by a CA, and I control the CA so I indeed validate those extensions.

Copy link
Member

@cpu cpu 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 like to give some thought to the general problem before comitting to this as the right solution, but it'd be nice to see the existing commits squashed in the meantime.

@cpu
Copy link
Member

cpu commented May 8, 2025

ci / Validate external types appearing in public API (pull_request) Failing after 1m

Also, this looks like a true positive: we don't want x509-parser leaking through the rcgen API. That suggests to me that we'll need to rework the closure argument at a minimum. If that happens does your upstream change in x509-parser lose some of its value?

@jean-airoldie
Copy link
Contributor Author

Also, this looks like a true positive: we don't want x509-parser leaking through the rcgen API. That suggests to me that we'll need to rework the closure argument at a minimum. If that happens does your upstream change in x509-parser lose some of its value?

Not at all, we can just implement a simple wrapper type that is then converted internally into the specific x509-parser type. However its important to note that the upstream is currently working on some major rework of its API, and there's discussion about introducing a whole new visitor API for CSRs. So I would say this PR is definitely gonna change, which is why its a draft. I though it would still be valuable to write this PR in case it is a controversial change etc.

@jean-airoldie jean-airoldie force-pushed the custom_extension branch 5 times, most recently from 17e72e7 to b4e43fd Compare May 8, 2025 15:26
* Allow user to parse otherwise unsupported extensions.
* Retain the custom extensions when parsing.
* Update to latest branch commit
* Ran rustfmt
* Fix clippy
* Add UnsupportedExtension wrapper type
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.

Modulo some nits, I think this looks pretty reasonable.

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.

Allow CSR parsing to handle custom extensions
3 participants