From 36fb79030e76dc1930f7fc2cd42af7d564988d48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 9 Mar 2022 17:36:40 +0100 Subject: [PATCH] crypto: fix X509Certificate toLegacyObject PR-URL: https://github.com/nodejs/node/pull/42124 Reviewed-By: Filip Skokan Reviewed-By: James M Snell Reviewed-By: Rich Trott --- lib/internal/crypto/x509.js | 8 ++++- src/crypto/crypto_common.cc | 38 ++++++---------------- src/crypto/crypto_common.h | 3 +- src/crypto/crypto_x509.cc | 2 +- test/parallel/test-crypto-x509.js | 49 +++++++++++++++-------------- test/parallel/test-x509-escaping.js | 18 +++++++++++ 6 files changed, 61 insertions(+), 57 deletions(-) diff --git a/lib/internal/crypto/x509.js b/lib/internal/crypto/x509.js index e7098d17da6aac..386b41f3e4a42f 100644 --- a/lib/internal/crypto/x509.js +++ b/lib/internal/crypto/x509.js @@ -56,6 +56,8 @@ const { kHandle, } = require('internal/crypto/util'); +let lazyTranslatePeerCertificate; + const kInternalState = Symbol('kInternalState'); function isX509Certificate(value) { @@ -345,7 +347,11 @@ class X509Certificate extends JSTransferable { } toLegacyObject() { - return this[kHandle].toLegacy(); + // TODO(tniessen): do not depend on translatePeerCertificate here, return + // the correct legacy representation from the binding + lazyTranslatePeerCertificate ??= + require('_tls_common').translatePeerCertificate; + return lazyTranslatePeerCertificate(this[kHandle].toLegacy()); } } diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 57ab74f6168396..34d7f5a523f019 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -1284,8 +1284,7 @@ MaybeLocal GetPeerCert( MaybeLocal X509ToObject( Environment* env, - X509* cert, - bool names_as_string) { + X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); Local info = Object::New(env->isolate()); @@ -1293,34 +1292,15 @@ MaybeLocal X509ToObject( BIOPointer bio(BIO_new(BIO_s_mem())); CHECK(bio); - if (names_as_string) { - // TODO(tniessen): this branch should not have to exist. It is only here - // because toLegacyObject() does not actually return a legacy object, and - // instead represents subject and issuer as strings. - if (!Set(context, - info, - env->subject_string(), - GetSubject(env, bio, cert)) || - !Set(context, - info, - env->issuer_string(), - GetIssuerString(env, bio, cert))) { - return MaybeLocal(); - } - } else { - if (!Set(context, - info, - env->subject_string(), - GetX509NameObject(env, cert)) || - !Set(context, - info, - env->issuer_string(), - GetX509NameObject(env, cert))) { - return MaybeLocal(); - } - } - if (!Set(context, + info, + env->subject_string(), + GetX509NameObject(env, cert)) || + !Set(context, + info, + env->issuer_string(), + GetX509NameObject(env, cert)) || + !Set(context, info, env->subjectaltname_string(), GetSubjectAltNameString(env, bio, cert)) || diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h index e0956395e91c18..5a6869c6ca8845 100644 --- a/src/crypto/crypto_common.h +++ b/src/crypto/crypto_common.h @@ -109,8 +109,7 @@ v8::MaybeLocal ECPointToBuffer( v8::MaybeLocal X509ToObject( Environment* env, - X509* cert, - bool names_as_string = false); + X509* cert); v8::MaybeLocal GetValidTo( Environment* env, diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc index 398b1a85795431..a301a1392152ec 100644 --- a/src/crypto/crypto_x509.cc +++ b/src/crypto/crypto_x509.cc @@ -477,7 +477,7 @@ void X509Certificate::ToLegacy(const FunctionCallbackInfo& args) { X509Certificate* cert; ASSIGN_OR_RETURN_UNWRAP(&cert, args.Holder()); Local ret; - if (X509ToObject(env, cert->get(), true).ToLocal(&ret)) + if (X509ToObject(env, cert->get()).ToLocal(&ret)) args.GetReturnValue().Set(ret); } diff --git a/test/parallel/test-crypto-x509.js b/test/parallel/test-crypto-x509.js index d1782359277dc5..03f7c209679d9c 100644 --- a/test/parallel/test-crypto-x509.js +++ b/test/parallel/test-crypto-x509.js @@ -198,27 +198,28 @@ const der = Buffer.from( // Verify that legacy encoding works const legacyObjectCheck = { - subject: 'C=US\n' + - 'ST=CA\n' + - 'L=SF\n' + - 'O=Joyent\n' + - 'OU=Node.js\n' + - 'CN=agent1\n' + - 'emailAddress=ry@tinyclouds.org', - issuer: - 'C=US\n' + - 'ST=CA\n' + - 'L=SF\n' + - 'O=Joyent\n' + - 'OU=Node.js\n' + - 'CN=ca1\n' + - 'emailAddress=ry@tinyclouds.org', - infoAccess: - common.hasOpenSSL3 ? - 'OCSP - URI:http://ocsp.nodejs.org/\n' + - 'CA Issuers - URI:http://ca.nodejs.org/ca.cert' : - 'OCSP - URI:http://ocsp.nodejs.org/\n' + - 'CA Issuers - URI:http://ca.nodejs.org/ca.cert\n', + subject: Object.assign(Object.create(null), { + C: 'US', + ST: 'CA', + L: 'SF', + O: 'Joyent', + OU: 'Node.js', + CN: 'agent1', + emailAddress: 'ry@tinyclouds.org', + }), + issuer: Object.assign(Object.create(null), { + C: 'US', + ST: 'CA', + L: 'SF', + O: 'Joyent', + OU: 'Node.js', + CN: 'ca1', + emailAddress: 'ry@tinyclouds.org', + }), + infoAccess: Object.assign(Object.create(null), { + 'OCSP - URI': ['http://ocsp.nodejs.org/'], + 'CA Issuers - URI': ['http://ca.nodejs.org/ca.cert'] + }), modulus: 'EF5440701637E28ABB038E5641F828D834C342A9D25EDBB86A2BF' + '6FBD809CB8E037A98B71708E001242E4DEB54C6164885F599DD87' + 'A23215745955BE20417E33C4D0D1B80C9DA3DE419A2607195D2FB' + @@ -243,9 +244,9 @@ const der = Buffer.from( const legacyObject = x509.toLegacyObject(); assert.deepStrictEqual(legacyObject.raw, x509.raw); - assert.strictEqual(legacyObject.subject, legacyObjectCheck.subject); - assert.strictEqual(legacyObject.issuer, legacyObjectCheck.issuer); - assert.strictEqual(legacyObject.infoAccess, legacyObjectCheck.infoAccess); + assert.deepStrictEqual(legacyObject.subject, legacyObjectCheck.subject); + assert.deepStrictEqual(legacyObject.issuer, legacyObjectCheck.issuer); + assert.deepStrictEqual(legacyObject.infoAccess, legacyObjectCheck.infoAccess); assert.strictEqual(legacyObject.modulus, legacyObjectCheck.modulus); assert.strictEqual(legacyObject.bits, legacyObjectCheck.bits); assert.strictEqual(legacyObject.exponent, legacyObjectCheck.exponent); diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js index c3b88646899a2d..6078314a999281 100644 --- a/test/parallel/test-x509-escaping.js +++ b/test/parallel/test-x509-escaping.js @@ -241,6 +241,15 @@ const { hasOpenSSL3 } = common; assert.deepStrictEqual(peerCert.infoAccess, Object.assign(Object.create(null), expected.legacy)); + + // toLegacyObject() should also produce the same properties. However, + // the X509Certificate is not aware of the chain, so we need to add + // the circular issuerCertificate reference manually for the assertion + // to be true. + const obj = cert.toLegacyObject(); + assert.strictEqual(obj.issuerCertificate, undefined); + obj.issuerCertificate = obj; + assert.deepStrictEqual(peerCert, obj); }, }, common.mustCall()); })); @@ -350,6 +359,15 @@ const { hasOpenSSL3 } = common; // self-signed. Otherwise, OpenSSL would have already rejected the // certificate while connecting to the TLS server. assert.deepStrictEqual(peerCert.issuer, expectedObject); + + // toLegacyObject() should also produce the same properties. However, + // the X509Certificate is not aware of the chain, so we need to add + // the circular issuerCertificate reference manually for the assertion + // to be true. + const obj = cert.toLegacyObject(); + assert.strictEqual(obj.issuerCertificate, undefined); + obj.issuerCertificate = obj; + assert.deepStrictEqual(peerCert, obj); }, }, common.mustCall()); }));