http conn man: add an option to strip trailing host dot. by maheshkurund · Pull Request #15568 · envoyproxy/envoy

maheshkurund · 2021-03-19T08:56:06Z

Signed-off-by: maheshkurund mahesh.kurund@oneconvergence.com

Commit Message:
Add an option in connection manager which allow to strip trailing host dot from Host/authority header. (e.g transforms "example.com." to "example.com").

Additional Description:
This change would eliminate the need of specifying domain entry with dot (e.g example.com.) while matching it with domain's inside the virtual host. For example, if the request has a host header "example.com." and strip_trailing_host_dot flag is set then it transforms into "example.com" by stripping the trailing dot and matches "domains" configured in VirtualHost.
Fixes #14748

Risk Level: Low
Testing: Unit, integration and manual.
Docs Changes: Inline in proto file.
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

repokitteh-read-only · 2021-03-19T08:56:10Z

Hi @maheshkurund, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15568 was opened by maheshkurund.

see: more, trace.

repokitteh-read-only · 2021-03-19T08:56:14Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15568 was opened by maheshkurund.

see: more, trace.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

dmitri-d · 2021-03-19T18:37:27Z

test/common/http/conn_manager_impl_test.cc

  filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
 }

+// Filters observe host header w/o trailing dot in host when trailing dot removal is configured


Do you think dot stripping could be tested more directly? Instantiate ConnectionManagerImpl, create a RequestDecoder by invoking ConnectionManagerImpl::newStream(), then invoke RequestDecoder::decodeHeaders() and verify the contents of the "host" header. WDYT?

yes, added test for this, please check and let me know. Thank you.

Ultimately, I'm not sure why three tests that very closely resemble each other are needed (they all verify that a) header map is updated, and b) that filters in the filter chain see that change. Perhaps just RemoveTrailingHostDot or FilterShouldUseHostWithoutTrailingDot is sufficient?

Agree, will remove RemoveTrailingHostDot test and add check to verify host header after RequestDecoder::decodeHeaders() in FilterShouldUseHostWithoutTrailingDot and RouteShouldUseHostWithoutTrailingDot.

…ders(). Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

alyssawilk · 2021-03-22T13:13:00Z

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

+  // This affects the upstream host header.
+  // Without setting this option, incoming requests with host `example.com.` will not match against
+  // route with :ref:`domains<envoy_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.
+  bool strip_trailing_host_dot = 44;


Drive by question for mdroth, do we think it's worth moving this and strip_matching_host_port together in a host mutation proto or just and wait and see if we get more?

At this point, there are a whole bunch of options here related to URI normalization, not just the host ones but also the path ones. I'd be open to moving all of those options to a separate UriNormalizationOptions message, but I'd also be okay with keeping them here, since they're unlikely to ever be used anywhere else.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-04-09T04:19:28Z

/retest

repokitteh-read-only · 2021-04-09T04:19:31Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

dmitri-d · 2021-04-09T22:30:26Z

test/common/http/conn_manager_impl_test.cc

+  EXPECT_CALL(*filter, setDecoderFilterCallbacks(_));
+
+  EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
+    decoder_ = &conn_manager_->newStream(response_encoder_);


Couldn't you reduce the test to setup and lines 854-857? Then verify the contents of authority in headers...

Is it ok to verify host header after RequestDecoder::decodeHeaders() in existing tests? Or should I keep only single test which just verifies host header after RequestDecoder::decodeHeaders() as you mentioned above?

…t_dot Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-04-26T04:39:38Z

/retest

repokitteh-read-only · 2021-04-26T04:39:42Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

dmitri-d · 2021-04-29T19:52:50Z

test/common/http/conn_manager_impl_test.cc

+        return Http::okStatus();
+      }));
+  Buffer::OwnedImpl fake_input("1234");
+  conn_manager_->onData(fake_input, false);


I don't think you need to call onData callback (and set the expectation above), instead you should be able to do

conn_manager_->createCodec(fake_input);

thank you. Updated the code with suggestion.

dmitri-d · 2021-04-29T19:57:25Z

The tests look good, with one minor suggestion for a change. Also, please merge in the latest changes from main.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-05-11T08:42:00Z

/retest

repokitteh-read-only · 2021-05-11T08:42:04Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

maheshkurund · 2021-05-11T12:13:36Z

/retest

repokitteh-read-only · 2021-05-11T12:13:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-05-12T12:19:24Z

/retest

repokitteh-read-only · 2021-05-12T12:19:27Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

snowp

Thanks for working on this!

snowp · 2021-05-14T14:29:32Z

source/common/http/header_utility.cc


+void HeaderUtility::stripTrailingHostDot(RequestHeaderMap& headers) {
+  auto host = headers.getHostValue();
+  // Find last dot and return if not found or take off end if last


Maybe just // If the host ends in a period, remove it.

Yes fits well here, will update.

snowp · 2021-05-14T14:31:11Z

source/common/http/header_utility.cc

+  // Check if the dot is just before a colon, which means it must be the port
+  // because although the host may contain a colon via an IPv6 bracketed
+  // address, and although that IPv6 address may also contain dots when
+  // embedding an address per RFC 4291 2.2.3, the dot will never directly
+  // precede the colon like it would in foo.com.:123


Maybe split this sentence up? It's very long

yah, will fix it.

snowp · 2021-05-14T14:33:41Z

source/common/http/header_utility.cc

+    headers.setHost(host);
+    return;
+  }
+  // Check if the dot is just before a colon, which means it must be the port


If the dot is just before a colon, it must be preceding the port number.

Will change.

snowp · 2021-05-14T14:34:04Z

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

+  // Determines if trailing dot of the host should be removed from host/authority header before any
+  // processing of request by HTTP filters or routing.
+  // This affects the upstream host header.
+  // Without setting this option, incoming requests with host `example.com.` will not match against
+  // route with :ref:`domains<envoy_v3_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.


This should probably include something about how this handles the host:port case

Will following comment helps here in case of host:port?

In case host/authority header value has port part with host, on setting this option it removes trailing dot from host keeping port value as is i.e host example.com.:443 will become example.com:443.

snowp · 2021-05-14T14:34:44Z

source/common/http/header_utility.cc

+  // precede the colon like it would in foo.com.:123
+  if (host[dot_index + 1] == ':') {
+    // Does a memcpy internally, but since we only have access to a string_view
+    // anyways, this is acceptable compared to a string::erase of a copy


nit: end comments with period

Will change.

snowp · 2021-05-14T14:37:00Z

source/common/http/header_utility.cc

+  // embedding an address per RFC 4291 2.2.3, the dot will never directly
+  // precede the colon like it would in foo.com.:123
+  if (host[dot_index + 1] == ':') {
+    // Does a memcpy internally, but since we only have access to a string_view


What does a memcpy? If you're talking about it copying data to a new string we do that on L238 as well, so this call out seems to be a bit unnecessary

Will remove it.

snowp · 2021-05-14T14:37:52Z

test/common/http/conn_manager_impl_test.cc

  filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
 }

+// Observe host header w/o trailing dot in host when trailing dot removal is configured


nit: here and below, end comments with period as comments should use proper grammar (even if other tests don't do this, no reason to not do it for new code :) )

agree, will fix this.

snowp · 2021-05-14T14:40:28Z

test/common/http/header_utility_test.cc

+      {"abc.com.", "abc.com"},             // dns w/ dot
+      {"abc.com:443", "abc.com:443"},      // dns port w/o dot
+      {"abc.com.:443", "abc.com:443"},     // dns port w/ dot
+      {"[fc00::1].:443", "[fc00::1]:443"}, // ipv6 w/ dot


qq is this a valid input? Not sure when we'd get a . after an IP?

Yah, its not valid input, its added by mistake and will remove it.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-05-18T14:54:56Z

/retest

repokitteh-read-only · 2021-05-18T14:55:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

snowp

Sorry for the delay here, got some for comments for you. Thanks for iterating on this!

snowp · 2021-05-20T10:46:32Z

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

+  // This affects the upstream host header.
+  // Without setting this option, incoming requests with host `example.com.` will not match against
+  // route with :ref:`domains<envoy_v3_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.
+  // When incoming requests with host/authority header has port part with host, on setting this option it removes trailing dot from host keeping port


Maybe When the incoming request contains a host/authority header includes a port number, setting this option will strip trailing dots from the host section, leaving the port as is (e.g. ...?

thanks, will update.

snowp · 2021-05-20T10:46:41Z

docs/root/version_history/current.rst


 New Features
 ------------
+


remove this newline

will remove.

snowp · 2021-05-20T10:54:10Z

source/common/http/header_utility.cc

+  // Because although the host may contain a colon via an IPv6 bracketed
+  // address, and although that IPv6 address may also contain dots when
+  // embedding an address per RFC 4291 2.2.3. In this case the dot will never


This is a bit tricky to read, maybe something like IPv6 addresses may contain colons or dots, but the dot will never directly precede the colon, so this check should be sufficient to detect a trailing port number.?

sure, will update it.

snowp · 2021-05-20T10:55:19Z

test/common/http/conn_manager_impl_test.cc

  filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
 }

+// Observe host header w/o trailing dot in host when trailing dot removal is configured.


This makes it sound like the input doesn't have a trailing dot, maybe Observe tat we strip the trailing dot ...?

thanks, will change.

snowp · 2021-05-20T10:56:13Z

test/common/http/conn_manager_impl_test.cc

 }

+// Observe host header w/o trailing dot in host when trailing dot removal is configured.
+TEST_F(HttpConnectionManagerImplTest, StripTrailingHostDot) {


Mind adding a test where we set this config option but pass through a header without a trailing dot?

Sure. will add.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

snowp

Some more nits, thanks for iterating!

snowp · 2021-05-20T17:18:15Z

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

  // route with :ref:`domains<envoy_v3_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.
-  // When incoming requests with host/authority header has port part with host, on setting this option it removes trailing dot from host keeping port
-  // value as is (e.g. host value `example.com.:443` will be updated to `example.com:443`).
+  // When the incoming request contains a host/authority header includes a port number, setting this option will strip trailing dots


that includes

thanks, addressed.

snowp · 2021-05-20T17:18:49Z

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

  // route with :ref:`domains<envoy_v3_api_field_config.route.v3.VirtualHost.domains>` match set to `example.com`. Defaults to `false`.
-  // When incoming requests with host/authority header has port part with host, on setting this option it removes trailing dot from host keeping port
-  // value as is (e.g. host value `example.com.:443` will be updated to `example.com:443`).
+  // When the incoming request contains a host/authority header includes a port number, setting this option will strip trailing dots


a trailing dot, if present, from the host section

otherwise this sounds like we strip multiple dots? I think we only do one right?

Yes, we do remove only one dot, updated in latest commit.

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund · 2021-05-21T09:30:30Z

/retest

repokitteh-read-only · 2021-05-21T09:30:34Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15568 (comment) was created by @maheshkurund.

see: more, trace.

snowp

LGTM, thanks!

@markdroth mind giving the API a look? Thanks

markdroth · 2021-05-24T15:29:14Z

/lgtm api

maheshkurund · 2021-05-25T11:36:34Z

Thanks @dmitri-d @snowp @markdroth for reviewing this change.

howardjohn

This is a bit late but it would be great if this, and the other strip_ options supported stripping only internally and preserving the original when forwarding upstream. Otherwise its not super usable for a transparent proxy

…15568) Add an option in connection manager which allow to strip trailing host dot from Host/authority header (e.g transforms "example.com." to "example.com"). Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Remove trailing host dot from host/authority header.

7c0674d

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund requested review from alyssawilk and mattklein123 as code owners March 19, 2021 08:56

repokitteh-read-only bot added the api label Mar 19, 2021

repokitteh-read-only bot assigned markdroth Mar 19, 2021

maheshkurund added 2 commits March 19, 2021 17:24

Fixed formatting.

d2e05b7

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Added missing variable entry.

595157c

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

dmitri-d reviewed Mar 19, 2021

View reviewed changes

maheshkurund added 2 commits March 22, 2021 10:35

Added unit test to check dot striping after RequestDecoder::decodeHea…

4945289

…ders(). Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Fixed test failure caused because of missing cleanup.

74f247f

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

alyssawilk reviewed Mar 22, 2021

View reviewed changes

maheshkurund added 2 commits April 8, 2021 15:39

Merge branch 'main' into remove_trailing_host_dot

bb62fda

Fixed proto field number

c3d1057

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund requested a review from dmitri-d April 9, 2021 11:30

dmitri-d reviewed Apr 9, 2021

View reviewed changes

maheshkurund added 4 commits April 22, 2021 16:19

Merge remote-tracking branch 'upstream/main' into remove_trailing_hos…

4b9c804

…t_dot Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Fixed tests.

468ce45

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Fixed tests.

3a4f449

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Fixed test cleanup.

40a1a44

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund requested a review from dmitri-d April 28, 2021 04:01

dmitri-d reviewed Apr 29, 2021

View reviewed changes

maheshkurund added 2 commits April 30, 2021 06:03

Merge upstream/main

8995b52

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Addressed review comment.

baed449

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Merge main into remove_trailing_host_dot

dca8659

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

snowp suggested changes May 14, 2021

View reviewed changes

maheshkurund added 2 commits May 18, 2021 05:11

Merge main branch into remove_trailing_host_dot

fffc458

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Addressed review comments.

d3cd808

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund requested a review from snowp May 19, 2021 04:19

Merge branch 'main' into remove_trailing_host_dot

ef66621

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

snowp suggested changes May 20, 2021

View reviewed changes

maheshkurund added 2 commits May 20, 2021 13:17

Addressed review comments.

c744100

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Merge branch 'main' into remove_trailing_host_dot

ec186fd

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

snowp suggested changes May 20, 2021

View reviewed changes

maheshkurund added 2 commits May 21, 2021 04:27

Merge branch 'main' into remove_trailing_host_dot

c94357b

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

Addressed review comments.

d117486

Signed-off-by: maheshkurund <mahesh.kurund@oneconvergence.com>

maheshkurund requested a review from snowp May 24, 2021 12:02

snowp approved these changes May 24, 2021

View reviewed changes

repokitteh-read-only bot removed the api label May 24, 2021

snowp merged commit 7cc10b0 into envoyproxy:main May 25, 2021

howardjohn reviewed May 25, 2021

View reviewed changes

Conversation

maheshkurund commented Mar 19, 2021

Uh oh!

repokitteh-read-only bot commented Mar 19, 2021

Uh oh!

repokitteh-read-only bot commented Mar 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maheshkurund commented Apr 9, 2021

Uh oh!

repokitteh-read-only bot commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maheshkurund commented Apr 26, 2021

Uh oh!

repokitteh-read-only bot commented Apr 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maheshkurund commented May 11, 2021

Uh oh!

repokitteh-read-only bot commented May 11, 2021

Uh oh!

maheshkurund commented May 11, 2021

Uh oh!

repokitteh-read-only bot commented May 11, 2021

Uh oh!

maheshkurund commented May 12, 2021

Uh oh!

repokitteh-read-only bot commented May 12, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maheshkurund May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmitri-d commented Apr 29, 2021 •

edited

Loading

maheshkurund May 15, 2021 •

edited

Loading