Consistent server.ssl settings

[kobelb]

Per #7777 we want to move towards consistent SSL settings across the stack.

This introduces the following new settings:

• server.ssl.enabled
• server.ssl.keyPassphrase
• server.ssl.certificateAuthorities
• server.ssl.clientAuthentication
• server.ssl.supportedProtocols

and deprecates the following:

• server.ssl.cert -> server.ssl.certificate

We're currently using an undocumented setting secureOptions when creating the http polyglot server; however, per nodejs/node#9025 this is acceptable and I'll be working on a pull request to node to get this documented.

[jbudz]

thoughts on changing this to false? for the most part these config examples are set as their defaults

[kobelb]

I'm fine switching it to false, especially if it makes it consistent

[jbudz]

Will also close #4262

[jbudz]

This looks great, I tested protocols, client auth, and passphrases. One minor issue I ran into:

dev mode + a cert specified in config/kibana.yml gives:

{
  statusCode: 502,
  error: "Bad Gateway",
  message: "Hostname/IP doesn't match certificate's altnames: "Host: localhost. is not cert's CN: undefined""
}

I also think it could be nice to add a friendlier error message with passphrases, I know in most areas we are just dumping a stack trace but this slightly more cryptic than usual:

FATAL Error: error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt at Error (native)...

Is it possible to use the server logger with a [fatal] too?

[kobelb]

Nice catch @jbudz you found another setting that the BasePathProxy doesn't support: keyPassphrase. I'll get that added in there.

That cryptic error is one that is thrown by the underlying OpenSSL library, I think we'd manually have to map the OpenSSL errors to something that we want the user's to see.

This error is being thrown before the server logger is configured, so we'd have to change that workflow around; what are your thoughts on this @spalger?

[kobelb]

@spalger Do you happen to remember your original reasoning for the following code in the BasePathProxy:

const httpsAgentConfig = {};
if (certificate === DEV_SSL_CERT_PATH && config.get('server.host') !== 'localhost') {
  httpsAgentConfig.rejectUnauthorized = false;
} else {
  httpsAgentConfig.ca = readFileSync(certificate);
}

In my preliminary testing, if we always set httpsAgentConfig.rejectUnauthorized = false; and the httpsAgentConfig.ca = readFileSync(certificate); we can eliminate some of those situations where we're seeing the browser print out errors like the following

{
  statusCode: 502,
  error: "Bad Gateway",
  message: "Hostname/IP doesn't match certificate's altnames: "Host: localhost. is not cert's CN: undefined""
}

and instead just server up the bad cert, more closely simulating what the user's see without the BasePathProxy.

[spalger]

See #8034 for the reasons behind that conditional.

If we set httpsAgentConfig.rejectUnauthorized = false; then there is no point in setting the ca as the agent will treat any ssl configuration as valid. I agree that better error messages from the BasePathProxy would be nice, but i don't think we should change the way that things are being done in KbnServer to accommodate them.

[kobelb]

Jenkins, test this

[kobelb]

The deprecation warnings on usage have been removed per our Zoom discussion, and the BasePathProxy has been modified to work with all settings per my Zoom discussion with @spalger.

@jbudz your peer reviews comments should have been addressed, if you wouldn't mind giving this a second look.

[kobelb]

I just talked to @epixa, and this PR won't be merged until all of the ssl settings are made consistent, so we don't risk only having some of the settings being changed and even more inconsistency.

I still think it'd be beneficial to have you guys look over it as it stands so I don't replicate the same mistakes more than once, but I'll leave that decision up to you all.

[kobelb]

@jbudz @spalger this is ready for a second review.

[epixa]

Let's target this for 5.2.0 instead of 5.1.0. It's a huge PR that hasn't undergone extensive review yet, and I suspect it would take each reviewer a good chunk of the day to review this properly. I know @spalger and @jbudz are working on other important stuff for 5.1 right now, so we might as well set the expectation now.

[kobelb]

@jbudz @spalger you guys think you'll have time to look at this after all the 5.1 release this week?

[jbudz]

@kobelb sorry for the long delay. Yeah, I'll hop on this shortly.

[kobelb]

@jbudz no worries at all, I know it's a large one so lemme know if I can do anything to help out

[epixa]

This branch has conflicts. I'm not sure if they are linting related.

[kobelb]

Tribe support is going to introduce a number of conflicts, so I've removed the 'review' label until I resolve all those.

[kobelb]

Closing this PR out in favor of #9823 which integrates all the tribe changes