refactor(ssl): replace openssl dependency with node-forge #29
Conversation
Fewer dependencies means better DX and UX! With this change, devcert no longer requires a local installation of `openssl`. The node-forge library: - implements the TLS standard - uses native crypto where available, with only a tiny perf hit - Configures keys and certificates using code, rather than external config file processing - generates PEM files, but also can connect keys, cAs, CSRs, and certs without bouncing off the filesystem Notes: - I had to try many times to get browsers and OSes to verify the chain. Ultimately I succeeded by: - Removing the `authorityKeyIdentifier` extension (considered redundant) - Generating serial numbers based on the ASN.1 standard - Removing all subject attributes but commonName I hope this remains an acceptable certificate configuration for all needs. - TODO: This can be additionally refactored to remove some of the filesystem-dependent steps. For instance, you could change the way that child certificate generation accesses the root CA, by reading the PEM from the filesystem only once and then threading the CA cert object through the necessary arguments. This would speed up and simplify the code. I chose not to do it for this round, because it would be too large a change for you to safely integrate. hore: remove openssl config files
There was a problem hiding this comment.
Thanks for this thorough PR!
I like the idea of losing the native OpenSSL dependency, but I'm slightly concerned about going with something like node-forge for crypto. It certainly seems like a large enough project to be relatively trustworthy for this kind of use, but I can also imagine some folks that might get a bit skittish about using a less battle tested crypto solution.
What do you think about perhaps a flag? I.e. default to using node-forge, but allow an --openssl flag to fall back to it if requested. That gives a good default experience for users with no strong preference (i.e. no native dependency) and a good fallback option for those users who prefer OpenSSL (and those who know enough to care would likely be the same crowd that knows how to install OpenSSL).
| function createCertificationRequest(): Certificate; | ||
| function createCaStore(): CertificateAuthorityStore; | ||
| function verifyCertificateChain(caStore: CertificateAuthorityStore, certs: Certificate[]): void; | ||
| } |
There was a problem hiding this comment.
I'd suggest filing a PR with the DefinitelyTyped repo to land these types for everyone to use!
There was a problem hiding this comment.
Yes, that's a good idea! I haven't opened that PR yet--would you strongly prefer to wait until DefinitelyTyped approves, merges and releases such a PR before merging this?
| @@ -1,7 +1,25 @@ | |||
| /// <reference types="node-forge" /> | |||
There was a problem hiding this comment.
I'm not super familiar with this syntax, but I was under the impression it was no longer necessary?
There was a problem hiding this comment.
It was the way I got TypeScript to refer to DefinitelyTyped node-forge types in this file. If I used an import declaration, then TypeScript stopped regarding the file as a definition file and would error on compilation.
|
While I wouldn't trust node-forge with generating certs or CSRs for production, it's already in wide use by devs who use webpack-dev-server over https, through this usage of the selfsigned library. I know, you gotta step lightly around crypto, but since devcert is for dev environments only, I think this isn't an unprecedented usage of the library. The idea of a flag is appealing, but I think it would be a pretty big maintenance burden; as browser TLS requirements continue to change, we would have to maintain cert generation configuration in two formats: one as an OpenSSL conf file, and another as a module that configures node-forge. |
|
Is it likely that you'll merge this? If not, that's okay, we'll just fork it into our own namespace (with no intent to compete with yours, we'll make that clear). |
|
Would love to see this merged as I'm struggling on windows with devcert not accepting the password provided to openssl |
|
I know it's been a while since this PR got any attention, but I'd love to see this picked back up if there's still interest from the author and maintainer (happy to try to pick this back up myself if the original author is too busy or no longer interested). Having to ask the end user to install OpenSSL separately might not seem like a big deal, but for building tools/libraries on top of devcert (which seems to be an intended use case), that small point of friction makes it impossible to offer a truly seamless onboarding experience, which is one of those things separates a decent DX from a great one. If we're not comfortable with node-forge being a replacement/default, I'd happily settle for it being an opt-in instead, so only tooling authors who value DX over a more-battle-tested cert generation process need to use it. |
|
👀 |
Fewer dependencies means better DX and UX! With this change, devcert no
longer requires a local installation of
openssl.The node-forge library:
config file processing
without bouncing off the filesystem
Notes:
I had to try many times to get browsers and OSes to verify the chain.
Ultimately I succeeded by:
authorityKeyIdentifierextension (consideredredundant)
I hope this remains an acceptable certificate configuration for all
needs.
TODO: This can be additionally refactored to remove some of the
filesystem-dependent steps. For instance, you could change the way
that child certificate generation accesses the root CA, by reading the
PEM from the filesystem only once and then threading the CA cert
object through the necessary arguments. This would speed up and
simplify the code. I chose not to do it for this round, because it
would be too large a change for you to safely integrate.