tls: multiple PFX support in createSecureContext#14793

djphoenix · 2017-08-12T17:12:41Z

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

mscdex · 2017-08-12T18:34:37Z

test/parallel/test-tls-multi-pfx.js

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.

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

jasnell · 2017-08-13T22:15:00Z

test/parallel/test-tls-multi-pfx.js

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

ACK, removed

jasnell · 2017-08-23T18:33:19Z

lib/_tls_common.js

Buffer.isBuffer()

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

see #14978 (comment) :D

jasnell · 2017-08-23T18:35:12Z

test/parallel/test-tls-multi-pfx.js

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

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

jasnell · 2017-08-23T18:35:59Z

test/parallel/test-tls-multi-pfx.js

common.mustCall() here

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

jasnell · 2017-08-23T18:36:17Z

test/parallel/test-tls-multi-pfx.js

common.mustCall() here also

jasnell · 2017-08-23T19:00:44Z

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

djphoenix · 2017-08-23T21:43:28Z

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

djphoenix · 2017-08-28T13:28:19Z

ping?

jasnell · 2017-08-29T05:10:15Z

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

BridgeAR · 2017-08-29T06:23:00Z

lib/_tls_common.js

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);

Looks as a good proposal. ACK.

BridgeAR · 2017-08-29T06:23:03Z

doc/api/tls.md

How come string is not supported anymore?

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 · 2017-08-29T08:49:54Z

@jasnell CI failures seems like unrelated to changes.

djphoenix · 2017-08-29T09:32:20Z

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

jasnell · 2017-08-29T17:06:04Z

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

indutny

LGTM, if it works.

indutny · 2017-08-31T17:09:26Z

cc @shigeki @bnoordhuis PTAL

Add support for multiple PFX files in tls.createSecureContext. Also added support for object-style PFX pass. Fixes: nodejs#14756

BridgeAR

Still LGTM

BridgeAR · 2017-09-04T00:11:43Z

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

indutny

LGTM

djphoenix · 2017-09-06T19:25:10Z

Still waiting for @shigeki and @bnoordhuis

BridgeAR · 2017-09-08T00:49:16Z

Landed in 372dc86

MylesBorins · 2017-09-10T02:44:09Z

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

MylesBorins · 2017-09-10T02:45:11Z

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

djphoenix · 2017-09-11T00:24:45Z

@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 · 2017-09-11T01:46:32Z

@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 · 2017-09-11T02:35:46Z

Sgtm Thanks for responding. Was just trying to figure out what was going on.

…

On Sep 11, 2017 3:46 AM, "Ruben Bridgewater" ***@***.***> wrote: @MylesBorins <https://github.com/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 <https://github.com/djphoenix> it felt more like a "nice to have" in this case. So I went ahead and landed it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14793 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV-fzYkngjHFw1rNLe5Al16O7ZJxyks5shJD-gaJpZM4O1d65> .

gibfahn · 2018-01-15T22:16:11Z

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

djphoenix · 2018-01-16T07:21:21Z

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

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Aug 12, 2017

djphoenix force-pushed the tls-multi-pfx branch 2 times, most recently from 1d13a0b to 41ca43b Compare August 12, 2017 17:35

mscdex reviewed Aug 12, 2017

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 13, 2017

jasnell reviewed Aug 13, 2017

View reviewed changes

djphoenix force-pushed the tls-multi-pfx branch from 41ca43b to 581acf9 Compare August 14, 2017 10:48

jasnell reviewed Aug 23, 2017

View reviewed changes

test/parallel/test-tls-multi-pfx.js Outdated

Copy link

Member

jasnell Aug 23, 2017

Choose a reason for hiding this comment

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

common.mustCall() here also

djphoenix force-pushed the tls-multi-pfx branch from 78a26e9 to 3465940 Compare August 23, 2017 18:59

BridgeAR reviewed Aug 29, 2017

View reviewed changes

djphoenix force-pushed the tls-multi-pfx branch from 0e32f76 to d0b000e Compare August 29, 2017 09:30

jasnell requested review from indutny and shigeki August 29, 2017 17:06

BridgeAR approved these changes Aug 31, 2017

View reviewed changes

indutny approved these changes Aug 31, 2017

View reviewed changes

djphoenix added 4 commits September 2, 2017 00:35

tls: multiple PFX in createSecureContext

21e2413

Add support for multiple PFX files in tls.createSecureContext. Also added support for object-style PFX pass. Fixes: nodejs#14756

Fix buffer type check

a389780

Use fixtures API in test

f8fdff4

Use mustCall for test callbacks

f2607b3

BridgeAR approved these changes Sep 2, 2017

View reviewed changes

jasnell approved these changes Sep 5, 2017

View reviewed changes

indutny approved these changes Sep 5, 2017

View reviewed changes

BridgeAR closed this Sep 8, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

djphoenix deleted the tls-multi-pfx branch September 11, 2017 23:50

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

Uh oh!

Conversation

djphoenix commented Aug 12, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

djphoenix commented Aug 23, 2017

Uh oh!

djphoenix commented Aug 28, 2017

Uh oh!

jasnell commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djphoenix commented Aug 29, 2017

Uh oh!

djphoenix commented Aug 29, 2017

Uh oh!

jasnell commented Aug 29, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny commented Aug 31, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 4, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

djphoenix commented Sep 6, 2017

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

djphoenix commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 11, 2017

Uh oh!

MylesBorins commented Sep 11, 2017 via email

Uh oh!

djphoenix commented Sep 11, 2017 •

edited

Loading