conn_manager: allow to remove port from host header #10960

tehnerd · 2020-04-27T17:24:45Z

Commit Message:

add an api option/conn manager feature which would allow to remove port part from Host header (e.g. would transform
example:443 to example. this would simplify domain's matching inside virtual host as well as would not require explicit matching on "domain:port" in upstream proxies.

Additional Description:
in production we see a lot of external requests from third party libraries, which are setting port part even for requests toward common port (443). w/o this diff on envoy side workaround was to match both on "example.com" and "example.com:443" in domains configuration. and to do this on every layer of proxies. as well as modifying application, which was running behind last level envoy, to do the same.

this diff would allow to set up remove_port option on first level envoy, and everywhere just to match on "example.com"

as a side effect it should also help to resolve #886

Risk Level:
LOW
Testing:
new unit tests + manual testing
Docs Changes:
inline in proto file

repokitteh-read-only · 2020-04-27T17:24:50Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10960 was opened by tehnerd.

see: more, trace.

tehnerd · 2020-04-27T18:30:38Z

i will fix failed builds/ signoff the diff. for now just waiting for everything else to complete (to make sure that there is nothing else to fix)

euroelessar · 2020-04-27T21:57:13Z

Please add test cases for authority headers with IPv4/IPv6 addresses

zuercher

Thanks. While we wait for review from the API maintainers, I provided a couple of quick comments.

zuercher · 2020-04-27T23:53:25Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // Without setting this option, incoming requests with Host `example:443` will not match against
+  // route with `domains` match set to `example`. Defaults to `false`. Note that port removal is not part
+  // of `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
+  bool remove_port = 38;


I think the name of this field should be more specific. Something like strip_host_port. Another thing to consider is how this interacts with auto host rewrite and other host-manipulation features.

thanks. yeah. naming is hard. i will wait for other comments and will address yours in next iteration.

as for auto host - from my understanding auto host is a part of Route component of the envoy. and this port stripping modifies host header before we hit it (see "RouteShouldUseNormalizedHost" unittest)

I will be honest that my initial gut on this one is to not strip the port, but to instead add a routing option that would ignore port during route matching. I think the latter would be the most natural way of solving #886. However, I see your point about doing stripping once and then not having to worry about upstream proxies.

I would definitely like to hear from @alyssawilk and @PiotrSikora on this one.

Also prefer moving to a routing option.

as Matt mentioned, matching in routing option would solve #866 (which was bonus point in this diff). but wont allow to simplify configuration for multiple layers of proxies and backends behind them (even if envoy on 2nd layer would ignore port matching, backend behind it wont; that would increas complexity of migrating from other proxies (nginx as example) to envoy).

I thought we already removed the port when it matched the scheme, i.e. an http://foo.com:80 and https://foo.com:443 but apparently I'm misremembering? Anyway I'd prefer this as a routing option. I'm OK with a configuration guarded removal iff the port matches the listening port - if they don't match removing it feels sketchy.

on matched the scheme. (unless it was change in last few days):

curl -k -v https://localhost/ -H 'Host:localhost' * Mark bundle as not supporting multiuse < HTTP/1.1 200 OK < content-length: 7 < content-type: text/plain < date: Tue, 28 Apr 2020 19:22:16 GMT < server: envoy < curl -k -v https://localhost/ -H 'Host:localhost:443' * Mark bundle as not supporting multiuse < HTTP/1.1 404 Not Found < date: Tue, 28 Apr 2020 19:23:06 GMT < server: envoy < content-length: 0 < part of envoy's test configuration which we are hitting in this example: - name: local_service domains: ["localhost"]

as for the matching port - i'm not that familiar with this codepath. is it even possible to get listener object (and therefor it's port) from connection manager and/or route object?

ok, so seems it is possible to get NetworkConnection from ActiveStream, and therefor local port. now the question is - do you want to make it part of the route object? (and therefor force users to match on "xxx:443" in domains (in this case this diff make little sense, and new one should be made), or you are ok w/ it being part of conn manager (but with removing port only if it is equal to the listeners port)?

In a perfect world I think we should have both options, route matching with port stripping, as well as this option to strip port IFF the port matches the bound port (per @alyssawilk), but it's fine if you only want to do the latter in this PR.

zuercher · 2020-04-27T23:55:11Z

source/common/http/path_utility.h

@@ -19,6 +19,8 @@ class PathUtil {
  // Merges two or more adjacent slashes in path part of URI into one.
  // Requires the Path header be present.
  static void mergeSlashes(RequestHeaderMap& headers);
+  // Remove port part from Host/authority header
+  static void removePortsFromHost(RequestHeaderMap& headers);


I don't think this belongs in PathUtil. There's at least one other authority-related method in header_utility.h/cc's HeaderUtility class.

alyssawilk

This is looking great, especially the thorough unit testing in RemovePortsFromHost :-)

alyssawilk · 2020-04-30T17:20:12Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -506,6 +506,14 @@ message HttpConnectionManager {
  //
  // 3. Tracing decision (sampled, forced, etc) is set in 14th byte of the UUID.
  RequestIDExtension request_id_extension = 36;
+
+  // Determines if port part should be removed from Host/authority header before any processing


mind changing Host -> host here and below?
I know Host is pretty standard for HTTP/1.1 but Envoy lowercases anyway and I think it's more consistent with other API docs.

alyssawilk · 2020-04-30T17:28:12Z

source/common/http/conn_manager_impl.cc

@@ -742,6 +742,18 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
  return &connection_manager_.read_callbacks_->connection();
 }

+uint32_t ConnectionManagerImpl::ActiveStream::localPort() {
+  auto conn = connection();
+  if (conn == nullptr) {


For sure looking at connection it must be a valid pointer as it's returning a pointer to a reference member.
I think it'd be fine to change both of these to asserts - I think if we create an HTTPConnectionManager with non-valid address we've violated earlier invariants.

alyssawilk · 2020-04-30T17:28:35Z

source/common/http/conn_manager_impl.h

@@ -673,6 +673,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    void onStreamMaxDurationReached();
    bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); }

+    // return local port of the connection


Super nitty, but
->
Return local port of the connection.

If you could check the comments below too that'd be awesome.

alyssawilk · 2020-04-30T17:31:25Z

source/common/http/conn_manager_utility.cc

+void ConnectionManagerUtility::maybeNormalizeHost(RequestHeaderMap& request_headers,
+                                                  const ConnectionManagerConfig& config,
+                                                  uint32_t port) {
+  if (!request_headers.Host()) {


At the point we call this, we've either added a default host for HTTP/1.0 or sent a local reply and will not reach this point (HCM 850)
I don't mind you making it resilient to changes in code order, but if you're going to leave this in please make sure it's unit tested.

alyssawilk · 2020-04-30T17:43:08Z

...voy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto

+  // Without setting this option, incoming requests with Host `example:443` will not match against
+  // route with `domains` match set to `example`. Defaults to `false`. Note that port removal is not part
+  // of `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
+  bool strip_host_port = 38;


optional, maybe something about the matching in the naming? strip_matching_host_port?

source/common/http/header_utility.cc

alyssawilk

Looking great - just a few more comment nits and you're good to go!

alyssawilk · 2020-05-04T15:10:27Z

source/common/http/conn_manager_config.h

@@ -427,7 +427,7 @@ class ConnectionManagerConfig {
  /**
   * @return if the HttpConnectionManager should remove ports from Host/authority header


I think there's still some Host -> host changes to track down :-)

alyssawilk · 2020-05-04T15:14:53Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -506,6 +506,14 @@ message HttpConnectionManager {
  //
  // 3. Tracing decision (sampled, forced, etc) is set in 14th byte of the UUID.
  RequestIDExtension request_id_extension = 36;
+
+  // Determines if port part should be removed from host/authority header before any processing
+  // of request by HTTP filters or routing. Port would be removed only if it is equal to the listener's


Port -> The port
and I'd change "connect" to CONNECT again just for consistency.

alyssawilk · 2020-05-04T15:15:17Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -506,6 +506,14 @@ message HttpConnectionManager {
  //
  // 3. Tracing decision (sampled, forced, etc) is set in 14th byte of the UUID.
  RequestIDExtension request_id_extension = 36;
+
+  // Determines if port part should be removed from host/authority header before any processing


really small nit but port -> the port ?

repokitteh-read-only · 2020-05-04T21:48:45Z

CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #10960 was synchronize by tehnerd.

see: more, trace.

alyssawilk

@mattklein123 up for API sign off?

mattklein123

Nice! Few small comments from a quick skim. Thank you!

/wait

mattklein123 · 2020-05-05T22:07:59Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+  // of request by HTTP filters or routing. The port would be removed only if it is equal to the listener's
+  // local port and request method is not CONNECT. This affects the upstream host header as well.
+  // Without setting this option, incoming requests with host `example:443` will not match against
+  // route with `domains` match set to `example`. Defaults to `false`. Note that port removal is not part


This needs a release note. Also, do you mind ref linking fields in here where you can? For example domains.

mattklein123 · 2020-05-05T22:08:35Z

source/common/http/conn_manager_impl.cc

@@ -742,6 +742,16 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
  return &connection_manager_.read_callbacks_->connection();
 }

+uint32_t ConnectionManagerImpl::ActiveStream::localPort() {
+  auto conn = connection();
+  ASSERT(conn != nullptr);


nit: generally we don't do asserts like this since it will crash in an obvious way on the next line. I would just remove the conn variable.

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

1. renaming remove_port to strip_host_port 2. stripping port only when it is equal to listener's local port Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

changes: 1. renaming strip_host_port to strip_matching_... 2. renaming shouldRemovePort to shouldRemoveMatchingPort 3. do not remove port if method is "connect" 4. removing some unnecessary checks Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

addressing nits/comments on spelling Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

* removed ASSERT from localPort helper * added ref into prot description * added ref to release notes * rebase Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

…p_patch Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

mattklein123

Awesome thank you. Just a small doc nit and let's ship!

/wait

mattklein123 · 2020-05-07T20:27:21Z

docs/root/version_history/current.rst

@@ -24,6 +24,7 @@ Changes
  Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.fix_upgrade_response` to false.
 * http: remove legacy connection pool code and their runtime features: `envoy.reloadable_features.new_http1_connection_pool_behavior` and
  `envoy.reloadable_features.new_http2_connection_pool_behavior`.
+* http connection manager: added :ref:`stripping port from host header <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.strip_matching_host_port>` support.


s/http connection manager/http

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

tehnerd · 2020-05-07T21:00:38Z

forget to format. will do another commit

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

mattklein123

Thanks!

mattklein123 · 2020-05-07T22:59:35Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-07T22:59:43Z

Azure Pipelines successfully started running 1 pipeline(s).

mattklein123 · 2020-05-07T23:52:49Z

/azp run envoy-presubmit

azure-pipelines · 2020-05-07T23:52:58Z

Azure Pipelines successfully started running 1 pipeline(s).

mattklein123 · 2020-05-08T03:25:03Z

Sorry looks like you lost the merge race. :( Can you merge master again?

/wait

…p_patch Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

mattklein123

Thanks!

tehnerd requested review from alyssawilk and mattklein123 as code owners April 27, 2020 17:24

repokitteh-read-only bot added the api label Apr 27, 2020

tehnerd force-pushed the http_patch branch from 340dfcf to 01e0b9e Compare April 27, 2020 20:23

zuercher reviewed Apr 28, 2020

View reviewed changes

mattklein123 assigned alyssawilk Apr 29, 2020

alyssawilk reviewed Apr 30, 2020

View reviewed changes

alyssawilk added the waiting label Apr 30, 2020

repokitteh-read-only bot removed the waiting label Apr 30, 2020

alyssawilk reviewed May 4, 2020

View reviewed changes

alyssawilk previously approved these changes May 5, 2020

View reviewed changes

mattklein123 self-assigned this May 5, 2020

mattklein123 requested changes May 5, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label May 5, 2020

tehnerd added 10 commits May 6, 2020 19:55

[conn_manager]: allow to remove port from host header

63262b6

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fix_spelling

dc041ac

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fix for v6 host header

a548c25

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fix_spelling_2

15cd1a6

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

addressing review comments.

4e99062

1. renaming remove_port to strip_host_port 2. stripping port only when it is equal to listener's local port Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

off by one fixing

d39f3f3

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fixing config test

8c679fe

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

addressing coments #2

494c9c3

changes: 1. renaming strip_host_port to strip_matching_... 2. renaming shouldRemovePort to shouldRemoveMatchingPort 3. do not remove port if method is "connect" 4. removing some unnecessary checks Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

addressing comments N3

a4642b3

addressing nits/comments on spelling Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

addressing comments N4

434da02

* removed ASSERT from localPort helper * added ref into prot description * added ref to release notes * rebase Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

tehnerd dismissed alyssawilk’s stale review via 434da02 May 6, 2020 20:44

tehnerd force-pushed the http_patch branch from 47627a9 to 434da02 Compare May 6, 2020 20:44

tehnerd added 3 commits May 7, 2020 16:32

fixing docs + merge

68682ae

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into htt…

eb75f1d

…p_patch Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fix_format after merge

db1daf1

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

mattklein123 requested changes May 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label May 7, 2020

addressing comments on doc

04ef47d

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

repokitteh-read-only bot removed the waiting label May 7, 2020

fixing docs

8e6fed8

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

mattklein123 previously approved these changes May 7, 2020

View reviewed changes

repokitteh-read-only bot removed the api label May 7, 2020

repokitteh-read-only bot added the waiting label May 8, 2020

tehnerd added 2 commits May 8, 2020 03:55

Merge branch 'master' of https://github.com/envoyproxy/envoy into htt…

b24a784

…p_patch Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

fix_format after merge

35e9197

Signed-off-by: Nikita V. Shirokov <tehnerd@tehnerd.com>

tehnerd dismissed mattklein123’s stale review via 35e9197 May 8, 2020 04:49

repokitteh-read-only bot added api and removed waiting labels May 8, 2020

mattklein123 approved these changes May 8, 2020

View reviewed changes

repokitteh-read-only bot removed the api label May 8, 2020

mattklein123 merged commit 111684f into envoyproxy:master May 8, 2020

alyssawilk mentioned this pull request May 26, 2020

Impossible to establish a WebSocket/SignalR connection from Xamarin application using Envoy as proxy on an AWS ec2 machine #11294

Closed

cuonglm mentioned this pull request Jun 22, 2020

404 if Host header include port number 443 pomerium/pomerium#959

Closed

crhino mentioned this pull request Jun 26, 2020

Append port number to ingress host domain hashicorp/consul#8190

Merged

matdehaast mentioned this pull request Aug 31, 2020

Host header may include port emissary-ingress/emissary#2276

Closed

guydc mentioned this pull request Sep 9, 2024

Support host:port host headers envoyproxy/gateway#4197

Closed

		@@ -427,7 +427,7 @@ class ConnectionManagerConfig {
		/**
		* @return if the HttpConnectionManager should remove ports from Host/authority header

conn_manager: allow to remove port from host header #10960

conn_manager: allow to remove port from host header #10960

Conversation

tehnerd commented Apr 27, 2020

repokitteh-read-only bot commented Apr 27, 2020

tehnerd commented Apr 27, 2020

euroelessar commented Apr 27, 2020

zuercher 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk 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

repokitteh-read-only bot commented May 4, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tehnerd commented May 7, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

mattklein123 commented May 7, 2020

azure-pipelines bot commented May 7, 2020

mattklein123 commented May 8, 2020

mattklein123 left a comment

Choose a reason for hiding this comment