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: multiple PFX support in createSecureContext #14793

Closed
wants to merge 7 commits into from

Conversation

djphoenix
Copy link
Contributor

Fixes: #14756

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
Affected core subsystem(s)

tls

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Aug 12, 2017
@djphoenix djphoenix force-pushed the tls-multi-pfx branch 2 times, most recently from 1d13a0b to 41ca43b Compare August 12, 2017 17:35
buffer: fs.readFileSync(`${common.fixturesDir}/keys/agent1-pfx.pem`),
passphrase: 'sample'
},
fs.readFileSync(`${common.fixturesDir}/keys/ec-pfx.pem`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add a third case for when an object is supplied with an encrypted key and the passphrase from options is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK (partially). Maybe add separate case out of "multi-pfx" test?

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 13, 2017
@@ -0,0 +1,71 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Member

Choose a reason for hiding this comment

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

The copyright and license header should not be added to new files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, removed

for (i = 0; i < options.pfx.length; i++) {
const pfx = options.pfx[i];
let buf;
if (pfx.buffer instanceof Buffer) {
Copy link
Member

Choose a reason for hiding this comment

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

Buffer.isBuffer()

Copy link
Member

Choose a reason for hiding this comment

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

Also, should this be limited to just Buffer instances? We likely should allow any Uint8Array

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK

const options = {
pfx: [
{
buffer: fs.readFileSync(`${common.fixturesDir}/keys/agent1-pfx.pem`),
Copy link
Member

Choose a reason for hiding this comment

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

Please use the new ../common/fixtures stuff... e.g.

const fixtures = require('../common/fixtures');
/*... */
{
  buffer: fixtures.readKey('agent1-pfx.pem')
}

const ecdsa = tls.connect(this.address().port, {
ciphers: 'ECDHE-ECDSA-AES256-GCM-SHA384',
rejectUnauthorized: false
}, function() {
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell in listen (L24) also? Or not necessary?

const rsa = tls.connect(server.address().port, {
ciphers: 'ECDHE-RSA-AES256-GCM-SHA384',
rejectUnauthorized: false
}, function() {
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall() here also

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

@djphoenix
Copy link
Contributor Author

Fixed FIPS failure (actually bad fixture). @jasnell another try?

@djphoenix
Copy link
Contributor Author

ping?

@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

Hey, sorry @djphoenix ... new CI here! https://ci.nodejs.org/job/node-test-pull-request/9862/

buf = crypto._toBuf(pfx.buffer);
} else {
buf = crypto._toBuf(pfx);
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not completely happy with the buffer check here.

I think it would be good to verify that the input is indeed a buffer and that does not happen here.

If the object attribute name would not be buffer it would be easy to distinguish the object from the buffer and you could write something like:

const raw = pfx.buf ? pfx.buf : pfx;
if (!ArrayBuffer.isView(raw))
  throw new Error("foobar");
const buf = crypto._toBuf(raw);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks as a good proposal. ACK.

doc/api/tls.md Outdated
@@ -932,10 +932,14 @@ changes:
-->

* `options` {Object}
* `pfx` {string|Buffer} Optional PFX or PKCS12 encoded private key and
certificate chain. `pfx` is an alternative to providing `key` and `cert`
* `pfx` {Buffer|Buffer[]|Object[]} Optional PFX or PKCS12 encoded private key
Copy link
Member

Choose a reason for hiding this comment

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

How come string is not supported anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PKCS12 is very rarely stored in PEM format, by default it's DER-encoded. But I forget about binary strings in JS. Will revert it, ACK.

@djphoenix
Copy link
Contributor Author

@jasnell CI failures seems like unrelated to changes.

@djphoenix
Copy link
Contributor Author

@BridgeAR fixed your picks. @jasnell seems like it's ready for another CI.

@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM, if it works.

@indutny
Copy link
Member

indutny commented Aug 31, 2017

cc @shigeki @bnoordhuis PTAL

Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

Fixes: nodejs#14756
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Still LGTM

@BridgeAR
Copy link
Member

BridgeAR commented Sep 4, 2017

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@djphoenix
Copy link
Contributor Author

Still waiting for @shigeki and @bnoordhuis

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

Landed in 372dc86

@BridgeAR BridgeAR closed this Sep 8, 2017
BridgeAR pushed a commit that referenced this pull request Sep 8, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: #14793
Fixes: #14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@BridgeAR it seems like @djphoenix was still waiting for a review from @shigeki and @bnoordhuis. Was there a reason it landed before?

@MylesBorins
Copy link
Contributor

Also fwiw I've landed this on v8.x-staging and it is on track to be released in the next 8.x release. Just wanted to confirm that this should be released before doing so

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: #14793
Fixes: #14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
@djphoenix
Copy link
Contributor Author

djphoenix commented Sep 11, 2017

@djphoenix was still waiting for a review

@MylesBorins not me, but @indutny tagged them before, so I pinged also. Not blocking for me (and for @BridgeAR also as I see).

@BridgeAR
Copy link
Member

@MylesBorins to me it looked like a trivial change that was good to go and there was no response since the review request for a week. And as it did not come from @djphoenix it felt more like a "nice to have" in this case. So I went ahead and landed it.

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 11, 2017 via email

MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: #14793
Fixes: #14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: #14793
Fixes: #14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@djphoenix djphoenix deleted the tls-multi-pfx branch September 11, 2017 23:50
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: #14793
Fixes: #14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Sep 12, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  #14875

* console:
  * Implement minimal `console.group()`.
  #14910

* deps:
  * upgrade libuv to 1.14.1
    #14866
  * update nghttp2 to v1.25.0
    #14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    #14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    #15034

* inspector:
  * Enable async stack traces
    #13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    #14369

* napi:
  * implement promise
    #14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    #14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    #14680

* tls:
  * multiple PFX in createSecureContext
    [#14793](#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: #15308
MylesBorins added a commit that referenced this pull request Sep 12, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  #14875

* console:
  * Implement minimal `console.group()`.
  #14910

* deps:
  * upgrade libuv to 1.14.1
    #14866
  * update nghttp2 to v1.25.0
    #14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    #14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    #15034

* inspector:
  * Enable async stack traces
    #13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    #14369

* napi:
  * implement promise
    #14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    #14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    #14680

* tls:
  * multiple PFX in createSecureContext
    [#14793](#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: #15308
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Add support for multiple PFX files in tls.createSecureContext.
Also added support for object-style PFX pass.

PR-URL: nodejs#14793
Fixes: nodejs#14756
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  nodejs#14875

* console:
  * Implement minimal `console.group()`.
  nodejs#14910

* deps:
  * upgrade libuv to 1.14.1
    nodejs#14866
  * update nghttp2 to v1.25.0
    nodejs#14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    nodejs#14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    nodejs#15034

* inspector:
  * Enable async stack traces
    nodejs#13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    nodejs#14369

* napi:
  * implement promise
    nodejs#14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    nodejs#14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    nodejs#14680

* tls:
  * multiple PFX in createSecureContext
    [nodejs#14793](nodejs#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: nodejs#15308
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

Release team were -1 on landing on v6.x, if you disagree let us know.

@djphoenix
Copy link
Contributor Author

@gibfahn I’m not sure, so let NodeJS team decide. For me it’s enough to have this feature in latest versions.

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.

None yet

10 participants