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

crypto: add pfx certs as CA certs too #5109

Closed
wants to merge 3 commits into from

Conversation

indutny
Copy link
Member

@indutny 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
Copy link
Member Author

indutny commented Feb 5, 2016

cc @nodejs/crypto

@indutny
Copy link
Member Author

indutny commented Feb 5, 2016

@indutny 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
Copy link
Member Author

indutny commented Feb 5, 2016

Probably needs to land on v5 too.

@@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, force pushed.

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
`sk_X509_pop_free` should be used instead of `sk_X509_free` to free all
items in queue too, not just the queue itself.
@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

LGTM

@indutny
Copy link
Member Author

indutny commented Feb 7, 2016

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

@shigeki
Copy link
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.

@indutny
Copy link
Member Author

indutny commented Feb 8, 2016

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

@shigeki
Copy link
Contributor

shigeki commented Feb 8, 2016

@indutny The test is fine. LGTM.

@indutny
Copy link
Member Author

indutny commented Feb 8, 2016

Landed in 23196fe and 106c6cf, thank you!

@indutny indutny closed this Feb 8, 2016
@indutny indutny deleted the fix/gh-5100 branch February 8, 2016 19:43
indutny added a commit that referenced this pull request Feb 8, 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
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
`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
Copy link
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...

shigeki pushed a commit to shigeki/node that referenced this pull request Feb 9, 2016
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 pushed a commit that referenced this pull request Feb 9, 2016
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
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
`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
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
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
`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
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
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
`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
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
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
`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
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 MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 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
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
`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
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
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
`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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants