ssl: support IO-like object as the underlying transport#736
Conversation
| BIO_clear_retry_flags(p->bio); | ||
|
|
||
| VALUE fargs[] = { INT2NUM(p->dlen), nonblock_kwargs }; | ||
| VALUE ret = rb_funcallv_public_kw(io, rb_intern("read_nonblock"), |
There was a problem hiding this comment.
should't this ret be somhow marked so it's not moved? (thinking of compaction here).
probably too early for optimizations, ,but this bit could benefit of a socket-local string to act as a buffer (second argument of :read_nonblock)
There was a problem hiding this comment.
The content is copied in this function and the String object won't be kept, so it shouldn't be necessary.
probably too early for optimizations, ,but this bit could benefit of a socket-local string to act as a buffer (second argument of
:read_nonblock)
Yes, this is definitely a possible optimization.
|
@rhenium anything I can help with to move this forward? |
|
@rhenium friendly ping |
ded7cb0 to
84ead32
Compare
|
I just pushed what I have in my local branch so far, rebased on top of current master to resolve merge conflicts. This is not ready to merge yet. Some unresolved issues:
|
Indeed, I didn't know that doing that was dangerous, though that One option could be to assume that the IO-like object must indeed be an SSLSocket, and one could deal with it by setting an ivar "flag" telling the internal implementation to skip 2nd-level
Can they happen at the same time? Perhaps suggestion above deals with it? An alternative approach could be to go back to the drawing board and try again the approach using |
Please see I think the segfault is fixable by (ab)using
|
|
Sorry for not replying sooner, have been stumped with other chores. I think "last exception wins" is reasonable, and considering this is a new feature, it's not really breaking compatibility. Lmk when you work around the jump tag issue, I can run a few sanity checks on my side as well. |
|
This branch needs more polishing, but it should be working aside from some edge cases. I'd appreciate if you could test it. I should have worded more accurately, but the last commit changed it so that the later tag jump (which includes raising an exception) wins against the former one. This should fix the segfault. One concern is that this can accidentally suppress an important jump like the one created by I haven't come up with a nice solution for this. |
|
just a heads-up that I've been trying to incorporate this in my branch's CI, but having issues due to C extension compilation and openssl being an stdlib I guess (opened a ticket here, in case you know why that happens and can recommend a workaround). |
That Gemfile snippet works as expected for me (ruby-build Ruby 3.4 on Linux). I'm not sure why it's failing for you. What happens if you clone the repository, run |
|
I did some effort to build openssl from branch in I may only be able to do some investigation in one week. |
I was able to reproduce it locally. This is a bug in |
|
thx, tests are passing now 👍 |
|
Just spent the day scratching my head over Net::HTTP's behavior around proxies and didn't see the tickets that led to this PR until now lol |
d8004f4 to
c9a1830
Compare
We can implement this now that we require Ruby 2.7 or later. The return value of OpenSSL's *_set_ex_data() functions is not checked since replacing an existing index should not fail.
We will need to store additional internal states in SSLSocket in the upcoming patches. Prepare a new struct for such purposes and store it in SSL's ex_data. Refactor existing internal states that are local to C to use this new struct: - The back reference to the Ruby object, currently in another ex_data - The callback jump tag, currently in an instance variable
Do not attempt to execute Ruby callback when another callback has already raised an exception and we are waiting for a chance to call rb_jump_tag(). As far as I know, this never happens currently. However, an upcoming patch to use custom BIO_METHOD in SSLSocket may make it possible. Let's handle it explicitly.
Set the callback to SSL_CTX instead of SSL so that we do not have to configure every SSL object, similar to all other existing callbacks. Also, wrap rb_funcall() in rb_protect() to prevent exiting from SSL_accept() prematurely.
Re-raise exceptions raised inside client_cert_cb from the originating Ruby method, which can be any of #connect, #sysread*, or #syswrite*.
Look up the callback procs by checking the corresponding instance variables on SSLContext directly. The removed methods SSLSocket#session_new_cb and #session_get_cb are private and have not been part of the public interface of ruby/openssl. Also remove the unused method SSLSocket#client_cert_cb.
Prepare the argument array object inside rb_protect(). The callback is already called in it, but the array allocation can also potentially fail and needs to be done there.
Store the IO object in struct ossl_ssl_data instead of an instance variable that is writable from Ruby.
Implement a bare minimal BIO_METHOD required for SSL/TLS. The underlying IO-like object must implement the following methods: - #read_nonblock(len, exception: false) - #write_nonblock(str, exception: false) - #flush A later commit will wire it into OpenSSL::SSL::SSLSocket.
The result value is used for generating an informative error message for OpenSSL::SSL::SSLError. We can simply skip it if it is not available.
Assume true (no buffering) if the underlying IO-like object does not respond to sync.
There are use cases to establish a TLS connection on top of a non-OS stream, such as another TLS connection. To do this today, a workaround using dummy socket pairs is necessary. Currently, OpenSSL::SSL::SSLSocket.new requires a T_FILE object because we pass the file descriptor the OpenSSL. This patch relaxes this to allow any Ruby object that responds to non-blocking IO methods, such as read_nonblock. OpenSSL's TLS implementation uses an IO abstraction layer called BIO to interact with the underlying socket. By passing the file descriptor to SSL_set_fd(), a BIO with the BIO_s_socket() BIO_METHOD is implicitly created. We can set up a BIO using our own custom BIO_METHOD implementation to redirect read/write request to Ruby method calls. The previous patch added such a BIO_METHOD implementation. We continue to use the BIO_s_socket() BIO_METHOD if the user passes a real IO object because it is more efficient. Behavior change is not expected for existing programs.
Let's see what will break with this.
An implementation of #731. The test suite passes on my box, but it needs more testing especially around the error handling.
This adds support for IO-like object that is not backed by a file descriptor by defining a
BIO_METHODto wrap the following Ruby methods.