Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 21, 2017
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c45b1ad
websocket: Connection http header may contain multiple values
tangxinfa Nov 16, 2017
d11de77
websocket: add tests
tangxinfa Nov 19, 2017
4ffbe41
websocket: avoid mistakenly add "transfer-encoding: chunked" header
tangxinfa Nov 19, 2017
4913e34
websocket: fix comments and tests
tangxinfa Nov 20, 2017
8d1b113
websocket: fix comments and tests
tangxinfa Nov 20, 2017
f922cb8
websocket: fix comments
tangxinfa Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions include/envoy/http/header_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,28 @@ class HeaderString {
*/
bool find(const char* str) const { return strstr(c_str(), str); }

/**
* HeaderString is in token list form, each token separated by commas or whitespace,
* see https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.1 for more information,
* header field value's case sensitivity depends on each header.
* @return whether contains token in case insensitive manner.
*/
bool caseInsensitiveContains(const char* token) const {
const int n = strlen(token);
if (n == 0) {
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test case for this branch?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should return false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ((p == tokens || strchr(separators, *(p - 1)) != NULL) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/NULL/nullptr in both places.

(*(p + n) == '\0' || strchr(separators, *(p + n)) != NULL)) {
return true;
}
}
return false;
}

/**
* Set the value of the string by copying data into it. This overwrites any existing string.
*/
Expand Down
23 changes: 18 additions & 5 deletions source/common/http/conn_manager_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,31 @@ void ConnectionManagerUtility::mutateRequestHeaders(
Runtime::RandomGenerator& random, Runtime::Loader& runtime,
const LocalInfo::LocalInfo& local_info) {
// Clean proxy headers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

request_headers.removeEnvoyInternalRequest();
request_headers.removeKeepAlive();
request_headers.removeProxyConnection();
request_headers.removeTransferEncoding();

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// upstream host, which add "transfer-encoding: chunked" request header if stream not end
// and content-length not exists.
// In HTTP1.1, if transfer-encoding and content-length both not exists means no request body,
// 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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const bool

if (no_body) {
request_headers.insertContentLength().value(uint64_t(0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} else {
request_headers.removeConnection();
request_headers.removeUpgrade();
}

request_headers.removeEnvoyInternalRequest();
request_headers.removeKeepAlive();
request_headers.removeProxyConnection();
request_headers.removeTransferEncoding();

// If we are "using remote address" this means that we create/append to XFF with our immediate
// peer. Cases where we don't "use remote address" include trusted double proxy where we expect
// our peer to have already properly set XFF, etc.
Expand Down
7 changes: 4 additions & 3 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,11 @@ bool Utility::isInternalRequest(const HeaderMap& headers) {
}

bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) {
// In firefox the "Connection" request header value is "keep-alive, Upgrade",
// we should check if it contains the "Upgrade" token.
return (headers.Connection() && headers.Upgrade() &&
(0 == StringUtil::caseInsensitiveCompare(
headers.Connection()->value().c_str(),
Http::Headers::get().ConnectionValues.Upgrade.c_str())) &&
headers.Connection()->value().caseInsensitiveContains(
Http::Headers::get().ConnectionValues.Upgrade.c_str()) &&
(0 == StringUtil::caseInsensitiveCompare(
headers.Upgrade()->value().c_str(),
Http::Headers::get().UpgradeValues.WebSocket.c_str())));
Expand Down
13 changes: 13 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,19 @@ TEST(HeaderStringTest, All) {
EXPECT_EQ(11U, string.size());
EXPECT_EQ(HeaderString::Type::Reference, string.type());
}

// caseInsensitiveContains
{
std::string static_string("keep-alive, Upgrade");
HeaderString string(static_string);
EXPECT_TRUE(string.caseInsensitiveContains("keep-alive"));
EXPECT_TRUE(string.caseInsensitiveContains("Keep-alive"));
EXPECT_TRUE(string.caseInsensitiveContains("Upgrade"));
EXPECT_TRUE(string.caseInsensitiveContains("upgrade"));
EXPECT_FALSE(string.caseInsensitiveContains("keep"));
EXPECT_FALSE(string.caseInsensitiveContains("alive"));
EXPECT_FALSE(string.caseInsensitiveContains("grade"));
}
}

TEST(HeaderMapImplTest, InlineInsert) {
Expand Down
24 changes: 12 additions & 12 deletions test/integration/integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,31 +193,31 @@ TEST_P(IntegrationTest, WebSocketConnectionDownstreamDisconnect) {
IntegrationTcpClientPtr tcp_client;
FakeRawConnectionPtr fake_upstream_connection;
const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: "
"Upgrade\r\nUpgrade: websocket\r\n\r\n";
"keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";
const std::string upgrade_resp_str =
"HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n";

tcp_client = makeTcpConnection(lookupPort("http"));
// Send websocket upgrade request
// The request path gets rewritten from /websocket/test to /websocket.
// The size of headers received by the destination is 225 bytes.
// The size of headers received by the destination is 228 bytes.
tcp_client->write(upgrade_req_str);
fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection();
fake_upstream_connection->waitForData(225);
fake_upstream_connection->waitForData(228);
// Accept websocket upgrade request
fake_upstream_connection->write(upgrade_resp_str);
tcp_client->waitForData(upgrade_resp_str);
// Standard TCP proxy semantics post upgrade
tcp_client->write("hello");
// datalen = 225 + strlen(hello)
fake_upstream_connection->waitForData(230);
// datalen = 228 + strlen(hello)
fake_upstream_connection->waitForData(233);
fake_upstream_connection->write("world");
tcp_client->waitForData(upgrade_resp_str + "world");
tcp_client->write("bye!");
// downstream disconnect
tcp_client->close();
// datalen = 225 + strlen(hello) + strlen(bye!)
fake_upstream_connection->waitForData(234);
// datalen = 228 + strlen(hello) + strlen(bye!)
fake_upstream_connection->waitForData(237);
fake_upstream_connection->waitForDisconnect();
}

Expand All @@ -230,23 +230,23 @@ TEST_P(IntegrationTest, WebSocketConnectionUpstreamDisconnect) {
IntegrationTcpClientPtr tcp_client;
FakeRawConnectionPtr fake_upstream_connection;
const std::string upgrade_req_str = "GET /websocket/test HTTP/1.1\r\nHost: host\r\nConnection: "
"Upgrade\r\nUpgrade: websocket\r\n\r\n";
"keep-alive, Upgrade\r\nUpgrade: websocket\r\n\r\n";
const std::string upgrade_resp_str =
"HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n";
tcp_client = makeTcpConnection(lookupPort("http"));
// Send websocket upgrade request
tcp_client->write(upgrade_req_str);
fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection();
// The request path gets rewritten from /websocket/test to /websocket.
// The size of headers received by the destination is 225 bytes.
fake_upstream_connection->waitForData(225);
// The size of headers received by the destination is 228 bytes.
fake_upstream_connection->waitForData(228);
// Accept websocket upgrade request
fake_upstream_connection->write(upgrade_resp_str);
tcp_client->waitForData(upgrade_resp_str);
// Standard TCP proxy semantics post upgrade
tcp_client->write("hello");
// datalen = 225 + strlen(hello)
fake_upstream_connection->waitForData(230);
// datalen = 228 + strlen(hello)
fake_upstream_connection->waitForData(233);
fake_upstream_connection->write("world");
// upstream disconnect
fake_upstream_connection->close();
Expand Down