tls: Add PSK support by stephank · Pull Request #6701 · nodejs/node

stephank · 2016-05-11T22:30:58Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

tls, crypto

Description of change

This is another attempt at adding support for TLS-PSK, building on earlier work done in nodejs/node-v0.x-archive#1162

The interface is different though, because the node tls interface has also changed some, and then there's also my opinion.

The default cipher suite is not changed, and still includes !PSK.
Unlike the previous PR, this one does not magically enable TLS-PSK if PSK options are specified. So even when pskCallback is specified, you'd still have to specify ciphers explicitly for anything to happen.
The previous PR contained discussion about rejectUnauthorized and socket.authorized, but these have gotten much more explicit in recent node version it seems. They always pertain to PKI. In scenario's where PSK is used without PKI (most?), rejectUnauthorized: false must be explicitly set on the client.
The pskCallback on client and server have compatible signatures, even though they have slightly different purposes. Applications dealing with both sides should have an easier time this way.
Both JS and C++ check the openssl version and omit the functionality if not supported (like in the previous PR). Builds linked with an older openssl may ignore the new options silently.
Tests are adapted from the original PR. They used to use tls.createSecurePair, but have been rewritten to use tls.connect and tls.createServer.
Would personally love to see this in v6.x as well!

Ref: #3553, nodejs/node-v0.x-archive#1162

mscdex · 2016-05-12T04:27:37Z

/cc @nodejs/crypto

indutny · 2016-05-12T06:52:29Z

Why is this removed?

That's a whoops, which I'll remove. (But in my defense, that reference is no longer used. I was planning on adding a reference to the TLS-PSK RFC, but backed out because I noticed we're not doing that sort of thing anywhere.)

stephank · 2016-05-12T13:14:39Z

Thanks for the feedback! Won't get around to it today, but will take a look tomorrow.

shigeki · 2016-05-12T15:00:18Z

This also should throw.

shigeki · 2016-05-12T15:06:47Z

@stephank Honestly saying, I don't want to support PSK on openssl-1.0.2 because it supports only 4 ciphers as

 0x00,0x8D - PSK-AES256-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(256) Mac=SHA1
0x00,0x8C - PSK-AES128-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(128) Mac=SHA1
0x00,0x8A - PSK-RC4-SHA SSLv3 Kx=PSK Au=PSK Enc=RC4(128) Mac=SHA1
0x00,0x8B - PSK-3DES-EDE-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=3DES(168) Mac=SHA1

and no forward secret is supported. The forthcoming openssl-1.1.0 supports bunch of PSK ciphers includes forward secrets and AEAD. If you don't have so much necessary right now, I'd like to wait to support this until we support 1.1.0.

jasnell · 2016-05-12T15:16:59Z

Overall the docs change is good but I would prefer not to use you in the docs. Perhaps:
Additionally, it may be necessary to disable `rejectUnauthorized` when not intending to use certificates.

stephank · 2016-05-12T15:21:48Z

@shigeki Oh, that's neat! That openssl release is maybe even today? (https://www.openssl.org/policies/releasestrat.html) Will node.js upgrade to this on v6.x or just master?

Regardless of what's supported in OpenSSL, maybe we need to introduce a shorthand for selecting secure ciphersuites? I've been using PSK+HIGH in tests, but I'm not sure if HIGH is good enough. Perhaps tls.DEFAULT_PSK_CIPHERS, to match the current tls.DEFAULT_CIPHERS?

shigeki · 2016-05-12T15:42:24Z

@stephank They are working very hard for these days but I don't know when it is to be released.
Current ciphers of HIGH is not so much high we think. Only two of PSK-AES256-CBC-SHA and PSK-AES128-CBC-SHA are can be available for PSK now. CBC mode and non ECDHE/DHE key exchange are not expected to be used in the future. LTS of v6 will be supported until fall of 2018 so I wonder if non-forward secret and CBC (not AEAD) ciphers can be used until end of 2018. I can't say when we upgrade 1.1.0 right now but I think current supported PSK ciphers seem to be too old for the future use.

shigeki · 2016-05-12T15:58:25Z

@stefanmb Here are lists of PSK are to be supported in 1.1.0. So much ciphers are supported include GCM and ChaCha20-Poly1305. PSK will also be supported in the future version of TLS1.3. As far as I can checked, fortunately PSK APIs in 1.0.2 are not changed in 1.1.0 but its old ciphers are only worried me.

ohtsu@omb:openssl$ ./apps/openssl ciphers -V PSK+HIGH
0x00,0xAD - RSA-PSK-AES256-GCM-SHA384 TLSv1.2 Kx=RSAPSK Au=RSA Enc=AESGCM(256) Mac=AEAD
0x00,0xAB - DHE-PSK-AES256-GCM-SHA384 TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESGCM(256) Mac=AEAD
0xCC,0xAE - RSA-PSK-CHACHA20-POLY1305 TLSv1.2 Kx=RSAPSK Au=RSA Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xCC,0xAD - DHE-PSK-CHACHA20-POLY1305 TLSv1.2 Kx=DHEPSK Au=PSK Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xCC,0xAC - ECDHE-PSK-CHACHA20-POLY1305 TLSv1.2 Kx=ECDHEPSK Au=PSK Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xC0,0xAB - DHE-PSK-AES256-CCM8 TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESCCM8(256) Mac=AEAD
0xC0,0xA7 - DHE-PSK-AES256-CCM TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESCCM(256) Mac=AEAD
0x00,0xA9 - PSK-AES256-GCM-SHA384 TLSv1.2 Kx=PSK Au=PSK Enc=AESGCM(256) Mac=AEAD
0xCC,0xAB - PSK-CHACHA20-POLY1305 TLSv1.2 Kx=PSK Au=PSK Enc=CHACHA20/POLY1305(256) Mac=AEAD
0xC0,0xA9 - PSK-AES256-CCM8 TLSv1.2 Kx=PSK Au=PSK Enc=AESCCM8(256) Mac=AEAD
0xC0,0xA5 - PSK-AES256-CCM TLSv1.2 Kx=PSK Au=PSK Enc=AESCCM(256) Mac=AEAD
0x00,0xAC - RSA-PSK-AES128-GCM-SHA256 TLSv1.2 Kx=RSAPSK Au=RSA Enc=AESGCM(128) Mac=AEAD
0x00,0xAA - DHE-PSK-AES128-GCM-SHA256 TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESGCM(128) Mac=AEAD
0xC0,0xAA - DHE-PSK-AES128-CCM8 TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESCCM8(128) Mac=AEAD
0xC0,0xA6 - DHE-PSK-AES128-CCM TLSv1.2 Kx=DHEPSK Au=PSK Enc=AESCCM(128) Mac=AEAD
0x00,0xA8 - PSK-AES128-GCM-SHA256 TLSv1.2 Kx=PSK Au=PSK Enc=AESGCM(128) Mac=AEAD
0xC0,0xA8 - PSK-AES128-CCM8 TLSv1.2 Kx=PSK Au=PSK Enc=AESCCM8(128) Mac=AEAD
0xC0,0xA4 - PSK-AES128-CCM TLSv1.2 Kx=PSK Au=PSK Enc=AESCCM(128) Mac=AEAD
0xC0,0x38 - ECDHE-PSK-AES256-CBC-SHA384 TLSv1 Kx=ECDHEPSK Au=PSK Enc=AES(256) Mac=SHA384
0xC0,0x36 - ECDHE-PSK-AES256-CBC-SHA SSLv3 Kx=ECDHEPSK Au=PSK Enc=AES(256) Mac=SHA1
0x00,0xB7 - RSA-PSK-AES256-CBC-SHA384 TLSv1 Kx=RSAPSK Au=RSA Enc=AES(256) Mac=SHA384
0x00,0xB3 - DHE-PSK-AES256-CBC-SHA384 TLSv1 Kx=DHEPSK Au=PSK Enc=AES(256) Mac=SHA384
0x00,0x95 - RSA-PSK-AES256-CBC-SHA SSLv3 Kx=RSAPSK Au=RSA Enc=AES(256) Mac=SHA1
0x00,0x91 - DHE-PSK-AES256-CBC-SHA SSLv3 Kx=DHEPSK Au=PSK Enc=AES(256) Mac=SHA1
0xC0,0x9B - ECDHE-PSK-CAMELLIA256-SHA384 TLSv1 Kx=ECDHEPSK Au=PSK Enc=Camellia(256) Mac=SHA384
0xC0,0x99 - RSA-PSK-CAMELLIA256-SHA384 TLSv1 Kx=RSAPSK Au=RSA Enc=Camellia(256) Mac=SHA384
0xC0,0x97 - DHE-PSK-CAMELLIA256-SHA384 TLSv1 Kx=DHEPSK Au=PSK Enc=Camellia(256) Mac=SHA384
0x00,0xAF - PSK-AES256-CBC-SHA384 TLSv1 Kx=PSK Au=PSK Enc=AES(256) Mac=SHA384
0x00,0x8D - PSK-AES256-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(256) Mac=SHA1
0xC0,0x95 - PSK-CAMELLIA256-SHA384 TLSv1 Kx=PSK Au=PSK Enc=Camellia(256) Mac=SHA384
0xC0,0x37 - ECDHE-PSK-AES128-CBC-SHA256 TLSv1 Kx=ECDHEPSK Au=PSK Enc=AES(128) Mac=SHA256
0xC0,0x35 - ECDHE-PSK-AES128-CBC-SHA SSLv3 Kx=ECDHEPSK Au=PSK Enc=AES(128) Mac=SHA1
0x00,0xB6 - RSA-PSK-AES128-CBC-SHA256 TLSv1 Kx=RSAPSK Au=RSA Enc=AES(128) Mac=SHA256
0x00,0xB2 - DHE-PSK-AES128-CBC-SHA256 TLSv1 Kx=DHEPSK Au=PSK Enc=AES(128) Mac=SHA256
0x00,0x94 - RSA-PSK-AES128-CBC-SHA SSLv3 Kx=RSAPSK Au=RSA Enc=AES(128) Mac=SHA1
0x00,0x90 - DHE-PSK-AES128-CBC-SHA SSLv3 Kx=DHEPSK Au=PSK Enc=AES(128) Mac=SHA1
0xC0,0x9A - ECDHE-PSK-CAMELLIA128-SHA256 TLSv1 Kx=ECDHEPSK Au=PSK Enc=Camellia(128) Mac=SHA256
0xC0,0x98 - RSA-PSK-CAMELLIA128-SHA256 TLSv1 Kx=RSAPSK Au=RSA Enc=Camellia(128) Mac=SHA256
0xC0,0x96 - DHE-PSK-CAMELLIA128-SHA256 TLSv1 Kx=DHEPSK Au=PSK Enc=Camellia(128) Mac=SHA256
0x00,0xAE - PSK-AES128-CBC-SHA256 TLSv1 Kx=PSK Au=PSK Enc=AES(128) Mac=SHA256
0x00,0x8C - PSK-AES128-CBC-SHA SSLv3 Kx=PSK Au=PSK Enc=AES(128) Mac=SHA1
0xC0,0x94 - PSK-CAMELLIA128-SHA256 TLSv1 Kx=PSK Au=PSK Enc=Camellia(128) Mac=SHA256

indutny · 2016-05-12T18:34:02Z

Quite sarcastically, I would also add a note like this "PSK implementation in OpenSSL has many security flaws in the recent years, mostly because it is used only by a minority of applications. Please consider all alternative solutions before switching to PSK ciphers".

stephank · 2016-05-13T18:26:59Z

Rebased on master (ae17883) and tried to address as much feedback as I could. Some discussion still open, so referencing here:

Callback this context: tls: Add PSK support #6701 (comment)
Assert bad callback returns: tls: Add PSK support #6701 (comment)
And the toplevel discussion about OpenSSL 1.1.0.

PSK is now implemented in SecureContext, so createSecurePair should now also work if a context with PSK configured is provided.

stephank · 2016-05-14T12:48:19Z

I kinda skipped the step where throwing from the callback doesn't actually have any place to go to, it's a toplevel callback. So those errors should probably be emitted, but SecureContext is no emitter.

It looks like the abort is because after the handshake fails, TLSWrap::ClearOut tries to send an Alert to its peer, which down the line means creating a WriteWrap, but JS land is in an error state. Both the callback (TLSWrap::ClearOut -> SSL_read) and the alert (TLSWrap::ClearOut -> TLSWrap::EncOut) have the same parent, but my suspicion is grandparent TLSWrap::Cycle assumes no JS errors will ever happen at all within its loop.

I guess we do have to move it down to SSLWrap, emit error for all assertions and callback throws, and also handle error listener throws with FatalException. Does that sound right?

indutny · 2016-05-18T22:05:49Z

Not sure how important it is, but this will be a fatal exception if it will fail (i.e. node will exit).

jasnell · 2017-03-01T01:25:11Z

@stephank ... still want to pursue this?

@nodejs/crypto

Add the `pskCallback` client/server option, which resolves an identity or identity (hint) to a pre-shared key. The function signature on client and server is compatible. Add the `pskIdentity` server option to set the identity hint for the ServerKeyExchange message. Based on work by: Chris Osborn <chris.osborn@sitelier.com>

stephank · 2017-03-02T07:42:45Z

@jasnell Well, I'm no longer working on the side project that required this, but it's not a huge diff.

I've rebased on master and tried to address the remaining feedback from @indutny.

For pskCallback, the C++ checks are now asserts, and the real checks are done on the JS side, throwing exceptions there as necessary.

shigeki · 2017-03-02T15:25:04Z

As I wrote in the previous comment, I'm not in favor of adopting PSK mode in the current Node with openssl-1.0.2 because of its poor cipher suite support, especially it has no Forward Secrecy and no AEAD. If PSK is really needed, it is best to have it after upgrading openssl-1.1.0.

I'm now woking on upgrading 1.1.0 but I cannot say when it is deployed for it has several issues to be resolved.

taylorzane · 2017-08-22T13:22:15Z

Is this something that's still being pursued? It appears that 1.1.0 has been released for a few months now.

shigeki · 2017-08-22T13:43:05Z

@taylorzane We are waiting for FIPS support of OpenSSL-1.1. Do you have any use case to need PSK in TLS?

taylorzane · 2017-08-22T13:53:55Z

@shigeki My use case would be using it with SRP. Since Node.js doesn't have native support for TLS-SRP; an SRP handshake, plus TLS-PSK would be the next closest option.

shigeki · 2017-08-22T14:14:02Z

@taylorzane Unfortunately in the current openssl, SRP has only very poor ciphers such as CBC and SHA1. PSK in 1.1.x is much better.

$ ./apps/openssl ciphers -v SRP
SRP-DSS-AES-256-CBC-SHA SSLv3 Kx=SRP Au=DSS Enc=AES(256) Mac=SHA1
SRP-RSA-AES-256-CBC-SHA SSLv3 Kx=SRP Au=RSA Enc=AES(256) Mac=SHA1
SRP-AES-256-CBC-SHA SSLv3 Kx=SRP Au=SRP Enc=AES(256) Mac=SHA1
SRP-DSS-AES-128-CBC-SHA SSLv3 Kx=SRP Au=DSS Enc=AES(128) Mac=SHA1
SRP-RSA-AES-128-CBC-SHA SSLv3 Kx=SRP Au=RSA Enc=AES(128) Mac=SHA1
SRP-AES-128-CBC-SHA SSLv3 Kx=SRP Au=SRP Enc=AES(128) Mac=SHA1

…pets, to implement TLS-PSK support.

jasnell · 2017-08-24T23:06:22Z

Does this need to stay open or can we close in favor of #14978?

jasnell · 2017-09-15T23:13:49Z

Closing this in favor of #14978

…pets, to implement TLS-PSK support.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 11, 2016

mscdex added tls Issues and PRs related to the tls subsystem. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels May 11, 2016

indutny reviewed May 12, 2016
View reviewed changes

shigeki reviewed May 12, 2016
View reviewed changes

Comment threadsrc/tls_wrap.cc Outdated
shigekiMay 12, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also should throw.

jasnell reviewed May 12, 2016
View reviewed changes

stephank force-pushed the feat-tls-psk branch from efaf2ab to 5789760Compare May 13, 2016 18:14

indutny reviewed May 18, 2016
View reviewed changes

yaronyg mentioned this pull request Jul 6, 2016
Get PSK into mainline node thaliproject/Thali_CordovaPlugin#747
Open

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

stephank force-pushed the feat-tls-psk branch from 5789760 to ceae0bcCompare March 2, 2017 07:39

refack force-pushed the master branch from 16073c0 to fbe946bCompare April 14, 2017 04:11

DrMegavolt mentioned this pull request Aug 7, 2017
Fix error in capturing key/cert file path - connects to #367 ExpressGateway/express-gateway#368
Closed

taylorzane added a commit to taylorzane/node that referenced this pull request Aug 22, 2017
tls: Applied PRnodejs#6701to master, and updated outdated code snip…
2c16836
…pets, to implement TLS-PSK support.

taylorzane mentioned this pull request Aug 22, 2017
tls: Add PSK support (v9.0.0-pre) #14978
Closed
4 tasks

jasnell closed this Sep 15, 2017

taylorzane added a commit to taylorzane/node that referenced this pull request Oct 1, 2018
tls: Applied PRnodejs#6701to master, and updated outdated code snip…
fc041a8
…pets, to implement TLS-PSK support.

Uh oh!

Uh oh!

Conversation

stephank commented May 11, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented May 12, 2016

Uh oh!

indutnyMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

stephankMay 13, 2016

Choose a reason for hiding this comment

Uh oh!

stephank commented May 12, 2016

Uh oh!

shigekiMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

shigeki commented May 12, 2016

Uh oh!

jasnellMay 12, 2016

Choose a reason for hiding this comment

Uh oh!

stephank commented May 12, 2016

Uh oh!

shigeki commented May 12, 2016

Uh oh!

shigeki commented May 12, 2016

Uh oh!

indutny commented May 12, 2016

Uh oh!

stephank commented May 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephank commented May 14, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indutnyMay 18, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

stephank commented Mar 2, 2017

Uh oh!

shigeki commented Mar 2, 2017

Uh oh!

taylorzane commented Aug 22, 2017

Uh oh!

shigeki commented Aug 22, 2017

Uh oh!

taylorzane commented Aug 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shigeki commented Aug 22, 2017

Uh oh!

jasnell commented Aug 24, 2017

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stephank commented May 11, 2016•
edited
Loading

stephank commented May 13, 2016•
edited
Loading

stephank commented May 14, 2016•
edited
Loading

taylorzane commented Aug 22, 2017•
edited
Loading