On Wed, Sep 4, 2024 at 9:22 AM Daniel Gustafsson <dan...@yesql.se> wrote:
> > On 3 Sep 2024, at 14:18, Daniel Gustafsson <dan...@yesql.se> wrote: > > > Attached is a v4 rebase over the recent OpenSSL 1.0.2 removal which made > this > > patch no longer apply. I've just started to dig into it so have no > comments on > > it right now, but wanted to get a cleaned up version into the CFBot. > > CFBot building green for this, I just have a few small questions/comments: > > + my_bio_index |= BIO_TYPE_SOURCE_SINK; > > According to the OpenSSL docs we should set BIO_TYPE_DESCRIPTOR as well as > this > BIO is socket based, but it's not entirely clear to me why. Is there a > specific reason it was removed? > Looking around at what uses it, it seems BIO_TYPE_DESCRIPTOR is how OpenSSL decides whether the BIO is expected to respond to BIO_get_fd (BIO_C_GET_FD). Since the custom BIO is not directly backed by an fd and doesn't implement that control, I think we don't want to include that bit. https://github.com/openssl/openssl/blob/openssl-3.3.2/ssl/ssl_lib.c#L1621-L1643 The other place I saw that cares about this bit is this debug callback. That one's kinda amusing because it assumes every fd-backed BIO stores its fd in bio->num, but bio->num is not even accessible to external BIOs. I assume this is an oversight because no one cares about this function. Perhaps that should be sampled from BIO_get_fd. https://github.com/openssl/openssl/blob/openssl-3.3.2/crypto/bio/bio_cb.c#L45-L62 Practically speaking, though, I don't think it makes any difference whether BIO_TYPE_DESCRIPTOR or even BIO_TYPE_SOURCE_SINK is set or unset. I couldn't find any code that's sensitive to BIO_TYPE_SOURCE_SINK and presumably Postgres is not calling SSL_get_rfd on an SSL object that it already knows is backed by a PGconn. TBH if you just passed 0 in for the index, it would probably work just as well. > + bio_method = port_bio_method(); > if (bio_method == NULL) > { > SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB); > > SSL_F_SSL_SET_FD is no longer the correct function context for this error > reporting. In OpenSSL 3.x it means nothing since SSLerr throws away the > function when calling ERR_raise_data, but we still support 1.1.0+. I > think the > correct error would be BIOerr(BIO_F_BIO_METH_NEW..) but I wonder if we > should > just remove it since BIO_meth_new and BIO_new already set errors for us to > consume? It doesn't seem to make sense to add more errors on the queue > here? > The same goes for the frontend part. > Ah yeah, +1 to removing them. I've always found external code adding to the error queue to be a little goofy. OpenSSL's error queue is weird enough without external additions! :-) > The attached v5 is a fresh rebase with my comments from above as 0002 to > illustrate. > LGTM David
