Rescue ENOTCONN

[ocowchun]

Description

Rescue Errno::ENOTCONN since it doesn't have socket address

Closes: #2335

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
• I have added appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[MSP-Greg]

I looked into this earlier, wondering about adding at test for it. Everything I tried raised a ConnectionError instead of an Puma::MiniSSL::SSLError.

Also, wondered whether it would be better to replace the following method in MiniSSL::Socket:

def peeraddr(swallow: false)
  @socket.peeraddr
end

with something like:

def peeraddr(swallow: false)
  @socket.peeraddr
rescue => e
  if swallow
    ["<unknown>"]
  else
    raise e
  end
end

I think it's used four times, three times inside rescue code, once outside?

@ocowchun

Thanks for the PR.

EDIT:

Re the test, I was working with net/http, later on I'll try a raw ssl socket. Re the peeraddr method:

We could remove the 'swallow' logic and make it alwasy trap errors, code needing the errors could just use

ssl_skt.to_io.peeraddr

-or-
we could leave the existing method and add a method like peeraddr_trap or peeraddr_no_err...

[ocowchun]

@MSP-Greg Good point, base on current use of #peeraddr I think we can just swallow the error. It doesn't take much effort to modify if somewhere want to handle particular error in the future.

[MSP-Greg]

@ocowchun

I noticed that in Server#normalize_env we have the following, which calls Client#peerip, and sets the value to LOCALHOST_IP if Errno::ENOTCONN is raised:

puma/lib/puma/server.rb

Lines 495 to 501 in 18f1810

	begin
	addr = client.peerip
	rescue Errno::ENOTCONN
	# Client disconnects can result in an inability to get the
	# peeraddr from the socket; default to localhost.
	addr = LOCALHOST_IP
	end

Hence, maybe change Client#peerip from:

puma/lib/puma/client.rb

Lines 269 to 279 in 18f1810

	def peerip
	return @peerip if @peerip
	if @remote_addr_header
	hdr = (@env[@remote_addr_header] || LOCALHOST_IP).split(/[\s,]/).first
	@peerip = hdr
	return hdr
	end
	@peerip ||= @io.peeraddr.last
	end

to:

def peerip
  @peerip ||= if @remote_addr_header
    (@env[@remote_addr_header] || LOCALHOST_IP).split(/[\s,]/).first
  else
    @io.to_io.peeraddr.last
  end
end

Other than cleaning up/shortening the logic, the real change is @io.peeraddr.last to @io.to_io.peeraddr.last, which allows it work with MiniSSL::Socket.

ping @nateberkopec...

[nateberkopec]

So, a regular TCPSocket will raise a SocketError as well if it can't get the peer:

TCPSocket.open("www.asdasdasdasdasdkjjkqwlkqwejkljkwqeeq.com", 80) {|sock|
  p sock.peeraddr
}
# SocketError (getaddrinfo: nodename nor servname provided, or not known)

So, from my perspective, MiniSSL sockets should have the same contract. That means we should fix this not in the MiniSSL object, but at the location of the callers. Or at least I'm reluctant to.

There's three callsites for peeraddr in Puma rn: Client, Reactor, and Server. Each is doing something slightly different.

I think putting a new abstraction in here would be kind of tough. But we need to think about what happens at each location if peeraddr raises an exception.

[MSP-Greg]

Sorry, I should have given a fuller explanation.

In Server & Reactor, the call to MiniSSL::Socket#peeraddr happens within a rescue MiniSSL::SSLError section, and the value is only used to call @events.ssl_error.

In Client, it's only used in #peerip, which can be any type of socket. Hence, adding the to_io for an ssl socket, which returns self for TCP or UNIX sockets.

Server also contains code to set env[REMOTE_ADDR], which calls Client#peerip, but it isn't using the value for Events.

Also, re Events, peeraddr is only used for #ssl_error calls.

So, I think this is an improvement, but I'm not sure if MiniSSL::Socket#peercert will raise and error. It isn't 'protected' in the current code.

[ocowchun]

@MSP-Greg

So, I think this is an improvement, but I'm not sure if MiniSSL::Socket#peercert will raise and error. It isn't 'protected' in the current code.

For the MiniSSL::Socket#peercert part,

puma/lib/puma/minissl.rb

Lines 188 to 196 in 18f1810

	def peercert
	return @peercert if @peercert
	raw = @engine.peercert
	return nil unless raw
	@peercert = OpenSSL::X509::Certificate.new raw
	end
	end

puma/ext/puma_http11/mini_ssl.c

Lines 427 to 464 in 18f1810

	VALUE engine_peercert(VALUE self) {
	ms_conn* conn;
	X509* cert;
	int bytes;
	unsigned char* buf = NULL;
	ms_cert_buf* cert_buf = NULL;
	VALUE rb_cert_buf;
	Data_Get_Struct(self, ms_conn, conn);
	cert = SSL_get_peer_certificate(conn->ssl);
	if(!cert) {
	/*
	* See if there was a failed certificate associated with this client.
	*/
	cert_buf = (ms_cert_buf*)SSL_get_app_data(conn->ssl);
	if(!cert_buf) {
	return Qnil;
	}
	buf = cert_buf->buf;
	bytes = cert_buf->bytes;
	} else {
	bytes = i2d_X509(cert, &buf);
	X509_free(cert);
	if(bytes < 0) {
	return Qnil;
	}
	}
	rb_cert_buf = rb_str_new((const char*)(buf), bytes);
	if(!cert_buf) {
	OPENSSL_free(buf);
	}
	return rb_cert_buf;
	}

I have zero knowledge with c, but looks like it will return nil when something went wrong. Thus I think MiniSSL::Socket#peercert won't raise an error.

[MSP-Greg]

@ocowchun

Thanks. I guess I kind of forgot that many client ssl connections are made without a cert. I'm sure the code is ok.

[ocowchun]

@MSP-Greg for the swallow error part, I personally preference maintain the same interface rather than a handy abstraction. Rescue the error in the caller might be a more suitable choice, what do you think?

[MSP-Greg]

@ocowchun

I don't have strong feelings about how this is fixed. But, some thoughts:

1. The original commit you had catches the error in Reactor. But, the same method is used in Server, and there isn't any rescue for it. See below. Can we say for sure that the error can't happen in Server?

2. Puma has several places where exceptions are rescued a bit up the call stack. That can get tedious to wade through. Also, handling exceptions for 'attributes' (like peeraddr) is different that 'actions', like socket.close.

3. At present there are three places where peeraddr is called. Reactor and Server call it only on an MiniSSL::Socket, and Client calls it on a generic socket. The MiniSSL::Socket#peeraddr calls are only to provide a string to the Events#ssl_error method.

Given the above, I still prefer to catch in peeraddr. Maybe now or later a comment should be added explaining that ssl_socket.to_io.peeraddr should be used if an error needs to be caught upstream.

Regardless, I'll defer to @nateberkopec. I'm an old guy that has strong opinions, but moving forward is more important.

puma/lib/puma/server.rb

Lines 211 to 218 in 18f1810

	rescue MiniSSL::SSLError => e
	ssl_socket = client.io
	addr = ssl_socket.peeraddr.last
	cert = ssl_socket.peercert
	client.close
	@events.ssl_error e, addr, cert

[ocowchun]

@MSP-Greg How about the previous idea you metioned?

def peeraddr(swallow: false)
  @socket.peeraddr
rescue => e
  if swallow
    ["<unknown>"]
  else
    raise e
  end
end

The default behavior of peeraddr will follow the interface and we can swallow the error in Reactor and Server, thus several tedious tasks can be saved.

[MSP-Greg]

How about the previous idea you mentioned?

Agreed.

[MSP-Greg]

@ocowchun @nateberkopec

I really screwed up. I'll fix asap. In #2368, I forgot that I split some attr_* statements to show version info. Dumb, dumb, dumb...

[MSP-Greg]

@ocowchun

LGTM.

Hate to ask, can you rebase?

[ocowchun]

@MSP-Greg Sure.

It's a great experience, thanks you and @nateberkopec to review my PR. 😺

[nateberkopec]

Closing in favor of #2375 for a more complete fix. Thanks for getting us started though @ocowchun.