crypto: fix X509Certificate toLegacyObject

PR-URL: #42124 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Mar 9, 2022 · 36fb79030e76dc1930f7fc2cd42af7d564988d48 · 36fb790
1 parent eacd456
commit 36fb79030e76dc1930f7fc2cd42af7d564988d48
Showing with 61 additions and 57 deletions.

+7 −1 lib/internal/crypto/x509.js

+9 −29 src/crypto/crypto_common.cc

+1 −2 src/crypto/crypto_common.h

+1 −1 src/crypto/crypto_x509.cc

+25 −24 test/parallel/test-crypto-x509.js

+18 −0 test/parallel/test-x509-escaping.js
diff --git 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
@@ -1284,43 +1284,23 @@ MaybeLocal<Value> GetPeerCert(
 
 MaybeLocal<Object> X509ToObject(
     Environment* env,
-    X509* cert,
-    bool names_as_string) {
+    X509* cert) {
   EscapableHandleScope scope(env->isolate());
   Local<Context> context = env->context();
   Local<Object> info = Object::New(env->isolate());
 
   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<Value>(context,
-                    info,
-                    env->subject_string(),
-                    GetSubject(env, bio, cert)) ||
-        !Set<Value>(context,
-                    info,
-                    env->issuer_string(),
-                    GetIssuerString(env, bio, cert))) {
-      return MaybeLocal<Object>();
-    }
-  } else {
-    if (!Set<Value>(context,
-                    info,
-                    env->subject_string(),
-                    GetX509NameObject<X509_get_subject_name>(env, cert)) ||
-        !Set<Value>(context,
-                    info,
-                    env->issuer_string(),
-                    GetX509NameObject<X509_get_issuer_name>(env, cert))) {
-      return MaybeLocal<Object>();
-    }
-  }
-
   if (!Set<Value>(context,
+                  info,
+                  env->subject_string(),
+                  GetX509NameObject<X509_get_subject_name>(env, cert)) ||
+      !Set<Value>(context,
+                  info,
+                  env->issuer_string(),
+                  GetX509NameObject<X509_get_issuer_name>(env, cert)) ||
+      !Set<Value>(context,
                   info,
                   env->subjectaltname_string(),
                   GetSubjectAltNameString(env, bio, cert)) ||

diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h
@@ -109,8 +109,7 @@ v8::MaybeLocal<v8::Object> ECPointToBuffer(
 
 v8::MaybeLocal<v8::Object> X509ToObject(
     Environment* env,
-    X509* cert,
-    bool names_as_string = false);
+    X509* cert);
 
 v8::MaybeLocal<v8::Value> GetValidTo(
     Environment* env,

diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc
@@ -477,7 +477,7 @@ void X509Certificate::ToLegacy(const FunctionCallbackInfo<Value>& args) {
   X509Certificate* cert;
   ASSIGN_OR_RETURN_UNWRAP(&cert, args.Holder());
   Local<Value> 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
@@ -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
@@ -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());
     }));