Обсуждение: BUG #17625: In PG15 PQsslAttribute returns different values than PG14 when SSL is not in use for the connection
BUG #17625: In PG15 PQsslAttribute returns different values than PG14 when SSL is not in use for the connection
От
PG Bug reporting form
Дата:
The following bug has been logged on the website: Bug reference: 17625 Logged by: Heath Lord Email address: heath.lord@crunchydata.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 (https://www.postgresql.org/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@postgresql.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 diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index aea4661736..e2eace2bc6 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -1745,14 +1745,18 @@ PQsslAttributeNames(PGconn *conn) const char * PQsslAttribute(PGconn *conn, const char *attribute_name) { - if (strcmp(attribute_name, "library") == 0) - return "OpenSSL"; - if (!conn) + { + if (strcmp(attribute_name, "library") == 0) + return "OpenSSL"; return NULL; + } if (conn->ssl == NULL) return NULL; + if (strcmp(attribute_name, "library") == 0) + return "OpenSSL"; + if (strcmp(attribute_name, "key_bits") == 0) { static char sslbits_str[12];
On Thu, Sep 29, 2022 at 4:16 PM Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@sss.pgh.pa.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@timescale.com> writes: > On Thu, Sep 29, 2022 at 1:16 PM Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@yesql.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@sss.pgh.pa.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@yesql.se> writes: >> On 29 Sep 2022, at 23:08, Tom Lane <tgl@sss.pgh.pa.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 diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 8908f775df..41864c6cf1 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2592,12 +2592,22 @@ const char *PQsslAttribute(const PGconn *conn, const char *attribute_name); <term><function>PQsslAttributeNames</function><indexterm><primary>PQsslAttributeNames</primary></indexterm></term> <listitem> <para> - Returns an array of SSL attribute names available. + Returns an array of SSL attribute names that can be used + in <function>PQsslAttribute()</function>. The array is terminated by a NULL pointer. <synopsis> const char * const * PQsslAttributeNames(const PGconn *conn); </synopsis> </para> + + <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> </listitem> </varlistentry> diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 74b5c5987a..b42a908733 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -1730,7 +1730,7 @@ PQsslStruct(PGconn *conn, const char *struct_name) const char *const * PQsslAttributeNames(PGconn *conn) { - static const char *const result[] = { + static const char *const openssl_attrs[] = { "library", "key_bits", "cipher", @@ -1738,8 +1738,19 @@ PQsslAttributeNames(PGconn *conn) "protocol", NULL }; + static const char *const empty_attrs[] = {NULL}; - return result; + if (!conn) + { + /* Return attributes of default SSL library */ + return openssl_attrs; + } + + /* No attrs for unencrypted connection */ + if (conn->ssl == NULL) + return empty_attrs; + + return openssl_attrs; } const char *
> On 29 Sep 2022, at 23:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >>> On 29 Sep 2022, at 23:08, Tom Lane <tgl@sss.pgh.pa.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@yesql.se> writes: > On 29 Sep 2022, at 23:58, Tom Lane <tgl@sss.pgh.pa.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