tls: set tlsSocket.servername as early as possible#27759
tls: set tlsSocket.servername as early as possible#27759oyyd wants to merge 2 commits intonodejs:masterfrom
Conversation
This commit makes `TLSSocket` set the `servername` property on `SSL_CTX_set_tlsext_servername_callback` so that we could get it later even if errors happen. Fixes: nodejs#27699
nodejs-github-bot
added
c++
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Left some comments but mostly LGTM. Cheers.
lib/_tls_wrap.js
Outdated
| return; | ||
|
|
||
| this.alpnProtocol = this._handle.getALPNNegotiatedProtocol(); | ||
| // The servername could be set by TLSWrap. |
There was a problem hiding this comment.
Can you be specific here and say "set by TLSWrap::SelectSNIContextCallback()"? That should save the next guy/gal some searching.
Would it make sense to make the assignment conditional on if (this.servername === undefined) to avoid duplicate work?
There was a problem hiding this comment.
Can you be specific here and say "set by TLSWrap::SelectSNIContextCallback()"? That should save the next guy/gal some searching.
Done.
Would it make sense to make the assignment conditional on if (this.servername === undefined) to avoid duplicate work?
Yes, it makes sense to me. And the default value of this.servername is null so that I have used if (this.servername === null) instead.
src/tls_wrap.cc
Outdated
| // Set the servername as early as possible | ||
| Local<Object> owner = p->GetOwner(); | ||
| USE(owner->Set(env->context(), env->servername_string(), | ||
| OneByteString(env->isolate(), servername))); |
There was a problem hiding this comment.
Tiny style nit: can you line up the arguments?
I'm a little ambivalent about discarding the return value with USE(...). I'm inclined to say it should return SSL_TLSEXT_ERR_NOACK when setting the property fails.
There was a problem hiding this comment.
Tiny style nit: can you line up the arguments?
Done.
I'm a little ambivalent about discarding the return value with USE(...). I'm inclined to say it should return SSL_TLSEXT_ERR_NOACK when setting the property fails.
Good reminding! I think returning SSL_TLSEXT_ERR_NOACK is okay as the owner looks abnormal in this scenario.
|
@bnoordhuis Thanks for the comments. PTAL. |
danbev
added
the
author ready
This commit makes `TLSSocket` set the `servername` property on `SSL_CTX_set_tlsext_servername_callback` so that we could get it later even if errors happen. Fixes: nodejs#27699 PR-URL: nodejs#27759 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
|
Landed in d2cabee, thanks! |
This commit makes `TLSSocket` set the `servername` property on `SSL_CTX_set_tlsext_servername_callback` so that we could get it later even if errors happen. Fixes: #27699 PR-URL: #27759 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
8 participants
This commit makes
TLSSocketset theservernameproperty onSSL_CTX_set_tlsext_servername_callbackso that we could get itlater even if errors happen.
Fixes: #27699
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes