FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: move tls.parseCertString to end-of-life · nodejs/node@807c7e1 · GitHub

/ node Public

Commit 807c7e1

Browse files
tniessen
committed
tls: move tls.parseCertString to end-of-life
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>
1 parent f7be6ab commit 807c7e1

6 files changed

Lines changed: 21 additions & 148 deletions

File tree

‎doc/api/deprecations.md‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1595,6 +1595,9 @@ Type: End-of-Life
15951595

15961596
<!-- YAML
15971597
changes:
1598+
- version: REPLACEME
1599+
pr-url: https://github.com/nodejs/node/pull/41479
1600+
description: End-of-Life.
15981601
- version: v9.0.0
15991602
pr-url: https://github.com/nodejs/node/pull/14249
16001603
description: Runtime deprecation.
@@ -1603,25 +1606,15 @@ changes:
16031606
description: Documentation-only deprecation.
16041607
-->
16051608

1606-
Type: Runtime
1607-
1608-
`tls.parseCertString()` is a trivial parsing helper that was made public by
1609-
mistake. This function can usually be replaced with:
1610-
1611-
```js
1612-
const querystring = require('querystring');
1613-
querystring.parse(str, '\n', '=');
1614-
```
1609+
Type: End-of-Life
16151610

1616-
This function is not completely equivalent to `querystring.parse()`. One
1617-
difference is that `querystring.parse()` does url decoding:
1611+
`tls.parseCertString()` was a trivial parsing helper that was made public by
1612+
mistake. While it was supposed to parse certificate subject and issuer strings,
1613+
it never handled multi-value Relative Distinguished Names correctly.
16181614

1619-
```console
1620-
> querystring.parse('%E5%A5%BD=1', '\n', '=');
1621-
{ '好': '1' }
1622-
> tls.parseCertString('%E5%A5%BD=1');
1623-
{ '%E5%A5%BD': '1' }
1624-
```
1615+
Earlier versions of this document suggested using `querystring.parse()` as an
1616+
alternative to `tls.parseCertString()`. However, `querystring.parse()` also does
1617+
not handle all certificate subjects correctly and should not be used.
16251618

16261619
### DEP0077: `Module._debug()`
16271620

‎lib/_tls_common.js‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,6 @@ const {
5555
configSecureContext,
5656
} = require('internal/tls/secure-context');
5757

58-
const {
59-
parseCertString,
60-
} = require('internal/tls/parse-cert-string');
61-
6258
function toV(which, v, def) {
6359
if (v == null) v = def;
6460
if (v === 'TLSv1') return TLS1_VERSION;
@@ -126,13 +122,9 @@ function translatePeerCertificate(c) {
126122
if (!c)
127123
return null;
128124

129-
// TODO(tniessen): can we remove parseCertString without breaking anything?
130-
if (typeof c.issuer === 'string') c.issuer = parseCertString(c.issuer);
131125
if (c.issuerCertificate != null && c.issuerCertificate !== c) {
132126
c.issuerCertificate = translatePeerCertificate(c.issuerCertificate);
133127
}
134-
// TODO(tniessen): can we remove parseCertString without breaking anything?
135-
if (typeof c.subject === 'string') c.subject = parseCertString(c.subject);
136128
if (c.infoAccess != null) {
137129
const info = c.infoAccess;
138130
c.infoAccess = ObjectCreate(null);

‎lib/internal/tls/parse-cert-string.js‎

Lines changed: 0 additions & 35 deletions
This file was deleted.

‎lib/tls.js‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ const { canonicalizeIP } = internalBinding('cares_wrap');
6464
const _tls_common = require('_tls_common');
6565
const _tls_wrap = require('_tls_wrap');
6666
const { createSecurePair } = require('internal/tls/secure-pair');
67-
const { parseCertString } = require('internal/tls/parse-cert-string');
6867

6968
// Allow {CLIENT_RENEG_LIMIT} client-initiated session renegotiations
7069
// every {CLIENT_RENEG_WINDOW} seconds. An error event is emitted if more
@@ -338,12 +337,6 @@ exports.Server = _tls_wrap.Server;
338337
exports.createServer = _tls_wrap.createServer;
339338
exports.connect = _tls_wrap.connect;
340339

341-
exports.parseCertString = internalUtil.deprecate(
342-
parseCertString,
343-
'tls.parseCertString() is deprecated. ' +
344-
'Please use querystring.parse() instead.',
345-
'DEP0076');
346-
347340
exports.createSecurePair = internalUtil.deprecate(
348341
createSecurePair,
349342
'tls.createSecurePair() is deprecated. Please use ' +

‎test/parallel/test-tls-parse-cert-string.js‎

Lines changed: 0 additions & 71 deletions
This file was deleted.

‎test/parallel/test-tls-translate-peer-certificate.js‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,6 @@ const { strictEqual, deepStrictEqual } = require('assert');
99
const { translatePeerCertificate } = require('_tls_common');
1010

1111
const certString = '__proto__=42\nA=1\nB=2\nC=3';
12-
const certObject = Object.create(null);
13-
certObject.__proto__ = '42';
14-
certObject.A = '1';
15-
certObject.B = '2';
16-
certObject.C = '3';
1712

1813
strictEqual(translatePeerCertificate(null), null);
1914
strictEqual(translatePeerCertificate(undefined), null);
@@ -23,27 +18,33 @@ strictEqual(translatePeerCertificate(1), 1);
2318

2419
deepStrictEqual(translatePeerCertificate({}), {});
2520

21+
// Earlier versions of Node.js parsed the issuer property but did so
22+
// incorrectly. This behavior has now reached end-of-life and user-supplied
23+
// strings will not be parsed at all.
2624
deepStrictEqual(translatePeerCertificate({ issuer: '' }),
27-
{ issuer: Object.create(null) });
25+
{ issuer: '' });
2826
deepStrictEqual(translatePeerCertificate({ issuer: null }),
2927
{ issuer: null });
3028
deepStrictEqual(translatePeerCertificate({ issuer: certString }),
31-
{ issuer: certObject });
29+
{ issuer: certString });
3230

31+
// Earlier versions of Node.js parsed the issuer property but did so
32+
// incorrectly. This behavior has now reached end-of-life and user-supplied
33+
// strings will not be parsed at all.
3334
deepStrictEqual(translatePeerCertificate({ subject: '' }),
34-
{ subject: Object.create(null) });
35+
{ subject: '' });
3536
deepStrictEqual(translatePeerCertificate({ subject: null }),
3637
{ subject: null });
3738
deepStrictEqual(translatePeerCertificate({ subject: certString }),
38-
{ subject: certObject });
39+
{ subject: certString });
3940

4041
deepStrictEqual(translatePeerCertificate({ issuerCertificate: '' }),
4142
{ issuerCertificate: null });
4243
deepStrictEqual(translatePeerCertificate({ issuerCertificate: null }),
4344
{ issuerCertificate: null });
4445
deepStrictEqual(
4546
translatePeerCertificate({ issuerCertificate: { subject: certString } }),
46-
{ issuerCertificate: { subject: certObject } });
47+
{ issuerCertificate: { subject: certString } });
4748

4849
{
4950
const cert = {};

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL