fix(dev-server): for HTTPS, support non-RSA TLS certs #12065
Conversation
…-ssl-key CLI flags Two related fixes to `getHttpsConfig` in `docusaurus start`: 1. The validator in `getHttpsConfig.ts` used `crypto.publicEncrypt` / `crypto.privateDecrypt` to sanity-check the cert/key pair, but those APIs only work for RSA keys. Passing ECDSA (or EdDSA) certs — common in many corporate PKI setups — caused the dev server to throw `operation not supported for this keytype` even though the cert is fine. Replaced with `crypto.X509Certificate` + `crypto.createPrivateKey`, which work for any key type and additionally catch genuine cert/key-pair mismatches (the old code silently produced garbage in that case rather than throwing). 2. Certs could previously only be supplied via the `SSL_CRT_FILE` and `SSL_KEY_FILE` environment variables. Added matching `--ssl-cert <path>`, `--ssl-key <path>`, and `--https` CLI flags. CLI takes precedence; passing both `--ssl-cert` and `--ssl-key` implies HTTPS.
✅ [V2]
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Thanks for the PR, that makes sense to me.
I've commented on a few things that could be improved/redesigned/simplified, and wonder if this should be a fix/feat/breaking change.
Please let me know how much you can help bring this PR to the finish line (I can do it)
If this is for Meta internal use, in what timeframe would you like to get this feature available? We have already released our last v3 planned minor version and respect semver.
- v3 (patch release) can be sooner to support ECDSA, but normally, we are not supposed to introduce new CLI options on patches.
- v4 (feat/breaking change) may take more time
An option could be to split this PR into 2:
- v3.10.x could fix support for ECDSA through env vars
- v4.0 could introduce new CLI flags
|
If you could bring it over the line I'd appreciate, you'll be more efficient than I will. I appreciate the hesitance to add CLI flags in a patch release. I would point out that use of global environmental variables is often a direct cause of bugs. Passing something like SSL_KEY_FILE or HTTPS could impact subprocesses — in a complex environment like Meta, this can cause unpredictable behavior. That said, if that's not compelling enough, i think a reasonable patch release approach could be to add a DOCUSAURUS_{HTTPS,...} env var that takes precedence over the generic one. That'd address my concern. |
|
I see thanks I'll bring this to the finish line and let you review before merging |
slorber
left a comment
There was a problem hiding this comment.
I refactored/simplified the code, handled a few more edge cases.
It seems good to merge now.
For now, I only added secret DOCUSAURUS_ prefixed env vars.
We'll re-introduce the extra CLI options in Docusaurus v4 as a separate PR, and mention them in our upcoming blog post to ensure feature discoverability.
I'll test this locally and merge asap. I have a few other fixes to backport, so this should be released soon as a v3.10.2 patch release.
|
It works 👍 Here's how to validate locally on our own website, in addition to unit tests: # RSA certs
DOCUSAURUS_SSL_CRT_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host.crt DOCUSAURUS_SSL_KEY_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host.key yarn start:website --host 0.0.0.0
# ECDSA certs
DOCUSAURUS_SSL_CRT_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host-ec.crt DOCUSAURUS_SSL_KEY_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host-ec.key yarn start:website --host 0.0.0.0
# Mismatch: throws
DOCUSAURUS_SSL_CRT_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host-ec.crt DOCUSAURUS_SSL_KEY_FILE=../packages/docusaurus/src/webpack/utils/__tests__/__fixtures__/getHttpsConfig/host.key yarn start:website --host 0.0.0.0 |
|
Separate PR reintroducing the CLI args: #12082 |
What got implemented:
DOCUSAURUS_prefix for env variables (DOCUSAURUS_HTTPS=true), taking precedence over unprefixed env variablesLet's consider all this as a v3 bug fix, that will be backported and released in a patch release.
On v4, we'll:
Original PR:
Two related fixes to
getHttpsConfigindocusaurus start:The validator in
getHttpsConfig.tsusedcrypto.publicEncrypt/crypto.privateDecryptto sanity-check the cert/key pair, but those APIs only work for RSA keys. Passing ECDSA (or EdDSA) certs — common in many corporate PKI setups — caused the dev server to throwoperation not supported for this keytypeeven though the cert is fine.Replaced with
crypto.X509Certificate+crypto.createPrivateKey, which work for any key type and additionally catch genuine cert/key-pair mismatches (the old code silently produced garbage in that case rather than throwing).Certs could previously only be supplied via the
SSL_CRT_FILEandSSL_KEY_FILEenvironment variables. Added matching--ssl-cert <path>,--ssl-key <path>, and--httpsCLI flags. CLI takes precedence; passing both--ssl-certand--ssl-keyimplies HTTPS.Test Plan
Unit tests
Extended
getHttpsConfig.test.tsfrom 5 to 13 tests. The original 5 still pass (no behavior change for the RSA happy path). New coverage:getHttpsConfig({ sslCert, sslKey })reads from CLI options when no env vars are set.You specified --ssl-cert...error (the env-var path still reportsSSL_CRT_FILE in your env...).--ssl-cert+--ssl-keytogether implies HTTPS without--https.--httpsalone returnstrue.New ECDSA fixtures generated with
openssl ecparam -name prime256v1are committed alongside the existing RSA fixtures.Manual
Generated a self-signed P-256 cert and ran:
Server starts,
curl --cacert ./host.crt https://localhost:3000/returns 200 withverify=0. Without this PR the same invocation fails at startup with theunsupported keytypeerror frompublicEncrypt.Test links
https://deploy-preview-12065--docusaurus-2.netlify.app/