tls: stronger validation for 'servername' in server.addContext #33943

mkrawczuk · 2020-06-18T14:44:41Z

If 'servername' is not provided, 'ERR_MISSING_ARGS is thrown.
If 'servername' is not a string, 'ERR_INVALID_ARG_TYPE' is thrown.
If 'servername' is an IP address, 'ERR_INVALID_ARG_VALUE' is thrown, since
literal IPv4 and IPv6 addresses are not permitted in SNI.

Fixed API documentation ('hostname' -> 'servername').

Also removed a redundant error 'ERR_TLS_REQUIRED_SERVER_NAME'.

This PR was inspired by: #19988

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

If 'servername' is not provided, 'ERR_MISSING_ARGS is thrown. If 'servername' is not a string, 'ERR_INVALID_ARG_TYPE' is thrown. If 'servername' is an IP address, 'ERR_INVALID_ARG_VALUE' is thrown, since literal IPv4 and IPv6 addresses are not permitted in SNI. Fixed API documentation ('hostname' -> 'servername'). Also removed a redundant error 'ERR_TLS_REQUIRED_SERVER_NAME'.

doc/api/errors.md

addaleax · 2020-06-19T13:30:57Z

@nodejs/crypto

nodejs-github-bot · 2020-06-19T13:31:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31942/

doc/api/errors.md

mildsunrise · 2020-06-20T09:42:30Z

For consistency, maybe it would be good to apply these validations to client connections too?
Our current policy is to emit a warning for IPs:

node/lib/_tls_wrap.js

Lines 1618 to 1629 in fdf10ad

    
           if (options.servername) { 
        
             if (!ipServernameWarned && net.isIP(options.servername)) { 
        
               process.emitWarning( 
        
                 'Setting the TLS ServerName to an IP address is not permitted by ' + 
        
                 'RFC 6066. This will be ignored in a future version.', 
        
                 'DeprecationWarning', 
        
                 'DEP0123' 
        
               ); 
        
               ipServernameWarned = true; 
        
             } 
        
             tlssock.setServername(options.servername); 
        
           }

mildsunrise · 2020-06-20T09:45:51Z

The warning message says it will be ignored in a future version, rather than rejected...
I would vote for rejection here, but I don't have a strong opinion 🤔

mkrawczuk · 2020-06-20T12:44:43Z

Me neither. I would leave it as is for now. I can investigate later and open a separate PR for it.

codecov-commenter · 2020-06-21T03:27:59Z

Codecov Report

Merging #33943 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master   #33943    +/-   ##
========================================
  Coverage   96.68%   96.69%            
========================================
  Files         204      204            
  Lines       67657    67795   +138     
========================================
+ Hits        65416    65556   +140     
+ Misses       2241     2239     -2

Impacted Files	Coverage Δ
lib/internal/process/task_queues.js	`100.00% <ø> (ø)`
lib/dgram.js	`98.36% <100.00%> (+0.01%)`	⬆️
lib/domain.js	`98.00% <100.00%> (+<0.01%)`	⬆️
lib/internal/async_hooks.js	`98.27% <100.00%> (+0.09%)`	⬆️
lib/internal/console/constructor.js	`100.00% <100.00%> (ø)`
lib/internal/constants.js	`100.00% <100.00%> (ø)`
lib/internal/errors.js	`96.93% <100.00%> (+<0.01%)`	⬆️
lib/internal/event_target.js	`92.35% <100.00%> (+0.16%)`	⬆️
lib/internal/http2/compat.js	`98.46% <100.00%> (+<0.01%)`	⬆️
lib/internal/modules/esm/module_job.js	`100.00% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4f3206...cb77037. Read the comment docs.

mkrawczuk · 2020-08-05T22:43:00Z

This PR seems to have gotten a little stuck. Is there anything that should be done to move further?
cc @jasnell @addaleax @mildsunrise

tniessen · 2020-08-06T14:05:58Z

lib/_tls_wrap.js

@@ -1411,7 +1411,19 @@ Server.prototype.setOptions = deprecate(function(options) {
 // SNI Contexts High-Level API
 Server.prototype.addContext = function(servername, context) {
  if (!servername) {
-    throw new ERR_TLS_REQUIRED_SERVER_NAME();
+    throw new ERR_MISSING_ARGS('servername');


This makes it semver-major, I think? (Meaning that it cannot land on any of the active release lines.)

tniessen · 2020-08-06T14:09:13Z

doc/api/tls.md

@@ -545,18 +545,19 @@ called:
 * `tlsSocket` {tls.TLSSocket} The `tls.TLSSocket` instance from which the
  error originated.

-### `server.addContext(hostname, context)`
+### `server.addContext(servername, context)`


I think hostname is better. This is how TLS defines a "server name":

struct { NameType name_type; select (name_type) { case host_name: HostName; } name; } ServerName;

It is correct that TLS does not allow IP addresses in the HostName field, but that is not necessarily true for ServerName. TLS simply doesn't support other NameTypes so far, but it might do so in the future.

I see the point, but... the extension has never changed and "server name" === "host name server name" in everyone's minds. I personally don't think it's worth it to expose this detail to users, nor have I seen any other API do it.

So I'm personally +1 for naming the parameter servername and stating things like "Setting the TLS ServerName to an IP address is not permitted", as we currently do.

tniessen · 2020-08-06T14:10:01Z

lib/_tls_wrap.js

+    throw new ERR_INVALID_ARG_TYPE('servername', 'string', servername);
+  }
+
+  if (net.isIP(servername)) {


Out of curiosity, what happens if servername is neither an IP nor a DNS hostname?

mildsunrise · 2020-08-08T12:37:00Z

I'm still not sure about throwing on IP addresses... I suppose it would break some code.
In any case, I think we should be consistent and apply the same validations on tls.connect too.

aduh95 · 2020-11-17T15:09:30Z

@mkrawczuk This needs a rebase to resolve the git conflict.

github-actions · 2020-11-17T15:09:54Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2021-01-09T00:17:57Z

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. labels Jun 18, 2020

mkrawczuk added 2 commits June 18, 2020 18:03

fixup! remove a nasty backtick.

6532918

fixup! correct doc link.

38d50aa

addaleax reviewed Jun 19, 2020

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 19, 2020

fixup! add deleted error into legacy codes.

9149d3b

jasnell reviewed Jun 19, 2020

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

fixup!

cb77037

github-actions bot mentioned this pull request Jun 20, 2020

CI Relability 2020-06-20 JaneaSystems/nodejs-reliability#13

Open

34 tasks

github-actions bot mentioned this pull request Jun 21, 2020

CI Relability 2020-06-21 JaneaSystems/nodejs-reliability#14

Open

31 tasks

This was referenced Jun 22, 2020

CI Relability 2020-06-22 JaneaSystems/nodejs-reliability#15

Open

CI Relability 2020-06-23 JaneaSystems/nodejs-reliability#16

Open

CI Relability 2020-06-24 JaneaSystems/nodejs-reliability#17

Open

CI Relability 2020-06-25 JaneaSystems/nodejs-reliability#18

Open

tniessen reviewed Aug 6, 2020

View reviewed changes

aduh95 added the stalled Issues and PRs that are stalled. label Nov 17, 2020

github-actions bot closed this Jan 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: stronger validation for 'servername' in server.addContext #33943

tls: stronger validation for 'servername' in server.addContext #33943

mkrawczuk commented Jun 18, 2020

addaleax commented Jun 19, 2020

nodejs-github-bot commented Jun 19, 2020

mildsunrise commented Jun 20, 2020

mildsunrise commented Jun 20, 2020

mkrawczuk commented Jun 20, 2020

codecov-commenter commented Jun 21, 2020

mkrawczuk commented Aug 5, 2020

tniessen Aug 6, 2020

tniessen Aug 6, 2020

mildsunrise Aug 8, 2020

tniessen Aug 6, 2020

mildsunrise commented Aug 8, 2020

aduh95 commented Nov 17, 2020

github-actions bot commented Nov 17, 2020

github-actions bot commented Jan 9, 2021

tls: stronger validation for 'servername' in server.addContext #33943

tls: stronger validation for 'servername' in server.addContext #33943

Conversation

mkrawczuk commented Jun 18, 2020

addaleax commented Jun 19, 2020

nodejs-github-bot commented Jun 19, 2020

mildsunrise commented Jun 20, 2020

mildsunrise commented Jun 20, 2020

mkrawczuk commented Jun 20, 2020

codecov-commenter commented Jun 21, 2020

Codecov Report

mkrawczuk commented Aug 5, 2020

tniessen Aug 6, 2020

Choose a reason for hiding this comment

tniessen Aug 6, 2020

Choose a reason for hiding this comment

mildsunrise Aug 8, 2020

Choose a reason for hiding this comment

tniessen Aug 6, 2020

Choose a reason for hiding this comment

mildsunrise commented Aug 8, 2020

aduh95 commented Nov 17, 2020

github-actions bot commented Nov 17, 2020

github-actions bot commented Jan 9, 2021