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.

Sign up for GitHub

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

Jump to bottom

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

Closed

indutny wants to merge 3 commits into nodejs:master from indutny:fix/gh-5100

Member

indutny commented Feb 5, 2016

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

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

Closed

Member Author

indutny commented Feb 5, 2016

cc @nodejs/crypto

Member Author

indutny commented Feb 5, 2016

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

indutny added tls c++ lts-watch-v4.x labels

Member Author

indutny commented Feb 5, 2016

Probably needs to land on v5 too.

indutny reviewed

View reviewed changes

src/node_crypto.cc

    
            @@ -990,7 +1001,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
          
                if (cert != nullptr)

                  X509_free(cert);

                if (extra_certs != nullptr)

                  sk_X509_free(extra_certs);

                  sk_X509_pop_free(extra_certs, X509_free);

Member Author

indutny Feb 5, 2016

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

Contributor

MylesBorins Feb 5, 2016

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

Member Author

indutny Feb 6, 2016

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

Member

jasnell commented Feb 7, 2016

LGTM

Member Author

indutny commented Feb 7, 2016

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

Contributor

shigeki commented Feb 8, 2016

@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

Member Author

indutny commented Feb 8, 2016

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

Contributor

shigeki commented Feb 8, 2016

@indutny The test is fine. LGTM.

MylesBorins mentioned this pull request

Release proposal: v5.5.1 (Stable) #5141

Closed

Member Author

indutny commented Feb 8, 2016

Landed in 23196fe and 106c6cf, thank you!

indutny closed this

indutny deleted the fix/gh-5100 branch

February 8, 2016 19:43

indutny added a commit that referenced this pull request


          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


          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>

Member

Trott commented Feb 8, 2016

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


          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

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

Closed

shigeki pushed a commit that referenced this pull request


          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


          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


          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


          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


          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


          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


          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


          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


          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

MylesBorins pushed a commit that referenced this pull request


          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


          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


          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


          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

V4.4.0 proposal #5301

Merged

MylesBorins pushed a commit that referenced this pull request


          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


          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


          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


          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


          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


          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

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

Labels

c++ tls