Unify handling of http over https errors

[julianz-]

Added an early probe to identify when a client attempts to speak unencrypted HTTP on an HTTPS port.

When a plaintext connection is detected, the server now attempts to use the raw TCP socket (if wrapped) to send a "400 Bad Request" error response before any TLS failure. This unifies the behavior of the 'builtin' and 'pyopenssl' adapters for this specific error condition.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.01%. Comparing base (c6da545) to head (720ce41).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #800      +/-   ##
==========================================
+ Coverage   77.70%   78.01%   +0.31%     
==========================================
  Files          41       41              
  Lines        4691     4749      +58     
  Branches      542      542              
==========================================
+ Hits         3645     3705      +60     
- Misses        904      905       +1     
+ Partials      142      139       -3     

[webknjaz]

We'll need to pass the raw socket smuggled through NoSSLError here. Although, I think that maybe we could just preserve the underlying socket before line 300 and pass that. Both these options will make the s._socket shennanigans redundant.

[julianz-]

For pyopenSSL, the socket has already been wrapped by the time it arrives at _from_server_socket() so you can't smuggle the raw socket through here unless you unwrap it. For the builtin adapter this is not the case so everything works fine. If you are against unwrapping it for sending the 400 bad request message then I suggest we drop the http check in pyopenSSL's wrap() function, and just apply this change for the builtin adapter. pyopenSSL's still successfully detects the http attempt later during the handshake phase and is able to send a 400 message. So this PR would not unify things but would enable the builtin adapter to handle this error condition better.

[webknjaz]

For pyopenSSL, the socket has already been wrapped by the time it arrives at _from_server_socket()

I don't see it — could you explain more?

My understanding is that s, addr = server_socket.accept() (line 288) works with a server socket, initialized somewhere here

cheroot/cheroot/server.py

Lines 1798 to 1871 in a475500

	def prepare(self): # noqa: C901 # FIXME
	"""Prepare server to serving requests.
	It binds a socket's port, setups the socket to ``listen()`` and does
	other preparing things.
	"""
	self._interrupt = None
	if self.software is None:
	self.software = '%s Server' % self.version
	# Select the appropriate socket
	self.socket = None
	msg = 'No socket could be created'
	if os.getenv('LISTEN_PID', None):
	# systemd socket activation
	self.socket = socket.fromfd(3, socket.AF_INET, socket.SOCK_STREAM)
	elif isinstance(self.bind_addr, (str, bytes)):
	# AF_UNIX socket
	try:
	self.bind_unix_socket(self.bind_addr)
	except socket.error as serr:
	msg = '%s -- (%s: %s)' % (msg, self.bind_addr, serr)
	raise socket.error(msg) from serr
	else:
	# AF_INET or AF_INET6 socket
	# Get the correct address family for our host (allows IPv6
	# addresses)
	host, port = self.bind_addr
	try:
	info = socket.getaddrinfo(
	host,
	port,
	socket.AF_UNSPEC,
	socket.SOCK_STREAM,
	0,
	socket.AI_PASSIVE,
	)
	except socket.gaierror:
	sock_type = socket.AF_INET
	bind_addr = self.bind_addr
	if ':' in host:
	sock_type = socket.AF_INET6
	bind_addr = bind_addr + (0, 0)
	info = [(sock_type, socket.SOCK_STREAM, 0, '', bind_addr)]
	for res in info:
	af, socktype, proto, _canonname, sa = res
	try:
	self.bind(af, socktype, proto)
	break
	except socket.error as serr:
	msg = '%s -- (%s: %s)' % (msg, sa, serr)
	if self.socket:
	self.socket.close()
	self.socket = None
	if not self.socket:
	raise socket.error(msg)
	# Timeout so KeyboardInterrupt can be caught on Win32
	self.socket.settimeout(1)
	self.socket.listen(self.request_queue_size)
	# must not be accessed once stop() has been called
	self._connections = connections.ConnectionManager(self)
	# Create worker threads
	self.requests.start()
	self.ready = True
	self._start_time = time.time()

.

It's a raw socket and s would be a raw socket too. Naming is unfortunate, though. Later on, s, ssl_env = self.server.ssl_adapter.wrap(s) (line 300) re-assigns a wrapped socket to the same variable so we can't access it anymore.

But what if we did raw_client_socket = s right after line 288, preserving that socket pointer? (or something more elegant along the lines)

Then, we'd be able to use it right here, without any smuggling.

Am I wrong?

[julianz-]

prepare() creates the socket, calls HTTPServer.bind(), which in turn calls prepare_socket(), which calls sock = ssl_adapter.bind(sock). Because pyopenSSL currently wraps the socket with its bind method, the result is a wrapped socket of type pyopenSSL.SSLConnection at the end of prepare(). So when we arrive at _from_server_socket() in connections.py, the server_socket parameter is already a wrapped socket, not a raw socket. Do you know why the wrapping was put in the bind method in pyopenSSL? Seems that was a mistake.

I have tried moving the wrapping from bind() to wrap() and after applying a few other fixes elsewhere in pyopenSSL got everything to work. I am inclined to do a new PR with that change first before we complete this PR. That way, we can fix this architectural flaw first. What do you think?

[julianz-]

Incidentally, pyOpenSSL currently relies on this code in server.py to send the 400 Bad Request message for flagging the http over https error. It relies on unwrapping the socket.

def _handle_no_ssl(self, req):
        if not req or req.sent_headers:
            return
        # Unwrap wfile
        try:
            resp_sock = self.socket._sock
        except AttributeError:
            # self.socket is of OpenSSL.SSL.Connection type
            resp_sock = self.socket._socket
        self.wfile = StreamWriter(resp_sock, 'wb', self.wbufsize)
        msg = (
            'The client sent a plain HTTP request, but '
            'this server only speaks HTTPS on this port.'
        )
        req.simple_response('400 Bad Request', msg)
        self.linger = True

[julianz-]

Although I think #802 might be worth doing first?

[webknjaz]

I'm not sure — some changes over there seem misplaced. Plus I'd feel more confident reviewing it after this refactoring.

[julianz-]

Okay sure.

[julianz-]

I figured out why the old code didn't need the line conn.set_accept_state() that we now have in wrap(). In the ConnectionsManager in connections.py we have:

def _from_server_socket(self, server_socket):  # noqa: C901  # FIXME
   try:
       s, addr = server_socket.accept() 

For the old code, server_socket was an SSL.Connection. Calling accept() on an SSL.Connection listening socket returns a new SSL.Connection that is set to server mode. In the new code, server_socket is a raw socket that's listening for clients. Calling accept() spawns a socket that has accepted a connection so it becomes a connected TCP socket. However, the raw socket doesn't know it's going to be a server socket. TCP sockets are bidirectional - a connected TCP socket looks the same whether it initiated the connection or accepted it. SSL sockets are not though so when the raw socket becomes wrapped (further down this function) the SSL.Connection doesn't know what mode it's supposed to be in. That's why we need to call set_accept_state() when the socket is wrapped.

[webknjaz]

Nice, thanks for documenting this!

[webknjaz]

So there's two things left to address:

• that small cosmetic change note fix
• attempt replacing mocking with a stub — if it's challenging, we can skip it, just document what the obstacle is in the related thread it's more important to make up for the lost coverage https://github.com/cherrypy/cheroot/pull/800/files#r2581708480

[julianz-]

I fixed the doc fragment and played around with your spy code but wasn't sure it achieved the purpose you had in mind. See my note above about this.

[webknjaz]

Great job!

[julianz-]

Appreciate your help and guidance as always!