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 support for writing certificate signing requests #5

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

djc
Copy link
Member

@djc djc commented Jun 2, 2019

Hey, would you be interested in merging something like this? Seems like a feature that mostly fits, at least, although it maybe widens the scope a bit.

On the other hand, I'm not sure how best to test this for now (although I suppose the openssl library exposes API to actually handle CSRs). So far this is based on just comparing openssl-generated CSRs with the JS ASN.1 thingy you point to in your README.

@est31
Copy link
Member

est31 commented Jun 3, 2019

@djc thanks for this PR. It's definitely in scope. I've wanted to add this feature eventually myself. It's really cool to have CSR generation because now maybe acme-client could switch to rcgen.

As for testing I think openssl can be used for it. I suggest creating a X509Req and calling its verify function.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

Added a test as per your suggestion.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

(This does introduce the slight naming oddity that a Certificate object no longer always represents an actual certificate, because in the CSR case it's merely the CertificateParams combined with the certificate's key pair without the means to sign it into a usable certificate. Not sure if that's something you want to address as part of this PR?)

src/lib.rs Outdated Show resolved Hide resolved
@est31
Copy link
Member

est31 commented Jun 3, 2019

naming oddity that a Certificate object no longer always represents an actual certificate

Yeah I think the API details are out of scope for the PR.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

Also I think at least some versions of ACME require support for custom extensions, which doesn't add yet. But ACME is the motivating use case for me as well, so I can look at supporting that next.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@est31 est31 merged commit a3a3d75 into rustls:master Jun 3, 2019
@djc
Copy link
Member Author

djc commented Jun 3, 2019

Thanks for the quick review!

@est31
Copy link
Member

est31 commented Jun 3, 2019

@djc if you want a new release, just ask.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

Will do. I don't think it's needed just yet.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

Do you know if the acme-client maintainer is amenable to replacing openssl with a ring-based stack?

@est31
Copy link
Member

est31 commented Jun 3, 2019

@djc I haven't had any communication with them if that's what you wonder about. I only thought it'd be a nice project.

@djc
Copy link
Member Author

djc commented Jun 3, 2019

The project seems a bit dead. I'm talking to the maintainer of an alternative here: breard-r/acmed#2.

@est31
Copy link
Member

est31 commented Jun 3, 2019

Oh yeah it kinda is. Getting acmed to use rcgen sounds like a good idea, too.

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.

None yet

2 participants