FazBrowse GitHub Viewer | Trending | | Home

Tools: [Download Repo ZIP] [Original HTTPS Page]

crypto: add `pfx` certs as CA certs too by indutny · Pull Request #5109 · nodejs/node · GitHub

crypto: add pfx certs as CA certs too - #5109

Closed

indutny wants to merge 3 commits into

nodejs:masterfrom

indutny:fix/gh-5100

indutny commented Feb 5, 2016

Copy link Copy Markdown

Member

According to documentation all certificates specified in pfx option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100

indutny mentioned this pull request Feb 5, 2016

TLS fails reading self-signed certificate on node 4.2.5+ #5100

Closed

indutny commented Feb 5, 2016

Copy link Copy Markdown

Member Author

cc @nodejs/crypto

indutny commented Feb 5, 2016

Copy link Copy Markdown

Member Author

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

indutny added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Feb 5, 2016

indutny commented Feb 5, 2016

Copy link Copy Markdown

Member Author

Probably needs to land on v5 too.

indutny reviewed Feb 5, 2016

View reviewed changes

Comment thread src/node_crypto.cc Outdated

indutny Feb 5, 2016

Copy link Copy Markdown

Member Author

This is unrelated bugfix for a memory leak, not sure if I should submit it separately. It won't have a test case anyway.

MylesBorins Feb 5, 2016

Copy link Copy Markdown

Contributor

might make sense to put as two commits for back-porting purposes... I don't see why they couldn't both come in on this PR though

indutny Feb 6, 2016

Copy link Copy Markdown

Member Author

Agree, force pushed.

indutny added 2 commits February 5, 2016 23:13

crypto: add pfx certs as CA certs too

c58197f

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: nodejs#5100

crypto: fix memory leak in LoadPKCS12

b5d9dcf

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

indutny force-pushed the fix/gh-5100 branch from e8a84cb to b5d9dcf Compare February 6, 2016 04:14

jasnell commented Feb 7, 2016

Copy link Copy Markdown

Member

LGTM

indutny commented Feb 7, 2016

Copy link Copy Markdown

Member Author

@jasnell thank you! May have an additional LGTM from @bnoordhuis or @shigeki, before landing?

shigeki commented Feb 8, 2016

Copy link Copy Markdown

Contributor

@indutny The fixes are LGTM but no tests. I've just made tests to check them as in
shigeki@1d0ae2a. How about this?

We should have changed the API to add a new option of cert_chain but it seems to be too late for now.

test

ad378a9

indutny commented Feb 8, 2016

Copy link Copy Markdown

Member Author

Ouch, I just forgot to include it in PR. Sorry! Included it.

shigeki commented Feb 8, 2016

Copy link Copy Markdown

Contributor

@indutny The test is fine. LGTM.

MylesBorins mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

indutny commented Feb 8, 2016

Copy link Copy Markdown

Member Author

Landed in 23196fe and 106c6cf, thank you!

indutny closed this Feb 8, 2016

indutny deleted the fix/gh-5100 branch February 8, 2016 19:43

indutny added a commit that referenced this pull request Feb 8, 2016

crypto: add pfx certs as CA certs too

23196fe

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

indutny added a commit that referenced this pull request Feb 8, 2016

crypto: fix memory leak in LoadPKCS12

106c6cf

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

Trott commented Feb 8, 2016

Copy link Copy Markdown

Member

Looks like this landed with a failing test in CI.

https://ci.nodejs.org/job/node-test-commit-linux-fips/834/nodes=ubuntu1404-64/console

So, uh, CI will fail every time...

This was referenced Feb 8, 2016

Enable and require use of node-accept-pull-request? nodejs/build#324

Closed

tools: disallow mixed spaces and tabs for indents #5135

Closed

shigeki pushed a commit to shigeki/node that referenced this pull request Feb 9, 2016

test: enable to work pkcs12 test in FIPS mode

6abe80f

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: nodejs#5144
Fix: nodejs#5109

shigeki mentioned this pull request Feb 9, 2016

test: enable to work pkcs12 test in FIPS mode #5150

Closed

shigeki pushed a commit that referenced this pull request Feb 9, 2016

test: enable to work pkcs12 test in FIPS mode

dccccbb

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

crypto: add pfx certs as CA certs too

131873d

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

crypto: fix memory leak in LoadPKCS12

e55dea9

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

test: enable to work pkcs12 test in FIPS mode

63e314a

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

crypto: add pfx certs as CA certs too

d9e934c

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

crypto: fix memory leak in LoadPKCS12

ec62789

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

rvagg pushed a commit that referenced this pull request Feb 9, 2016

test: enable to work pkcs12 test in FIPS mode

9dcc45e

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

crypto: add pfx certs as CA certs too

16313bf

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

crypto: fix memory leak in LoadPKCS12

91fe5a0

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 18, 2016

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

test: enable to work pkcs12 test in FIPS mode

0b63a85

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

crypto: add pfx certs as CA certs too

5d9c1cf

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

crypto: fix memory leak in LoadPKCS12

a7955d5

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins pushed a commit that referenced this pull request Feb 18, 2016

test: enable to work pkcs12 test in FIPS mode

46dc12b

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins mentioned this pull request Feb 18, 2016

V4.4.0 proposal #5301

Merged

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

crypto: add pfx certs as CA certs too

689e1fa

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

crypto: fix memory leak in LoadPKCS12

b3050a5

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: #5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

test: enable to work pkcs12 test in FIPS mode

ed6d47b

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: #5144
Fix: #5109
PR-URL: #5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

crypto: add pfx certs as CA certs too

c539504

According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: nodejs#5100
PR-URL: nodejs#5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

crypto: fix memory leak in LoadPKCS12

b1711da

`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.

PR-URL: nodejs#5109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

test: enable to work pkcs12 test in FIPS mode

31f7f11

The pfx file created by pkcs12 command of openssl causes an error in
FIPS mode because its certificate is encrypted with RC2 by default.
Adding `-descert` option resolves the error.

Fix: nodejs#5144
Fix: nodejs#5109
PR-URL: nodejs#5150
Reviewed-By: Rich Trott <rtrott@gmail.com>

This was referenced Apr 28, 2023

[Snyk] Fix for 114 vulnerabilities aliscco/alisco-node#338

Open

[Snyk] Fix for 4 vulnerabilities aliscco/alisco-node#349

Open

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters

Show hidden characters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.

Projects

Milestone

No milestone

Back | FazBrowse Home | New Git URL