crypto: load PFX chain the same way as regular one #4165
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 ;) |
| // possibly followed by a sequence of CA certificates that should be | ||
| // sent to the peer in the Certificate message. | ||
| // | ||
| // Taken from OpenSSL - editted for style. |
|
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. |
| int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, | ||
| BIO* in, | ||
| X509* x, | ||
| STACK_OF(X509)* extra_certs, | ||
| X509** cert, | ||
| X509** issuer) { |
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 |
| x = PEM_read_bio_X509_AUX(in, nullptr, CryptoPemCallback, nullptr); | ||
|
|
||
| if (x == nullptr) { | ||
| SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_PEM_LIB); |
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 <[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]>
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.
Fix: #4127