Add Native SSL Support

[mdconaway]

Closes #1024
-Add native SSL support
-Add deprecation warning for missing host value (no behavior change)
-Add self-signed certificate factory
-Add certificate file loaders
-Add prettier formatter for server code npm run formatserver
-Formatted server code
-Update README

[saubyk]

Hi @mdconaway this is a great PR, thanks for all the effort you have put on it so far. Having SSL support natively (as an option) is a great addition to RTL. Although, there is an issue with self signed certificates for some browsers e.g. Chrome where the app will not work at all (I am not considering that a big deal though).

Nonetheless, we can merge the change as long as it is an optional configuration, which the user can enable and we keep it OFF by default. As we need to keep in mind that there are vendor integrations for RTL, where https access is enabled via reverse proxy setup and the default should always be ssl as OFF or we will end up breaking such integrations.

As far as the review of the PR is concerned, I would request to limit the changes to SSL only and roll back the rest e.g. the formatted server code and prettier formatter etc. This would enable us to do a thorough review of the relevant changes only.

Looking forward to review and work on this PR with you. Thanks.

[mdconaway]

Yeah, I can do that.

In this pull request the default value for SSL is definitely false. Regarding Chrome, I am currently able to use this RTL SSL feature branch with self signed certificates in the browser pretty much anywhere on my LAN, a user just has to click "proceed" through the warning messages. Ideally, users would generate their own CA and certs for their LAN/WAN and use the other config options for this enhancement like key, cert, and ca. (These options are absolute file paths to find the key files) Using actual key files also allows x509 client certificates to be requested to have a fully verified SSL tunnel.

This pull request also adds a deprecation warning on server boot if a user has not added a "host" value to their config. You guys can leave the warning as long as you'd like, and someday eventually change the host binding default when you feel sufficient time has passed. This is mainly a security related warning, but does not change any default configs or behavior. I added this deprecation warning because it occurs approximately where the ssl certs are loaded into the server.

While working on this pull I didn't see any linting script/config to check the server/backend code. I could easily include the addition of prettier as just a package.json update, then you guys can run npm run formatserver on your end in order to trust the server linting / formatting. Essentially the server formatter setup is just 2 package.json lines. The prettier changes are fully confined to package.json.

[mdconaway]

Just force pushed an update that matches the conversation above. Let me know if that is sufficient!

[ShahanaFarooqui]

@saubyk The PR is blocked due to your decision on adding this support.

[3nprob]

Native SSL/TLS: 👍
Generating key and self-signed cert: 👎

There is an issue on establishing initial trust with self-signed dynamic certs generated by the app like this.

It would still be nice to be able to supply cert+key, making the new config:

 "tls": {
    "key": "<Full path of a server TLS key file to enable TLS, default is null, Optional>",
    "cert": "<Full path of a server TLS pem format certificate bundle to enable TLS, default is null, Optional>"
  }

Note that CA here isn't really needed unless mTLS with client certificates is also implemented, which doesn't seem implemented here?

If that's the case, then:

 "tls": {
    "key": "<Full path of a server TLS key file to enable TLS, default is null, Optional>",
    "cert": "<Full path of a server TLS pem format certificate bundle to enable TLS, default is null, Optional>",
    "ca": <Full path of a TLS certificate authority file to enable TLS, default is null, Optional>
  }

[mdconaway]

Native SSL/TLS: 👍 Generating key and self-signed cert: 👎

There is an issue on establishing initial trust with self-signed dynamic certs generated by the app like this.

It would still be nice to be able to supply cert+key, making the new config:

 "tls": {
    "key": "<Full path of a server TLS key file to enable TLS, default is null, Optional>",
    "cert": "<Full path of a server TLS pem format certificate bundle to enable TLS, default is null, Optional>"
  }

Note that CA here isn't really needed unless mTLS with client certificates is also implemented, which doesn't seem implemented here?

If that's the case, then:

 "tls": {
    "key": "<Full path of a server TLS key file to enable TLS, default is null, Optional>",
    "cert": "<Full path of a server TLS pem format certificate bundle to enable TLS, default is null, Optional>",
    "ca": <Full path of a TLS certificate authority file to enable TLS, default is null, Optional>
  }

Per this file, inputs of key, cert and ca are all supported:

RTL/server/utils/ssl.ts

Lines 43 to 45 in 021f4e0

	filePaths.key = options.key;
	filePaths.cert = options.cert;
	filePaths.ca = options.ca;

The self-signed certificate factory is generally not for production use, but rather a way to easily test that SSL functions work without supplying a genuine certificate. In production, you should always generate and supply legitimate certificate files!

[mdconaway]

This line also only runs the self-signed cert generator if absolutely no certificate files are supplied:

RTL/server/utils/ssl.ts

Line 72 in 021f4e0

if (!this.key && !this.cert && !this.ca) {

[3nprob]

The self-signed certificate factory is generally not for production use, but rather a way to easily test that SSL functions work without supplying a genuine certificate.

This is not obvious to the user and a footgun. It accommodates and incentivizes insecure setups.

Since it is intended for test/dev, I think it can be solved by a separate shell script generating it using openssl in docs. This way, no need to pull in node-forge and add all the extra complexity to the production app.

If doing generation in JS is still desired, I suggest breaking it out to a separate entrypoint so that node-forge can be moved from dependencies to devDependencies.

[saubyk]

/rtl review

[rtlreview]

✅ Review posted: #1027 (review)

7 finding(s); 0 inline, 7 in body.

🔁 Need a re-review after pushing changes? Reply with /rtl re-review.
Maintainers can also /rtl dismiss <id> to silence specific findings, or anyone can /rtl explain <id> for elaboration.

[rtlreview]

This PR adds native SSL/TLS support to RTL with three modes: off (false), self-signed (true/object with cert factory options), or static cert files (key/cert/ca paths). It also adds optional mTLS via requestCert, a deprecation warning for missing host config, and a prettier formatter that has been applied across the server code.

The intent is sound and there is real value in giving operators an in-process TLS option. However, several defects affect correctness of the security-relevant paths: the SSL config parser misclassifies the literal string 'false', the CSRF cookie's secure flag is wired up incorrectly and read at the wrong time so it never takes effect, and the self-signed cert path generates 2048-bit RSA keys synchronously in pure JS on every restart, which both blocks the event loop and breaks any client trust established on the previous run. Maintainer (saubyk) also asked back in 2022 for the prettier-only reformatting to be rolled back so the SSL diff would be reviewable in isolation; that didn't happen and the noise persists.

---

Findings

• F1 (major) server/utils/config.ts:95 — normalizeSSL mishandles the string 'false'. The first branch sets ssl = val === 'true' ? true : val === 'false' ? false : JSON.parse(val); and then falls through to destructure ssl and return a populated default object. Destructuring a primitive false does not throw;...
• F2 (major) server/utils/csrf.ts:7 — The CSRF cookie's secure flag is broken in two independent ways. First, csurf({ cookie: true, secure: ... }) puts secure at the top level, but csurf only honors cookie options nested inside the cookie object — i.e. it must be csurf({ cookie: { secure: true } }). The ...
• F3 (major) server/utils/certificateFactory.ts:2 — forge.options.usePureJavaScript = true; forces node-forge into the pure-JS RSA path, and generateRandomCerts calls pki.rsa.generateKeyPair(this.encryptionBits) synchronously. A 2048-bit RSA keypair in pure JS routinely takes 5–30+ seconds depending on the host, and durin...
• F4 (major) server/utils/ssl.ts:68 — When key/cert/ca are not provided, SSL calls selfSignedCertificateFactory.getStaticBundle('buffer'), which generates a fresh keypair and self-signed certificate every time the SSL class is constructed. Because SSL is instantiated on each rtl.js start with no on-d...
• F5 (minor) server/utils/certificateFactory.ts:183 — cert.serialNumber = Math.round(Math.random() * max32BitInt).toString(16); uses non-cryptographic randomness for the serial. RFC 5280 §4.1.2.2 requires serials to be unique per issuer and recommends ≥64 bits of entropy from a strong source; Math.random() can collide and is pr...
• F6 (minor) server/utils/certificateIdentity.ts:22 — The mTLS middleware has two rough edges. (1) if (protocol === 'https' && (connection.authorized || rejectUnauthorized === false)) proceeds when rejectUnauthorized is false even on unauthorized connections, then unconditionally reads `connection.getPeerCertificate().subject...
• F7 (minor) server/utils/common.ts:1 — saubyk asked on 2022-05-23 to limit the PR to SSL changes and roll back the prettier reformatting so the security-relevant changes could be reviewed in isolation. The prettier dependency and formatserver script remain, and large unrelated reformat diffs are still present i...

[rtlreview]

🤖 rtlreviewbot audit metadata for this PR — auto-generated, please don't edit.