Horton

Thread

Commits

  1. Avoid mixing custom and OpenSSL BIO functions

  2. Use BIO_{get,set}_app_data instead of BIO_{get,set}_data.

  1. [PATCH] Avoid mixing custom and OpenSSL BIO functions

    David Benjamin <davidben@google.com> — 2024-02-11T18:19:00Z

    Hi all,

I've attached a patch for the master branch to fix up the custom BIOs used
by PostgreSQL, in light of the issues with the OpenSSL update recently.
While c82207a548db47623a2bfa2447babdaa630302b9 (switching from BIO_get_data
to BIO_get_app_data) resolved the immediate conflict, I don't think it
addressed the root cause, which is that PostgreSQL was mixing up two BIO
implementations, each of which expected to be driving the BIO.

It turns out the parts that came from the OpenSSL socket BIO were a no-op,
and in fact PostgreSQL is relying on it being a no-op. Instead, it's
cleaner to just define a custom BIO the normal way, which then leaves the
more standard BIO_get_data mechanism usable. This also avoids the risk that
a future OpenSSL will add a now BIO_ctrl to the socket type, with libssl
calling into it, and then break some assumptions made by PostgreSQL.

I've attached a patch which does that. The existing SSL tests pass with
it, tested on Debian stable. (Though it took me a few iterations to figure
out how to run the SSL tests, so it's possible I've missed something.)

The patch is not expected to change behavior, so nothing new to document,
nor any expected performance impact.

David

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-12T14:38:39Z

    > On 11 Feb 2024, at 19:19, David Benjamin <davidben@google.com> wrote:
> 
> Hi all,
> 
> I've attached a patch for the master branch to fix up the custom BIOs used by PostgreSQL, in light of the issues with the OpenSSL update recently. While c82207a548db47623a2bfa2447babdaa630302b9 (switching from BIO_get_data to BIO_get_app_data) resolved the immediate conflict, I don't think it addressed the root cause, which is that PostgreSQL was mixing up two BIO implementations, each of which expected to be driving the BIO.

Thanks for your patch, I'm still digesting it so will provide more of a review
later.

> It turns out the parts that came from the OpenSSL socket BIO were a no-op, and in fact PostgreSQL is relying on it being a no-op. Instead, it's cleaner to just define a custom BIO the normal way, which then leaves the more standard BIO_get_data mechanism usable. This also avoids the risk that a future OpenSSL will add a now BIO_ctrl to the socket type, with libssl calling into it, and then break some assumptions made by PostgreSQL.

+		case BIO_CTRL_FLUSH:
+			/* libssl expects all BIOs to support BIO_flush. */
+			res = 1;
+			break;

Will this always be true?  Returning 1 implies that we have flushed all data on
the socket, but what if we just before called BIO_set_retry..XX()?

> I've attached a patch which does that. The existing SSL tests pass with it, tested on Debian stable. (Though it took me a few iterations to figure out how to run the SSL tests, so it's possible I've missed something.)

We've done a fair bit of work on making them easier to run, so I'm curious if
you saw any room for improvements there as someone coming to them for the first
time?

--
Daniel Gustafsson

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

    David Benjamin <davidben@google.com> — 2024-02-12T16:31:21Z

    On Mon, Feb 12, 2024 at 9:38 AM Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 11 Feb 2024, at 19:19, David Benjamin <davidben@google.com> wrote:
> > It turns out the parts that came from the OpenSSL socket BIO were a
> no-op, and in fact PostgreSQL is relying on it being a no-op. Instead, it's
> cleaner to just define a custom BIO the normal way, which then leaves the
> more standard BIO_get_data mechanism usable. This also avoids the risk that
> a future OpenSSL will add a now BIO_ctrl to the socket type, with libssl
> calling into it, and then break some assumptions made by PostgreSQL.
>
> +               case BIO_CTRL_FLUSH:
> +                       /* libssl expects all BIOs to support BIO_flush. */
> +                       res = 1;
> +                       break;
>
> Will this always be true?  Returning 1 implies that we have flushed all
> data on
> the socket, but what if we just before called BIO_set_retry..XX()?
>

The real one is also just a no-op. :-)
https://github.com/openssl/openssl/blob/master/crypto/bio/bss_sock.c#L215

This is used in places like buffer BIO or the FILE* BIO, where BIO_write
might accept data, but stage it into a buffer, to be flushed later. libssl
ends up calling BIO_flush at the end of every flight, which in turn means
that BIOs used with libssl need to support it, even if to return true
because there's nothing to flush. (Arguably TCP sockets could have used a
flush concept, to help control Nagle's algorithm, but for better or worse,
that's a socket-wide TCP_NODELAY option, rather than an explicit flush
call.)

BIO_set_retry.. behaves like POSIX I/O, where a failed EWOULDBLOCK write is
as if you never wrote to the socket at all and doesn't impact socket state.
That is, the data hasn't been accepted yet. It's not expected for BIO_flush
to care about the rejected write data. (Also I don't believe libssl will
ever trigger this case.) It's confusing because unlike an EWOULDBLOCK
errno, BIO_set_retry.. *is* itself BIO state, but that's just because the
BIO calling convention is goofy and didn't just return the error out of the
return value. So OpenSSL just stashes the bit on the BIO itself, for you to
query out immediately afterwards.


> > I've attached a patch which does that. The existing SSL tests pass with
> it, tested on Debian stable. (Though it took me a few iterations to figure
> out how to run the SSL tests, so it's possible I've missed something.)
>
> We've done a fair bit of work on making them easier to run, so I'm curious
> if
> you saw any room for improvements there as someone coming to them for the
> first
> time?
>

 A lot of my time was just trying to figure out how to run the tests in the
first place, so perhaps documentation? But I may just have been looking in
the wrong spot and honestly didn't really know what I was doing. I can try
to summarize what I did (from memory), and perhaps that can point to
possible improvements?

- I looked in the repository for instructions on running the tests and
couldn't find any. At this point, I hadn't found src/test/README.
- I found
https://wiki.postgresql.org/wiki/Developer_FAQ#How_do_I_test_my_changes.3F,
linked from https://www.postgresql.org/developer/
- I ran the configure build with --enable-cassert, ran make check, tests
passed.
- I wrote my patch and then spent a while intentionally adding bugs to see
if the tests would catch it (I wasn't sure whether there was ssl test
coverage), finally concluding that I wasn't running any ssl tests
- I looked some more and found src/test/ssl/README
- I reconfigured with --enable-tap-tests and ran make check
PG_TEST_EXTRA=ssl per those instructions, but the SSL tests still weren't
running
- I grepped for PG_TEST_EXTRA and found references in the CI config, but
using the meson build
- I installed meson, mimicked a few commands from the CI. That seemed to
work.
- I tried running only the ssl tests, looking up how you specify individual
tests in meson, to make my compile/test cycles a bit faster, but they
failed.
- I noticed that the first couple "tests" were named like setup tasks, and
guessed that the ssl tests depended on this setup to run. But by then I
just gave up and waited out the whole test suite per run. :-)

Once I got it running, it was quite smooth. I just wasn't sure how to do it.

David

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

    David Benjamin <davidben@google.com> — 2024-02-15T03:58:17Z

    By the way, I'm unable to add the patch to the next commitfest due to the
cool off period for new accounts. How long is that period? I don't suppose
there's a way to avoid it?

On Mon, Feb 12, 2024 at 11:31 AM David Benjamin <davidben@google.com> wrote:

> On Mon, Feb 12, 2024 at 9:38 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> > On 11 Feb 2024, at 19:19, David Benjamin <davidben@google.com> wrote:
>> > It turns out the parts that came from the OpenSSL socket BIO were a
>> no-op, and in fact PostgreSQL is relying on it being a no-op. Instead, it's
>> cleaner to just define a custom BIO the normal way, which then leaves the
>> more standard BIO_get_data mechanism usable. This also avoids the risk that
>> a future OpenSSL will add a now BIO_ctrl to the socket type, with libssl
>> calling into it, and then break some assumptions made by PostgreSQL.
>>
>> +               case BIO_CTRL_FLUSH:
>> +                       /* libssl expects all BIOs to support BIO_flush.
>> */
>> +                       res = 1;
>> +                       break;
>>
>> Will this always be true?  Returning 1 implies that we have flushed all
>> data on
>> the socket, but what if we just before called BIO_set_retry..XX()?
>>
>
> The real one is also just a no-op. :-)
> https://github.com/openssl/openssl/blob/master/crypto/bio/bss_sock.c#L215
>
> This is used in places like buffer BIO or the FILE* BIO, where BIO_write
> might accept data, but stage it into a buffer, to be flushed later. libssl
> ends up calling BIO_flush at the end of every flight, which in turn means
> that BIOs used with libssl need to support it, even if to return true
> because there's nothing to flush. (Arguably TCP sockets could have used a
> flush concept, to help control Nagle's algorithm, but for better or worse,
> that's a socket-wide TCP_NODELAY option, rather than an explicit flush
> call.)
>
> BIO_set_retry.. behaves like POSIX I/O, where a failed EWOULDBLOCK write
> is as if you never wrote to the socket at all and doesn't impact
> socket state. That is, the data hasn't been accepted yet. It's not expected
> for BIO_flush to care about the rejected write data. (Also I don't believe
> libssl will ever trigger this case.) It's confusing because unlike an
> EWOULDBLOCK errno, BIO_set_retry.. *is* itself BIO state, but that's just
> because the BIO calling convention is goofy and didn't just return the
> error out of the return value. So OpenSSL just stashes the bit on the BIO
> itself, for you to query out immediately afterwards.
>
>
>> > I've attached a patch which does that. The existing SSL tests pass with
>> it, tested on Debian stable. (Though it took me a few iterations to figure
>> out how to run the SSL tests, so it's possible I've missed something.)
>>
>> We've done a fair bit of work on making them easier to run, so I'm
>> curious if
>> you saw any room for improvements there as someone coming to them for the
>> first
>> time?
>>
>
>  A lot of my time was just trying to figure out how to run the tests in
> the first place, so perhaps documentation? But I may just have been looking
> in the wrong spot and honestly didn't really know what I was doing. I can
> try to summarize what I did (from memory), and perhaps that can point to
> possible improvements?
>
> - I looked in the repository for instructions on running the tests and
> couldn't find any. At this point, I hadn't found src/test/README.
> - I found
> https://wiki.postgresql.org/wiki/Developer_FAQ#How_do_I_test_my_changes.3F,
> linked from https://www.postgresql.org/developer/
> - I ran the configure build with --enable-cassert, ran make check, tests
> passed.
> - I wrote my patch and then spent a while intentionally adding bugs to see
> if the tests would catch it (I wasn't sure whether there was ssl test
> coverage), finally concluding that I wasn't running any ssl tests
> - I looked some more and found src/test/ssl/README
> - I reconfigured with --enable-tap-tests and ran make check
> PG_TEST_EXTRA=ssl per those instructions, but the SSL tests still weren't
> running
> - I grepped for PG_TEST_EXTRA and found references in the CI config, but
> using the meson build
> - I installed meson, mimicked a few commands from the CI. That seemed to
> work.
> - I tried running only the ssl tests, looking up how you specify
> individual tests in meson, to make my compile/test cycles a bit faster, but
> they failed.
> - I noticed that the first couple "tests" were named like setup tasks, and
> guessed that the ssl tests depended on this setup to run. But by then I
> just gave up and waited out the whole test suite per run. :-)
>
> Once I got it running, it was quite smooth. I just wasn't sure how to do
> it.
>
> David
>

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-02-15T12:58:45Z

    > On 15 Feb 2024, at 04:58, David Benjamin <davidben@google.com> wrote:
> 
> By the way, I'm unable to add the patch to the next commitfest due to the cool off period for new accounts. How long is that period? I don't suppose there's a way to avoid it?

There is a way to expedite the cooling-off period (it's a SPAM prevention
measure), but I don't have access to it.  In the meantime I've added the patch
for you, and once the cooling off is over we can add your name as author.

	https://commitfest.postgresql.org/47/4835/

I'm currently reviewing it and will get back to you soon on that.

--
Daniel Gustafsson

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

    Andres Freund <andres@anarazel.de> — 2024-02-15T21:17:41Z

    Hi,

On 2024-02-11 13:19:00 -0500, David Benjamin wrote:
> I've attached a patch for the master branch to fix up the custom BIOs used
> by PostgreSQL, in light of the issues with the OpenSSL update recently.
> While c82207a548db47623a2bfa2447babdaa630302b9 (switching from BIO_get_data
> to BIO_get_app_data) resolved the immediate conflict, I don't think it
> addressed the root cause, which is that PostgreSQL was mixing up two BIO
> implementations, each of which expected to be driving the BIO.

Yea, that's certainly not nice - and I think we've been somewhat lucky it
hasn't caused more issues. There's some nasty possibilities, e.g. sock_ctrl()
partially enabling ktls without our initialization having called
ktls_enable(). Right now that just means ktls is unusable, but it's not hard
to imagine accidentally ending up sending unencrypted data.



I've in the past looked into not using a custom BIO [1], but I have my doubts
about that being a good idea. I think medium term we want to be able to do
network IO asynchronously, which seems quite hard to do when using openssl's
socket BIO.



> Once we've done that, we're free to use BIO_set_data. While BIO_set_app_data
> works fine, I've reverted back to BIO_set_data because it's more commonly used.
> app_data depends on OpenSSL's "ex_data" mechanism, which is a tad heavier under
> the hood.

At first I was a bit wary of that, because it requires us to bring back the
fallback implementation. But you're right, it's noticeably heavier than
BIO_get_data(), and we do call BIO_get_app_data() fairly frequently.



> That leaves ctrl. ctrl is a bunch of operations (it's ioctl). The only
> operation that matters is BIO_CTRL_FLUSH, which is implemented as a no-op. All
> other operations are unused. It's once again good that they're unused because
> otherwise OpenSSL might mess with postgres's socket, break the assumptions
> around interrupt handling, etc.

How did you determine that only FLUSH is required? I didn't even really find
documentation about what the intended semantics actually are.

E.g. should we implement BIO_CTRL_EOF? Sure, it wasn't really supported so
far, because we never set it, but is that right? What about
BIO_CTRL_GET_CLOSE/BIO_CTRL_SET_CLOSE?


Another issue is that 0 doesn't actually seem like the universal error return
- e.g. BIO_C_GET_FD seems to return -1, because 0 is a valid fd.


As of your patch the bio doesn't actually have an FD anymore, should it still
set BIO_TYPE_DESCRIPTOR?



> +static long
> +my_sock_ctrl(BIO *h, int cmd, long num, void *ptr)
> +{
> +	long		res = 0;
> +
> +	switch (cmd)
> +	{
> +		case BIO_CTRL_FLUSH:
> +			/* libssl expects all BIOs to support BIO_flush. */
> +			res = 1;
> +			break;
> +	}
> +
> +	return res;
> +}

I'd move the res = 0 into a default: block. That way the compiler can warn if
some case doesn't set it in all branches.


>  static BIO_METHOD *
>  my_BIO_s_socket(void)
>  {

Wonder if we should rename this. It's pretty odd that we still call it's not
really related to s_socket anymore, and doesn't even really implement the same
interface (e.g. get_fd doesn't work anymore).  Similarly, my_SSL_set_fd()
doesn't actually call set_fd() anymore, which sure seems odd.


Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20210715021747.h2hih7mw56ivyntt%40alap3.anarazel.de

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

    David Benjamin <davidben@google.com> — 2024-02-21T22:04:08Z

    Thanks for the very thorough comments! I've attached a new version of the
patch.

On Thu, Feb 15, 2024 at 4:17 PM Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> On 2024-02-11 13:19:00 -0500, David Benjamin wrote:
> > I've attached a patch for the master branch to fix up the custom BIOs
> used
> > by PostgreSQL, in light of the issues with the OpenSSL update recently.
> > While c82207a548db47623a2bfa2447babdaa630302b9 (switching from
> BIO_get_data
> > to BIO_get_app_data) resolved the immediate conflict, I don't think it
> > addressed the root cause, which is that PostgreSQL was mixing up two BIO
> > implementations, each of which expected to be driving the BIO.
>
> Yea, that's certainly not nice - and I think we've been somewhat lucky it
> hasn't caused more issues. There's some nasty possibilities, e.g.
> sock_ctrl()
> partially enabling ktls without our initialization having called
> ktls_enable(). Right now that just means ktls is unusable, but it's not
> hard
> to imagine accidentally ending up sending unencrypted data.
>

Yeah. Even if, say, the ktls bits work, given you all care enough about I/O
to have wanted to wrap the BIO, I assume you'd want to pick up those
features on your own terms, e.g. by implementing the BIO_CTRLs yourself.


> I've in the past looked into not using a custom BIO [1], but I have my
> doubts
> about that being a good idea. I think medium term we want to be able to do
> network IO asynchronously, which seems quite hard to do when using
> openssl's
> socket BIO.



> Once we've done that, we're free to use BIO_set_data. While
> BIO_set_app_data
> > works fine, I've reverted back to BIO_set_data because it's more
> commonly used.
> > app_data depends on OpenSSL's "ex_data" mechanism, which is a tad
> heavier under
> > the hood.
>
> At first I was a bit wary of that, because it requires us to bring back the
> fallback implementation. But you're right, it's noticeably heavier than
> BIO_get_data(), and we do call BIO_get_app_data() fairly frequently.
>

TBH, I doubt it makes any real difference perf-wise. But I think
BIO_get_data is a bit more common in this context.


> > That leaves ctrl. ctrl is a bunch of operations (it's ioctl). The only
> > operation that matters is BIO_CTRL_FLUSH, which is implemented as a
> no-op. All
> > other operations are unused. It's once again good that they're unused
> because
> > otherwise OpenSSL might mess with postgres's socket, break the
> assumptions
> > around interrupt handling, etc.
>
> How did you determine that only FLUSH is required? I didn't even really
> find
> documentation about what the intended semantics actually are.
>

The unhelpful answer is that my day job is working on BoringSSL, so I've
spent a lot of time with this mess. :-) But, yeah, it's not well-documented
at all. OpenSSL ends up calling BIO_flush at the end of each batch of
writes in libssl. TBH, I suspect that was less intentional and more an
emergent property of them internally layering a buffer BIO at one point in
the process, but it's long been part of the (undocumented) API contract.
Conversely, I don't think OpenSSL can possibly make libssl *require* a new
BIO_CTRL because they'd break every custom BIO anyone has ever used with
the library.


> E.g. should we implement BIO_CTRL_EOF? Sure, it wasn't really supported so
> far, because we never set it, but is that right?


Ah hmm, BIO_CTRL_EOF is... a bit of a mess. OpenSSL kind of messed things
up. So, up until recently, I would have said that BIO_CTRL_EOF was not part
of the interface here. OpenSSL 1.0.x did not implement it for sockets, and
the BIO_read API *already* had a way to signal EOF: did you return zero or
-1?

Then OpenSSL 1.1.x introduced size_t-based BIO_read_ex APIs. However, in
the process, they *forgot that EOF and error are different things* and made
it impossible to detect EOFs if you use BIO_read_ex! They never noticed
this, because they didn't actually convert their own code to their new API.
See this discussion, which alas ended with OpenSSL deciding to ignore the
problem and not even document their current interface.
https://github.com/openssl/openssl/issues/8208

Though they never responded, they seem to have tacitly settled using the
out-of-band BIO_eof function (which is BIO_CTRL_EOF) as the way to signal
EOF for BIO_read_ex. This is kind of fiddly, but is at least a well-defined
option. But the problem is no one's BIO_METHODs, including their own, are
read_ex-based. They all implement the old read callback. But someone might
call BIO_read_ex on a read-based BIO_METHOD. IMO, BIO_read_ex should be
responsible for translating between the two EOF conventions. For example,
it could automatically set a flag when the read callback returns 0 and then
make BIO_ctrl check the flag and automatically implement BIO_CTRL_EOF for
BIOs that don't do it themselves. Alas, OpenSSL did not do this and instead
they just made their built-in BIOs implement BIO_CTRL_EOF, even though this
new expectation is a compatibility break. That leaves BIO_read, the one
everyone uses, in a pretty ambiguous state that they seem uninterested in
addressing.
https://github.com/openssl/openssl/pull/10882
https://github.com/openssl/openssl/pull/10907

Honestly, I suspect nothing in postgres actually cares about this, given
you all didn't notice things breaking around here in the early days of
1.1.x. Still, that is a good point. I've updated the patch to implement
BIO_CTRL_EOF for completeness' sake.

(For BoringSSL, we did not go down this route because, unlike OpenSSL
apparently, we do not think breaking backwards compatibility in the EOF
convention is OK. I do want to add the size_t APIs eventually, but I'm
thinking we'll do the automatic BIO_CTRL_EOF thing described above, to
avoid breaking compatibility with existing BIO_METHODs.)

Beyond BIO_CTRL_EOF, I assume what you're really asking here is "how do we
know OpenSSL won't change the interface again?". And, honestly, we don't.
They promise API and ABI stability, which in theory means they shouldn't.
But their track record with custom BIOs is not great. That said, if they do
break it, I think it will unambiguously be an API break on their part and
hopefully it'll be possible to get them to fix the issue. The EOF issue
snuck in because it mostly only impacted people who tried to migrate to
their new APIs. It broke existing APIs too, but the one project that
noticed (CPython) misdiagnosed the issue and worked around it.
(Incorrectly, but no one noticed. See
https://github.com/python/cpython/pull/95495.) So, I raised the issue,
they'd long since shipped it and evidently felt no burning need to fix
their regression or unclear APIs. :-(

But the alternative, picking up the real socket BIO's ctrl function, is
even more unsound, so I think this is the better place to be.


> What about
> BIO_CTRL_GET_CLOSE/BIO_CTRL_SET_CLOSE?
>

The close flag is pretty silly. All BIOs do slightly different things with
it, which means that libssl cannot possibly call it generically. So if your
BIO doesn't have any need for it, you don't need to bother. It's usually
used to indicate whether the BIO owns the underlying fd/socket/BUF_MEM/etc,


> Another issue is that 0 doesn't actually seem like the universal error
> return
> - e.g. BIO_C_GET_FD seems to return -1, because 0 is a valid fd.
>

Yeah, that is... also a mess. All of OpenSSL's ctrl functions return zero
for unrecognized commands. It seems they just don't have a convention for
distinguishing unimplemented commands from zero returns. As you note, this
is a problem for BIO_C_GET_FD. OpenSSL's other non-fd BIOs have the same
problem though:
https://github.com/openssl/openssl/blob/master/crypto/bio/bss_file.c#L340

Realistically, type-specific ioctls like that don't matter much because
you're really only going to call them if you already know you have an
applicable BIO. Hopefully, between that, and removing BIO_TYPE_DESCRIPTOR
(below), it's mostly okay? Also happy to add a `ret = -1` implementation
of BIO_C_GET_FD if you'd prefer. But, in the general case, I suspect we
just have to live with the zero thing, and hopefully OpenSSL will stop
introducing ctrls where zero is an unsafe return value.

Perhaps BIO_ctrl in OpenSSL should have some logic like...
```
if (!(type & BIO_TYPE_DESCRIPTOR) && cmd == BIO_C_GET_FD) {
  return -1;
}
```
Although I suspect there are people who implement BIO_C_GET_FD without
setting BIO_TYPE_DESCRIPTOR because none of this was ever documented.


> As of your patch the bio doesn't actually have an FD anymore, should it
> still
> set BIO_TYPE_DESCRIPTOR?
>

Ah good call. Done. I don't think much code really cares, but it does mean
that, if you all call SSL_get_fd (why would you?), it won't get confused.
:-)

> +static long
> > +my_sock_ctrl(BIO *h, int cmd, long num, void *ptr)
> > +{
> > +     long            res = 0;
> > +
> > +     switch (cmd)
> > +     {
> > +             case BIO_CTRL_FLUSH:
> > +                     /* libssl expects all BIOs to support BIO_flush. */
> > +                     res = 1;
> > +                     break;
> > +     }
> > +
> > +     return res;
> > +}
>
> I'd move the res = 0 into a default: block. That way the compiler can warn
> if
> some case doesn't set it in all branches.
>

Done.


> >  static BIO_METHOD *
> >  my_BIO_s_socket(void)
> >  {
>
> Wonder if we should rename this. It's pretty odd that we still call it's
> not
> really related to s_socket anymore, and doesn't even really implement the
> same
> interface (e.g. get_fd doesn't work anymore).  Similarly, my_SSL_set_fd()
> doesn't actually call set_fd() anymore, which sure seems odd.
>

Done. I wasn't sure what to name them, or even what the naming convention
was, but I opted to name them after the Port and PGconn objects they wrap.
Happy to switch to another name if you have a better idea though.

David

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

    David Benjamin <davidben@google.com> — 2024-04-19T13:13:45Z

    Circling back here, anything else needed from my end on this patch?

On Wed, Feb 21, 2024, 17:04 David Benjamin <davidben@google.com> wrote:

> Thanks for the very thorough comments! I've attached a new version of the
> patch.
>
> On Thu, Feb 15, 2024 at 4:17 PM Andres Freund <andres@anarazel.de> wrote:
>
>> Hi,
>>
>> On 2024-02-11 13:19:00 -0500, David Benjamin wrote:
>> > I've attached a patch for the master branch to fix up the custom BIOs
>> used
>> > by PostgreSQL, in light of the issues with the OpenSSL update recently.
>> > While c82207a548db47623a2bfa2447babdaa630302b9 (switching from
>> BIO_get_data
>> > to BIO_get_app_data) resolved the immediate conflict, I don't think it
>> > addressed the root cause, which is that PostgreSQL was mixing up two BIO
>> > implementations, each of which expected to be driving the BIO.
>>
>> Yea, that's certainly not nice - and I think we've been somewhat lucky it
>> hasn't caused more issues. There's some nasty possibilities, e.g.
>> sock_ctrl()
>> partially enabling ktls without our initialization having called
>> ktls_enable(). Right now that just means ktls is unusable, but it's not
>> hard
>> to imagine accidentally ending up sending unencrypted data.
>>
>
> Yeah. Even if, say, the ktls bits work, given you all care enough about
> I/O to have wanted to wrap the BIO, I assume you'd want to pick up those
> features on your own terms, e.g. by implementing the BIO_CTRLs yourself.
>
>
>> I've in the past looked into not using a custom BIO [1], but I have my
>> doubts
>> about that being a good idea. I think medium term we want to be able to do
>> network IO asynchronously, which seems quite hard to do when using
>> openssl's
>> socket BIO.
>
>
>
> > Once we've done that, we're free to use BIO_set_data. While
>> BIO_set_app_data
>> > works fine, I've reverted back to BIO_set_data because it's more
>> commonly used.
>> > app_data depends on OpenSSL's "ex_data" mechanism, which is a tad
>> heavier under
>> > the hood.
>>
>> At first I was a bit wary of that, because it requires us to bring back
>> the
>> fallback implementation. But you're right, it's noticeably heavier than
>> BIO_get_data(), and we do call BIO_get_app_data() fairly frequently.
>>
>
> TBH, I doubt it makes any real difference perf-wise. But I think
> BIO_get_data is a bit more common in this context.
>
>
>> > That leaves ctrl. ctrl is a bunch of operations (it's ioctl). The only
>> > operation that matters is BIO_CTRL_FLUSH, which is implemented as a
>> no-op. All
>> > other operations are unused. It's once again good that they're unused
>> because
>> > otherwise OpenSSL might mess with postgres's socket, break the
>> assumptions
>> > around interrupt handling, etc.
>>
>> How did you determine that only FLUSH is required? I didn't even really
>> find
>> documentation about what the intended semantics actually are.
>>
>
> The unhelpful answer is that my day job is working on BoringSSL, so I've
> spent a lot of time with this mess. :-) But, yeah, it's not well-documented
> at all. OpenSSL ends up calling BIO_flush at the end of each batch of
> writes in libssl. TBH, I suspect that was less intentional and more an
> emergent property of them internally layering a buffer BIO at one point in
> the process, but it's long been part of the (undocumented) API contract.
> Conversely, I don't think OpenSSL can possibly make libssl *require* a
> new BIO_CTRL because they'd break every custom BIO anyone has ever used
> with the library.
>
>
>> E.g. should we implement BIO_CTRL_EOF? Sure, it wasn't really supported so
>> far, because we never set it, but is that right?
>
>
> Ah hmm, BIO_CTRL_EOF is... a bit of a mess. OpenSSL kind of messed things
> up. So, up until recently, I would have said that BIO_CTRL_EOF was not part
> of the interface here. OpenSSL 1.0.x did not implement it for sockets, and
> the BIO_read API *already* had a way to signal EOF: did you return zero
> or -1?
>
> Then OpenSSL 1.1.x introduced size_t-based BIO_read_ex APIs. However, in
> the process, they *forgot that EOF and error are different things* and
> made it impossible to detect EOFs if you use BIO_read_ex! They never
> noticed this, because they didn't actually convert their own code to their
> new API. See this discussion, which alas ended with OpenSSL deciding to
> ignore the problem and not even document their current interface.
> https://github.com/openssl/openssl/issues/8208
>
> Though they never responded, they seem to have tacitly settled using the
> out-of-band BIO_eof function (which is BIO_CTRL_EOF) as the way to signal
> EOF for BIO_read_ex. This is kind of fiddly, but is at least a well-defined
> option. But the problem is no one's BIO_METHODs, including their own, are
> read_ex-based. They all implement the old read callback. But someone might
> call BIO_read_ex on a read-based BIO_METHOD. IMO, BIO_read_ex should be
> responsible for translating between the two EOF conventions. For example,
> it could automatically set a flag when the read callback returns 0 and then
> make BIO_ctrl check the flag and automatically implement BIO_CTRL_EOF for
> BIOs that don't do it themselves. Alas, OpenSSL did not do this and instead
> they just made their built-in BIOs implement BIO_CTRL_EOF, even though this
> new expectation is a compatibility break. That leaves BIO_read, the one
> everyone uses, in a pretty ambiguous state that they seem uninterested in
> addressing.
> https://github.com/openssl/openssl/pull/10882
> https://github.com/openssl/openssl/pull/10907
>
> Honestly, I suspect nothing in postgres actually cares about this, given
> you all didn't notice things breaking around here in the early days of
> 1.1.x. Still, that is a good point. I've updated the patch to implement
> BIO_CTRL_EOF for completeness' sake.
>
> (For BoringSSL, we did not go down this route because, unlike OpenSSL
> apparently, we do not think breaking backwards compatibility in the EOF
> convention is OK. I do want to add the size_t APIs eventually, but I'm
> thinking we'll do the automatic BIO_CTRL_EOF thing described above, to
> avoid breaking compatibility with existing BIO_METHODs.)
>
> Beyond BIO_CTRL_EOF, I assume what you're really asking here is "how do we
> know OpenSSL won't change the interface again?". And, honestly, we don't.
> They promise API and ABI stability, which in theory means they shouldn't.
> But their track record with custom BIOs is not great. That said, if they do
> break it, I think it will unambiguously be an API break on their part and
> hopefully it'll be possible to get them to fix the issue. The EOF issue
> snuck in because it mostly only impacted people who tried to migrate to
> their new APIs. It broke existing APIs too, but the one project that
> noticed (CPython) misdiagnosed the issue and worked around it.
> (Incorrectly, but no one noticed. See
> https://github.com/python/cpython/pull/95495.) So, I raised the issue,
> they'd long since shipped it and evidently felt no burning need to fix
> their regression or unclear APIs. :-(
>
> But the alternative, picking up the real socket BIO's ctrl function, is
> even more unsound, so I think this is the better place to be.
>
>
>> What about
>> BIO_CTRL_GET_CLOSE/BIO_CTRL_SET_CLOSE?
>>
>
> The close flag is pretty silly. All BIOs do slightly different things with
> it, which means that libssl cannot possibly call it generically. So if your
> BIO doesn't have any need for it, you don't need to bother. It's usually
> used to indicate whether the BIO owns the underlying fd/socket/BUF_MEM/etc,
>
>
>> Another issue is that 0 doesn't actually seem like the universal error
>> return
>> - e.g. BIO_C_GET_FD seems to return -1, because 0 is a valid fd.
>>
>
> Yeah, that is... also a mess. All of OpenSSL's ctrl functions return zero
> for unrecognized commands. It seems they just don't have a convention for
> distinguishing unimplemented commands from zero returns. As you note, this
> is a problem for BIO_C_GET_FD. OpenSSL's other non-fd BIOs have the same
> problem though:
> https://github.com/openssl/openssl/blob/master/crypto/bio/bss_file.c#L340
>
> Realistically, type-specific ioctls like that don't matter much because
> you're really only going to call them if you already know you have an
> applicable BIO. Hopefully, between that, and removing BIO_TYPE_DESCRIPTOR
> (below), it's mostly okay? Also happy to add a `ret = -1` implementation
> of BIO_C_GET_FD if you'd prefer. But, in the general case, I suspect we
> just have to live with the zero thing, and hopefully OpenSSL will stop
> introducing ctrls where zero is an unsafe return value.
>
> Perhaps BIO_ctrl in OpenSSL should have some logic like...
> ```
> if (!(type & BIO_TYPE_DESCRIPTOR) && cmd == BIO_C_GET_FD) {
>   return -1;
> }
> ```
> Although I suspect there are people who implement BIO_C_GET_FD without
> setting BIO_TYPE_DESCRIPTOR because none of this was ever documented.
>
>
>> As of your patch the bio doesn't actually have an FD anymore, should it
>> still
>> set BIO_TYPE_DESCRIPTOR?
>>
>
> Ah good call. Done. I don't think much code really cares, but it does mean
> that, if you all call SSL_get_fd (why would you?), it won't get confused.
> :-)
>
> > +static long
>> > +my_sock_ctrl(BIO *h, int cmd, long num, void *ptr)
>> > +{
>> > +     long            res = 0;
>> > +
>> > +     switch (cmd)
>> > +     {
>> > +             case BIO_CTRL_FLUSH:
>> > +                     /* libssl expects all BIOs to support BIO_flush.
>> */
>> > +                     res = 1;
>> > +                     break;
>> > +     }
>> > +
>> > +     return res;
>> > +}
>>
>> I'd move the res = 0 into a default: block. That way the compiler can
>> warn if
>> some case doesn't set it in all branches.
>>
>
> Done.
>
>
>> >  static BIO_METHOD *
>> >  my_BIO_s_socket(void)
>> >  {
>>
>> Wonder if we should rename this. It's pretty odd that we still call it's
>> not
>> really related to s_socket anymore, and doesn't even really implement the
>> same
>> interface (e.g. get_fd doesn't work anymore).  Similarly, my_SSL_set_fd()
>> doesn't actually call set_fd() anymore, which sure seems odd.
>>
>
> Done. I wasn't sure what to name them, or even what the naming convention
> was, but I opted to name them after the Port and PGconn objects they wrap.
> Happy to switch to another name if you have a better idea though.
>
> David
>

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-04-19T13:17:10Z

    > On 19 Apr 2024, at 15:13, David Benjamin <davidben@google.com> wrote:
> 
> Circling back here, anything else needed from my end on this patch?

Patience mostly, v17 very recently entered feature freeze so a lof of the
developers are busy finding and fixing bugs to stabilize the release.  When the
window for new features opens again I am sure this patch will get reviews (I
have it on my personal TODO and hope to get to it).

--
Daniel Gustafsson

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-05-19T16:21:07Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 19 Apr 2024, at 15:13, David Benjamin <davidben@google.com> wrote:
>> Circling back here, anything else needed from my end on this patch?

> Patience mostly, v17 very recently entered feature freeze so a lof of the
> developers are busy finding and fixing bugs to stabilize the
> release.

Per the cfbot [1], this patch needs a rebase over the ALPN-related
commits.  It still isn't likely to get human attention before the
July commitfest, but you can save some time by making sure it's
in a reviewable state before that.

			regards, tom lane

[1] http://commitfest.cputube.org/david-benjamin.html

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

    David Benjamin <davidben@google.com> — 2024-05-19T18:07:22Z

    On Sun, May 19, 2024 at 12:21 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Per the cfbot [1], this patch needs a rebase over the ALPN-related
> commits.  It still isn't likely to get human attention before the
> July commitfest, but you can save some time by making sure it's
> in a reviewable state before that.
>

Rebased version attached. (The conflict was pretty trivial. Both patches
add a field to some struct.)

David

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

    Peter Eisentraut <peter@eisentraut.org> — 2024-05-19T20:21:37Z

    On 19.05.24 20:07, David Benjamin wrote:
> On Sun, May 19, 2024 at 12:21 PM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     Per the cfbot [1], this patch needs a rebase over the ALPN-related
>     commits.  It still isn't likely to get human attention before the
>     July commitfest, but you can save some time by making sure it's
>     in a reviewable state before that.
> 
> 
> Rebased version attached. (The conflict was pretty trivial. Both patches 
> add a field to some struct.)

Note that there is a concurrent plan to drop support for OpenSSL older 
than 1.1.1 [0], which would obsolete your configure test for 
BIO_get_data.  Whoever commits these should be sure to coordinate this.


[0]: https://www.postgresql.org/message-id/flat/ZG3JNursG69dz1lr@paquier.xyz

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-05-19T20:32:45Z

    > On 19 May 2024, at 22:21, Peter Eisentraut <peter@eisentraut.org> wrote:

> Whoever commits these should be sure to coordinate this.

Thanks for the reminder.  I have this patchset, the OCSP stapling patchset and
the multiple-cert one on my radar for when I do the 1.1.0 removal work in the
July CF. Will do another scan for SSL related patches in flight at the time.

--
Daniel Gustafsson

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-09-03T12:18:07Z

    > On 19 May 2024, at 22:32, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 19 May 2024, at 22:21, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>> Whoever commits these should be sure to coordinate this.
> 
> Thanks for the reminder.  I have this patchset, the OCSP stapling patchset and
> the multiple-cert one on my radar for when I do the 1.1.0 removal work in the
> July CF. Will do another scan for SSL related patches in flight at the time.

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.

--
Daniel Gustafsson

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-09-04T13:22:12Z

    > On 3 Sep 2024, at 14:18, Daniel Gustafsson <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?


+	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.

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

--
Daniel Gustafsson

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

    David Benjamin <davidben@google.com> — 2024-09-04T21:19:18Z

    On Wed, Sep 4, 2024 at 9:22 AM Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 3 Sep 2024, at 14:18, Daniel Gustafsson <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.


> +       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

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-09-04T22:10:17Z

    > 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

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-10-11T21:20:43Z

    > On 5 Sep 2024, at 00:10, Daniel Gustafsson <daniel@yesql.se> wrote:

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

That ended up not being shortly, but having spent a fair bit of time reading
the diff over and testing on multiple versions of OpenSSL and LibreSSL I've now
pushed it.  The buildfarm has built green on multiple platforms tonight but
I'll keep monitoring it.

--
Daniel Gustafsson

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

    David Benjamin <davidben@google.com> — 2024-11-29T17:21:33Z

    Thanks! I got asked about release branches, so I thought I'd pass it along:
how do you all handle merges to release branches and would it make sense to
merge this change? On the one hand, nothing is actively on fire yet, but
the current setup does risk breakage if OpenSSL ever migrates BIO_s_socket
to their new size_t-clean internals.

On my end, I found some time to write up the compatibility riskiness to
OpenSSL upstream and they agreed with me that BIO_meth_get_* are
problematic:
https://github.com/openssl/openssl/issues/26047
https://github.com/openssl/openssl/pull/26056

I also sent them a documentation fix so the BIO_CTRL_FLUSH requirement is
clearly written down.
https://github.com/openssl/openssl/pull/26060
I didn't write down the expectations around BIO_CTRL_EOF yet because I'm
still not really sure what they are with
https://github.com/openssl/openssl/issues/8208 and all. (If we get around
to adding BIO_read_ex to BoringSSL, I'll see if we can do something better
there---have some half-baked ideas---and, if successful, I'll try to
convince OpenSSL to do the same.)

David

On Fri, Oct 11, 2024 at 5:20 PM Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 5 Sep 2024, at 00:10, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > Thanks for reviewing, I plan on going ahead with this patch shortly.
>
> That ended up not being shortly, but having spent a fair bit of time
> reading
> the diff over and testing on multiple versions of OpenSSL and LibreSSL
> I've now
> pushed it.  The buildfarm has built green on multiple platforms tonight but
> I'll keep monitoring it.
>
> --
> Daniel Gustafsson
>
>

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-29T18:36:21Z

    David Benjamin <davidben@google.com> writes:
> Thanks! I got asked about release branches, so I thought I'd pass it along:
> how do you all handle merges to release branches and would it make sense to
> merge this change? On the one hand, nothing is actively on fire yet, but
> the current setup does risk breakage if OpenSSL ever migrates BIO_s_socket
> to their new size_t-clean internals.

We theoretically could back-patch 6f782a2a1, as it doesn't appear to
introduce any ABI-breaking changes.  (The new field in struct Port
could be an issue, but it looks like it fits into what was padding
space, so probably fine.)  However, I'm not sure that it's attractive
to do so from a risk/benefit standpoint.  That code's received only
minimal testing so far, and the problem it's fixing is as yet
hypothetical.

On balance I think I'd vote against a back-patch now.  We could
reconsider next year once PG v18 has gotten a reasonable amount of
beta testing.

			regards, tom lane

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-11-29T19:57:43Z

    > On 29 Nov 2024, at 19:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> David Benjamin <davidben@google.com> writes:
>> Thanks! I got asked about release branches, so I thought I'd pass it along:
>> how do you all handle merges to release branches and would it make sense to
>> merge this change? On the one hand, nothing is actively on fire yet, but
>> the current setup does risk breakage if OpenSSL ever migrates BIO_s_socket
>> to their new size_t-clean internals.
> 
> We theoretically could back-patch 6f782a2a1, as it doesn't appear to
> introduce any ABI-breaking changes.  (The new field in struct Port
> could be an issue, but it looks like it fits into what was padding
> space, so probably fine.)  However, I'm not sure that it's attractive
> to do so from a risk/benefit standpoint.  That code's received only
> minimal testing so far, and the problem it's fixing is as yet
> hypothetical.
> 
> On balance I think I'd vote against a back-patch now.  We could
> reconsider next year once PG v18 has gotten a reasonable amount of
> beta testing.

I agree with all of the above.

--
Daniel Gustafsson