crypto: load PFX chain the same way as regular one#4165
crypto: load PFX chain the same way as regular one#4165indutny wants to merge 11 commits intonodejs:masterfrom
Conversation
Load the certificate chain from the PFX file the same as we do it for a regular certificate chain. Fix: nodejs#4127
|
cc @nodejs/crypto |
indutny
added
tls
|
Actually just added the test that demonstrates the problem, feel free to review it now ;) |
src/node_crypto.cc
Outdated
|
There was failure on CI FIPS bot, fixed in latest commit. Running CI again: https://ci.nodejs.org/job/node-test-pull-request/935/ |
|
Gosh, messed it up. New CI: https://ci.nodejs.org/job/node-test-pull-request/936/ |
|
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.
|
@bnoordhuis PTAL, should be better now. |
|
@bnoordhuis ping ;) |
|
erm @bnoordhuis or @nodejs/crypto : I need your LGTM ;) |
|
Anyone? |
|
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.
|
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. |
|
@paddybyers yep, it has lts-watch tag ;) |
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 <info@bnoordhuis.nl>
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 <info@bnoordhuis.nl>
5 participants
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.
Fix: #4127