Re: [PATCH] Avoid mixing custom and OpenSSL BIO functions

> On 4 Sep 2024, at 23:19, David Benjamin <davidben(at)google(dot)com> wrote:
>
> On Wed, Sep 4, 2024 at 9:22 AM Daniel Gustafsson <daniel(at)yesql(dot)se <mailto:daniel(at)yesql(dot)se>> wrote:
>> > On 3 Sep 2024, at 14:18, Daniel Gustafsson <daniel(at)yesql(dot)se <mailto:daniel(at)yesql(dot)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.

Following the bouncing ball around the code tonight I agree with that. I think
we should stick to setting BIO_TYPE_SOURCE_SINK though, if only for passing in
zero might seem incorrect enough that we get emails about that from future readers.

>> + 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! :-)

I wholeheartedly agree. I've previously gone on record saying that every day
with the OpenSSL API is an adventure, and the errorhandling code doubly so.

>> The attached v5 is a fresh rebase with my comments from above as 0002 to
>> illustrate.
>
> LGTM

Thanks for reviewing, I plan on going ahead with this patch shortly.

--
Daniel Gustafsson