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: load PFX chain the same way as regular one #4165

Closed
wants to merge 11 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 87 additions & 50 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
@@ -530,46 +530,32 @@ int SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) {
}


// Read a file that contains our certificate in "PEM" format,
// 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.
int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
BIO* in,
X509* x,
STACK_OF(X509)* extra_certs,
X509** cert,
X509** issuer) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a CHECK_EQ(*issuer, nullptr) and maybe a CHECK_EQ(*cert, nullptr) while you're here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

int ret = 0;
X509* x = nullptr;

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);
goto end;
}

ret = SSL_CTX_use_certificate(ctx, x);
int ret = SSL_CTX_use_certificate(ctx, x);

if (ret) {
// If we could set up our certificate, now proceed to
// the CA certificates.
X509 *ca;
int r;
unsigned long err;

if (ctx->extra_certs != nullptr) {
sk_X509_pop_free(ctx->extra_certs, X509_free);
ctx->extra_certs = nullptr;
}

while ((ca = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) {
for (int i = 0; i < sk_X509_num(extra_certs); i++) {
X509* ca = sk_X509_value(extra_certs, i);

// NOTE: Increments reference count on `ca`
r = SSL_CTX_add1_chain_cert(ctx, ca);

if (!r) {
X509_free(ca);
ret = 0;
*issuer = nullptr;
goto end;
}
// Note that we must not free r if it was successfully
@@ -582,16 +568,6 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
continue;
*issuer = ca;
}

// When the while loop ends, it's usually just EOF.
err = ERR_peek_last_error();
if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
ERR_clear_error();
} else {
// some real error
ret = 0;
}
}

// Try getting issuer from a cert store
@@ -608,8 +584,74 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
}

end:
if (x != nullptr)
if (ret && x != nullptr) {
*cert = x;
CRYPTO_add(&(*cert)->references, 1, CRYPTO_LOCK_X509);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't SSL_CTX_use_certificate() already do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We X509_free(cert_) separately in ~SSLContext

Copy link
Member

Choose a reason for hiding this comment

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

That's a lot of spooky action at a distance. This function is not even a member of that class.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that the OpenSSL API lacks this doesn't mean that no-one will ever want to use it. We also use this for issuer for quite a long time. How do you suggest to hold a reference to it, otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I could use X509_dup here, will it look better to you?

}
return ret;
}


// Read a file that contains our certificate in "PEM" format,
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/editted/edited/

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
BIO* in,
X509** cert,
X509** issuer) {
X509* x = nullptr;

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would keep it as it is right now, just because it may make this PR a semver-major. Let's reconsider it later.

return 0;
}

X509* extra = nullptr;
int ret = 0;
unsigned long err = 0;

// Read extra certs
STACK_OF(X509)* extra_certs = sk_X509_new_null();
if (extra_certs == nullptr) {
// XXX(indutny): Is there a need for SSLerr here?
Copy link
Member

Choose a reason for hiding this comment

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

sk_X509_new_null() doesn't push an error so...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

goto done;
}

while ((extra = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) {
if (sk_X509_push(extra_certs, extra))
continue;

// Failure, free all certs
goto done;
}
extra = nullptr;

// When the while loop ends, it's usually just EOF.
err = ERR_peek_last_error();
Copy link
Member

Choose a reason for hiding this comment

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

ERR_peek_last_error() is a bad idea according to #4221.

EDIT: I imagine you may need to call ERR_clear_error() at the start of this function if you want to take this approach? I don't think openssl has functions for checking the current error stack depth.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I will put ERR_clear_error there. Thanks for suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
ERR_clear_error();
} else {
// some real error
goto done;
}

ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs, cert, issuer);
if (!ret)
goto done;

done:
if (extra_certs != nullptr)
sk_X509_pop_free(extra_certs, X509_free);
if (extra != nullptr)
X509_free(extra);
if (x != nullptr)
X509_free(x);

return ret;
}

@@ -898,7 +940,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
PKCS12* p12 = nullptr;
EVP_PKEY* pkey = nullptr;
X509* cert = nullptr;
STACK_OF(X509)* extraCerts = nullptr;
STACK_OF(X509)* extra_certs = nullptr;
char* pass = nullptr;
bool ret = false;

@@ -924,27 +966,22 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
}

if (d2i_PKCS12_bio(in, &p12) &&
PKCS12_parse(p12, pass, &pkey, &cert, &extraCerts) &&
SSL_CTX_use_certificate(sc->ctx_, cert) &&
PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
SSL_CTX_use_certificate_chain(sc->ctx_,
cert,
extra_certs,
&sc->cert_,
&sc->issuer_) &&
SSL_CTX_use_PrivateKey(sc->ctx_, pkey)) {
// set extra certs
while (X509* x509 = sk_X509_pop(extraCerts)) {
if (!sc->ca_store_) {
sc->ca_store_ = X509_STORE_new();
SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
}

X509_STORE_add_cert(sc->ca_store_, x509);
SSL_CTX_add_client_CA(sc->ctx_, x509);
X509_free(x509);
}
ret = true;
}

if (pkey != nullptr)
EVP_PKEY_free(pkey);
if (cert != nullptr)
X509_free(cert);
sk_X509_free(extraCerts);

ret = true;
}
if (extra_certs != nullptr)
sk_X509_free(extra_certs);

PKCS12_free(p12);
BIO_free_all(in);
3 changes: 3 additions & 0 deletions test/fixtures/keys/Makefile
Original file line number Diff line number Diff line change
@@ -79,6 +79,9 @@ agent1-cert.pem: agent1-csr.pem ca1-cert.pem ca1-key.pem
-CAcreateserial \
-out agent1-cert.pem

agent1-pfx.pem: agent1-cert.pem
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this depend on agent1-key.pem and ca1-cert.pem as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

openssl pkcs12 -export -in agent1-cert.pem -inkey agent1-key.pem -certfile ca1-cert.pem -out agent1-pfx.pem -password pass:sample
Copy link
Member

Choose a reason for hiding this comment

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

Can you break this up into lines < 80 columns?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.


agent1-verify: agent1-cert.pem ca1-cert.pem
openssl verify -CAfile ca1-cert.pem agent1-cert.pem

Binary file added test/fixtures/keys/agent1-pfx.pem
Binary file not shown.
31 changes: 26 additions & 5 deletions test/parallel/test-tls-ocsp-callback.js
Original file line number Diff line number Diff line change
@@ -22,11 +22,14 @@ var constants = require('constants');
var fs = require('fs');
var join = require('path').join;

test({ response: false }, function() {
test({ response: 'hello world' }, function() {
test({ ocsp: false });
});
});
var pfx = fs.readFileSync(join(common.fixturesDir, 'keys', 'agent1-pfx.pem'));

var tests = [
{ response: false },
{ response: 'hello world' },
{ ocsp: false },
{ pfx: pfx, passphrase: 'sample', response: 'hello pfx' }
];

function test(testOptions, cb) {

@@ -47,6 +50,13 @@ function test(testOptions, cb) {
var ocspResponse;
var session;

if (testOptions.pfx) {
delete options.key;
delete options.cert;
options.pfx = testOptions.pfx;
options.passphrase = testOptions.passphrase;
}

var server = tls.createServer(options, function(cleartext) {
cleartext.on('error', function(er) {
// We're ok with getting ECONNRESET in this test, but it's
@@ -60,6 +70,7 @@ function test(testOptions, cb) {
});
server.on('OCSPRequest', function(cert, issuer, callback) {
++ocspCount;
console.log(cert, issuer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this intended to be left in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, ack.

assert.ok(Buffer.isBuffer(cert));
assert.ok(Buffer.isBuffer(issuer));

@@ -106,3 +117,13 @@ function test(testOptions, cb) {
assert.equal(ocspCount, 1);
});
}

function runTests(i) {
if (i === tests.length) return;

test(tests[i], common.mustCall(function() {
runTests(i + 1);
}));
}

runTests(0);