tls: move tls.parseCertString to end-of-life

tniessen · tniessen · commit 807c7e14f411 · 2022-01-15T17:00:25.000+01:00
The internal use of tls.parseCertString was removed in a336444. The function does not handle multi-value RDNs correctly, leading to incorrect representations and security concerns. This change is breaking in two ways: tls.parseCertString is removed (but has been runtime-deprecated since Node.js 9) and _tls_common.translatePeerCertificate does not translate the `subject` and `issuer` properties anymore. This change also removes the recommendation to use querystring.parse instead, which is similarly dangerous. PR-URL: #41479 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -1595,6 +1595,9 @@ Type: End-of-Life
 
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/41479
+    description: End-of-Life.
   - version: v9.0.0
     pr-url: https://github.com/nodejs/node/pull/14249
     description: Runtime deprecation.
@@ -1603,25 +1606,15 @@ changes:
     description: Documentation-only deprecation.
 -->
 
-Type: Runtime
-
-`tls.parseCertString()` is a trivial parsing helper that was made public by
-mistake. This function can usually be replaced with:
-
-```js
-const querystring = require('querystring');
-querystring.parse(str, '\n', '=');
-```
+Type: End-of-Life
 
-This function is not completely equivalent to `querystring.parse()`. One
-difference is that `querystring.parse()` does url decoding:
+`tls.parseCertString()` was a trivial parsing helper that was made public by
+mistake. While it was supposed to parse certificate subject and issuer strings,
+it never handled multi-value Relative Distinguished Names correctly.
 
-```console
-> querystring.parse('%E5%A5%BD=1', '\n', '=');
-{ '&#22909;': '1' }
-> tls.parseCertString('%E5%A5%BD=1');
-{ '%E5%A5%BD': '1' }
-```
+Earlier versions of this document suggested using `querystring.parse()` as an
+alternative to `tls.parseCertString()`. However, `querystring.parse()` also does
+not handle all certificate subjects correctly and should not be used.
 
 ### DEP0077: `Module._debug()`
 
diff --git a/lib/_tls_common.js b/lib/_tls_common.js
@@ -55,10 +55,6 @@ const {
   configSecureContext,
 } = require('internal/tls/secure-context');
 
-const {
-  parseCertString,
-} = require('internal/tls/parse-cert-string');
-
 function toV(which, v, def) {
   if (v == null) v = def;
   if (v === 'TLSv1') return TLS1_VERSION;
@@ -126,13 +122,9 @@ function translatePeerCertificate(c) {
   if (!c)
     return null;
 
-  // TODO(tniessen): can we remove parseCertString without breaking anything?
-  if (typeof c.issuer === 'string') c.issuer = parseCertString(c.issuer);
   if (c.issuerCertificate != null && c.issuerCertificate !== c) {
     c.issuerCertificate = translatePeerCertificate(c.issuerCertificate);
   }
-  // TODO(tniessen): can we remove parseCertString without breaking anything?
-  if (typeof c.subject === 'string') c.subject = parseCertString(c.subject);
   if (c.infoAccess != null) {
     const info = c.infoAccess;
     c.infoAccess = ObjectCreate(null);
diff --git a/lib/internal/tls/parse-cert-string.js b/lib/internal/tls/parse-cert-string.js
diff --git a/lib/tls.js b/lib/tls.js
@@ -64,7 +64,6 @@ const { canonicalizeIP } = internalBinding('cares_wrap');
 const _tls_common = require('_tls_common');
 const _tls_wrap = require('_tls_wrap');
 const { createSecurePair } = require('internal/tls/secure-pair');
-const { parseCertString } = require('internal/tls/parse-cert-string');
 
 // Allow {CLIENT_RENEG_LIMIT} client-initiated session renegotiations
 // every {CLIENT_RENEG_WINDOW} seconds. An error event is emitted if more
@@ -338,12 +337,6 @@ exports.Server = _tls_wrap.Server;
 exports.createServer = _tls_wrap.createServer;
 exports.connect = _tls_wrap.connect;
 
-exports.parseCertString = internalUtil.deprecate(
-  parseCertString,
-  'tls.parseCertString() is deprecated. ' +
-  'Please use querystring.parse() instead.',
-  'DEP0076');
-
 exports.createSecurePair = internalUtil.deprecate(
   createSecurePair,
   'tls.createSecurePair() is deprecated. Please use ' +
diff --git a/test/parallel/test-tls-parse-cert-string.js b/test/parallel/test-tls-parse-cert-string.js
diff --git a/test/parallel/test-tls-translate-peer-certificate.js b/test/parallel/test-tls-translate-peer-certificate.js
@@ -9,11 +9,6 @@ const { strictEqual, deepStrictEqual } = require('assert');
 const { translatePeerCertificate } = require('_tls_common');
 
 const certString = '__proto__=42\nA=1\nB=2\nC=3';
-const certObject = Object.create(null);
-certObject.__proto__ = '42';
-certObject.A = '1';
-certObject.B = '2';
-certObject.C = '3';
 
 strictEqual(translatePeerCertificate(null), null);
 strictEqual(translatePeerCertificate(undefined), null);
@@ -23,27 +18,33 @@ strictEqual(translatePeerCertificate(1), 1);
 
 deepStrictEqual(translatePeerCertificate({}), {});
 
+// Earlier versions of Node.js parsed the issuer property but did so
+// incorrectly. This behavior has now reached end-of-life and user-supplied
+// strings will not be parsed at all.
 deepStrictEqual(translatePeerCertificate({ issuer: '' }),
-                { issuer: Object.create(null) });
+                { issuer: '' });
 deepStrictEqual(translatePeerCertificate({ issuer: null }),
                 { issuer: null });
 deepStrictEqual(translatePeerCertificate({ issuer: certString }),
-                { issuer: certObject });
+                { issuer: certString });
 
+// Earlier versions of Node.js parsed the issuer property but did so
+// incorrectly. This behavior has now reached end-of-life and user-supplied
+// strings will not be parsed at all.
 deepStrictEqual(translatePeerCertificate({ subject: '' }),
-                { subject: Object.create(null) });
+                { subject: '' });
 deepStrictEqual(translatePeerCertificate({ subject: null }),
                 { subject: null });
 deepStrictEqual(translatePeerCertificate({ subject: certString }),
-                { subject: certObject });
+                { subject: certString });
 
 deepStrictEqual(translatePeerCertificate({ issuerCertificate: '' }),
                 { issuerCertificate: null });
 deepStrictEqual(translatePeerCertificate({ issuerCertificate: null }),
                 { issuerCertificate: null });
 deepStrictEqual(
   translatePeerCertificate({ issuerCertificate: { subject: certString } }),
-  { issuerCertificate: { subject: certObject } });
+  { issuerCertificate: { subject: certString } });
 
 {
   const cert = {};
