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

crypto and TLS implementation support #2

Open
djc opened this issue May 16, 2019 · 24 comments
Open

crypto and TLS implementation support #2

djc opened this issue May 16, 2019 · 24 comments
Labels
enhancement New feature or request question Further information is requested wontfix This will not be worked on

Comments

@djc
Copy link

djc commented May 16, 2019

Hi, this looks like a very interesting project! I'm thinking about building ACME support into a project for which I would like to use rustls (which relies on ring). Would you be open to changes that allow the use of ring and rustls as an alternative to openssl?

@breard-r
Copy link
Owner

Hello!
First of all, thank you and sorry for the delay, I missed the notification. The thing is, I wish to stay with only one crypto library. Unfortunately, ring is too high-level and does not implement the primitives I need to manage the private keys and certificates.
That said, since I do not like OpenSSL/LibreSSL, I am considering replacing the openssl crate with something else. My preference will go for something from the RustCrypto project if there is any. Because RustCrypto does not include TLS (yet?), I might then move the HTTPS crate to one which uses ring. However, that would not happen soon.
Meanwhile, I can recommend you acme-lib which uses ring for the HTTPS part. It has a few downsides like using both ring and openssl and not storing the account public key, but it is a good start for a new project.

@djc
Copy link
Author

djc commented May 25, 2019

Don't worry about the delay!

So if ring would implement the primitives you need, would you be interested? I regard ring as the best crypto implementation by far, and don't trust the maintainer(s) of RustCrypto nearly as much. I know the ring maintainer a bit and have contributed code before, so if you can articulate what additions we'd need we can probably get it done.

@breard-r
Copy link
Owner

So if ring would implement the primitives you need, would you be interested?

I don't really like ring, but yes, I would be interested. Unfortunately, I think many of those primitives are against ring's philosophy of a « easy-to-use (and hard-to-misuse) API ».

A quick and maybe incomplete list of required primitives is:

  • generation of RSA2048/RSA4096/P256/P384/Curve25519 key pairs and possibility to export those in PEM and DER
  • X.509 certificate generation with full control over the content, including custom extensions
  • PKCS always use NAMED_CURVE format for EC key storage; fixes #9 #10 CSR generation with full control over the content
  • X.509 certificate parsing and access to all of its content

@breard-r breard-r added enhancement New feature or request question Further information is requested labels May 26, 2019
@djc
Copy link
Author

djc commented May 27, 2019

Very curious to hear why you don't like ring!

I think the thing about ring is that it's only about the crytography, while a number of things you mention are more about the web PKI ecosystem. Have you looked at the webpki and rcgen crates? I know rcgen doesn't cover CSR generation yet, but I could probably implement that.

@breard-r
Copy link
Owner

Very curious to hear why you don't like ring!

The fist thing that comes in my mind is static linking. Although I really like it in many situations, I don't think that's a good thing in the context of a cryptographic library. Using ring would mean I should release a new version of ACMEd every time ring releases a bugfix that affects functions I uses. I usually don't care that much about other dependencies, but we are talking about crypto here, it's a highly important and sensible subject. Therefore, having [Open|Libre]SSL dynamically linked is quite an advantage since the update it up to the package managers of each distribution.

@djc
Copy link
Author

djc commented Jun 3, 2019

I agree the crypto dependency is highly important and sensitive. On the other hand, this is exactly why I would go the other way. openssl has had a slew of dependencies over a period where ring has had one relatively limited vulnerability. As long as no vulnerabilities happen, I don't think you need to upgrade for every semver-compatible release, just for releases that break backwards compatibility.

Anyway, I noticed that you added a note about a potential ring dependency to your CONTRIBUTING. Does this mean you're also not interested in having ring as an alternative dependency that can replace the openssl dependency, or would that still be okay? I would move all the crypto API into acme_common and create some kind of interface over it (similar to quinn-rs/quinn#351, actually) that would be implemented by openssl by default but optionally with ring instead.

rcgen can now generate certificate signing requests, so I think most of the pieces you mentioned are probably available at least for ECDSA certificates (and I can likely fill in what's needed for custom extensions as I run into it).

@breard-r
Copy link
Owner

breard-r commented Jun 3, 2019

Anyway, I noticed that you added a note about a potential ring dependency to your CONTRIBUTING. Does this mean you're also not interested in having ring as an alternative dependency that can replace the openssl dependency, or would that still be okay?

This note is how I see the situation in the short term, it is not meant to be permanent. I am not hostile to have ring as an optional dependency, but it will not happen before version 0.7. Because 0.6 is almost finished (I think I'll release it right after implementing HTTPS rate limits), it can be quite soon.

I don't know where rcgen development is going, but right now it does not include the functionalities I listed above, which are absolutely required if I was to use it along ring. Let's see if it does include them over time and maybe, meanwhile, use something else.

@est31
Copy link

est31 commented Jun 3, 2019

Hi, rcgen author here. ring and especially webpki APIs are a bit restricted, yeah. This was done on purpose I think to prevent misuse and is in general a good idea for security libraries. As for my own crate, it also has a great focus on giving control to users about what to do.

Helping ACME implementations is definitely on my radar and I'm willing to add APIs that fall into the general scope of certificate generation, like custom extensions for example. If you need anything from rcgen, just file an issue.

generation of RSA2048/RSA4096/P256/P384/Curve25519 key pairs and possibility to export those in PEM and DER

ring can generate key pairs for the elliptic curves you mentioned and in fact rcgen wraps that to generate entire certificates. For RSA, there is no key generation support yet. I have created a PR to add it but it's blocked on @briansmith telling me how I should do multiplications.

X.509 certificate generation with full control over the content, including custom extensions

IDK what you mean with full content but if you need anything, just file an issue.

PKCS #10 CSR generation with full control over the content

This one just got added to rcgen by @djc !

X.509 certificate parsing and access to all of its content

This is out of scope of the rcgen crate. The webpki crate has parsing code but its results are not fully exposed to users.

@breard-r
Copy link
Owner

breard-r commented Jun 5, 2019

Thank you for your feedback, it's much appreciated 😃

Now, let's see how everything is going. Currently, the situation is as follows:

Requirement Crate Status Description
P256/P384/Curve25519 key pairs generation and signature ring ✔️
RSA2048/RSA4096 key pairs generation and signature ring 🚧 Under construction: briansmith/ring#733
PKCS #10 CSR generation rcgen 🚧 Not released yet: rustls/rcgen#5
self-signed X.509 certificate generation with custom extension rcgen Not implemented yet: rustls/rcgen#6
X.509 certificate parsing and access to all of its content x509-parser ✔️

The fist thing I'll do is to parse certificates using x509-parser instead of OpenSSL. This change will be permanent since, for this purpose, using openssl is a nightmare.

Once rcgen is released with custom extensions (and therefore CSR), I'll be able to add a standalone feature which does not use OpenSSL at all. If ring still doesn't support RSA at this time, I'll disable it in the feature.

@djc
Copy link
Author

djc commented Jun 5, 2019

Can you make explicit what extensions you need, exactly? Ideally we'd just support those needed for ACME in rcgen directly.

@breard-r
Copy link
Owner

breard-r commented Jun 5, 2019

It's the acmeIdentifier extension described in draft-ietf-acme-tls-alpn. I use it in tacd which reads the already-computed content of this ext.

@est31
Copy link

est31 commented Jun 5, 2019

@breard-r if RSA key generation is a blocker for you, I'd recommend you don't wait until the ring PR is merged as I doubt it gets merged any time soon. Instead, once rcgen can take pre-generated keys, you will be able to use e.g. the rsa crate.

@est31
Copy link

est31 commented Jun 5, 2019

Ideally we'd just support those needed for ACME in rcgen directly.

Yeah that'd be ideal as then @breard-r doesn't have to include yasna themselves. I'm working on a commit to add support. Do you know of any test vectors (example certificates) that I can use to check whether my code is correct? I've also added support for rcgen users to specify any custom extensions: rustls/rcgen@67facaf rustls/rcgen@608472b

@est31
Copy link

est31 commented Jun 5, 2019

I've pushed a commit to create acmeIdentifier extensions but it's untested: rustls/rcgen@bb2d369

@cpu
Copy link

cpu commented Jun 5, 2019

Do you know of any test vectors (example certificates) that I can use to check whether my code is correct?

👋 Here's a small test program that can generate test TLS-ALPN-01 challenge response certificates (example cert). I made it quickly with the TLS-ALPN-01 code we wrote in the github.com/letsencrypt/challtestsrv package. It's used for integration tests of Let's Encrypt's implementation of TLS-ALPN-01 and should be correct as a comparison point.

@est31
Copy link

est31 commented Jun 5, 2019

@cpu thanks for the example cert. Optimally, @rolandshoemaker would include one in the RFC. Thanks to your help I've found a mistake in my code which I fixed in rustls/rcgen@5acb017 . Comparing the example cert with certs generated by me yields no significant differences.

@est31
Copy link

est31 commented Jun 6, 2019

0.3.1 release is out. Changelog entry. The only thing missing is RSA support (both when you specify the key yourself and RSA key generation support).

@breard-r
Copy link
Owner

breard-r commented Jun 7, 2019

@est31 That's great news, thank you !
Since I have to refactor a lot of code, it may take some time before I commit anything without openssl, but I'm working on it.

breard-r added a commit that referenced this issue Jun 7, 2019
Since it is planned to add a "standalone" feature that will replace
OpenSSL by crates not linking to any external library, it is required to
abstract all the OpenSSL specific types. This is a huge work and
therefore is divided in several steps. This first one is dedicated to
public and private keys.

rel #2
breard-r added a commit that referenced this issue Jun 8, 2019
As for public and private keys, the certificate should also be
abstracted.

rel #2
breard-r added a commit that referenced this issue Jun 8, 2019
@breard-r
Copy link
Owner

After refactoring, sometimes twice, the crypto part of the code base, starting implementing several thing with ring and the others libraries, I have come to realize the truth: it is NOT possible at all to do it at the moment.

I forgot one very important requirement: an opaque type for key pairs cannot be accepted since I need to extract the public key components. For RSA, it is the modulus n and the exponent e, for the NIST EC it is the x and y coordinates. Without access to those or, at least, the ability to export the public key into the JWK format, there is nothing I can do. In ring, such functionnality is followed in briansmith/ring#579.

Furthermore, I also enjoy to see briansmith/ring#859 fixed.

I have no other choice but to suspend this ring experiment for now. I will look back to it once ring is mature enough to be actually used.

breard-r added a commit that referenced this issue Jun 25, 2019
As discussed in #2, ring is not mature enough to replace OpenSSL. Hence,
the standalone mode which has been made to implement such a replacement
has to be removed until ring becomes usable.
@breard-r breard-r added the wontfix This will not be worked on label Jun 25, 2019
@djc
Copy link
Author

djc commented Jun 26, 2019

When I asked Brian Smith about these things, he mentioned that it was pretty much solved in jsonwebtoken and biscuit, so he recommended building on top of either of those.

@breard-r
Copy link
Owner

No, it does not seems to be solved at all. As far as I know, jsonwebtoken does not support JWK at all and biscuit cannot import the parameters from a given key since ring does not expose those. Furthermore, biscuit also does not support JWK thumbprints. biscuit documentation is very clear about ring's limitations.

@est31
Copy link

est31 commented Jun 26, 2019

From my research, with only an ASN.1 decoding library like yasna, it's non-trivial to extract x and y values from the public key of an ECDSA key. I think the algo is described in this document section 2.3.4. As for RSA, the n and e values can easily be extracted from public keys as well as private keys in the second representation. It's non trivial for private keys in the first representation.

So that makes us kinda reliant on a proper implementation of ECDSA willing to open those parameters for us or to give native JWK support.

@clarfonthey
Copy link
Contributor

Does anyone know what the status of this is now that it's a year later? I know that rustls has gained a lot of ground and has even gotten audited for correctness, and that multiple projects are now offering it as an alternative to openssl.

Not sure what the status of key generation is though.

@breard-r
Copy link
Owner

breard-r commented Aug 2, 2020

@clarfon As far as I know, ring is still not suitable since its has no plans to support the API parts required to implement the ACME protocol.
However, I am seriously looking at Botan as an alternative to OpenSSL. This cryptographic library seems robust, has a nice API and Rust bindings, which makes it a good candidate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants