Node: tls.connect Error: self signed certificate when connect to imap.gmail.com:933

[CoNETProject]

• Version: v12.0.0 ~ v12.4.0 (Old ~ v11.15.0 have no same issue )
• Platform: MacOS, Debian, Windows
• Subsystem: no
• Module (and version) (if relevant): tls.connect(options[, callback])

Error: self signed certificate

When?
looks happened connect to imap.gmail.com:933 only.

imap.mail.me.com have no same issue.

//  This only happen at v12.0.0 ~ v12.4.0 (Old version ~ v11.15.0 have no same issue )

const socket = tls.connect (993, 'imap.gmail.com', () => {
    ...
)
socket.on('error', err => {
    // Will ERROR!
    // Error: self signed certificate
    ...
})

//  Apple mail have no same problem
const socket = tls.connect (993, 'imap.mail.me.com', () => {
    ...
)
socket.on('error', err => {
    ...
})

I checked tls connect with openssl command to imap.gmail.com at same OS, looks Good. It have not Man-in-the-middle attack OS. The code have no problem when I back NodeJS to v11.

[addaleax]

@nodejs/crypto

[bnoordhuis]

The issue is that Node.js (no longer?) sends a SNI record by default. You can work around it like this:

const options = {
  port: 993,
  host: 'imap.gmail.com',
  servername: 'imap.gmail.com',  // SNI
};
tls.connect(options, () => { /* ... */ });

It might have been caused by an openssl upgrade because I can reproduce with out/Release/openssl-cli, that also won't validate unless you pass -servername imap.gmail.com.

(GMail helpfully sends back a certificate with Issuer: OU = "No SNI provided; please fix your client.", CN = invalid2.invalid ^^)

[CoNETProject]

Thank you.
Looks have no problem now via use both host and servername.

[silverwind]

I can reproduce with out/Release/openssl-cli

Sending SNI on the openssl CLI was always opt-in via -servername, so that's probably not related.

[sam-github]

This might be a bug. 11.x and 12.x have the same OpenSSL version, but different behaviour, so it looks like something changed in node, not in openssl:

w/core % cat imap.gmail.js
console.log(process.versions.openssl);
require('tls').connect(993, 'imap.gmail.com', function() {
  console.log('CONNECT');
  this.end();
});
w/core % nvm run 11 imap.gmail.js
Running node v11.15.0 (npm v6.7.0)
1.1.1b
CONNECT
w/core % nvm run 12 imap.gmail.js
Running node v12.4.0 (npm v6.9.0)
1.1.1b
events.js:177
      throw er; // Unhandled 'error' event
      ^

Error: self signed certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1317:34)
    at TLSSocket.emit (events.js:200:13)
    at TLSSocket._finishInit (_tls_wrap.js:792:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:606:12)
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:91:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
    at processTicksAndRejections (internal/process/task_queues.js:84:9) {
  code: 'DEPTH_ZERO_SELF_SIGNED_CERT'
}

[sam-github]

I'm bisecting, maybe I can pinpoint a specific change.

[bnoordhuis]

Sending SNI on the openssl CLI was always opt-in via -servername, so that's probably not related.

The reason I mention it is that the stock openssl s_client on my system works without -servername.

[silverwind]

openssl s_client on my system works without -servername

We're offtopic but it's because it was changed in 1.1.1 as per this changelog entry:

s_client will now send the Server Name Indication (SNI) extension by
default unless the new "-noservername" option is used. The server name is
based on the host provided to the "-connect" option unless overridden by
using "-servername".

[sam-github]

bisect pointed to 42dbaed4605f44c393a057aad75a31cac1d0e5f5, my commit to add TLS1.3 support, and indeed it works with TLS1.2:

core/lts (master $% u=) % ./out/Release/node --tls-max-v1.3 ../imap.gmail.js
1.1.1b
events.js:177
      throw er; // Unhandled 'error' event
      ^

Error: self signed certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1317:34)
    at TLSSocket.emit (events.js:200:13)
    at TLSSocket._finishInit (_tls_wrap.js:792:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:606:12)
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:91:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
    at processTicksAndRejections (internal/process/task_queues.js:74:11) {
  code: 'DEPTH_ZERO_SELF_SIGNED_CERT'
}
core/lts (master $% u=) % ./out/Release/node --tls-max-v1.2 ../imap.gmail.js 
1.1.1b
CONNECT

However, running with --trace-tls I see something strange: SNI is not sent for TLS1.2 or TLS1.3 (by default), but only for TLS1.3 is the server returning the bogus self-signed cert with Subject = Issuer = "No SNI provided; please fix your client.", CN = invalid2.invalid. If I add SNI, it does fix the TLS1.3 connect, but why? Is the SNI extension required for TLS1.3? Well, no, but it is by google's servers: https://github.com/openssl/openssl/issues/5362#issuecomment-365617850

So, this is confirmed as a gmail bug.

Possibly, tls.connect() should default servername: to host: (in the case that host is a name and not an IP address) as http.request() does, but that would be a breaking change.

Thoughts?

More discussion: https://mta.openssl.org/pipermail/openssl-project/2018-April/000623.html

[bnoordhuis]

I suppose GMail is technically in the right because the spec says that a server MAY require SNI and imap.gmail.com does....

...although it's "interesting" that it doesn't actually care _what_ you set the servername to: it happily accepts -servername panopticon.bigbro. Can I suggest we add a special case for imap.gmail.com and send that servername? ^^

We could turn on SNI by default although that leaks the servername because the SNI payload is unencrypted (and it'll be a while before ESNI is a thing.)

[silverwind]

I guess we have to default to sending a SNI (and mimic what browsers do). It is indeed a data leak, but I guess more services will work with it present than without.

BTW, I don't expect ESNI in the current draft form to get any widespread adoption because of it's complexity.

[sam-github]

@silverwind We already mimic what browsers do by defaulting to sending SNI for https (see Agent.prototype.addRequest), so it is only direct use of tls that doesn't set servername. I'll PR a default use of servername to tls.connect (unless someone else beats me to it).

[bnoordhuis]

@sam-github Did you open that PR and if so, can you link to it?

[sam-github]

@bnoordhuis nope, still on my todo list

[edmorley]

Is the SNI extension required for TLS1.3? Well, no, but it is by google's servers: openssl/openssl#5362 (comment)

Hi! Just to add some clarification here - RFC 8446 section 9.2 says:

Additionally, all implementations MUST support the use of the
"server_name" extension with applications capable of using it.
Servers MAY require clients to send a valid "server_name" extension.
Servers requiring this extension SHOULD respond to a ClientHello
lacking a "server_name" extension by terminating the connection with
a "missing_extension" alert.

ie: The "MAY" only refers to GMail choosing to be strict about this - clients are covered by the "MUST" - so Node's tls.connect is currently not spec-compliant.

[ronag]

@sam-github

I'll PR a default use of servername to tls.connect (unless someone else beats me to it).

What would be the default for servername? The hostname if a name and not an IP?

[ronag]

@mildsunrise

[mildsunrise]

I'm not familiar with https but servername is calculated here and yes, we derive it from the Host header and don't set it for IPs.

TBH I only recently found tls doesn't set SNI by default, I can agree it's not intuitive but it seems to be conventional. Maybe we could mention it more explicitely in the docs?

[mildsunrise]

I propose to add the warning and close this (but regardless, I'm not against changing the default if we decide to do so)

[mildsunrise]

@edmorley sorry, I hadn't seen your comment...

clients are covered by the "MUST" - so Node's tls.connect is currently not spec-compliant.

But that "MUST" only requires implementations to support the use of SNI. We do support it, we just don't enable it by default, right?

[edmorley]

@mildsunrise Hi! Yeah I suppose that's technically correct, however I feel like having the default tls.connect API behaviour be non-spec-compliant wouldn't match user expectations?

Perhaps the best option is to add a warning to the docs for now, and then in the next Node major make it the default for TLS 1.3 connections only?

[ronag]

make it the default for TLS 1.3 connections only?

Sorry to bump in. This is the part I'm missing. Make what exactly the default, i.e. what is "it"?

[mildsunrise]

Current behaviour doesn't match user expectations, that's for sure (but it is spec-compliant imo, since it's the application (i.e. the user) who's in charge of enabling it)

I understand you want to enable SNI by default for TLS 1.3 connections?

[edmorley]

but it is spec-compliant imo

I'm not saying that tls.connect is not spec compliant per se, but that the "out of the box defaults" make it easy to accidentally write a non-spec-compliant client.

This is the part I'm missing. Make what exactly the default, i.e. what is "it"?

To hopefully clarify:

• tls.connect does not enable SNI by default (presumably a combination of "because it never has since SNI is newish" + the privacy reasons mentioned above)
• for TLS 1.2 and older, this is not normally a problem, however the TLS 1.3 spec says that clients MUST enable SNI
• whilst the end user is ultimately responsible for writing a client to correctly meet the spec, and tls.connect is ultimately a pretty low-level API, from an API user-experience point of view it still seems best to have the tls.connect defaults match the requirements of the spec
• ie instead of documenting that users should enable SNI (by setting servername to the same value as host) manually, tls.connect could do so by default iff host is not an IP and the protocol is TLS 1.3

Anyway happy to let others make the judgement call on this :-)

[mildsunrise]

Oh, TLSv1.3 mandates SNI? If you're referring to the paragraph where it says «In the absence of an application profile standard specifying otherwise, a TLS-compliant application MUST implement» like you said it's the application who's responsible of following spec (but yes, I agree that this is a valid reason for us to enable SNI by default).

If we want to enable SNI by default, I'd vote we do it on all connections... doing it only for TLSv1.3 could be confusing, IMHO.

Edit: The potential for breakage is small by itself, but as @bnoordhuis said we'll be suddenly leaking data and this could break applications that run behind an SNI-based firewall, for instance.

[edmorley]

I'd vote we do it on all connections... doing it only for TLSv1.3 could be confusing, IMHO.

I agree, I only mentioned that as a way to mitigate the privacy concerns for pre -TLSv1.3 use-cases, but (a) it would be confusing, (b) in those cases users could always disable it explicitly if desired I suppose.

[bnoordhuis]

RFC 8446 lists SNI in the mandatory-to-implement extensions section but nowhere does it seem to say TLSv1.3 clients MUST use SNI. SHOULD yes, MUST no.

https://tools.ietf.org/html/rfc8446#section-4.4.2.2:

   -  The "server_name" [RFC6066] and "certificate_authorities"
      extensions are used to guide certificate selection.  As servers
      MAY require the presence of the "server_name" extension, clients
      SHOULD send this extension, when applicable.

https://tools.ietf.org/html/rfc8446#section-9.2:

   Additionally, all implementations MUST support the use of the
   "server_name" extension with applications capable of using it.
   Servers MAY require clients to send a valid "server_name" extension.
   Servers requiring this extension SHOULD respond to a ClientHello
   lacking a "server_name" extension by terminating the connection with
   a "missing_extension" alert.

(server_name = SNI)

[mildsunrise]

Warning added! On whether to enable SNI by default, I'd recommend to open a PR or separate issue to continue discussion there