tls: Add PSK support

[lundibundi]

Basically #14978.
Wanted to push to that branch but started with a rebase which resulted in github closing the PR and disallowing me to push there.

Add the pskCallback client/server option, which resolves an identity
or identity hint to a pre-shared key.

Add the pskIdentityHint server option to set the identity hint for the
ServerKeyExchange message.

Co-authored-by: Chris Osborn chris.osborn@sitelier.com
Co-authored-by: stephank gh@stephank.nl
Co-authored-by: Taylor Zane Glaeser tzglaeser@gmail.com

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Additional changes:

• removed ifdef check for openssl < 1.0.0 as we don't support that
• hardened args/return value checks in client/server psk callbacks
• small style refactorings in _tsl_common.js
• updated docs according to new styles/linting

---

Edit: note for all reviewers, thanks for reviewing, though this is currently waiting on @taylorzane, I'm not sure if he will have time to finish this.
* If he does have time then we will either float those comments and changes to #14978 or I'll just give him push access to this branch.
* If he doesn't have time for this then I'll pick it up and address comments/review more thoroughly.

Initially, I've only just rebased this on master and did simple cleanups without changing much.
Adding 'In Progress' to be verbose.

I'll proceed with this now.

Addressed review comments and updated PR description.

[nodejs-github-bot]

@lundibundi build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1080/pipeline

[tniessen]

cc @nodejs/crypto

Why is the PSK passed as a hexadecimal string instead of a buffer?

[lundibundi]

Updated initial post on the status of this PR.

[lundibundi]

It's been 25 days already 😮, sorry for the delay, wanted to get back to this in a week or two.
I'll proceed with this now.

[lundibundi]

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

[lundibundi]

@tniessen I assume that's due to https://tools.ietf.org/html/rfc4279#section-5.4

5.4. Requirements for Management Interfaces
In the absence of an application profile specification specifying
otherwise, a management interface for entering the PSK and/or PSK
identity MUST support the following:
...

• Entering PSKs up to 64 octets in length as ASCII strings and in
  hexadecimal encoding.

Though perhaps this should be changed to use Buffer (as strings are utf8)?

[sam-github]

Its a good feature to have, thank you.

I feel that there is some lack of clarity in the types. Some things that are null terminated strings are Buffer in the API, some things that are arbitrary binary data are hex strings, etc. But perhaps I misunderstand, please see my comments, and review the API.

[lundibundi]

Currently, we cannot connect to a server using PSK without rejectUnauthorized: false as I assume due to

node/src/node_crypto.cc

Lines 2150 to 2154 in e35f671

	// XXX(bnoordhuis) The UNABLE_TO_GET_ISSUER_CERT error when there is no
	// peer certificate is questionable but it's compatible with what was
	// here before.
	long x509_verify_error = // NOLINT(runtime/int)
	X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT;

and if we remove that then, later on, we will get

node/lib/tls.js

Lines 236 to 238 in e35f671

	} else {
	reason = 'Cert is empty';
	}

(because, obviously, there is no certificate).
@bnoordhuis could you maybe help (as that was your comment initially).
I'm not sure how to procceed with this one.
(refs #23188 (comment))

[lundibundi]

ping @bnoordhuis PTAL #23188 (comment) and #23188 (comment)).
ping @sam-github @addaleax @vsemozhetbyt.

[sam-github]

@lundibundi its quite a bit of work to verify whether you have addressed all comments. You didn't address https://github.com/nodejs/node/pull/23188/files#r229014173 or https://github.com/nodejs/node/pull/23188/files#r228850947. It doesn't look like the identity hint was constrained to utf-8, it still accepts Buffer/arbitrary binary data (which will get truncated to the first NUL byte by ossl). I don't have the time today to go through every comment and re-read the diff to check what you did to address the comments.

You can make re-review easier by following up on all comments with a thumbs up icon, or a statement saying you agree and will address, or some other indication of what happened.

[Trott]

What's the status of this one? In progress? Stalled? Ready for reviews? @lundibundi @sam-github

[lundibundi]

@Trott @sam-github, sorry for the delay, was swamped at work.
At first, I wanted to wait for the feedback on 'make hint Unicode' and 'certificate checking', but I've gone ahead and changed those now.

Could someone from @nodejs/crypto take a look at https://github.com/nodejs/node/pull/23188/files#diff-801e3948990f4965a8ea4aca4a423864R2264 as I'm not sure if those changes are ok.

This is ready for review.

[lundibundi]

Rebased on master just in case.

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

ping @sam-github @addaleax @bnoordhuis @vsemozhetbyt.

[sam-github]

There are some nits, and we have to sort out the oddity with the hex-encoded args.

#23188 (comment) is also blocking - we can't release this telling people to provide rejectUnauthorized as an argument, that would make client auth completely insecure. Better to figure out another way, or to disallow mixing PSK and requestCert or PSK and any non-PSK ciphers.

[sam-github]

This is a bug. If openssl allows a client to have both a PSK cipher suite configured, and a non-PSK certificate-based authentication configured so the server can choose which one to use, then the connection should be secure. disabling rejectUnauthorized means the cert-based auth won't happen, and the connection will be insecure. I don't see how we can release this feature with a security hole like this.

If this is fundamentally a problem (bug?) with openssl, we can raise with them, and then perhaps work harder in this PR to make it not possible for a server to both request a client certificate, and enable PSK ciphers.

It is also possible that this is something that could be fixed in our code, where we are validating a cert when we should not be when PSK was used.

You said to @addaleax that you would investigate this further. Have you? If you can't figure out what's going on, I could take a shot at looking at this.

[lundibundi]

Sorry, I forgot to remove this from here. Refer to #23188 (comment) and
https://github.com/nodejs/node/pull/23188/files#diff-f6e3a86962eaf0897ab59e88b418e64fR986

    It will be necessary to provide a custom [`tls.checkServerIdentity()`][]
    for the connection as the default one will try to check hostname/IP
    of the server against the certificate but that's not applicable for PSK
    because there won't be a certificate present.

[sam-github]

https://wiki.openssl.org/index.php/TLS1.3#PSKs

I'm reviewing what we'll need for TLSv1.3 support, and came across the above.

I haven't looked really closely, yet, but my initial impression is that while some more work might be necessary to support PSKs with TLS1.3, that it wouldn't cause the API here to break, though some aspects might become optional (identity hints).

It would be unfortunate to add a new API and shortly after have to change it, so even though TLS1.3 isn't in Node.js yet, lets make sure we know the impact.

[lundibundi]

@sam-github

#23188 (comment) is also blocking - we can't release this telling people to provide rejectUnauthorized as an argument, that would make client auth completely insecure. Better to figure out another way, or to disallow mixing PSK and requestCert or PSK and any non-PSK ciphers.

This is resolved with
https://github.com/nodejs/node/blob/ab82ec5af640a6bf8000711951756d0aaf43e66c/src/node_crypto.cc#L2273-L2278
and the need to provide custom (or empty) checkServerIdentity but I want someone more experienced with security and TLS to take a look.

As for the TLS1.3, this should work as is. Based on the docs of the methods we use here (in openssl) they have kept the current methods as backups if the new methods are not implemented.

[lundibundi]

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

[lundibundi]

So I changed the server pskCallback to make it possible to associate the socket with the identity if needed. Docs changed accordingly.

Also, this could use a few more reviews, ping @nodejs/crypto, maybe @addaleax @vsemozhetbyt.

/cc @sam-github

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[BridgeAR]

Woohoo! This finally landed in f8d7e22 after more than one year! 🎉

@lundibundi awesome work