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