crypto: explicitly enable auto cert chaining

[nornagon]

OpenSSL enables this feature by default, but BoringSSL doesn't, because it's expensive and unnecessary (see boringssl/27 and boringssl/42). This change makes it so that when building node with BoringSSL, the behaviour matches OpenSSL's.

Electron will be building node.js with BoringSSL in future, and we've floated this patch on our fork (see electron/node#47).

Checklist

• commit message follows commit guidelines

[addaleax]

@nodejs/crypto

[Trott]

@nodejs/security-wg

[tniessen]

Not objecting, but a couple of questions:

• You are saying it is "expensive and unnecessary" and one of their commit messages contains this paragraph:

  In transition to removing it altogether, set SSL_MODE_NO_AUTO_CHAIN by
  default. If we find some consumer was relying on it, this will allow
  them to revert locally with SSL_(CTX_)clear_mode, but hopefully this was
  just unused.

  How does node rely on this behavior when built with BoringSSL? Is their assessment correct that node shouldn't rely on it?

• The same commit suggests that the flag will be removed altogether in the future. How would we compensate for that?

• Is this the only required change to make node work with BoringSSL?

[nornagon]

How does node rely on this behavior when built with BoringSSL?

The specific behaviour I observed was when setting up a server with a self-signed cert, and providing the root/intermediate certs in the ca list, rather than writing the cert chain directly into the pem.

const server = https.createServer({
  key: fs.readFileSync(path.join(certPath, 'server.key')),
  cert: fs.readFileSync(path.join(certPath, 'server.pem')),
  ca: [
    fs.readFileSync(path.join(certPath, 'rootCA.pem')),
    fs.readFileSync(path.join(certPath, 'intermediateCA.pem'))
  ],
  requestCert: true,
  rejectUnauthorized: false
}, ...)

With automatic cert chaining, which is enabled by default in OpenSSL, a trust chain will be built for the cert on every handshake (see OpenSSL and BoringSSL, which also caches the chain). If this behaviour is disabled, which is the default in BoringSSL, then the certificate chain will not be automatically built from certs that are available in the CA list.

I think changing this behaviour in node—to not build these chains automatically—has the potential to be fairly disruptive, since it changes core APIs (https and tls) in a subtly breaking way. That said, there is a simple workaround, which is to build certificate chains ahead of time.

The same commit suggests that the flag will be removed altogether in the future. How would we compensate for that?

See boringssl/42, which is closed as WontFix, since wpa_supplicant depends on the auto-chaining feature too. So its removal from BoringSSL seems unlikely. Even if it were removed from BoringSSL, since Node.js doesn't officially support BoringSSL, I imagine there'd be no work for the Node.js team to do at all :)

Is this the only required change to make node work with BoringSSL?

In Electron, the only other changes we need to get node to compile with BoringSSL are a few #defines.
That said, I'm not an expert on the differences between BoringSSL and OpenSSL, and the full scope of what you mean when you say "work" is beyond me. OpenSSL is a massive library with a lot of functionality. But it compiles and runs and serves HTTPS—even without this PR!

[tniessen]

We most likely won't support BoringSSL officially (which isn't recommended by BoringSSL anyway). Personally, I am okay with merging this with a comment explaining why this line is necessary and maybe also a preprocessor directive to skip this for non-BoringSSL builds. Pinging @nodejs/tsc for opinions.

[Trott]

No opinion on the change, but without at least a comment explaining the line, this risks being removed in a refactor at a later date.

[ryzokuken]

CI: https://ci.nodejs.org/job/node-test-pull-request/17296/

[addaleax]

New CI: https://ci.nodejs.org/job/node-test-pull-request/17325/

[bnoordhuis]

LGTM. CI failure is infrastructural. I'd say this PR is ready to land.

[tniessen]

Landed in 7dae872.