crypto: prettify othername in PrintGeneralName

Refs: nodejs@466e541 PR-URL: nodejs#42123 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
xtx1130 · Apr 25, 2022 · a9a0bcc · a9a0bcc
1 parent 1714c6a
commit a9a0bcc
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 26 deletions.
diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc
@@ -692,9 +692,8 @@ static inline void PrintUtf8AltName(const BIOPointer& out,
                true, safe_prefix);
 }
 
-// This function currently emulates the behavior of i2v_GENERAL_NAME in a safer
-// and less ambiguous way.
-// TODO(tniessen): gradually improve the format in the next major version(s)
+// This function emulates the behavior of i2v_GENERAL_NAME in a safer and less
+// ambiguous way. "othername:" entries use the GENERAL_NAME_print format.
 static bool PrintGeneralName(const BIOPointer& out, const GENERAL_NAME* gen) {
   if (gen->type == GEN_DNS) {
     ASN1_IA5STRING* name = gen->d.dNSName;
@@ -767,33 +766,32 @@ static bool PrintGeneralName(const BIOPointer& out, const GENERAL_NAME* gen) {
     OBJ_obj2txt(oline, sizeof(oline), gen->d.rid, true);
     BIO_printf(out.get(), "Registered ID:%s", oline);
   } else if (gen->type == GEN_OTHERNAME) {
-    // TODO(tniessen): the format that is used here is based on OpenSSL's
-    // implementation of i2v_GENERAL_NAME (as of OpenSSL 3.0.1), mostly for
-    // backward compatibility. It is somewhat awkward, especially when passed to
-    // translatePeerCertificate, and should be changed in the future, probably
-    // to the format used by GENERAL_NAME_print (in a major release).
+    // The format that is used here is based on OpenSSL's implementation of
+    // GENERAL_NAME_print (as of OpenSSL 3.0.1). Earlier versions of Node.js
+    // instead produced the same format as i2v_GENERAL_NAME, which was somewhat
+    // awkward, especially when passed to translatePeerCertificate.
     bool unicode = true;
     const char* prefix = nullptr;
-    // OpenSSL 1.1.1 does not support othername in i2v_GENERAL_NAME and may not
-    // define these NIDs.
+    // OpenSSL 1.1.1 does not support othername in GENERAL_NAME_print and may
+    // not define these NIDs.
 #if OPENSSL_VERSION_MAJOR >= 3
     int nid = OBJ_obj2nid(gen->d.otherName->type_id);
     switch (nid) {
       case NID_id_on_SmtpUTF8Mailbox:
-        prefix = " SmtpUTF8Mailbox:";
+        prefix = "SmtpUTF8Mailbox";
         break;
       case NID_XmppAddr:
-        prefix = " XmppAddr:";
+        prefix = "XmppAddr";
         break;
       case NID_SRVName:
-        prefix = " SRVName:";
+        prefix = "SRVName";
         unicode = false;
         break;
       case NID_ms_upn:
-        prefix = " UPN:";
+        prefix = "UPN";
         break;
       case NID_NAIRealm:
-        prefix = " NAIRealm:";
+        prefix = "NAIRealm";
         break;
     }
 #endif  // OPENSSL_VERSION_MAJOR >= 3

diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js
@@ -88,22 +88,22 @@ const { hasOpenSSL3 } = common;
     // OpenSSL should not know it.
     'Registered ID:1.3.9999.12.34',
     hasOpenSSL3 ?
-      'othername: XmppAddr::abc123' :
+      'othername:XmppAddr:abc123' :
       'othername:<unsupported>',
     hasOpenSSL3 ?
-      'othername:" XmppAddr::abc123\\u002c DNS:good.example.com"' :
+      'othername:"XmppAddr:abc123\\u002c DNS:good.example.com"' :
       'othername:<unsupported>',
     hasOpenSSL3 ?
-      'othername:" XmppAddr::good.example.com\\u0000abc123"' :
+      'othername:"XmppAddr:good.example.com\\u0000abc123"' :
       'othername:<unsupported>',
     // This is unsupported because the OID is not recognized.
     'othername:<unsupported>',
-    hasOpenSSL3 ? 'othername: SRVName::abc123' : 'othername:<unsupported>',
+    hasOpenSSL3 ? 'othername:SRVName:abc123' : 'othername:<unsupported>',
     // This is unsupported because it is an SRVName with a UTF8String value,
     // which is not allowed for SRVName.
     'othername:<unsupported>',
     hasOpenSSL3 ?
-      'othername:" SRVName::abc\\u0000def"' :
+      'othername:"SRVName:abc\\u0000def"' :
       'othername:<unsupported>',
   ];
 
@@ -173,14 +173,14 @@ const { hasOpenSSL3 } = common;
       },
     },
     hasOpenSSL3 ? {
-      text: 'OCSP - othername: XmppAddr::good.example.com\n' +
+      text: 'OCSP - othername:XmppAddr:good.example.com\n' +
             'OCSP - othername:<unsupported>\n' +
-            'OCSP - othername: SRVName::abc123',
+            'OCSP - othername:SRVName:abc123',
       legacy: {
         'OCSP - othername': [
-          ' XmppAddr::good.example.com',
+          'XmppAddr:good.example.com',
           '<unsupported>',
-          ' SRVName::abc123',
+          'SRVName:abc123',
         ],
       },
     } : {
@@ -196,10 +196,10 @@ const { hasOpenSSL3 } = common;
       },
     },
     hasOpenSSL3 ? {
-      text: 'OCSP - othername:" XmppAddr::good.example.com\\u0000abc123"',
+      text: 'OCSP - othername:"XmppAddr:good.example.com\\u0000abc123"',
       legacy: {
         'OCSP - othername': [
-          ' XmppAddr::good.example.com\0abc123',
+          'XmppAddr:good.example.com\0abc123',
         ],
       },
     } : {