General improvements to HTTPS API

[Giotino]

Checklist

• I have read the documentation.
• I have included a pull request description of my changes.
• I have included some tests.
• If it's a new feature, I have included documentation updates.

Fixes #1254

checkServerIdentity signature:

(hostname: string, certificate: PeerCertificate) => Error | undefined

According to DefinitelyTyped certificate is PeerCertificate, but after testing I discovered that certificate has the property issuerCertificate that's mean it's DetailedPeerCertificate

I can't find any documentation about using this property with https.Request on Node.JS, I think this is a side effect of the implementation of https.Request. But, on the Node.JS documentation there's an example using it "Example pinning on certificate fingerprint, or the public key (similar to pin-sha256)"

Node.JS even have a test about it:
https://github.com/nodejs/node/blob/8b4af64f50c5e41ce0155716f294c24ccdecad03/test/parallel/test-https-client-checkServerIdentity.js

[Giotino]

With this PR I also wanted to add the documentation described in #1191

[Giotino]

2 out of the 3 current HTTPS test are broken, the first 2 make plain HTTP requests.

You should have used got.secure instead of got

[szmarczak]

2 out of the 3 current HTTPS test are broken, the first 2 make plain HTTP requests.

You should have used got.secure instead of got

Could you fix that please? 🙏

[Giotino]

2 out of the 3 current HTTPS test are broken, the first 2 make plain HTTP requests.
You should have used got.secure instead of got

Could you fix that please? 🙏

Yeah, I'm working on it

[Giotino]

I was working on a test for checkServerIdentity and i discovered that it's not called when connecting to 127.0.0.1 (and other localhost addresses)...

const tls = require('tls');

tls.connect({
  host: '127.0.0.1',
  port: 49637,
  checkServerIdentity: (hostname, certificate) => {
    // This is never called for localhost addresses
  }
})

Of course there's no documentation by Node.JS, there's only a comment in an example:

// Necessary only if the server's cert isn't for "localhost".
checkServerIdentity: () => { return null; },

I've to keep in mind this when writing the documentation, also the test need to be performed using an external service, we can use https://badssl.com/

[szmarczak]

Maybe there's a servername option missing? Try setting it to localhost.

[szmarczak]

Or maybe try setting host to localhost instead of 127.0.0.1...

[Giotino]

Maybe there's a servername option missing? Try setting it to localhost.
Or maybe try setting host to localhost instead of 127.0.0.1...

I've tried all of them, same result

[Giotino]

I made a mistake, it seems that checkServerIdentity is only called if the certificate is valid. That's why localhost was not working (I was testing without the test CA). I'm going to add some tests.

[Giotino]

While Options extends SecureContextOptions these features are not documented.

For example key, cert and ca useful for using a client certificate, or other params to tweak the TLS options.

My idea would be to add an HTTPS section to the documentation.

[szmarczak]

I agree.

[Giotino]

This is a first draft of the HTTPS documentation, there're some other parameter. The params that I've included there are the most used.

Since key, cert and passphrase are always used together I grouped the examples in a single block.

[Giotino]

@sindresorhus If you want to have different names (from the tls module) for the properties related to HTTPS I think that Options should stop extending SecureContextOptions and start defining its HTTPS properties and then translate them before the request.
I'm working on a commit with an example of that.

[Giotino]

"Deprecation Warning" tests must be serial or else they'll steal each other process events.

[sindresorhus]

There are a few more unresolved inline comments.

[Giotino]

There are a few more unresolved inline comments.

Sorry, I missed them, I was watching only the "Conversation" tab on Github.

[sindresorhus]

This looks great now. Thanks for your hard work on this 🙌