Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: Add PSK support #23188

Closed
wants to merge 3 commits into from
Closed

tls: Add PSK support #23188

wants to merge 3 commits into from

Conversation

@lundibundi
Copy link
Member

lundibundi commented Sep 30, 2018

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.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Sep 30, 2018

cc @nodejs/crypto

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

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@addaleax addaleax mentioned this pull request Sep 30, 2018
4 of 4 tasks complete
doc/api/tls.md Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-tls-psk-circuit.js Outdated Show resolved Hide resolved
test/parallel/test-tls-psk-client.js Outdated Show resolved Hide resolved
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Oct 2, 2018

Updated initial post on the status of this PR.

@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Oct 28, 2018

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 lundibundi force-pushed the lundibundi:pr-14978 branch from 3bb2498 to 6ff755f Oct 29, 2018
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Oct 29, 2018

@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Oct 29, 2018

@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)?

Copy link
Member

sam-github left a comment

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.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
lib/_tls_wrap.js Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
src/tls_wrap.cc Outdated Show resolved Hide resolved
test/parallel/test-tls-psk-circuit.js Outdated Show resolved Hide resolved
test/parallel/test-tls-psk-circuit.js Outdated Show resolved Hide resolved
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Oct 31, 2018

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

This comment has been minimized.

Copy link
Member Author

lundibundi commented Nov 10, 2018

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 13, 2018

@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.

@lundibundi lundibundi force-pushed the lundibundi:pr-14978 branch from cca4f8b to 2a4f20d Nov 25, 2018
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 28, 2018

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

@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Nov 28, 2018

@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 lundibundi force-pushed the lundibundi:pr-14978 branch from 2a4f20d to ab82ec5 Dec 9, 2018
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Dec 9, 2018

Copy link
Member

sam-github left a comment

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.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
with "decrypt_error" before it will be finished completely.
Note that PSK ciphers are disabled by default, and using TLS-PSK thus
requires explicitly specifying a cipher suite with the `ciphers` option.
Additionally, it may be necessary to disable `rejectUnauthorized` if a

This comment has been minimized.

Copy link
@sam-github

sam-github Dec 13, 2018

Member

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.

This comment has been minimized.

Copy link
@lundibundi

lundibundi Dec 17, 2018

Author Member

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.
lib/_tls_wrap.js Outdated Show resolved Hide resolved
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Dec 14, 2018

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

This comment has been minimized.

Copy link
Member Author

lundibundi commented Dec 17, 2018

@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

node/src/node_crypto.cc

Lines 2273 to 2278 in ab82ec5

} else {
// allow no-cert for PSK authentication
const SSL_CIPHER* curr_cipher = SSL_get_current_cipher(w->ssl_.get());
if (SSL_CIPHER_get_auth_nid(curr_cipher) == NID_auth_psk)
return args.GetReturnValue().SetNull();
}

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 lundibundi force-pushed the lundibundi:pr-14978 branch 2 times, most recently from 70557ee to 6a52fad Dec 17, 2018
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Dec 18, 2018

@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Nov 24, 2019

@latysheff done, added a small note in the main description of PSK, PTAL.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Nov 25, 2019

@latysheff

This comment has been minimized.

Copy link

latysheff commented Nov 25, 2019

@lundibundi I'm satisfied, thanks :)

Awaiting this PR to be accepted...

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 26, 2019

@latysheff

This comment has been minimized.

Copy link

latysheff commented Nov 28, 2019

Is it possible and would it be reasonable to somehow tie identity and the socket being established?

@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Dec 3, 2019

@latysheff hmm, now that I look at it there seems to be no way to associate the identity with a specific socket which may be desirable. I think it is reasonable to change server pskCallback signature to take tlssocket as its first argument: pskCallback(socket, identity). Or maybe there is a better way? @sam-github WDYT?

lundibundi and others added 2 commits Oct 29, 2018
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>
@lundibundi lundibundi force-pushed the lundibundi:pr-14978 branch from 72f39d8 to 596296f Dec 21, 2019
@lundibundi

This comment has been minimized.

Copy link
Member Author

lundibundi commented Dec 21, 2019

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

This comment has been minimized.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/tls_wrap.cc Outdated Show resolved Hide resolved
src/tls_wrap.cc Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 25, 2019
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>

PR-URL: nodejs#23188
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 25, 2019

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

@lundibundi awesome work

@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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>

PR-URL: #23188
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
targos added a commit that referenced this pull request Jan 14, 2020
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>

PR-URL: #23188
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.