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: support reading multiple cas from one input #4099

Merged
merged 2 commits into from
Dec 8, 2015

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis added the tls Issues and PRs related to the tls subsystem. label Dec 1, 2015
X509_free(x509);
unsigned cert_count = 0;
if (BIO* bio = LoadBIO(env, args[0])) {
while (X509* x509 = // NOLINT(whitespace/if-one-line)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @trevnorris - you can be the arbitrator.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@indutny
Copy link
Member

indutny commented Dec 1, 2015

LGTM with lint comment.

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

LGTM

@bnoordhuis
Copy link
Member Author

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>
@bnoordhuis bnoordhuis closed this Dec 8, 2015
@bnoordhuis bnoordhuis deleted the fix4096 branch December 8, 2015 21:02
@bnoordhuis bnoordhuis merged commit 82e0974 into nodejs:master Dec 8, 2015
@bnoordhuis 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
@bnoordhuis
Copy link
Member Author

Conservatively tagging this semver-minor.

bnoordhuis added a commit that referenced this pull request Dec 9, 2015
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>
bnoordhuis added a commit that referenced this pull request Dec 9, 2015
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>
@rvagg
Copy link
Member

rvagg commented Dec 9, 2015

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

rvagg added a commit that referenced this pull request Dec 9, 2015
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
rvagg added a commit that referenced this pull request Dec 9, 2015
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
@bnoordhuis
Copy link
Member Author

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.

@bnoordhuis
Copy link
Member Author

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.

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Dec 9, 2015
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>
@MylesBorins
Copy link
Contributor

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

/cc @rvagg @jasnell

@rvagg
Copy link
Member

rvagg commented Dec 15, 2015

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

bnoordhuis added a commit that referenced this pull request Dec 15, 2015
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>
@MylesBorins
Copy link
Contributor

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

@jasnell feel free to reverse this

bnoordhuis added a commit that referenced this pull request Dec 30, 2015
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>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
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>
@Trott
Copy link
Member

Trott commented Feb 18, 2016

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

@indutny
Copy link
Member

indutny commented Feb 18, 2016

@Trott yep

@Trott
Copy link
Member

Trott commented Feb 18, 2016

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

@rvagg
Copy link
Member

rvagg commented Feb 18, 2016

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

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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>
suryagh added a commit to suryagh/node that referenced this pull request May 6, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor 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