Refactor: dust off OpenSSL::BIO [fixup #16480]#16640
Refactor: dust off OpenSSL::BIO [fixup #16480]#16640ysbaddaden merged 1 commit intocrystal-lang:masterfrom
OpenSSL::BIO [fixup #16480]#16640Conversation
- Remove dead code since we dropped support for OpenSSL < 1.1.1 - Reinstate usage of read_ex and write_ex (inadvertently removed) - Use class methods instead of local procs
| def self.write(bio, data, len) | ||
| io = Box(IO).unbox(LibCrypto.BIO_get_data(bio)) | ||
| io.write Slice.new(data, len) | ||
| len |
There was a problem hiding this comment.
thought: Not directly related to this change: What happens when these methods raise?
Do we need to rescue exceptions and return an error value?
There was a problem hiding this comment.
Exactly my thoughts when I saw that. It looks like it's working in practice, but we might want to investigate.
We should review the write and write_ex methods (and read counterparts) to see if we shall rescue and return -errno or -1 and set errno, or set the SSL error, or something else entirely.
(in a follow up)
There was a problem hiding this comment.
Let's review the whole BIO and compare it with https://github.com/openssl/openssl/blob/baf4156f7052cf5fa08aaf5187dc1f5d25e49664/crypto/bio/bss_sock.c
There's likely a bunch of details to fix (and KTLS support to hack in).
There was a problem hiding this comment.
Example: BIO_meth_new(Int32::MAX, "crystal") should be (omitting LibCrypto for readability):
id = BIO_get_new_index
methods = BIO_meth_new(id | BIO_TYPE_SOURCE_SINK | BIO_TYPE_DESCRIPTOR, "crystal")There was a problem hiding this comment.
Reading the OpenSSL source code, I believe it expects the BIO methods to return -1 with the system error set (Errno, WinError, ...). In practice the nested C calls are expected to each fail immediately and return -1 with the system error already set by the libc functions.
We'd probably want to do that. Exceptions going from Crystal -> C -> Crystal, thus bypassing the C land, might be a bad idea. Though it might not be an issue in practice: read or write fails and the C functions might just return ret if ret <= 0 (bubble the error up with no cleanup).
An issue is that we'd raise twice. If we could assume that io is a Socket, then maybe we could bypass IO (i.e. call Crystal::EventLoop#read(Socket) directly), and rework the Crystal::EventLoop interface to return system errors (e.g. #read : Errno | WinError | SizeT) instead of raising 🤔
There was a problem hiding this comment.
Interacting directly with Socket on the event loop would align well with #16642.
There was a problem hiding this comment.
We actually need it to be a general IO. There's one usage in stdlib where we use an IO::Memory for getting the value out of x509 certificate.
Now, nothing prevents us from having two custom BIOs: one for general IO and another dedicated for Socket.
There's no reason autoflush the write buffer of the underlying IO object (e.g. TCP socket) when reading from the socket. Especially since we already disable the buffers in `OpenSSL::SSL::Socket#initialize`, so it ends up a no-op in practice. `Crystal::BIO#initialize` now always disables the buffers, so the logic is grouped together, instead of having `OpenSSL::SSL::Socket` do it. NOTE: this is potentially a breaking change if someone re-enabled buffers or expects the buffers to be buffered (in non SSL socket situations). See #16640 (comment)
Properly initializes the custom BIO by asking `libcrypto` for an available BIO identifier and declaring the actual BIO types (`SOURCE SINK`, `DESCRIPTOR`) instead of enabling all flags and hard-coding an arbitrary identifier. Implements `BIO_C_GET_FD` when the underlying IO is a file descriptor, since we declare the BIO as being a `DESCRIPTOR`. Also fixes `Crystal::BIO#initialize` that stored the boxed IO, but `Box.box(IO)` merely casts `IO` as a `Void*` so we end up storing the same pointer to IO twice. Follow up to #16640. Extracted from #16642.
2 participants
Fixup / follow-up of #16480.