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 option to override signature algorithms #29598

Closed
wants to merge 1 commit into from

Conversation

@OYTIS
Copy link
Contributor

commented Sep 18, 2019

Passes the list down to SSL_CTX_set1_sigalgs_list.

My use case for this is using crypto hardware that has limited support for different algorithms (e.g. no RSA-PSS or no MD hashes) through an OpenSSL engine (enabled in another PR).

Can have other applications like hardening the server by disallowing short hashes, or disabling RSA-PKCS1 even when connecting via tls v1.2.

Copy link
Member

left a comment

Looks good code-wise, /cc @nodejs/crypto

doc/api/tls.md Show resolved Hide resolved
Copy link
Member

left a comment

Thanks, this has been requested a couple times.

doc/api/tls.md Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
doc/api/tls.md Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-tls-set-sigalgs.js Outdated Show resolved Hide resolved
test/parallel/test-tls-set-sigalgs.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Also, on freebsd:

17:33:56 not ok 2319 parallel/test-worker-message-port-message-before-close
17:33:56   ---
17:33:56   duration_ms: 120.81
17:33:56   severity: fail
17:33:56   exitcode: -15
17:33:56   stack: |-
17:33:56     timeout
17:33:56   ...

Probably is not related to this PR (don't have a FreeBSD environment to tell for sure unfortunately).

Copy link
Member

left a comment

The PR itself LGTM but I think it would be good to also have an API to query the signature algorithms.

src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-tls-set-sigalgs.js Outdated Show resolved Hide resolved
@sam-github

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

node.js is indeed inconsistently sloppy with its treatment of falsy values, and the code here is internally consistent, but the sloppiness has caused problems in various APIs (26af728), and is slowly getting better (04633ee), but better type-checking can be semver-major, so I'd prefer we not introduce new code that allows 0 and false to be accepted (and silently ignored) by an argument with a documented type of STRING.

@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from ba21877 to fce68a2 Sep 19, 2019
@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@bnoordhuis Thank you, I missed that function, added a better test.

Other fixes coming soon.

@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from fce68a2 to 598c2a0 Sep 20, 2019
@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

OK, I believe everything is fixed now. Arrays of sigalgs are still not supported though, I understand it was optional (and should better go together with the respective fix for ciphers). CI failure seems unrelated.

Copy link
Member

left a comment

A couple nits, but looks pretty good, almost ready to land.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

The travis failure looks completely unrelated, I kicked of a full CI.

@nodejs/streams , FYI:

=== release test-stream-writable-write-writev-finish ===
272Path: parallel/test-stream-writable-write-writev-finish
273--- stderr ---
274assert.js:129
275  throw err;
276  ^
277
278AssertionError [ERR_ASSERTION]: function should not have been called at /home/travis/build/nodejs/node/test/parallel/test-stream-writable-write-writev-finish.js:18
279    at Writable.mustNotCall (/home/travis/build/nodejs/node/test/common/index.js:429:12)
280    at Writable.emit (events.js:209:13)
281    at prefinish (_stream_writable.js:645:14)
282    at finishMaybe (_stream_writable.js:653:5)
283    at endWritable (_stream_writable.js:673:3)
284    at Writable.end (_stream_writable.js:596:5)
285    at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-stream-writable-write-writev-finish.js:23:12)
286    at Module._compile (internal/modules/cjs/loader.js:943:30)
287    at Object.Module._extensions..js (internal/modules/cjs/loader.js:960:10)
288    at Module.load (internal/modules/cjs/loader.js:796:32) {
289  generatedMessage: false,
290  code: 'ERR_ASSERTION',
291  actual: undefined,
292  expected: undefined,
293  operator: 'fail'
294}
295Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-stream-writable-write-writev-finish.js
@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from 598c2a0 to 0367cd5 Sep 20, 2019
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
doc/api/tls.md Show resolved Hide resolved
@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from 0367cd5 to 546e1e0 Sep 23, 2019
@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@addaleax Thank you for the review, all should be fixed now.

@addaleax

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

and please also add an entry to the top of the changes section for tls.createSecureContext() below.

I think this is still missing :) We generally document when a new option has been added, so that people can figure out what versions provide a certain functionality.

@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from 546e1e0 to 3d18720 Sep 23, 2019
@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Done, thank you.

Copy link
Member

left a comment

@sam-github Can you take another look? This seems ready to me.

doc/api/tls.md Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

Passes the list down to SSL_CTX_set1_sigalgs_list.

Option to get the list of shared signature algorithms
from a TLS socket added as well for testing.

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>
@OYTIS OYTIS force-pushed the OYTIS:sigalgs branch from 3d18720 to 2b976ab Sep 23, 2019
@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

The Jenkins failure is because of the error @addaleax pointed me to, needs restarting.

@nodejs-github-bot

This comment has been minimized.

@OYTIS

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Not sure what these *linked* tests are, but it seems these results are also old.

@Trott

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

Landed in 0c32ca9

@Trott Trott closed this Sep 24, 2019
Trott added a commit that referenced this pull request Sep 24, 2019
Passes the list down to SSL_CTX_set1_sigalgs_list.

Option to get the list of shared signature algorithms
from a TLS socket added as well for testing.

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

PR-URL: #29598
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

Thanks for the contribution! 🎉

BridgeAR added a commit that referenced this pull request Sep 24, 2019
Passes the list down to SSL_CTX_set1_sigalgs_list.

Option to get the list of shared signature algorithms
from a TLS socket added as well for testing.

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

PR-URL: #29598
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR added a commit that referenced this pull request Sep 24, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512
@BridgeAR BridgeAR referenced this pull request Sep 24, 2019
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Passes the list down to SSL_CTX_set1_sigalgs_list.

Option to get the list of shared signature algorithms
from a TLS socket added as well for testing.

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

PR-URL: #29598
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
BridgeAR added a commit that referenced this pull request Sep 25, 2019
Notable changes:

* crypto:
  * Add `oaepLabel` option #29489
* deps:
  * Update V8 to 7.7.299.11 #28918
    * More efficient memory handling
    * Stack trace serialization got faster
    * The `Intl.NumberFormat` API gained new functionality
    * For more information: https://v8.dev/blog/v8-release-77
* events:
  * Add support for `EventTarget` in `once`
    #29498
* fs:
  * Expose memory file mapping flag `UV_FS_O_FILEMAP`
    #29260
* inspector:
  * New API - `Session.connectToMainThread`
    #28870
* process:
  * Initial SourceMap support via `env.NODE_V8_COVERAGE`
    #28960
* stream:
  * Make `_write()` optional when `_writev()` is implemented
    #29639
* tls:
  * Add option to override signature algorithms
    #29598
* util:
  * Add `encodeInto` to `TextEncoder`
    #29524
* worker:
  * The `worker_thread` module is now stable
    #29512

PR-URL: #29695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.