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

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

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