SSLSocket#accept is confusing.

[ioquatix]

I would like to propose that we rename SSLSocket#accept and SSLSocket#accept_nonblock to SSLSocket#start and SSLSocket#start_nonblock.

There are two reasons:

1. It aligns better with SSLServer#start_immediately. In other words, start_immediately -> call SSLSocket#start immediately on accept.
2. It avoids confusion with Socket#accept (and indirectly SSLServer#accept) which serve an entirely different purpose.

I tried to write generic socket handling code like this:

server.accept do |socket|
  if socket.respond_to?(:accept)
    socket.accept
  end
  
  yield socket
end

However this code failed because Socket implements #accept - I did not think the design through fully. But, I think overloading SSLSocket#accept in this way is basically confusing.

Of course, we should keep aliases for backwards compatibility.

@rhenium wdyt?

[rhenium]

SSLSocket#start would be a confusing name for a method that can only be used by the server. I imagine clients would also want to "start" a TLS connection.

I think SSLSocket#{accept,connect} might have been given more explicit names like SSLSocket#start_{server,client} when this library was initially written, but I don't think the current method names are particularly confusing. They have different functions from those on Socket, but are only used in a different context.

I tried to write generic socket handling code like this:

server.accept do |socket|
  if socket.respond_to?(:accept)
    socket.accept
  end
  
  yield socket
end

This feels like a wrong abstraction to me. I suppose the server.accept is a method that is accepting a TCP socket and instantiating SSLSocket. Shouldn't SSLSocket#accept be called in it, too?

[rhenium]

SSLSocket#accept wraps SSL_accept(ssl), which can be broken down to two function calls SSL_set_accept_state(ssl) and SSL_do_handshake(ssl).

I think an unfortunate design choice here is that SSLSocket doesn't know if it will be a client or a server until either #accept or #connect is called. I can't think of a situation where SSLSocket can become both, so we could have provided SSLSocket.client(underlying_socket) and SSLSocket.server(underlying_socket) instead of the current SSLSocket.new(underlying_socket), and provide a single SSLSocket#start method instead of #accept and #connect.

However, I'm worried changing a fundamental interface now will cause another kind of confusion.

[ioquatix]

For background, the current model of performing SSL_do_handshake as part of the accept loop is extremely problematic as there is no guarantee it will complete, potentially blocking the accept loop and causing a DoS on the server.

The correct strategy as you may already know is something like this:

server.start_immediately = false

server.accept do |socket|
  Thread.new do
    if socket.respond_to?(:accept)
      socket.accept
    end
    
    yield socket
  end
end

Trying to solve this problem in a way which merges these two operations is thus not a solution.

SSLSocket#start would be a confusing name for a method that can only be used by the server. I imagine clients would also want to "start" a TLS connection.

Can't they? Is it not possible to call SSL_do_handshake on the client explicitly? I believe the similar problem can exist for both server and client - i.e. a client connect operation doing IO can also be a source of problems. But I have not personally experienced it yet. I'd be in favour of making this separation both for the server and the client if that's resonable.

[rhenium]

Yes, a naive implementation like SSLServer with start_immediately has the problem, but safely copying the behavior of Socket.tcp_server_loop by using IO.select and accept_nonblock methods should be possible.

A simplistic implementation that just delays the handshake, like SSLServer without start_immediately, looks intentionally breaking the abstraction boundary. Users should be aware of it, and checking the type (socket.is_a?(OpenSSL::SSL::SSLSocket)) would be more appropriate in that case, not the existence of a method.

Since Socket#accept would not be called on a connected TCP socket normally, using the same name #accept for a method for starting a TLS handshake doesn't seem problematic to me. I think it could have been named differently, but I doubt it's worth renaming now.

---

Is it not possible to call SSL_do_handshake on the client explicitly?

SSL_connect() is SSL_set_connect_state() + SSL_do_handshake().

It is. But before calling SSL_do_handshake(), either SSL_set_connect_state() or SSL_set_accept_state() must be called first, for which we don't have bindings. In theory we can add new methods corresponding to these three functions, but since we can't remove the current interface, mixing all of them on SSLSocket would make the situation even more confusing IMO.

# We currently have this
s = SSLSocket.new(tcp, tx)     # SSL_new()
s.accept                       # SSL_accept() (== SSL_set_accept_state() + SSL_do_handshake())

# a possible alternative interface we could have opted for
s = SSLSocket.server(tcp, ctx) # SSL_new() + SSL_set_accept_state()
s.start                        # SSL_do_handshake()

# but if we have both -- it's an error to do this. Isn't it confusing?
s = SSLSocket.new(tcp, tx)     # SSL_new()
s.start                        # SSL_do_handshake()

[ioquatix]

Since Socket#accept would not be called on a connected TCP socket normally, using the same name #accept for a method for starting a TLS handshake doesn't seem problematic to me. I think it could have been named differently, but I doubt it's worth renaming now.

I basically disagree, and think it's worth renaming and/or adding an alias. I think it's extremely confusing and as I already stated it can be dangerous. Expecting users to use accept_nonblock is also non-trivial. I'll spend some time thinking about your proposed interface, but honestly I'm fine with an alias start accept.