tls: support reading multiple cas from one input by bnoordhuis · Pull Request #4099 · nodejs/node · GitHub

R=@nodejs/crypto

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

Let's do while (true) here, and break in loop if x509 === nullptr. I don't think that this deserves disabling lint rules.

The NOLINT works around what I suspect is a bug in the lint rule. If you put the while on a single line (and s/nullptr/0/ to keep it < 80 columns) it won't trigger.

idk, it doesn't look like a good code style to me, splitting while's condition between lines. That's just my opinion, though

/cc @trevnorris - you can be the arbitrator.

Is the following not possible?

  if (BIO* bio = LoadBIO(env, args[0])) {
    X509* x509;
    while (x509 = PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
     // ...

If not, I'd say how it is now isn't the prettiest but personally find it easier to logically follow. Which wins for me.

LGTM with lint comment.

LGTM

One more CI run: https://ci.nodejs.org/job/node-test-pull-request/954/

Writing `// NOLINT(whitespace/if-one-line)` was not possible because the directive was not listed in the list of known lint rules. You can now. PR-URL: nodejs#4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Before this commit you had to pass multiple CA certificates as an array of strings. For convenience you can now pass them as a single string. Fixes: nodejs#4096 PR-URL: nodejs#4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Conservatively tagging this semver-minor.

Writing `// NOLINT(whitespace/if-one-line)` was not possible because the directive was not listed in the list of known lint rules. You can now. PR-URL: #4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Before this commit you had to pass multiple CA certificates as an array of strings. For convenience you can now pass them as a single string. Fixes: #4096 PR-URL: #4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

@bnoordhuis shouldn't this have a doc change too?

Notable changes: * build: - Add support for Intel's VTune JIT profiling when compiled with --enable-vtune-profiling. For more information about VTune, see https://software.intel.com/en-us/node/544211. (Chunyang Dai) #3785. - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) #3962. * crypto: - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via crypto.createECDH(curve_name)) with private keys that are not dynamically generated via generateKeys(). The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the setPublicKey() method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) #3511. - Update root certificates from the current list stored maintained by Mozilla NSS. (Ben Noordhuis) #3951. - Multiple CA certificates can now be passed with the ca option to TLS methods as an array of strings or in a single new-line separated string. (Ben Noordhuis) #4099 * tools: Include a tick processor in core, exposed via the --prof-process command-line argument which can be used to process V8 profiling output files generated when using the --prof command-line argument. (Matt Loring) #4021. PR-URL: #4181

I don't know. The report was from someone who assumed a PEM with multiple certificates would Just Work(TM) and that's what I assumed as well.

But now that I look at tls.markdown, we say different things in different places.

* `ca` : Either a string or list of strings of PEM encoded CA
  certificates to trust.

vs.

  - `ca`: An array of strings or `Buffer`s of trusted certificates in PEM
    format. If this is omitted several well known "root" CAs will be used,
    like VeriSign. These are used to authorize connections.

I'll try to come up with a PR later today that harmonizes them.

Different sections said different things about what the `ca` argument should look like. This commit harmonizes them. Ref: nodejs#4099 PR-URL: nodejs#4213 Reviewed-By: Roman Reiss <me@silverwind.io>

@bnoordhuis if this is semver-minor should it land in lts?

/cc @rvagg @jasnell

IMO this is not a big enough deal to warrant shipping in v4, it can wait till v6.

Different sections said different things about what the `ca` argument should look like. This commit harmonizes them. Ref: #4099 PR-URL: #4213 Reviewed-By: Roman Reiss <me@silverwind.io>

based on @rvagg's comment I am removing lts-watch.

@jasnell feel free to reverse this

Different sections said different things about what the `ca` argument should look like. This commit harmonizes them. Ref: #4099 PR-URL: #4213 Reviewed-By: Roman Reiss <me@silverwind.io>

Should this be labeled dont-land-on-v4.x?

@Trott yep

I've taken the liberty/initiative to apply dont-land-on-v4.x. /cc @thealphanerd @jasnell in case that's all wrong...

+1, although semver-minor makes it unlikely that it'll be in scope anyway

Writing `// NOLINT(whitespace/if-one-line)` was not possible because the directive was not listed in the list of known lint rules. You can now. PR-URL: nodejs#4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Before this commit you had to pass multiple CA certificates as an array of strings. For convenience you can now pass them as a single string. Fixes: nodejs#4096 PR-URL: nodejs#4099 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * build: - Add support for Intel's VTune JIT profiling when compiled with --enable-vtune-profiling. For more information about VTune, see https://software.intel.com/en-us/node/544211. (Chunyang Dai) nodejs#3785. - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) nodejs#3962. * crypto: - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via crypto.createECDH(curve_name)) with private keys that are not dynamically generated via generateKeys(). The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the setPublicKey() method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) nodejs#3511. - Update root certificates from the current list stored maintained by Mozilla NSS. (Ben Noordhuis) nodejs#3951. - Multiple CA certificates can now be passed with the ca option to TLS methods as an array of strings or in a single new-line separated string. (Ben Noordhuis) nodejs#4099 * tools: Include a tick processor in core, exposed via the --prof-process command-line argument which can be used to process V8 profiling output files generated when using the --prof command-line argument. (Matt Loring) nodejs#4021. PR-URL: nodejs#4181

Different sections said different things about what the `ca` argument should look like. This commit harmonizes them. Ref: nodejs#4099 PR-URL: nodejs#4213 Reviewed-By: Roman Reiss <me@silverwind.io>

if the valid `ca` is the first item within the concatinated string then the bug addressed by nodejs#4099 was not getting exposed. This test makes sure the order of valid `ca` should not effect the expected behavior when multiple `ca` certs are concatinated.

bnoordhuis added the tls Issues and PRs related to the tls subsystem. label Dec 1, 2015

bnoordhuis mentioned this pull request Dec 1, 2015

TLS Root CA not recognized when the file contains two CAs #4096

Closed

indutny reviewed Dec 1, 2015
View reviewed changes

bnoordhuis force-pushed the fix4096 branch from 2515455 to c354045 Compare December 8, 2015 18:11

bnoordhuis added 2 commits December 8, 2015 22:01

bnoordhuis force-pushed the fix4096 branch from c354045 to 82e0974 Compare December 8, 2015 21:02

bnoordhuis closed this Dec 8, 2015

bnoordhuis deleted the fix4096 branch December 8, 2015 21:02

bnoordhuis merged commit 82e0974 into nodejs:master Dec 8, 2015

bnoordhuis added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v4.x labels Dec 8, 2015

rvagg mentioned this pull request Dec 9, 2015

Release proposal: v5.2.0 (Stable) #4181

Merged

bnoordhuis mentioned this pull request Dec 9, 2015

doc: harmonize description of ca argument #4213

Merged

MylesBorins removed the lts-watch-v4.x label Dec 29, 2015

Trott added the dont-land-on-v4.x label Feb 18, 2016

suryagh mentioned this pull request May 5, 2016

test: multiple cas works in any order #6606

Closed

3 tasks

tls: support reading multiple cas from one input by bnoordhuis · Pull Request #4099 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: support reading multiple cas from one input - #4099

tls: support reading multiple cas from one input#4099bnoordhuis merged 2 commits intonodejs:masternodejs/node:masterfrom bnoordhuis:fix4096bnoordhuis/io.js:fix4096Copy head branch name to clipboard

Conversation

bnoordhuis commented Dec 1, 2015

Uh oh!

indutny Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

indutny Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

trevnorris Dec 8, 2015

Choose a reason for hiding this comment

Uh oh!

indutny commented Dec 1, 2015

Uh oh!

jasnell commented Dec 3, 2015

Uh oh!

bnoordhuis commented Dec 8, 2015

Uh oh!

bnoordhuis commented Dec 8, 2015

Uh oh!

rvagg commented Dec 9, 2015

Uh oh!

bnoordhuis commented Dec 9, 2015

Uh oh!

bnoordhuis commented Dec 9, 2015

Uh oh!

MylesBorins commented Dec 15, 2015

Uh oh!

rvagg commented Dec 15, 2015

Uh oh!

MylesBorins commented Dec 29, 2015

Uh oh!

Trott commented Feb 18, 2016

Uh oh!

indutny commented Feb 18, 2016

Uh oh!

Trott commented Feb 18, 2016

Uh oh!

rvagg commented Feb 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tls: support reading multiple cas from one input#4099
bnoordhuis merged 2 commits into
nodejs:masterfrom
bnoordhuis:fix4096