crypto: load PFX chain the same way as regular one#4165
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: nodejs#4127
indutny commented Dec 5, 2015
cc @nodejs/crypto |
indutny added tls indutny commented Dec 6, 2015
Actually just added the test that demonstrates the problem, feel free to review it now ;) |
indutny commented Dec 6, 2015
src/node_crypto.cc Outdated
indutny commented Dec 6, 2015
There was failure on CI FIPS bot, fixed in latest commit. Running CI again: https://ci.nodejs.org/job/node-test-pull-request/935/ |
indutny commented Dec 6, 2015
Gosh, messed it up. New CI: https://ci.nodejs.org/job/node-test-pull-request/936/ |
indutny commented Dec 6, 2015
CI seems to be green, failures are completely unrelated. |
There was a problem hiding this comment.
Can you add a CHECK_EQ(*issuer, nullptr) and maybe a CHECK_EQ(*cert, nullptr) while you're here?
There was a problem hiding this comment.
You are totally right. It needs something more complex though, since we don't prevent people from passing in both cert+key and pfx for TLS server.
indutny commented Dec 7, 2015
@bnoordhuis PTAL, should be better now. |
indutny commented Dec 9, 2015
@bnoordhuis ping ;) |
indutny commented Dec 11, 2015
erm @bnoordhuis or @nodejs/crypto : I need your LGTM ;) |
indutny commented Dec 14, 2015
Anyone? |
indutny commented Dec 17, 2015
ping @bnoordhuis @nodejs/crypto |
There was a problem hiding this comment.
I realize it's not new code but I believe PEM_read_bio_X509_AUX() also registers an error, but with tag ERR_LIB_PEM, not ERR_LIB_SSL.
There was a problem hiding this comment.
I would keep it as it is right now, just because it may make this PR a semver-major. Let's reconsider it later.
paddybyers commented Jan 3, 2016
Is it planned that this will be be included in 4.2.x as well as 5.x? The reason I ask is that I believe a side-effect of this change is to fix a memory leak which is relevant to production stability. I can raise an issue/PR against 4.2.x with a fix only for the memory leak if necessary. |
indutny commented Jan 4, 2016
@paddybyers yep, it has lts-watch tag ;) |
paddybyers commented Jan 4, 2016
I see, thanks :) |
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: nodejs#4127 PR-URL: nodejs#4165 Reviewed-By: Ben Noordhuis <[email protected]>
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: #4127 PR-URL: #4165 Reviewed-By: Ben Noordhuis <[email protected]>
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: #4127 PR-URL: #4165 Reviewed-By: Ben Noordhuis <[email protected]>
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: nodejs#4127 PR-URL: nodejs#4165 Reviewed-By: Ben Noordhuis <[email protected]>
5 participants
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.
Fix: #4127