Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: move parseCertString to internal #14218

Closed
wants to merge 2 commits into from
Closed

tls: move parseCertString to internal #14218

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a031f2
tls: move parseCertString to internal
XadillaX Jul 13, 2017
6c18793
update after reviewing
XadillaX Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,13 @@ Type: Runtime

*Note*: change was made while `async_hooks` was an experimental API.

<a id="DEP0073"></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deprecation code should be DEP00XX until the PR lands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whose job is it to replace?

Copy link
Contributor Author

@XadillaX XadillaX Jul 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Will be replaced when landing by the lander?

My new PR for v8.x-staging made DEP0073 to DEP00XX.

### DEP0073: tls.parseCertString()

Type: Runtime

`tls.parseCertString()` was move to `internal/tls.js`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

`tls.parseCertString()` is a trivial parsing helper that was made public by mistake.  
Can be replaced with
```js
const querystring = require('querystring');
querystring.parse(str, '\n', '=')`;
```

Copy link
Contributor Author

@XadillaX XadillaX Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO I don't think they are equivalent since parseCertString() does no encoding or decoding.

Copy link
Contributor Author

@XadillaX XadillaX Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg.

> querystring.parse("%E5%A5%BD=1", "\n", "=");
{ '好': '1' }
> tls.parseCertString("%E5%A5%BD=1");
{ '%E5%A5%BD': '1' }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like us to have a suggested alternative available, so maybe:

`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', '=')`;
```
*Note*: One difference is that `querystring.parse` does URLDecoding, e.g.:
```js
> querystring.parse("%E5%A5%BD=1", "\n", "=");
{ '好': '1' }
> tls.parseCertString("%E5%A5%BD=1");
{ '%E5%A5%BD': '1' }
```


[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
[`Buffer.from(buffer)`]: buffer.html#buffer_class_method_buffer_from_buffer
Expand Down
2 changes: 1 addition & 1 deletion lib/_tls_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

'use strict';

const tls = require('tls');
const tls = require('internal/tls');

const SSL_OP_CIPHER_SERVER_PREFERENCE =
process.binding('constants').crypto.SSL_OP_CIPHER_SERVER_PREFERENCE;
Expand Down
33 changes: 33 additions & 0 deletions lib/internal/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';

const DEFAULT_CIPHERS = process.binding('constants').crypto.defaultCipherList;
const DEFAULT_ECDH_CURVE = 'prime256v1';

// Example:
// C=US\nST=CA\nL=SF\nO=Joyent\nOU=Node.js\nCN=ca1\nemailAddress=ry@clouds.org
function parseCertString(s) {
var out = {};
var parts = s.split('\n');
for (var i = 0, len = parts.length; i < len; i++) {
var sepIndex = parts[i].indexOf('=');
if (sepIndex > 0) {
var key = parts[i].slice(0, sepIndex);
var value = parts[i].slice(sepIndex + 1);
if (key in out) {
if (!Array.isArray(out[key])) {
out[key] = [out[key]];
}
out[key].push(value);
} else {
out[key] = value;
}
}
}
return out;
}

module.exports = {
parseCertString,
DEFAULT_CIPHERS,
DEFAULT_ECDH_CURVE
};
37 changes: 11 additions & 26 deletions lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
const internalUtil = require('internal/util');
internalUtil.assertCrypto();

const internalTLS = require('internal/tls');
const net = require('net');
const url = require('url');
const binding = process.binding('crypto');
Expand All @@ -39,10 +40,12 @@ exports.CLIENT_RENEG_WINDOW = 600;

exports.SLAB_BUFFER_SIZE = 10 * 1024 * 1024;

exports.DEFAULT_CIPHERS =
process.binding('constants').crypto.defaultCipherList;

exports.DEFAULT_ECDH_CURVE = 'prime256v1';
[ 'DEFAULT_CIPHERS', 'DEFAULT_ECDH_CURVE' ].forEach((key) => {
Object.defineProperty(exports, key, {
get: () => { return internalTLS[key]; },
set: (c) => { internalTLS[key] = c; }
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining accessors for otherwise simple data properties is kind of wasteful, just re-export the values from internal/tls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis, I saw a test that modified the value.

Refs: https://github.com/nodejs/node/blob/v8.1.4/test/parallel/test-tls-honorcipherorder.js#L97

If we don't tie these two variables as getter and setter, the value may be not equivalent.

});

exports.getCiphers = internalUtil.cachedResult(
() => internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true)
Expand Down Expand Up @@ -228,28 +231,10 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
}
};

// Example:
// C=US\nST=CA\nL=SF\nO=Joyent\nOU=Node.js\nCN=ca1\nemailAddress=ry@clouds.org
exports.parseCertString = function parseCertString(s) {
var out = {};
var parts = s.split('\n');
for (var i = 0, len = parts.length; i < len; i++) {
var sepIndex = parts[i].indexOf('=');
if (sepIndex > 0) {
var key = parts[i].slice(0, sepIndex);
var value = parts[i].slice(sepIndex + 1);
if (key in out) {
if (!Array.isArray(out[key])) {
out[key] = [out[key]];
}
out[key].push(value);
} else {
out[key] = value;
}
}
}
return out;
};
exports.parseCertString = internalUtil.deprecate(
Copy link
Contributor

@refack refack Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO there's no need to move the function, just replace the export with this deprecation-wrapped-export.

Copy link
Contributor Author

@XadillaX XadillaX Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other file depends on this function.

node/lib/_tls_common.js

Line 176 in 1a031f2

if (c.subject) c.subject = tls.parseCertString(c.subject);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

internalTLS.parseCertString,
'tls.parseCertString is deprecated',
'DEP0073');

// Public API
exports.createSecureContext = require('_tls_common').createSecureContext;
Expand Down
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@
'lib/internal/repl.js',
'lib/internal/socket_list.js',
'lib/internal/test/unicode.js',
'lib/internal/tls.js',
'lib/internal/url.js',
'lib/internal/util.js',
'lib/internal/v8_prof_polyfill.js',
Expand Down
18 changes: 17 additions & 1 deletion test/parallel/test-tls-parse-cert-string.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
'use strict';

// Flags: --expose_internals
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const tls = require('tls');
const tls = require('internal/tls');

{
const singles = 'C=US\nST=CA\nL=SF\nO=Node.js Foundation\nOU=Node.js\n' +
Expand Down Expand Up @@ -36,3 +38,17 @@ const tls = require('tls');
const invalidOut = tls.parseCertString(invalid);
assert.deepStrictEqual(invalidOut, {});
}

{
const regexp = new RegExp('^\\(node:\\d+\\) [DEP0073] DeprecationWarning: ' +
'tls\\.parseCertString is deprecated$');

// test for deprecate message
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/deprecate/deprecation/ and can you capitalize + punctuate the comment?

Copy link
Member

@jasnell jasnell Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can/should use common.expectWarning() to check for this without having to hijack stderr

common.hijackStderr(common.mustCall(function(data) {
assert.ok(regexp.test(data));
common.restoreStderr();
}));

const ret = require('tls').parseCertString('foo=bar');
assert.deepStrictEqual(ret, { foo: 'bar' });
}