HTTP CONNECT

[SteveSelva]

For HTTP CONNECT request, the Connection: keep-alive header is discarded and Connection: close header is added, even though the Connection: keep-alive header is set in the upstream for 407 status code.

Can you fix this issue.

[jbeshay]

We can take a PR if you have identified where the issue is.

[SteveSelva]

In my case, I want to do a proxy chain, so the proxygen has to chain the response is received from upstream. In this case, it should set the headers as it is instead of removing Connection: keep-alive header.

Since it seems like a protocol issue. Can you fix this.

[afrind]

@SteveSelva Where is the Connection: keep-alive header being stripped? Is this happening within the codec when parsing a response?

[SteveSelva]

@afrind This is happening while generating headers for downstream from the headers received from upstream.
The upstream headers object contains the Connection: keep-alive header. But when the downstream headers object is generated from the upstream, the Connection: keep-alive is removed and instead Connection: close is added to it for a HTTP CONNECT request where the response is not between 200 - 300.

The Connection: keep-alive header is stripped at this line:

proxygen/proxygen/lib/http/codec/HTTP1xCodec.cpp

Lines 565 to 571 in 5c69360

	} else if (!caseInsensitiveEqual(token, kKeepAlive)) {
	connectionTokens[lastConnectionToken++] = token;
	} // else eat the keep-alive token
	}
	connectionTokens.resize(lastConnectionToken);
	// We'll generate a new Connection header based on the keepalive_
	// state

Here they have decided to generate Connection: keep-alive header using the keepAlive_ member.
But the keepAlive_ is already set to false for response of HTTP CONNECT request where the response status code is not between 200-300.

proxygen/proxygen/lib/http/codec/HTTP1xCodec.cpp

Lines 439 to 454 in 5c69360

	if (connectRequest_ && (statusCode >= 200 && statusCode < 300)) {
	// Set egress upgrade flag if we are sending a 200 response
	// to a CONNECT request we received earlier.
	egressUpgrade_ = true;
	} else if (statusCode == 101) {
	// Set the upgrade flags if we upgraded after the request from client.
	ingressUpgrade_ = true;
	egressUpgrade_ = true;
	} else if (connectRequest_ && ingressUpgrade_) {
	// Disable upgrade when rejecting CONNECT request
	ingressUpgrade_ = false;
	// This codec/session is no longer useful as we might have
	// forwarded some data before receiving the 200.
	keepalive_ = false;
	}

Atlast when the keepAlive_ is false, then Connection: close is added at this line.

proxygen/proxygen/lib/http/codec/HTTP1xCodec.cpp

Lines 676 to 680 in 5c69360

	if (keepalive_) {
	appendLiteral(writeBuf, len, "keep-alive");
	} else {
	appendLiteral(writeBuf, len, "close");
	}

I don't know how to fix this issue properly without impacting the HTTP Protocol. Can you fix it please.

[afrind]

As long as we haven't received any bytes beyond the headers, it should be ok to allow keep-alive on 4xx. Maybe adding a setParserPaused(true); when detecting the end of an upgrade request is the right move? If the app unpauses the parser before generating a 200-300, set keepAlive_ = false;?