websocket: http request header Connection may contains multiple values #2070

tangxinfa · 2017-11-16T12:29:33Z

http request header Connection may contains multiple values, such as: keep-alive, Upgrade

Description:
Don't use "strcasecmp" to do strict match, but use "strcasestr" to do partial match, this fix the 400 error response in firefox, because the Connection and Upgrade request header removed by envoy, become a invalid Websocket request.

mattklein123 · 2017-11-16T15:48:40Z

@tangxinfa thank you for the fix! We will need DCO, format fix, and some tests. Please let us know if you are willing to do those things. cc @rshriram

htuch · 2017-11-16T19:14:26Z

source/common/http/utility.cc

-          (0 == StringUtil::caseInsensitiveCompare(
-                    headers.Connection()->value().c_str(),
-                    Http::Headers::get().ConnectionValues.Upgrade.c_str())) &&
+          (NULL != strcasestr(headers.Connection()->value().c_str(), Http::Headers::get().ConnectionValues.Upgrade.c_str())) &&


Don't we still want this to be case insensitive? Maybe we should also do a split operation on ,, it would be more robust, at the expense of performance.

I was thinking the same thing about splitting on comma. I was looking for a helper/utility that would do this, and I don't see one (which surprised me).

Perhaps we can add a method to HeaderString for bool entryContains(const LowerCaseString& str) const;

@ggreenway we have https://github.com/envoyproxy/envoy/blob/master/source/common/common/utility.h#L131 and when we gain absl in #1974 we could also use https://github.com/abseil/abseil-cpp/blob/master/absl/strings/str_split.h

In this case, do you think the perf cost will be too high to split it and return it? It would be faster to search in place, but the interface would be less-nice and less-reusable.

I agree that in general we shouldn't copy and split on the hot path when serving reqs. In this particular case, it might be OK to do this perhaps, since we already know we're most likely doing an upgrade due to the earlier test for the Upgrade: header, and if we're doing a Websockets upgrade, it's probably fine to pay a tiny bit extra for what is presumably a long lived session ¯_(ツ)_/¯

I'm fine with that. If we profile and find we're spending too much time there, we can fix it then.

yeah in this case because we are already guarded by the existence of the upgrade header I think it's fine to split after that fact.

Strcasestr is case insensitive. The question here is whether to abstract this into stringutil or not. This particular case, given that the contributor is doing a one off contribution, I am reluctant to insist on him debugging more :). I can do the follow up and clean it up as it’s not bad as is. But I leave it to you guys to decide on this.

rshriram · 2017-11-17T05:12:19Z

Thank you for testing, tracking down the issue and fixing this. Would you mind copy pasting the explanation in the PR into a comment above the if block, so that others would understand why substring search is being used?

With regard to the tests, tweak one of the websocket integration tests in test/integration/ and change the connection upgrade to a keepalive,upgrade. This will test if any other piece in Envoy breaks (none should as we use tcp upstream).

firefox's Connection header value is: keep-alive, Upgrade Signed-off-by: tangxinfa <[email protected]>

rshriram

Lgtm except for missing comment about why token compare is needed.

rshriram · 2017-11-18T09:19:06Z

source/common/http/utility.cc

+          headers.Connection()->value().caseInsensitiveContains(
+                  Http::Headers::get().ConnectionValues.Upgrade.c_str()) &&
+          headers.Upgrade()->value().caseInsensitiveContains(
+                  Http::Headers::get().UpgradeValues.WebSocket.c_str()));


Nit: please copy paste the explanation you had for doing the token comparison (about Firefox sending multiple tokens in the connection string)

rshriram · 2017-11-18T09:20:26Z

source/common/http/conn_manager_utility.cc

  request_headers.removeTransferEncoding();
+  if (contentLength0) {
+    request_headers.insertContentLength().value(uint64_t(0));
+  }


This seems unrelated to this PR. What is the rationale for this?

Signed-off-by: tangxinfa <[email protected]>

mattklein123

Generally LGTM. Thanks a lot for fixing this. A few small comments.

mattklein123 · 2017-11-19T18:22:42Z

include/envoy/http/header_map.h

+  bool caseInsensitiveContains(const char* token) const {
+    const int n = strlen(token);
+    if (n == 0) {
+      return true;


Do we have a test case for this branch?

mattklein123 · 2017-11-19T18:27:34Z

source/common/http/conn_manager_utility.cc

+    // after transfer-encoding striped here, the upstream request become invalid:
+    //   chunked encoding with no request body.
+    // We can fix it by explicitly add a "content-length: 0" request header here.
+    bool no_body = (!request_headers.TransferEncoding() && !request_headers.ContentLength());


nit: const bool

mattklein123 · 2017-11-19T18:28:02Z

source/common/http/conn_manager_utility.cc

+    // We can fix it by explicitly add a "content-length: 0" request header here.
+    bool no_body = (!request_headers.TransferEncoding() && !request_headers.ContentLength());
+    if (no_body) {
+      request_headers.insertContentLength().value(uint64_t(0));


Do we have a test case where we check for the existence of content-length: 0? We should be able to look for this in one of the conn_manager_impl websocket tests.

mattklein123 · 2017-11-19T18:28:50Z

include/envoy/http/header_map.h

+    const char* tokens = c_str();
+    const char* separators = " ,";
+    for (const char* p = tokens; (p = strcasestr(p, token)); p += n) {
+      if ((p == tokens || strchr(separators, *(p - 1)) != NULL) &&


nit: s/NULL/nullptr in both places.

mattklein123 · 2017-11-19T18:29:26Z

include/envoy/http/header_map.h

+    }
+    const char* tokens = c_str();
+    const char* separators = " ,";
+    for (const char* p = tokens; (p = strcasestr(p, token)); p += n) {


this logic is really hard to quickly read. Can you add a small comment on what it is doing?

mattklein123 · 2017-11-19T18:34:48Z

source/common/http/conn_manager_utility.cc


  // If this is a WebSocket Upgrade request, do not remove the Connection and Upgrade headers,
  // as we forward them verbatim to the upstream hosts.
-  if (protocol != Protocol::Http11 || !Utility::isWebSocketUpgradeRequest(request_headers)) {
+  if (protocol == Protocol::Http11 && Utility::isWebSocketUpgradeRequest(request_headers)) {
+    // Current WebSocket implementation re-use the HTTP1 codec to send upgrade headers to


Grammar nits, please just replace with (reflow as needed to 100 col):

// The current WebSocket implementation re-uses the HTTP1 codec to send upgrade headers to // the upstream host. This adds the "transfer-encoding: chunked" request header if the stream // has not ended and content-length does not exist. In HTTP1.1, if transfer-encoding and // content-length both do not exist this means there is no request body. After transfer-encoding // is stripped here, the upstream request becomes invalid. We can fix it by explicitly adding a // "content-length: 0" request header here.

Signed-off-by: tangxinfa <[email protected]>

mattklein123

Thank you, a few more comments.

mattklein123 · 2017-11-20T16:28:48Z

include/envoy/http/header_map.h

+    const char* tokens = c_str();
+    for (const char* p = tokens; (p = strcasestr(p, token)); p += n) {
+      if ((p == tokens || *(p - 1) == ' ' || *(p - 1) == ',') &&
+          (*(p + n) == '\0' || *(p + n) == ' ' || *(p + n) == ',')) {


Don't you need to check here that the length of the remaining header is >= n before you do this? I think this code can overrun the header. Please make sure you add more test cases to cover small headers.

Because it advance the pointer p only if found the token, there is no overrun. I will add more test cases cover small headers.

Ah I see. Sorry got it. Yup you are right. Please just add some small header tests and we are good.

mattklein123 · 2017-11-20T16:28:56Z

include/envoy/http/header_map.h

+    // Avoid dead loop if token argument is empty.
+    const int n = strlen(token);
+    if (n == 0) {
+      return true;


Seems like this should return false?

Yes, you are right.

mattklein123 · 2017-11-20T16:31:38Z

test/integration/integration_test.cc

+  const std::string data = fake_upstream_connection->waitForData(228);
+  // The chunked transfer-encoding for no body upstream request must valid
+  if (std::string::npos != data.find("transfer-encoding: chunked")) {
+    EXPECT_NE(std::string::npos, data.find("\r\n\r\n0\r\n"));


It's not really clear what this is testing for, can you search for the entire "content-length" etc. header? Same below.

Signed-off-by: tangxinfa <[email protected]>

mattklein123

Awesome. thank you.

mattklein123 · 2017-11-20T18:22:25Z

source/common/http/conn_manager_utility.cc

@@ -22,18 +22,30 @@ void ConnectionManagerUtility::mutateRequestHeaders(
    Runtime::RandomGenerator& random, Runtime::Loader& runtime,
    const LocalInfo::LocalInfo& local_info) {
  // Clean proxy headers.


oops sorry, one more nit: Can you please move this comment down with the rest of header removes?

Signed-off-by: tangxinfa <[email protected]>

* Authenticate an exchanged token * Change issuer name and jwt-authn output with key being original issuer * Revised the code based on the discussion * Address review comments and add a test * Address new review comments * Add integration tests and address review comments * Fix a flaky test and address new review comments * Small grammar fixes * Revise the function of finding the token header * Use case-insensitive compare for the header name * Change the name of a variable * Revise log statements

Description: Allow tests to avoid TLS Cert Verification - With this PR, only one `UpstreamTlsContext` supports the configurable `trust_chain_verification`: [`&base_tls_h2_socket`](https://github.com/envoyproxy/envoy-mobile/blob/v0.4.5.02082022/library/common/config/config.cc#L112). The other one, `&base_tls_socket`, does not. - `&base_tls_h2_socket` has been inlined. Consequently, `&base_tls_h2_socket` does not exist anymore. Unfortunately, when a config variable is assigned in the Java code, it won't get substituted when encountered inside another config variable. - Swift/objective-c/c++ configuration classes were not updated. Only Java and Kotlin were. Risk Level: Moderate Testing: New test and CI Docs Changes: N/A Release Notes: N/A Signed-off-by: Charles Le Borgne <[email protected]> Signed-off-by: JP Simard <[email protected]>

mattklein123 requested a review from rshriram November 16, 2017 15:48

htuch reviewed Nov 16, 2017

View reviewed changes

websocket: Connection http header may contain multiple values

c45b1ad

firefox's Connection header value is: keep-alive, Upgrade Signed-off-by: tangxinfa <[email protected]>

rshriram approved these changes Nov 18, 2017

View reviewed changes

tangxinfa added 2 commits November 19, 2017 09:46

websocket: add tests

d11de77

Signed-off-by: tangxinfa <[email protected]>

websocket: avoid mistakenly add "transfer-encoding: chunked" header

4ffbe41

Signed-off-by: tangxinfa <[email protected]>

mattklein123 reviewed Nov 19, 2017

View reviewed changes

websocket: fix comments and tests

4913e34

Signed-off-by: tangxinfa <[email protected]>

mattklein123 reviewed Nov 20, 2017

View reviewed changes

websocket: fix comments and tests

8d1b113

Signed-off-by: tangxinfa <[email protected]>

mattklein123 previously approved these changes Nov 20, 2017

View reviewed changes

mattklein123 reviewed Nov 20, 2017

View reviewed changes

websocket: fix comments

f922cb8

Signed-off-by: tangxinfa <[email protected]>

tangxinfa dismissed mattklein123’s stale review via f922cb8 November 20, 2017 23:31

mattklein123 approved these changes Nov 20, 2017

View reviewed changes

mattklein123 merged commit 3e6e4a7 into envoyproxy:master Nov 21, 2017

tangxinfa deleted the fix-isWebSocketUpgradeRequest branch November 21, 2017 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

websocket: http request header Connection may contains multiple values #2070

websocket: http request header Connection may contains multiple values #2070

tangxinfa commented Nov 16, 2017

mattklein123 commented Nov 16, 2017

htuch Nov 16, 2017

ggreenway Nov 16, 2017

htuch Nov 16, 2017

ggreenway Nov 16, 2017

htuch Nov 16, 2017

ggreenway Nov 16, 2017

mattklein123 Nov 16, 2017

rshriram Nov 17, 2017

rshriram commented Nov 17, 2017

rshriram left a comment

rshriram Nov 18, 2017

rshriram Nov 18, 2017

mattklein123 left a comment

mattklein123 Nov 19, 2017

mattklein123 Nov 19, 2017

mattklein123 Nov 19, 2017

mattklein123 Nov 19, 2017

mattklein123 Nov 19, 2017

mattklein123 Nov 19, 2017

mattklein123 left a comment

mattklein123 Nov 20, 2017

tangxinfa Nov 20, 2017

mattklein123 Nov 20, 2017

mattklein123 Nov 20, 2017

tangxinfa Nov 20, 2017

mattklein123 Nov 20, 2017

tangxinfa Nov 20, 2017

mattklein123 left a comment

mattklein123 Nov 20, 2017

websocket: http request header Connection may contains multiple values #2070

websocket: http request header Connection may contains multiple values #2070

Conversation

tangxinfa commented Nov 16, 2017

mattklein123 commented Nov 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram commented Nov 17, 2017

rshriram 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

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

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

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

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment