Conversation
config/kibana.yml
Outdated
There was a problem hiding this comment.
thoughts on changing this to false? for the most part these config examples are set as their defaults
There was a problem hiding this comment.
I'm fine switching it to false, especially if it makes it consistent
|
Will also close #4262 |
|
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: 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: Is it possible to use the server logger with a [fatal] too? |
|
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? |
|
@spalger Do you happen to remember your original reasoning for the following code in the BasePathProxy: In my preliminary testing, if we always set and instead just server up the bad cert, more closely simulating what the user's see without the BasePathProxy. |
|
See #8034 for the reasons behind that conditional. If we set |
|
Jenkins, test this |
7ad854c to
80bfca5
Compare
|
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. |
|
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 sorry for the long delay. Yeah, I'll hop on this shortly. |
|
@jbudz no worries at all, I know it's a large one so lemme know if I can do anything to help out |
|
This branch has conflicts. I'm not sure if they are linting related. |
|
Tribe support is going to introduce a number of conflicts, so I've removed the 'review' label until I resolve all those. |
|
Closing this PR out in favor of #9823 which integrates all the tribe changes |
Per #7777 we want to move towards consistent SSL settings across the stack.
This introduces the following new settings:
server.ssl.enabledserver.ssl.keyPassphraseserver.ssl.certificateAuthoritiesserver.ssl.clientAuthenticationserver.ssl.supportedProtocolsand deprecates the following:
server.ssl.cert->server.ssl.certificateWe're currently using an undocumented setting
secureOptionswhen 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.