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.

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

tls: Add PSK support (v9.0.0-pre) #14978

Closed
wants to merge 28 commits into from
Closed

Conversation

taylorzane
Copy link

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

tls, crypto

This is an updated PR based off of #6701 that supports Node.js v9.0.0.

I understand that the Node.js team is waiting for FIPS support in OpenSSL-1.1, but this will get the ball rolling for TLS-PSK support in the latest Node.js versions.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 22, 2017
if (typeof ret.psk === 'string') {
ret.psk = Buffer.from(ret.psk);
} else if (!Buffer.isBuffer(ret.psk)) {
throw new TypeError('Pre-shared key is not a string or buffer');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the rest of this file has been converted to use the internal/errors mechanism, but new errors introduced definitely should.

throw new TypeError('Pre-shared key is not a string or buffer');
}
if (ret.psk.length > maxPskLen) {
throw new TypeError(`Pre-shared key exceed ${maxPskLen} bytes`);
Copy link
Member

Choose a reason for hiding this comment

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

This should likely be a RangeError

throw new TypeError('PSK identity is not a string');
}
if (ret.identity.length > maxIdentLen) {
throw new TypeError(`PSK identity exceeds ${maxIdentLen} bytes`);
Copy link
Member

Choose a reason for hiding this comment

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

RangeError

void SecureContext::SetPskIdentity(const FunctionCallbackInfo<Value>& args) {
SecureContext* wrap = Unwrap<SecureContext>(args.Holder());

if (args.Length() != 1 || !args[0]->IsString()) {
Copy link
Member

Choose a reason for hiding this comment

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

Not convinced that this should error if more than 1 argument is provided. Perhaps just if (!args[0]->IsString()) {

return wrap->env()->ThrowTypeError("Argument must be a string");
}

String::Utf8Value identity(args[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure the documentation indicates that the PSK should be UTF8 only.

const s = tls.connect(common.PORT, {
ciphers: CIPHERS,
rejectUnauthorized: false,
pskCallback: (hint) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit ... pskCallback(hint) {

@taylorzane
Copy link
Author

I'll get these changes implemented right away, thanks @jasnell.

@mscdex
Copy link
Contributor

mscdex commented Aug 22, 2017

/cc @nodejs/crypto

@taylorzane
Copy link
Author

@jasnell I updated my changes as suggested. I have a few questions though, I'll add comments to my commit.

['string', 'buffer']);
}
if (ret.psk.length > maxPskLen) {
throw new errors.RangeError(
Copy link
Author

Choose a reason for hiding this comment

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

Is this the proper error to throw? There wasn't an existing error that satisfied the previous generic RangeError, so I just used ERR_ASSERTION.

Copy link
Author

Choose a reason for hiding this comment

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

Also, how is the formatting on these lines? It complies with eslint, but it's ugly as sin.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Great work! Some of the more code here uses deprecated APIs, it would be nice if we could avoid that. Let me know if you need any help with it!

doc/api/tls.md Outdated
<string>}`. Note that PSK ciphers are disabled by default, and using
TLS-PSK thus requires explicitly specifying a cipher suite with the
`ciphers` option. Additionally, it may be necessary to disable
`rejectUnauthorized` when not intending to use certificates.
Copy link
Member

Choose a reason for hiding this comment

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

“it may be necessary” sounds a bit vague … can you clarify when that is necessary?


if (typeof ret.psk === 'string') {
ret.psk = Buffer.from(ret.psk);
} else if (!Buffer.isBuffer(ret.psk)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you use ArrayBuffer.isVew(ret.psk) instead and adjust the thrown error accordingly? Like in
https://github.com/nodejs/node/pull/14807/files#diff-1c8a7b9222892c3457fa64cbbfab6573R56

// Only set for the client callback, which must provide an identity.
if (maxIdentLen) {
if (typeof ret.identity === 'string') {
ret.identity = Buffer.from(ret.identity + '\0');
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it would be easier to pass a string to C++ instead? Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the C++ method is using the identity in its buffer form, so we could pass a string and then convert it to a buffer in C++. Since Buffer::Data returns a char*, that's directly compatible with identity. I suppose we could convert the string directly to a char* as well. Seems like it would be about the same amount of work on either side.

Copy link
Member

Choose a reason for hiding this comment

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

One difference that this is always creating new intermediate JS objects in the process, which is part of why we don’t usually do that (I guess)

Copy link
Contributor

@mscdex mscdex Aug 23, 2017

Choose a reason for hiding this comment

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

I agree, if we can avoid creating unnecessary (Buffer) objects and such in between then that would be better.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I'll get on this refactor in the morning.

Isolate* isolate = env->isolate();

Local<Value> argv[] = {
String::NewFromUtf8(isolate, identity),
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the overload that uses v8::NewStringType/returns a MaybeLocal? This one is being deprecated by V8.

sc->object(),
env->onpskexchange_string(),
arraysize(argv),
argv);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is only called synchronously, right (i.e. there is a JS stack somewhere below it)? In that case, it’s better to just use Function::Call instead of MakeCallback

Local<Object> obj = ret.As<Object>();

Local<Value> psk_buf = obj->Get(env->psk_string());
assert(Buffer::HasInstance(psk_buf));
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can you use CHECK instead of assert? Also, this is equivalent to psk_buf->IsArrayBufferView(), just btw.

Local<Value> psk_buf = obj->Get(env->psk_string());
assert(Buffer::HasInstance(psk_buf));
size_t psk_len = Buffer::Length(psk_buf);
assert(psk_len <= max_psk_len);
Copy link
Member

Choose a reason for hiding this comment

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

nit: CHECK_LE?

}
Local<Object> obj = ret.As<Object>();

Local<Value> psk_buf = obj->Get(env->psk_string());
Copy link
Member

Choose a reason for hiding this comment

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

This variant of Get is being deprecated as well, the replacement API basically looks like obj->Get(env->context(), env->psk_string()).ToLocalChecked() in this case

}

process.on('exit', () => {
assert.strictEqual(serverConnections, 3);
Copy link
Member

Choose a reason for hiding this comment

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

We have a test utility that’s called common.mustCall() and allows you to have a bit more code locality in tests; e.g. you could wrap the callback that increases serverConnections in common.mustCall() and wouldn’t need to have an explicit counter variable :)

if (!common.hasCrypto) {
console.log('1..0 # Skipped: missing crypto');
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Just fyi, in most tests this looks like this by now:

if (!common.hasCrypto)
common.skip('missing crypto');

@addaleax
Copy link
Member

@taylorzane
Copy link
Author

Thanks for the follow-up, I'll get these changes implemented.

taylorzane added a commit to taylorzane/node that referenced this pull request Aug 22, 2017
@taylorzane
Copy link
Author

taylorzane commented Aug 22, 2017

I believe that all issues brought up by @addaleax have been resolved. Some code snippets were updated in both PskServerCallback and PskClientCallback even though they weren't explicitly mentioned. (Missed a couple asserts, fixed after this comment was made.)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM if @nodejs/crypto is happy

Local<Value> argv[] = {
String::NewFromUtf8(isolate,
identity,
String::kNormalString,
Copy link
Member

Choose a reason for hiding this comment

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

v8::NewStringType::kNormal (the overload that uses a Maybe – sorry, should have been clearer about that)

if (hint != nullptr) {
argv[0] = String::NewFromUtf8(isolate,
hint,
String::kNormalString,
Copy link
Member

Choose a reason for hiding this comment

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

(ditto)

String::kNormalString,
strlen(hint));
}
Local<Value> value = sc->object()->Get(env->onpskexchange_string());
Copy link
Member

Choose a reason for hiding this comment

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

This is the deprecated Get() overload (no context argument)

size_t identity_len = Buffer::Length(identity_buf);
CHECK_LE(identity_len, max_identity_len);
memcpy(identity, Buffer::Data(identity_buf), identity_len);
assert(identity[identity_len - 1] == '\0');
Copy link
Member

Choose a reason for hiding this comment

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

This is also a reason why using passing strings to C++ might be nice :)

@taylorzane
Copy link
Author

Didn't realize I was using the wrong methods, I must have been looking at an old version of the V8 documentation. My MaybeLocal => Local logic seems right to me based on my experience with other languages with optional types.

@taylorzane
Copy link
Author

Alright, TLS-PSK API now only accepts a hex-encoded string for psk (akin to the openssl s_server command); psk and identity are now passed as strings to C++ and converted to char arrays. I also updated a nit on a deprecated ->Get call.

@@ -132,6 +133,49 @@ exports.createSecureContext = function createSecureContext(options, context) {
}
}

if (options.pskCallback && c.context.enablePskCallback) {
c.context.onpskexchange = (identity, maxPskLen, maxIdentLen) => {
Copy link
Member

Choose a reason for hiding this comment

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

should there be a check here to ensure that pskCallback is a function?

if (options.pskCallback && c.context.enablePskCallback) {
c.context.onpskexchange = (identity, maxPskLen, maxIdentLen) => {
let ret = options.pskCallback(identity);
if (typeof ret !== 'object') {
Copy link
Member

@jasnell jasnell Aug 24, 2017

Choose a reason for hiding this comment

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

what if ret === null? (I know the check before looks for that, but we should be clear here... just to be safe :-) ...)


return ret;
};
c.context.enablePskCallback();
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading the #ifdef statements write in the source below, there's a chance the enablePskCallback() will not be defined if OPENSSL_PSK_SUPPORT is switched off, correct? I may be missing something because I haven't done a complete walkthrough, but if that's the case, I'd recommend gating these with a flag in node_config.cc

Copy link
Author

Choose a reason for hiding this comment

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

There shouldn't be a chance that enablePskCallback is enabled, since the definition of that is also inside of an #ifdef, see here

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see, I was misreading the location of the closing brackets. This call is made inside the if that checks c.context.enablePskCallback is not falsy. :-)


MaybeLocal<Value> maybe_ret;
if (value->IsFunction()) {
Local<Function> func = Local<Function>::Cast(value);
Copy link
Member

Choose a reason for hiding this comment

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

value.As<Function>();?

HandleScope scope(isolate);

Local<Value> argv[] = {
Null(isolate),
Copy link
Member

Choose a reason for hiding this comment

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

May be helpful to add a comment here reminding folks that this is a error-back style callback, which explains why the first argument is passed as a Null

Copy link
Author

@taylorzane taylorzane Aug 25, 2017

Choose a reason for hiding this comment

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

So that's actually the object passed to the pskCallback function, where the first argument (initially defined as Null) is the hint provided by OpenSSL/TLS. The hint is an optional field, which is why we're not erroring when it's not provided.


function startTest() {
function connectClient(options, next) {
const client = tls.connect(common.PORT, 'localhost', options, () => {
Copy link
Member

Choose a reason for hiding this comment

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

s/common.PORT/0


client.end(TEST_DATA);

client.on('data', (data) => {
Copy link
Member

Choose a reason for hiding this comment

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

perhaps wrap this in a common.mustCallAtLeast()

assert.strictEqual(data.toString(), TEST_DATA);
});

client.on('close', next);
Copy link
Member

Choose a reason for hiding this comment

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

common.mustCall(next)

let gotWorld = false;
let opensslExitCode = -1;

server.listen(common.PORT, () => {
Copy link
Member

Choose a reason for hiding this comment

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

s/common.PORT/0

note that in general we've been moving away from using common.PORT in the parallel tests.

taylorzane and others added 22 commits September 30, 2018 23:53
…hange` to use `Function::Call`. Updated PSK callbacks to use new/non-deprecated functions.
…f8`. Updated PSK callbacks to use `MaybeLocal` overload of Function::Call.
…ded some `mustCall` wrappers in TLS-PSK circuit test.
…d TLS-PSK callback to ensure that the returned value is not null.
Add the `pskCallback` client/server option, which resolves an identity
or identity (hint) to a pre-shared key. The function signature on client
and server is compatible.

Add the `pskIdentity` server option to set the identity hint for the
ServerKeyExchange message.

Co-authored-by: Chris Osborn <chris.osborn@sitelier.com>
Co-authored-by: stephank <gh@stephank.nl>
Co-authored-by: Denys Otrishko <shishugi@gmail.com>
@taylorzane
Copy link
Author

Alright, I believe I've rebased correctly (again), and cherry-picked @lundibundi's commit. Though, I'm not seeing my changes appear in the "Commits" or "Files changed" tabs. Is there a way to resolve that issue?

@lundibundi lundibundi reopened this Oct 1, 2018
@lundibundi
Copy link
Member

lundibundi commented Oct 1, 2018

@taylorzane you just had to reopen the PR. =)
Though it seems you didn't remove all of the previous commits (I squashed them in one there were becoming too much of them, therefore all of them are present in my commit, unless you were making changes and havent pushed them to github) and have a few commits from master here, did you perhaps use git merge and not git rebase?

@taylorzane
Copy link
Author

Ah, I see what you did. I didn't actually remove the previous commits, I missed that part of your comment. Let me see what I can do about this this morning.

@lundibundi
Copy link
Member

@taylorzane ping, will you have time for this?
Also, could you look through my note at the end here #23188 (comment) if you haven't seen it yet?

@Trott
Copy link
Member

Trott commented Nov 13, 2018

@jasnell @addaleax Do either of you want to rebase this and push it back up? Or should we close this PR?

@lundibundi
Copy link
Member

@Trott I continued working on this in #23188.
I'll close this for now in favor of that PR.

@lundibundi lundibundi closed this Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.