Thread: [PATCH] Enable SSL library detection via PQsslAttribute
Hello, As part of the NSS work it came up [1] that clients don't have a good way to ask libpq what SSL library it was compiled with, unless they already have a connection pointer so that they can call PQsslAttribute(conn, "library"). This poses a chicken-and-egg problem: with the NSS proposal, the client may have to know which library is in use before it can construct a proper connection string. For example, I have a test suite that needs to set up an NSS database with certificates before telling libpq to connect using that database. The simplest proposal was to just allow PQsslAttribute() to take NULL as the connection parameter when querying the "library" attribute, and that's what I've done in this patch. In current versions of libpq, the "library" attribute will always be NULL if you pass NULL as the connection; a client that needs to know whether this new behavior is present can look at the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro. If this looks good, I'm not sure how best to test it in the regression suite. I see that libpq has an installcheck recipe that compiles a test executable for URI parsing; should I add a simple test alongside that? Thanks, --Jacob [1] https://www.postgresql.org/message-id/a1798e46d6d801344ebc93672c6947ef5297c8a0.camel%40vmware.com
Attachment
On 2/23/22 13:38, Jacob Champion wrote: > Hello, > > As part of the NSS work it came up [1] that clients don't have a good > way to ask libpq what SSL library it was compiled with, unless they > already have a connection pointer so that they can call > PQsslAttribute(conn, "library"). This poses a chicken-and-egg problem: > with the NSS proposal, the client may have to know which library is in > use before it can construct a proper connection string. For example, I > have a test suite that needs to set up an NSS database with > certificates before telling libpq to connect using that database. > > The simplest proposal was to just allow PQsslAttribute() to take NULL > as the connection parameter when querying the "library" attribute, and > that's what I've done in this patch. In current versions of libpq, the > "library" attribute will always be NULL if you pass NULL as the > connection; a client that needs to know whether this new behavior is > present can look at the LIBPQ_HAS_SSL_LIBRARY_DETECTION feature macro. > > If this looks good, I'm not sure how best to test it in the regression > suite. I see that libpq has an installcheck recipe that compiles a test > executable for URI parsing; should I add a simple test alongside that? Create a TAP tests that calls a small client? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, 2022-02-23 at 14:11 -0500, Andrew Dunstan wrote: > On 2/23/22 13:38, Jacob Champion wrote: > > > > If this looks good, I'm not sure how best to test it in the regression > > suite. I see that libpq has an installcheck recipe that compiles a test > > executable for URI parsing; should I add a simple test alongside that? > > Create a TAP tests that calls a small client? First stab in v2-0002. Though I see that Andres is overhauling the tests in this folder today [1], so I'll need to watch that thread. :) Thanks! --Jacob [1] https://www.postgresql.org/message-id/20220223203031.ezrd73ohvjgfksow%40alap3.anarazel.de
Attachment
On Wed, 2022-02-23 at 23:20 +0000, Jacob Champion wrote: > First stab in v2-0002. Though I see that Andres is overhauling the > tests in this folder today [1], so I'll need to watch that thread. :) v3 rebases over Andres' changes and actually adds the Perl driver that I missed the git-add for. --Jacob
Attachment
On Mon, Feb 28, 2022 at 3:21 PM Jacob Champion <pchampion@vmware.com> wrote: > v3 rebases over Andres' changes and actually adds the Perl driver that > I missed the git-add for. This seems totally reasonable. However, I think it should update the documentation somehow. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, 2022-03-25 at 15:32 -0400, Robert Haas wrote: > On Mon, Feb 28, 2022 at 3:21 PM Jacob Champion <pchampion@vmware.com> wrote: > > v3 rebases over Andres' changes and actually adds the Perl driver that > > I missed the git-add for. > > This seems totally reasonable. However, I think it should update the > documentation somehow. Done in v4. Do I need to merge my tiny test program into the libpq_pipeline tests? I'm not sure what the roadmap is for those. Thanks! --Jacob
Attachment
> On 25 Mar 2022, at 22:25, Jacob Champion <pchampion@vmware.com> wrote: > On Fri, 2022-03-25 at 15:32 -0400, Robert Haas wrote: >> This seems totally reasonable. However, I think it should update the >> documentation somehow. > > Done in v4. I would prefer to not introduce a <note> for this, I think adding it as a <para> under PQsslAttribute is better given the rest of the libpq API documentation. The proposed text reads fine to me. -- Daniel Gustafsson https://vmware.com/
Jacob Champion <pchampion@vmware.com> writes:
> Do I need to merge my tiny test program into the libpq_pipeline tests?
Doesn't seem worth the trouble to me, notably because you'd
then have to cope with non-SSL builds too.
regards, tom lane
On Fri, 2022-03-25 at 18:00 -0400, Tom Lane wrote: > Jacob Champion <pchampion@vmware.com> writes: > > Do I need to merge my tiny test program into the libpq_pipeline tests? > > Doesn't seem worth the trouble to me, notably because you'd > then have to cope with non-SSL builds too. Fine by me. v5 moves the docs out of the Note, as requested by Daniel. Thanks, --Jacob
Attachment
> On 25 Mar 2022, at 23:45, Jacob Champion <pchampion@vmware.com> wrote: > > On Fri, 2022-03-25 at 18:00 -0400, Tom Lane wrote: >> Jacob Champion <pchampion@vmware.com> writes: >>> Do I need to merge my tiny test program into the libpq_pipeline tests? >> >> Doesn't seem worth the trouble to me, notably because you'd >> then have to cope with non-SSL builds too. > > Fine by me. > > v5 moves the docs out of the Note, as requested by Daniel. I went over this again and I think this version is ready for committer. Having tried to add implement a new TLS library I would add a small comment on PQsslAttributeNames() for this, reverse-engineering what to implement is hard as it is without special cases easily identifiable. That can easily be done when pushed, no need for a new version IMHO. -- Daniel Gustafsson https://vmware.com/
Pushed with a few small tweaks to make it match project style, thanks! -- Daniel Gustafsson https://vmware.com/
On Tue, 2022-03-29 at 14:08 +0200, Daniel Gustafsson wrote: > Pushed with a few small tweaks to make it match project style, thanks! Thank you! --Jacob