Re: BUG #17625: In PG15 PQsslAttribute returns different values than PG14 when SSL is not in use for the connection

The following bug has been logged on the website:

Bug reference: 17625
Logged by: Heath Lord
Email address: heath(dot)lord(at)crunchydata(dot)com
PostgreSQL version: Unsupported/Unknown
Operating system: All
Description:

Hello folks,

I think I found a small bug on PG15. My guess is it's a small documentation
issue, but I'll let the experts here decide.

According to the documentation
(/docs/15/libpq-status.html#LIBPQ-PQSSLATTRIBUTE)
PQsslAttribute should return NULL if the SSL implementation is not in use.
Here is the relevant line snipped from the documentation:

library
Name of the SSL implementation in use. (Currently, only "OpenSSL" is
implemented)

According to the connection I am using, PQsslInUse is returning FALSE.
However, when I check the PQsslAttribute values, they are returning
"OpenSSL, NULL, NULL, NULL, NULL". In PostgreSQL 14, the library returns
NULL when the connection is not using OpenSSL.

It looks like commit ebc8b7d4416d8e0dfb7c05132ef6182fd3daf885 changed this
behavior.

We found this issue since psycopg2 2.9.3 is failing its regression tests
when run against PG15 because it is assuming functionality according to the
documentation. If the issue is just a documentation problem, we will gladly
raise a github issue for that extension suggesting that they make a fix
accordingly. But, before doing so, we wanted to confirm if this is just a
documentation issue, or an accidental change in behavior.

Thanks!

PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> According to the connection I am using, PQsslInUse is returning FALSE.
> However, when I check the PQsslAttribute values, they are returning
> "OpenSSL, NULL, NULL, NULL, NULL". In PostgreSQL 14, the library returns
> NULL when the connection is not using OpenSSL.

> It looks like commit ebc8b7d4416d8e0dfb7c05132ef6182fd3daf885 changed this
> behavior.

AFAICS that behavioral change is deliberate: for the single case
of inquiring about "library", PQsslAttribute now tells you which
SSL implementation libpq *can* use, not which one it's actually
using on a given connection. I'm not sure that this is a great
definition, since it's so unlike the behavior for other attributes.
I can see the point of being able to check that, but should we
have invented a new function instead of overloading PQsslAttribute
this way? One concrete point against this is that should we ever
be able to build libpq with the ability to use more than one GSS
library, this API would be utterly broken.

However, given that we're past rc1, I'm afraid that ship may have
sailed. Assuming we leave it like this, how would you propose
changing the documentation to make it more clear?

regards, tom lane

I wrote:
> AFAICS that behavioral change is deliberate: for the single case
> of inquiring about "library", PQsslAttribute now tells you which
> SSL implementation libpq *can* use, not which one it's actually
> using on a given connection. I'm not sure that this is a great
> definition, since it's so unlike the behavior for other attributes.

Actually, wait a minute: both the documentation and the commit
message claim the new behavior is something different than what it
actually is. The intention seems to have been to change the
behavior only for the conn == NULL case. So maybe we need to
fix it as attached. This'd still be broken for the
multiple-libraries scenario, but I admit that that's pretty
hypothetical.

regards, tom lane

On Thu, Sep 29, 2022 at 4:16 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I wrote:
> > AFAICS that behavioral change is deliberate: for the single case
> > of inquiring about "library", PQsslAttribute now tells you which
> > SSL implementation libpq *can* use, not which one it's actually
> > using on a given connection. I'm not sure that this is a great
> > definition, since it's so unlike the behavior for other attributes.
>
> Actually, wait a minute: both the documentation and the commit
> message claim the new behavior is something different than what it
> actually is. The intention seems to have been to change the
> behavior only for the conn == NULL case. So maybe we need to
> fix it as attached. This'd still be broken for the
> multiple-libraries scenario, but I admit that that's pretty
> hypothetical.
>
> regards, tom lane
>
>
Tom,

I was in the process of drafting a reply, but this new patch seems to
resolve my issues with the documentation and functionality. I really
appreciate you looking into this and I am good with this patch.

Thanks,
Heath

> On 29 Sep 2022, at 22:16, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I wrote:
>> AFAICS that behavioral change is deliberate: for the single case
>> of inquiring about "library", PQsslAttribute now tells you which
>> SSL implementation libpq *can* use, not which one it's actually
>> using on a given connection. I'm not sure that this is a great
>> definition, since it's so unlike the behavior for other attributes.

That was the intention of the patch, as different libraries may require
different connstrings there needs to be a way to know the library which will be
use when connecting.

> Actually, wait a minute: both the documentation and the commit
> message claim the new behavior is something different than what it
> actually is. The intention seems to have been to change the
> behavior only for the conn == NULL case. So maybe we need to
> fix it as attached.

Hrmpf, yes, I agree with your patch.

> This'd still be broken for the
> multiple-libraries scenario, but I admit that that's pretty
> hypothetical.

We can cross that bridge if get there, nothing here prevents the case in the
hypothetical future unless I'm missing something.

We still need to change the docs though, maybe along the lines of the below
(but with better wording):

- Name of the SSL implementation in use. (Currently, only
- <literal>"OpenSSL"</literal> is implemented)
+ Name of the implementation which will be used for connections
+ using SSL in case <literal>conn</literal> is NULL, or in case
+ <literal>conn</literal> is an SSL enabled connection. If
+ <literal>conn</literal> is a a non-SSL connection NULL is returned.
+ (Currently, only <literal>"OpenSSL"</literal> is implemented)

--
Daniel Gustafsson https://vmware.com/

On Thu, Sep 29, 2022 at 1:16 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The intention seems to have been to change the
> behavior only for the conn == NULL case. So maybe we need to
> fix it as attached.

Yeah, that makes sense. Sorry for the oversight.

> This'd still be broken for the
> multiple-libraries scenario, but I admit that that's pretty
> hypothetical.

Since the goal is to let clients decide which connection options to
hardcode based on the SSL implementation, I think it stays
forward-compatible with multiple libraries, as long as this API
returns the "default" library that you get if you're an older,
clueless client. We would need a new API of some sort to let newer
clients figure out their choices.

Thanks,
--Jacob

Jacob Champion <jchampion(at)timescale(dot)com> writes:
> On Thu, Sep 29, 2022 at 1:16 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This'd still be broken for the
>> multiple-libraries scenario, but I admit that that's pretty
>> hypothetical.

> Since the goal is to let clients decide which connection options to
> hardcode based on the SSL implementation, I think it stays
> forward-compatible with multiple libraries, as long as this API
> returns the "default" library that you get if you're an older,
> clueless client. We would need a new API of some sort to let newer
> clients figure out their choices.

Yeah, that was in my mind too: we could leave it like this as long
as we define the result for (NULL, "library") as being the default
SSL library choice. Good enough for now, then.

I'll go tweak the documentation as per Daniel's thoughts and push.

regards, tom lane

BTW ... while I'm looking at this, it seems like PQsslAttributeNames
is defined in a pretty schizophrenic way. It takes a "conn" argument
but does nothing whatever with that argument. You get back OpenSSL's
attribute list, or an empty attribute list, depending on compilation
options but not on the properties of the connection. None of this
is explained in the docs, and it would not scale to multiple supported
libraries either. Should we clean that up while we're at it?

A definition that'd be consistent with what we just agreed to for
PQsslAttribute is:

PQsslAttributeNames(NULL): the attributes for the default SSL library,
or an empty list if there is none.

PQsslAttributeNames(conn): the attributes for the SSL library in use
on this connection, or an empty list if not encrypted.

This doesn't cover how to find out the attributes for a non-default
SSL library in advance of using it, but since PQsslAttribute would
also need extension for multiple libraries, we could leave that
for later.

regards, tom lane

> On 29 Sep 2022, at 22:45, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I'll go tweak the documentation as per Daniel's thoughts and push.

Thanks!

Reading a few lines further down in the docs I noticed this description for the
compression parameter to PQsslAttribute:

"If SSL compression is in use, returns the name of the compression
algorithm, or "on" if compression is used but the algorithm is not
known. If compression is not in use, returns "off"."

AFAICT that has never been the case since 91fa7b4719ac, it has always just
returned "on" or "off" and never the name of the compression algorithm. I
propose to apply the attached backpatched to make the docs align with the code.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> Reading a few lines further down in the docs I noticed this description for the
> compression parameter to PQsslAttribute:

> "If SSL compression is in use, returns the name of the compression
> algorithm, or "on" if compression is used but the algorithm is not
> known. If compression is not in use, returns "off"."

> AFAICT that has never been the case since 91fa7b4719ac, it has always just
> returned "on" or "off" and never the name of the compression algorithm. I
> propose to apply the attached backpatched to make the docs align with the code.

Yeah, it clearly doesn't do that. +1

regards, tom lane

> On 29 Sep 2022, at 23:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> A definition that'd be consistent with what we just agreed to for
> PQsslAttribute is:
>
> PQsslAttributeNames(NULL): the attributes for the default SSL library,
> or an empty list if there is none.
>
> PQsslAttributeNames(conn): the attributes for the SSL library in use
> on this connection, or an empty list if not encrypted.

I think that makes sense, it keeps the API consistent.

> This doesn't cover how to find out the attributes for a non-default
> SSL library in advance of using it, but since PQsslAttribute would
> also need extension for multiple libraries, we could leave that
> for later.

Agreed, let's leave that for later.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>> On 29 Sep 2022, at 23:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> A definition that'd be consistent with what we just agreed to for
>> PQsslAttribute is:
>> PQsslAttributeNames(NULL): the attributes for the default SSL library,
>> or an empty list if there is none.
>> PQsslAttributeNames(conn): the attributes for the SSL library in use
>> on this connection, or an empty list if not encrypted.

> I think that makes sense, it keeps the API consistent.

So more or less as attached, then.

Since this is mostly about future-proofing, I'd personally be content
to put it in HEAD. Is there a case for shoehorning this into
v15 at this late date? Consistency with PQsslAttribute would be
good, but I'm not sure we want to make this kind of change post-RC1.

regards, tom lane

> On 29 Sep 2022, at 23:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>>> On 29 Sep 2022, at 23:08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> A definition that'd be consistent with what we just agreed to for
>>> PQsslAttribute is:
>>> PQsslAttributeNames(NULL): the attributes for the default SSL library,
>>> or an empty list if there is none.
>>> PQsslAttributeNames(conn): the attributes for the SSL library in use
>>> on this connection, or an empty list if not encrypted.
>
>> I think that makes sense, it keeps the API consistent.
>
> So more or less as attached, then.

LGTM.

- Returns an array of SSL attribute names available.
+ Returns an array of SSL attribute names that can be used
+ in <function>PQsslAttribute()</function>.

Maybe hairsplitting, but should we note that it can be used in PQsslAttribute()
for the conn which was passed as param to PQsslAttributeNames()? In a (still
hypothetical) multi-library case there is no guarantee that it will be valid
for another conn.

> Since this is mostly about future-proofing, I'd personally be content
> to put it in HEAD. Is there a case for shoehorning this into
> v15 at this late date? Consistency with PQsslAttribute would be
> good, but I'm not sure we want to make this kind of change post-RC1.

I'd be fine just doing this in HEAD.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
> On 29 Sep 2022, at 23:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> - Returns an array of SSL attribute names available.
>> + Returns an array of SSL attribute names that can be used
>> + in <function>PQsslAttribute()</function>.

> Maybe hairsplitting, but should we note that it can be used in PQsslAttribute()
> for the conn which was passed as param to PQsslAttributeNames()? In a (still
> hypothetical) multi-library case there is no guarantee that it will be valid
> for another conn.

I thought about doing this, but the subsequent para seems to make it clear
enough:

+ <para>
+ If <literal>conn</literal> is NULL, the attributes available for the
+ default SSL library are returned, or an empty list
+ if <application>libpq</application> was compiled without any SSL
+ support. If <literal>conn</literal> is not NULL, the attributes
+ available for the SSL library in use for the connection are returned,
+ or an empty list if the connection is not encrypted.
+ </para>

The actual constraint is "names that can be used on connections using
the same SSL library", which seems too verbose for the introductory
sentence.

> I'd be fine just doing this in HEAD.

Pushed to HEAD only.

regards, tom lane