Disable tunnel for http2 requests

[Nightrider0098]

Closes #3298

[pquentin]

Thanks, can you please add an unit test to make sure this isn't lost in a later refactoring? And make sure this does not break existing tests.

Also, please make sure to add "Closes #XXXX" in your pull request description. This closes the issue automatically when the pull request is merged, and helps understanding what you're fixing. I've just edited your pull request to include that.

[sethmlarson]

I'm also imagining a test case that checks the bytes sent over the socket for the absence of the "h2" ALPN in the TLS ClientHello, I believe we do something similar for server name?

[pquentin]

I'm also imagining a test case that checks the bytes sent over the socket for the absence of the "h2" ALPN in the TLS ClientHello, I believe we do something similar for server name?

I did not know, but we do have a SNI socketlevel test: test_hostname_in_first_request_packet. And we even have an ALPN test:

urllib3/test/with_dummyserver/test_socketlevel.py

Lines 108 to 133 in d7bb83b

	def test_alpn_protocol_in_first_request_packet(self) -> None:
	if not has_alpn():
	pytest.skip("ALPN-support not available")
	done_receiving = Event()
	self.buf = b""
	def socket_handler(listener: socket.socket) -> None:
	sock = listener.accept()[0]
	self.buf = sock.recv(65536) # We only accept one packet
	done_receiving.set() # let the test know it can proceed
	sock.close()
	self._start_server(socket_handler)
	with HTTPSConnectionPool(self.host, self.port) as pool:
	try:
	pool.request("GET", "/", retries=0)
	except MaxRetryError: # We are violating the protocol
	pass
	successful = done_receiving.wait(LONG_TIMEOUT)
	assert successful, "Timed out waiting for connection accept"
	for protocol in util.ALPN_PROTOCOLS:
	assert (
	protocol.encode("ascii") in self.buf
	), "missing ALPN protocol in SSL handshake"

We will eventually have to adapt this to test to include checking that the h2 ALPN is included for normal requests, but that can happen in another PR. I also tried checking what happens when setting up a tunnel in a modified version of this test, but I realized that this pull request makes sure the request does not even go out (raises NotImplementedError).

And since the request does not go out, it does not have to be a test that includes a server in test_with_dummyserver/test_proxy_poolmanager.py. It can be in test/test_proxymanager.py. When using the http_version fixture and http_version is equal to http2, doing a tunnel request (with HTTP CONNECT) should raise NotImplementedError.

[Nightrider0098]

@pquentin Should i override set_tunnel in connection pool or writing the check in init is fine?

[sethmlarson]

Thank you for this PR, I have a suggestion that'll ensure the test case is useful into the near future where HTTPSConnection begins speaking HTTP/2 on its own (without a separate connection).

The approach would be:

• Setup a socket server that records the handshake bytes
• Send a tunnel proxy connection with TLS handshake
• Assert that the bytes for an ALPN of http/1.1 are there, but not h2.

This way in the future when the two connections are combined together this test case is still useful and it removes the need for the other tests.

[sethmlarson]

See this test which verifies that the hostname is in the initial TLS handshake via SNI: https://github.com/urllib3/urllib3/blob/main/test/with_dummyserver/test_socketlevel.py#L103

Can assert the http/1.1 and h2 ALPN strings in a similar way.

[pquentin]

Thanks! Can you please look into the CI failures?

[sethmlarson]

This is using HTTP, tunneling and ALPN are TLS features so requires HTTPS. It's expected for the handshake to fail, we want to see what ALPN values are being offered and not complete the handshake.

[sethmlarson]

For ALPN the value is b"http/1.1".