Thread: [PATCH] Avoid mixing custom and OpenSSL BIO functions
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
Attachment
> 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 theOpenSSL 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 wasmixing 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 beinga no-op. Instead, it's cleaner to just define a custom BIO the normal way, which then leaves the more standard BIO_get_datamechanism 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 mea 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
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. :-)
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
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. :-)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
> 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
isthat 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
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
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.
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.
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:
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
Attachment
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.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.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: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
> 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
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
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
Attachment
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
> 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
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.
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.
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
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> 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.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.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
--
Daniel Gustafsson
Daniel Gustafsson
> 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
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:
I also sent them a documentation fix so the BIO_CTRL_FLUSH requirement is clearly written down.
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
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
> 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