Revert same port http -> https redirect #10930

jbudz · 2017-03-29T16:19:40Z

httpolyglot is removed, we no longer automatically redirect from http to https
server.ssl.redirectHttpFromPort option added to allow for http -> https redirect from one port to another
We no longer start the dev server with tls by default, it can be turned on with the --ssl flag, npm start -- --ssl, or ./bin/kibana --dev --ssl
There will be error log messages if you connect over the wrong protocol, we have [logging] Downgrade hapi connection errors when connecting with the w… #11209 for downgrading these

Release Note: Kibana 5.x redirected requests from http to https on the same port if TLS was configured. Starting in Kibana 6.0.0 Kibana no longer redirects basic http traffic to https.

epixa · 2017-03-29T16:40:56Z

I got to thinking about this recently. I think there may still be value in http->https redirection, I just think the current approach was too heavy handed. Doing this on a single port is kind of nuts, but if they are binding to port 443 for SSL, it certainly would be convenient to also bind to port 80 purely for the sake of redirection. This would be the obvious use case for most people, I think.

Anyone thoughts?

Edit: This wouldn't require third party dependencies.

tylersmalley · 2017-03-29T17:04:38Z

@epixa to accomplish that wouldn't we need to expose an additional port and provide configuration for it?

jbudz · 2017-03-29T17:56:59Z

I'll make an update, something like server.ssl.redirectPort.

tylersmalley · 2017-03-29T18:04:00Z

Thoughts on server.httpPort and server.httpsPort? server.port could set both of the values to provide backwards compatibility. To allow for redirection httpPort and httpsPort would have to differ.

epixa · 2017-03-29T18:05:54Z

Would it need any other configurations? The use case is you pointing hostname to Kibana, and running Kibana with ssl.enabled. In that case, can we not just assume port 80 with all the same relevant configurations from the main Kibana config (e.g. basePath, etc)? I don't see why we would need to make this port configurable.

I could imagine someone wanting to disable this functionality, though.

tylersmalley · 2017-03-29T18:08:37Z

Using port 80 would require privileged permissions to start Kibana. It's possible, but it would be a large change from what we have now. In addition, we would be more prone to port conflicts.

epixa · 2017-03-29T18:18:33Z

@tylersmalley As would 443 (or any other port that doesn't show up in URLs), so that should be fine. In other words, someone should only be able to trigger this behavior if they are already using privileged permissions.

If someone configures Kibana to use SSL on port 5601, for example, then there's no need to do extra redirects.

The idea here is that if people are exposing Kibana directly to the web, we'll handle https redirects from port 80 to ensure their browser redirects to https. But if they put Kibana behind a reverse proxy, they should handle the https redirect on their own.

tylersmalley · 2017-03-29T18:30:05Z

@epixa, I believe we are on the same page but I might have been miss-directed by your previous comment about not requiring additional configuration beyond hostname and ssl.enabled. With this, we will still need the ability to configure the http/https ports. If we default to any port under 1024 we will most definitely need a way for them to change the ports and not require root to run.

tylersmalley · 2017-03-29T18:56:01Z

Let's decouple the redirect discussion from the PR and leave it as-is. I will create a new issue documenting the solutions brought up thus far.

tylersmalley

This LGTM - I have created a new issue to track the discussion on how to handle the redirection.

tylersmalley · 2017-03-29T23:30:36Z

Here is that issue: #10948

ycombinator

LGTM. This seems like a breaking change - links to http://... would now no longer automagically work. Should this be documented somewhere?

ycombinator

Spoke too soon with the LGTM :)

Besides documenting the breaking change, there's another thing I just noticed running this PR:

If I make a request to http://localhost:5601, I see errors like this in the Kibana server log:

server    log   [18:32:52.755] [error][client][connection] Error: 140736487396288:error:1407609C:SSL routines:SSL23_GET_CLIENT_HELLO:http request:../deps/openssl/openssl/ssl/s23_srvr.c:394:

    at Error (native)

ycombinator · 2017-03-31T18:39:17Z

Per out-of-band conversation with @jbudz, the server log error I mentioned in the previous comment will be addressed via #10929.

If we merge this PR, the server log error will be reported a lot more frequently so I think we should try to resolve 10929 before this PR can be merged.

spalger · 2017-04-01T20:05:42Z

For the record, I really don't like the way this feels in development

epixa · 2017-04-03T16:59:05Z

@spalger Perhaps this is something we could bake into the basePath proxy (and rename it to dev proxy or something).

spalger · 2017-04-03T22:05:26Z

@jbudz what do you think about keeping this for dev? I'm 100% for it

jbudz · 2017-04-03T22:10:31Z

Leaving it in dev mode minimizes any concerns I had with it, so I'm up for it. We already have the base path proxy which we of course aren't shipping either.

It was frustrating me while writing this PR, but I figured once my browser autocomplete fixed on the right protocol it would be fine.

jbudz · 2017-04-03T22:13:27Z

Is anyone against this in dev mode only?

spalger · 2017-04-03T22:27:01Z

I've been using it for a couple days and keep feeling dislike :/

tylersmalley · 2017-04-03T22:35:11Z

I am fine keeping it only for development. We can always review down the road, but this at least removes it from builds for now.

epixa

The more I think about it, the more important the port 80 redirection seems to be, and I don't think we should move forward with this change without it in the same PR. The comment that I left on the breaking change is just one example of the strangeness this will have if we went forward as-is. And the error messages thing when accessed using the wrong protocol is probably worth blocking the release on, so merging this PR effectively puts the release in limbo until we address the other thing.

I actually think the error message itself is fine so long as the port redirection feature is available, though it would be awesome if we could provide a more useful error message that points people to this configuration.

epixa · 2017-05-20T15:11:18Z

docs/migration/migrate_6_0.asciidoc

+=== Removed same port http to https redirect behavior
+*Details:* Kibana 5.x redirected requests from http to https on the same port if TLS was configured.  This was removed.
+
+*Impact:* You may need to add a redirect on a different port if a redirect is expected.


I know the plan here is to follow up with a different PR to handle port redirection stuff, but when this specific PR is merged, the reality is going to be that there is no way to do http -> https redirection in Kibana. We should update the breaking changes impact to mention that the only way to accomplish this would be to custom handle http -> https redirection in a reverse proxy or at the load balancer.

The follow up PR that handles http -> https support will need to then modify this breaking change documentation, obviously.

My overall review comment makes this feedback moot, but I'm leaving it here for posterity's sake.

epixa · 2017-05-20T15:12:34Z

docs/migration/migrate_6_0.asciidoc

+
+[float]
+=== Removed same port http to https redirect behavior
+*Details:* Kibana 5.x redirected requests from http to https on the same port if TLS was configured.  This was removed.


Rather than "This was removed", let's err on the side of more information even if it seems a bit redundant. Perhaps "Kibana 5.x redirected requests from http to https on the same port if TLS was configured. Starting in Kibana 6.0.0, Kibana no longer redirects basic http traffic to https."

kimjoar · 2017-07-06T13:04:56Z

I pushed a suggestion for redirecting http to https here. @epixa can you take a look?

This reverts commit 47030b8.

epixa · 2017-07-07T12:48:27Z

tasks/lib/notice/__tests__/notice.js

@@ -9,11 +9,11 @@ const NODE_MODULES = resolve(__dirname, '../../../../node_modules');
 const NODE_DIR = resolve(process.execPath, '../..');
 const PACKAGES = [
  {
-    name: '@elastic/httpolyglot',
-    version: '0.1.2-elasticpatch1',
+    name: '@spalger/filesaver',


Let's switch this to being a controlled package that we know will always exist because we've created it specifically for this purpose. We can create an npm package under the @elastic org that does nothing except provide the license we want to verify against, and then we can depend on that package in our devDependencies. We should never need to update this test just because we decided to no longer depend on a given package in our source.

Okey if I do it in a separate PR? (Just want to make sure this gets ready for 6.0)

epixa · 2017-07-07T12:51:54Z

src/server/http/setup_redirect_server.js

+import { format as formatUrl } from 'url';
+import Hapi from 'hapi';
+
+// If a redirect port is specified, we need to start a server a non-ssl


we need to start a server a non-ssl server at this doesn't english good.

epixa · 2017-07-07T12:54:56Z

src/server/http/setup_redirect_server.js

+  const host = config.get('server.host');
+  const sslPort = config.get('server.port');
+
+  const redirectServer = new Hapi.Server();


We shouldn't block on this, but I really wish we just used a native node http server for this. I have much greater confidence in that abstraction in general than I do in hapi, both in terms of how lightweight it is, and how stable the API is.

This works though.

epixa

Can we provide a more useful error message before exiting when the redirect port is the same as the main port? Right now it just does a generic error:

server    log   [13:11:39.894] [fatal] Port 5601 is already in use. Another instance of Kibana may be running!
FATAL Port 5601 is already in use. Another instance of Kibana may be running!

But ideally we'd instead say something specific to let the user know that ssl.server.redirectHttpFromPort cannot be set to the same port as server.port, or something like that.

epixa · 2017-07-07T13:20:27Z

LGTM other than the generic error message

We're going to tackle the downgrading alongside this PR, so this feedback should be addressed with that.

jbudz added Team:Operations Team label for Operations Team release_note:breaking review v6.0.0 labels Mar 29, 2017

jbudz requested review from tylersmalley and solostyle March 29, 2017 16:19

tylersmalley approved these changes Mar 29, 2017

View reviewed changes

ycombinator self-requested a review March 31, 2017 18:06

ycombinator approved these changes Mar 31, 2017

View reviewed changes

ycombinator previously requested changes Mar 31, 2017

View reviewed changes

jbudz force-pushed the revert/httpolyglot branch from 0e7bd10 to b0e1482 Compare April 3, 2017 21:47

jbudz added the updates_needed label Apr 3, 2017

epixa suggested changes May 20, 2017

View reviewed changes

epixa removed review v6.0.0-alpha2 labels Jun 2, 2017

jbudz and others added 7 commits July 6, 2017 18:57

Revert httpolyglot

c5903c9

Remove httpolyglot from package.json

221970c

Add breaking changes docs for http redirect

165f05c

Use httpolyglot in dev mode

dd6b6f8

Revert "Use httpolyglot in dev mode"

3a7a835

This reverts commit 47030b8.

[dev mode] Turn tls off by default

82360c4

Redirect from http to https

6e415ce

kimjoar force-pushed the revert/httpolyglot branch from 83b6a9a to 6e415ce Compare July 6, 2017 16:59

kimjoar added 3 commits July 6, 2017 21:26

No longer use 'httpolyglot' in notice text tests

fb8f624

Update migration doc

dfc0cfb

fix config setting path

89bf3f1

epixa reviewed Jul 7, 2017

View reviewed changes

epixa suggested changes Jul 7, 2017

View reviewed changes

english yo

df08d01

words

9da9690

epixa approved these changes Jul 7, 2017

View reviewed changes

Handle redirect server failing to start

f2f71d7

kimjoar merged commit c79e223 into elastic:master Jul 7, 2017

kimjoar mentioned this pull request Jul 7, 2017

Add docs about 'redirectHttpFromPort' setting #12708

Merged

jbudz mentioned this pull request Jul 18, 2017

Redirect to HTTPS when SSL is enabled #10948

Closed

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

azasypkin mentioned this pull request Jun 22, 2018

Move BasePathProxy to the new platform #19424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert same port http -> https redirect #10930

Revert same port http -> https redirect #10930

jbudz commented Mar 29, 2017 •

edited by kimjoar

Loading

epixa commented Mar 29, 2017 •

edited

Loading

tylersmalley commented Mar 29, 2017

jbudz commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

epixa commented Mar 29, 2017

tylersmalley commented Mar 29, 2017 •

edited

Loading

epixa commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

tylersmalley left a comment

tylersmalley commented Mar 29, 2017

ycombinator left a comment

ycombinator left a comment

ycombinator commented Mar 31, 2017 •

edited

Loading

spalger commented Apr 1, 2017

epixa commented Apr 3, 2017

spalger commented Apr 3, 2017

jbudz commented Apr 3, 2017

jbudz commented Apr 3, 2017

spalger commented Apr 3, 2017

tylersmalley commented Apr 3, 2017

epixa left a comment

epixa May 20, 2017

epixa May 20, 2017

epixa May 20, 2017

epixa May 20, 2017

kimjoar commented Jul 6, 2017

epixa Jul 7, 2017

kimjoar Jul 7, 2017

epixa Jul 7, 2017

epixa Jul 7, 2017

epixa Jul 7, 2017

epixa left a comment

epixa commented Jul 7, 2017

Revert same port http -> https redirect #10930

Revert same port http -> https redirect #10930

Conversation

jbudz commented Mar 29, 2017 • edited by kimjoar Loading

epixa commented Mar 29, 2017 • edited Loading

tylersmalley commented Mar 29, 2017

jbudz commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

epixa commented Mar 29, 2017

tylersmalley commented Mar 29, 2017 • edited Loading

epixa commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

tylersmalley commented Mar 29, 2017

tylersmalley left a comment

Choose a reason for hiding this comment

tylersmalley commented Mar 29, 2017

ycombinator left a comment

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

ycombinator commented Mar 31, 2017 • edited Loading

spalger commented Apr 1, 2017

epixa commented Apr 3, 2017

spalger commented Apr 3, 2017

jbudz commented Apr 3, 2017

jbudz commented Apr 3, 2017

spalger commented Apr 3, 2017

tylersmalley commented Apr 3, 2017

epixa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa left a comment

Choose a reason for hiding this comment

epixa commented Jul 7, 2017

jbudz commented Mar 29, 2017 •

edited by kimjoar

Loading

epixa commented Mar 29, 2017 •

edited

Loading

tylersmalley commented Mar 29, 2017 •

edited

Loading

ycombinator commented Mar 31, 2017 •

edited

Loading