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: throw when unable to set ciphers #21557

Merged
merged 0 commits into from
Jul 3, 2018

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jun 27, 2018

This PR both clarifies the ciphers option for tls connections and makes tls.createSecureContext() throw (with an OpenSSL error message) if ciphers is not acceptable.

It may be worth mimicking this same behavior for other setter functions. While most others at least throw a generic error (while SetCiphers() silently ignored the return value when setting ciphers), I think tweaking those to include the OpenSSL error may be more helpful.

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

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

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 27, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Jun 27, 2018
@mscdex mscdex removed the crypto Issues and PRs related to the crypto subsystem. label Jun 27, 2018
@mscdex mscdex force-pushed the tls-throw-error-set-ciphers branch from 7c7709e to bfbeb1d Compare June 27, 2018 07:16
@mscdex mscdex changed the title Tls throw error set ciphers tls: throw when unable to set ciphers Jun 27, 2018
if (!err) {
return env->ThrowError("Failed to set ciphers");
}
return ThrowCryptoError(env, err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise this is an existing pattern but you can just return ThrowCryptoError(env, err, "Failed to set ciphers"); - it will use the message when err == 0. Either way is fine though, it's the kind of cleanup that can wait for another PR.

@mscdex
Copy link
Contributor Author

mscdex commented Jun 29, 2018

/cc @nodejs/tsc

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mscdex mscdex force-pushed the tls-throw-error-set-ciphers branch from bfbeb1d to 132a188 Compare July 3, 2018 18:42
@mscdex mscdex removed the tsc-review label Jul 3, 2018
@mscdex
Copy link
Contributor Author

mscdex commented Jul 3, 2018

One last CI run before landing: https://ci.nodejs.org/job/node-test-pull-request/15714/

@mscdex mscdex closed this Jul 3, 2018
@mscdex mscdex force-pushed the tls-throw-error-set-ciphers branch from 132a188 to a15ea5d Compare July 3, 2018 20:31
mscdex added a commit to mscdex/io.js that referenced this pull request Jul 3, 2018
PR-URL: nodejs#21557
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@mscdex mscdex merged commit a15ea5d into nodejs:master Jul 3, 2018
@mscdex mscdex deleted the tls-throw-error-set-ciphers branch July 3, 2018 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants