Horton

Thread

Commits

  1. Fix sslkeylogfile error handling logging

  2. Mark sslkeylogfile as Debug option

  3. libpq: Add support for dumping SSL key material to file

  1. Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-01-08T23:32:57Z

    Hello folks,

Attached is a patch to add support for logging secrets used in TLS
connection after psql is initialized. This adds a new env var
SSLKEYLOGFILE on the client side that points to a text file where keys
will be logged. If a user runs psql multiple times with the same
SSLKEYLOGFILE, new entries will be appended to that file. There is no
change in behavior if that env var is not set or set to an empty
string. This is useful for cases when a client wants to analyze TCP
packets using a tool like wireshark while using TLS. This will enable
wireshark to decrypt the packets and decode as postgres wire protocol
messages. I did not add this to the backend because I thought using
wireshark is more common on the frontend.

The keylogfile format is documented here
https://www.ietf.org/archive/id/draft-thomson-tls-keylogfile-00.html

Example usage:

root@guest:~/postgres# SSLKEYLOGFILE=./key.txt
/usr/local/pgsql/bin/psql
"postgresql://user:pass@host:5432/postgres?sslmode=require"
psql (18devel, server 17.2)
SSL connection (protocol: TLSv1.3, cipher: TLS_AES_256_GCM_SHA384,
compression: off, ALPN: postgresql)
Type "help" for help.

postgres=>
\q
root@guest:~/postgres# cat key.txt
SERVER_HANDSHAKE_TRAFFIC_SECRET ****
EXPORTER_SECRET ****
SERVER_TRAFFIC_SECRET_0 ****
CLIENT_HANDSHAKE_TRAFFIC_SECRET ***
CLIENT_TRAFFIC_SECRET_0 ****


A few things I am not sure about:
1. Where should I add automated tests and docs for this? I did not see
any unit tests for the surrounding functions.
2. Should I use perror to report error here? I did not want to use
libpq_append_conn_error because this is not a connection related
error.

Please let me know if I can clarify anything.

-- 
Thanks and regards
Abhishek Chanda

  2. Re: Adding support for SSLKEYLOGFILE in the frontend

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-09T01:17:30Z

    Abhishek Chanda <abhishek.becs@gmail.com> writes:
> Attached is a patch to add support for logging secrets used in TLS
> connection after psql is initialized. This adds a new env var
> SSLKEYLOGFILE on the client side that points to a text file where keys
> will be logged.

I wonder if this poses a security risk, ie something forcing
extraction of TLS secrets at a distance via the environment variable.
I think it might be safer if we only accepted it as a connection
parameter and not via an environment variable.  (I'm aware of the
rule of thumb that if you control the environment then you own your
callees anyway, via modifying PATH and suchlike.  But there's a lot
of code that thinks it can sanitize the environment by overriding
PATH and other well-known variables.  Nobody is going to be aware
that SSLKEYLOGFILE is a security hazard.)

> 2. Should I use perror to report error here?

No.

> I did not want to use
> libpq_append_conn_error because this is not a connection related
> error.

Yes it is, IMO anyway.  Anything like this should be treated as a
libpq connection parameter.  The reason you couldn't find a place
to document this feature is you were ignoring libpq's conventions.

Just looking at the code itself, I'm a bit distressed that it's
not making any effort to secure the log file against prying eyes.
Shouldn't we be trying to create it with mode 0600?

			regards, tom lane

  3. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-01-09T12:24:13Z

    > On 9 Jan 2025, at 02:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I think it might be safer if we only accepted it as a connection
> parameter and not via an environment variable.

+1

--
Daniel Gustafsson

  4. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-01-09T20:36:16Z

    On Wed, Jan 8, 2025 at 5:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think it might be safer if we only accepted it as a connection
> parameter and not via an environment variable.

Making it a connection parameter also keeps us from colliding with any
other linked libraries' use of SSLKEYLOGFILE (I'm thinking about
libcurl at the moment, but I think maybe NSS used it too?).

--Jacob

  5. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-01-10T03:59:21Z

    Thanks for the feedback, everyone. Attached a followup with the
following changes compared to the initial version:

1. Converted sslkeylogfile to a connection parameter
2. Added a mechanism to chmod the key log file to 0600
3. Added docs and tests

I tested this manually. Also ran make check and make check-world
locally. Please let me know if this needs any other changes.

Thanks

On Thu, Jan 9, 2025 at 2:36 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Wed, Jan 8, 2025 at 5:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think it might be safer if we only accepted it as a connection
> > parameter and not via an environment variable.
>
> Making it a connection parameter also keeps us from colliding with any
> other linked libraries' use of SSLKEYLOGFILE (I'm thinking about
> libcurl at the moment, but I think maybe NSS used it too?).
>
> --Jacob



-- 
Thanks and regards
Abhishek Chanda

  6. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-01-10T15:29:05Z

    > On 10 Jan 2025, at 04:59, Abhishek Chanda <abhishek.becs@gmail.com> wrote:

Thanks for the new version.

+	{"sslkeylogfile", "SSLKEYLOGFILE",
The env var should be PGSSLKEYLOGFILE with the PG prefix.

> 3. Added docs and tests

There is no test in the attached patch, did you write one but forgot to git add
it before committing locally?

--
Daniel Gustafsson

  7. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-01-10T23:16:44Z

    Thanks, Daniel. Here is an updated patch with all previous changes,
added a simple connection test and another check to make sure file
mode is correct, and the env var fix. Please let me know if anything
needs to be changed. I tested this locally using meson running all TAP
tests, and also manually.

Thanks
Abhishek

On Fri, Jan 10, 2025 at 9:29 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 10 Jan 2025, at 04:59, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
>
> Thanks for the new version.
>
> +       {"sslkeylogfile", "SSLKEYLOGFILE",
> The env var should be PGSSLKEYLOGFILE with the PG prefix.
>
> > 3. Added docs and tests
>
> There is no test in the attached patch, did you write one but forgot to git add
> it before committing locally?
>
> --
> Daniel Gustafsson
>


-- 
Thanks and regards
Abhishek Chanda

  8. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-02-20T03:36:26Z

    Hi all,

Please find attached a new version of this patch. I rebased on master,
added better error reporting and skipped the permissions check on
windows. Please let me know if this needs any changes. I tested this
locally using meson running all TAP tests.

Thanks

On Fri, Jan 10, 2025 at 5:16 PM Abhishek Chanda <abhishek.becs@gmail.com> wrote:
>
> Thanks, Daniel. Here is an updated patch with all previous changes,
> added a simple connection test and another check to make sure file
> mode is correct, and the env var fix. Please let me know if anything
> needs to be changed. I tested this locally using meson running all TAP
> tests, and also manually.
>
> Thanks
> Abhishek
>
> On Fri, Jan 10, 2025 at 9:29 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > > On 10 Jan 2025, at 04:59, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> >
> > Thanks for the new version.
> >
> > +       {"sslkeylogfile", "SSLKEYLOGFILE",
> > The env var should be PGSSLKEYLOGFILE with the PG prefix.
> >
> > > 3. Added docs and tests
> >
> > There is no test in the attached patch, did you write one but forgot to git add
> > it before committing locally?
> >
> > --
> > Daniel Gustafsson
> >
>
>
> --
> Thanks and regards
> Abhishek Chanda



-- 
Thanks and regards
Abhishek Chanda

  9. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-02-20T22:25:31Z

    > On 20 Feb 2025, at 04:36, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> Please find attached a new version of this patch. I rebased on master,
> added better error reporting and skipped the permissions check on
> windows. Please let me know if this needs any changes. I tested this
> locally using meson running all TAP tests.

Thanks for the new version, a few comments on this one from reading:

+./src/test/ssl/key.txt
The toplevel .gitignore should not be used for transient test output, there is
a .gitignore in src/test/ssl for that.  The key.txt file should also be placed
in PostgreSQL::Test::Utils::tempdir and then cleaned up after the test.


+     <varlistentry id="libpq-connect-sslkeylogfile" xreflabel="sslkeylogfile">
The documentation should state that the file will use the NSS format.


+       if (log_file == NULL) {
+               libpq_append_conn_error(conn, "could not open ssl key log ...
+       }
This raise an error for not being able to operate on the file, but it doesn't
error out from the function and instead keeps going with more operations on the
file which couldn't be opened.


+       if (chmod(conn->sslkeylogfile, 0600) == -1) {
Rather than opening and try to set permissions separately, why not use open(2)
and set the umask?  We probably also want to set O_NOFOLLOW.


+       if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
+               SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
This callback does exist in OpenSSL 1.1.1 but it's only available in LibreSSL
from OpenBSD 7.1 and onwards where we support 7.0 as the earliest version.
This feature seems like a good enough reason to bump the minimum LibreSSL
version, and 7.1 is well out support (7.5 goes EOL next), but it would need to
get done here.


+    # Skip file mode check on Windows
+    return 1 if $windows_os;
It should be up to each individual test to determine what to skip, not the
library code (and it should use SKIP:).  src/test/ssl/t/001_ssltests.pl is an
example which skips permissions checks on Windows.  Also, I'm not sure we need
a generic function in the testlibrary for something so basic.


+            print STDERR sprintf("%s mode must be %04o\n",
+                               $path, $expected_file_mode);
Test code should not print anything to stdout/stderr, it should use the TAP
compliant methods like diag() etc for this.


+       "connect with server root certand sslkeylogfile=key.txt");
s/certand/cert and/ ?

--
Daniel Gustafsson

  10. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-02-28T05:39:59Z

    Thanks for the review, Daniel. Please find v5 of this patch attached.
I tried to bump up the minimum OpenBSD version in installation.sgml,
do I need to add this anywhere else? Please let me know if this needs
anything else.

Thanks

On Thu, Feb 20, 2025 at 4:25 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 20 Feb 2025, at 04:36, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> > Please find attached a new version of this patch. I rebased on master,
> > added better error reporting and skipped the permissions check on
> > windows. Please let me know if this needs any changes. I tested this
> > locally using meson running all TAP tests.
>
> Thanks for the new version, a few comments on this one from reading:
>
> +./src/test/ssl/key.txt
> The toplevel .gitignore should not be used for transient test output, there is
> a .gitignore in src/test/ssl for that.  The key.txt file should also be placed
> in PostgreSQL::Test::Utils::tempdir and then cleaned up after the test.
>
>
> +     <varlistentry id="libpq-connect-sslkeylogfile" xreflabel="sslkeylogfile">
> The documentation should state that the file will use the NSS format.
>
>
> +       if (log_file == NULL) {
> +               libpq_append_conn_error(conn, "could not open ssl key log ...
> +       }
> This raise an error for not being able to operate on the file, but it doesn't
> error out from the function and instead keeps going with more operations on the
> file which couldn't be opened.
>
>
> +       if (chmod(conn->sslkeylogfile, 0600) == -1) {
> Rather than opening and try to set permissions separately, why not use open(2)
> and set the umask?  We probably also want to set O_NOFOLLOW.
>
>
> +       if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
> +               SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
> This callback does exist in OpenSSL 1.1.1 but it's only available in LibreSSL
> from OpenBSD 7.1 and onwards where we support 7.0 as the earliest version.
> This feature seems like a good enough reason to bump the minimum LibreSSL
> version, and 7.1 is well out support (7.5 goes EOL next), but it would need to
> get done here.
>
>
> +    # Skip file mode check on Windows
> +    return 1 if $windows_os;
> It should be up to each individual test to determine what to skip, not the
> library code (and it should use SKIP:).  src/test/ssl/t/001_ssltests.pl is an
> example which skips permissions checks on Windows.  Also, I'm not sure we need
> a generic function in the testlibrary for something so basic.
>
>
> +            print STDERR sprintf("%s mode must be %04o\n",
> +                               $path, $expected_file_mode);
> Test code should not print anything to stdout/stderr, it should use the TAP
> compliant methods like diag() etc for this.
>
>
> +       "connect with server root certand sslkeylogfile=key.txt");
> s/certand/cert and/ ?
>
> --
> Daniel Gustafsson
>


-- 
Thanks and regards
Abhishek Chanda

  11. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-02-28T06:20:17Z

    Attached a v6 with O_NOFOLLOW removed, I noticed that it failed on
windows. Please let me know if I should do this in any other way that
is portable across platforms.

Thanks

On Thu, Feb 27, 2025 at 11:39 PM Abhishek Chanda
<abhishek.becs@gmail.com> wrote:
>
> Thanks for the review, Daniel. Please find v5 of this patch attached.
> I tried to bump up the minimum OpenBSD version in installation.sgml,
> do I need to add this anywhere else? Please let me know if this needs
> anything else.
>
> Thanks
>
> On Thu, Feb 20, 2025 at 4:25 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > > On 20 Feb 2025, at 04:36, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> > > Please find attached a new version of this patch. I rebased on master,
> > > added better error reporting and skipped the permissions check on
> > > windows. Please let me know if this needs any changes. I tested this
> > > locally using meson running all TAP tests.
> >
> > Thanks for the new version, a few comments on this one from reading:
> >
> > +./src/test/ssl/key.txt
> > The toplevel .gitignore should not be used for transient test output, there is
> > a .gitignore in src/test/ssl for that.  The key.txt file should also be placed
> > in PostgreSQL::Test::Utils::tempdir and then cleaned up after the test.
> >
> >
> > +     <varlistentry id="libpq-connect-sslkeylogfile" xreflabel="sslkeylogfile">
> > The documentation should state that the file will use the NSS format.
> >
> >
> > +       if (log_file == NULL) {
> > +               libpq_append_conn_error(conn, "could not open ssl key log ...
> > +       }
> > This raise an error for not being able to operate on the file, but it doesn't
> > error out from the function and instead keeps going with more operations on the
> > file which couldn't be opened.
> >
> >
> > +       if (chmod(conn->sslkeylogfile, 0600) == -1) {
> > Rather than opening and try to set permissions separately, why not use open(2)
> > and set the umask?  We probably also want to set O_NOFOLLOW.
> >
> >
> > +       if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0)
> > +               SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb);
> > This callback does exist in OpenSSL 1.1.1 but it's only available in LibreSSL
> > from OpenBSD 7.1 and onwards where we support 7.0 as the earliest version.
> > This feature seems like a good enough reason to bump the minimum LibreSSL
> > version, and 7.1 is well out support (7.5 goes EOL next), but it would need to
> > get done here.
> >
> >
> > +    # Skip file mode check on Windows
> > +    return 1 if $windows_os;
> > It should be up to each individual test to determine what to skip, not the
> > library code (and it should use SKIP:).  src/test/ssl/t/001_ssltests.pl is an
> > example which skips permissions checks on Windows.  Also, I'm not sure we need
> > a generic function in the testlibrary for something so basic.
> >
> >
> > +            print STDERR sprintf("%s mode must be %04o\n",
> > +                               $path, $expected_file_mode);
> > Test code should not print anything to stdout/stderr, it should use the TAP
> > compliant methods like diag() etc for this.
> >
> >
> > +       "connect with server root certand sslkeylogfile=key.txt");
> > s/certand/cert and/ ?
> >
> > --
> > Daniel Gustafsson
> >
>
>
> --
> Thanks and regards
> Abhishek Chanda



-- 
Thanks and regards
Abhishek Chanda

  12. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-03T15:23:12Z

    > On 28 Feb 2025, at 07:20, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> 
> Attached a v6 with O_NOFOLLOW removed, I noticed that it failed on
> windows. Please let me know if I should do this in any other way that
> is portable across platforms.

Not sure if there is a portable way to can support.

       required version is 3.4 (from <systemitem class="osname">OpenBSD</systemitem>
-      version 7.0).
+      version 7.5).
Bumping the version needs a bit more infrastructure than this.  Doing a bit
more research unveiled that there is no need to do this though since LibreSSL
doesn't support keylogging at all, they have only implemented stubs for
compatibility.  I've added checks to autoconf and meson to identify the
capability and conditional compilation of the support.  The tests are also
updated to reflect this.

-       bytes_written = dprintf(fd, "%s\n", line);
We use write(2) everywhere so I've changed to patch to do the same.


The attached 0002 also contains documentation touchups and comments etc.  0001
is your patch from v6.

--
Daniel Gustafsson

  13. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-05T17:21:40Z

    > On 3 Mar 2025, at 16:23, Daniel Gustafsson <daniel@yesql.se> wrote:

> The attached 0002 also contains documentation touchups and comments etc.  0001
> is your patch from v6.

I managed to misunderstand skip blocks in TAP tests in the 0002, so the
attached version fixes that.  It has been failing on Debian in CI which I have
yet to look into.

--
Daniel Gustafsson

  14. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-13T18:29:16Z

    On Wed, Mar 5, 2025 at 9:21 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> I managed to misunderstand skip blocks in TAP tests in the 0002, so the
> attached version fixes that.  It has been failing on Debian in CI which I have
> yet to look into.

Drive-by comment:

> +    {"sslkeylogfile", "PGSSLKEYLOGFILE",
> +        "", NULL,
> +        "SSL-Key-Log-File", "", 0, /* sizeof("") = 0 */
> +    offsetof(struct pg_conn, sslkeylogfile)},

Adding the PG prefix to the envvar name addresses my collision
concern, but I think Tom's comment upthread [1] was saying that we
should not provide any envvar at all:

> I think it might be safer if we only accepted it as a connection
> parameter and not via an environment variable.

Is the addition of the PG prefix enough to address that concern too?
(Are people already sanitizing their environments for all PG*
variables?)

Thanks,
--Jacob

[1] https://postgr.es/m/1774813.1736385450%40sss.pgh.pa.us

  15. Re: Adding support for SSLKEYLOGFILE in the frontend

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-13T18:31:24Z

    Jacob Champion <jacob.champion@enterprisedb.com> writes:
> Adding the PG prefix to the envvar name addresses my collision
> concern, but I think Tom's comment upthread [1] was saying that we
> should not provide any envvar at all:

>> I think it might be safer if we only accepted it as a connection
>> parameter and not via an environment variable.

> Is the addition of the PG prefix enough to address that concern too?

Indeed, I was advocating for *no* environment variable.  The PG prefix
does not comfort me.

			regards, tom lane

  16. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-13T22:07:33Z

    
> On 13 Mar 2025, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>> Adding the PG prefix to the envvar name addresses my collision
>> concern, but I think Tom's comment upthread [1] was saying that we
>> should not provide any envvar at all:
> 
>>> I think it might be safer if we only accepted it as a connection
>>> parameter and not via an environment variable.
> 
>> Is the addition of the PG prefix enough to address that concern too?
> 
> Indeed, I was advocating for *no* environment variable.  The PG prefix
> does not comfort me.

Attached is a rebased version which fixes the test failure under autoconf (I
had missed git adding the configure file..) and Windows where the backslashes
weren't escaped properly.  It also removes the environment variable and has
documentation touchups.

--
Daniel Gustafsson

  17. Re: Adding support for SSLKEYLOGFILE in the frontend

    Abhishek Chanda <abhishek.becs@gmail.com> — 2025-03-13T23:02:35Z

    Thanks, Daniel.

Should there be the ifdef guard in here as well?

+ {"sslkeylogfile", NULL, NULL, NULL,
+ "SSL-Key-Log-File", "", 0, /* sizeof("") = 0 */
+ offsetof(struct pg_conn, sslkeylogfile)},
+

A small nit, this line should say NULL

+ /* line is guaranteed by OpenSSL to be NUL terminated */

Thanks

On Thu, Mar 13, 2025 at 5:07 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
>
>
> > On 13 Mar 2025, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Jacob Champion <jacob.champion@enterprisedb.com> writes:
> >> Adding the PG prefix to the envvar name addresses my collision
> >> concern, but I think Tom's comment upthread [1] was saying that we
> >> should not provide any envvar at all:
> >
> >>> I think it might be safer if we only accepted it as a connection
> >>> parameter and not via an environment variable.
> >
> >> Is the addition of the PG prefix enough to address that concern too?
> >
> > Indeed, I was advocating for *no* environment variable.  The PG prefix
> > does not comfort me.
>
> Attached is a rebased version which fixes the test failure under autoconf (I
> had missed git adding the configure file..) and Windows where the backslashes
> weren't escaped properly.  It also removes the environment variable and has
> documentation touchups.
>
> --
> Daniel Gustafsson
>


-- 
Thanks and regards
Abhishek Chanda

  18. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-14T08:34:26Z

    > On 14 Mar 2025, at 00:02, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
> 
> Thanks, Daniel.
> 
> Should there be the ifdef guard in here as well?
> 
> + {"sslkeylogfile", NULL, NULL, NULL,
> + "SSL-Key-Log-File", "", 0, /* sizeof("") = 0 */
> + offsetof(struct pg_conn, sslkeylogfile)},

No, we want the option to work even if the feature doesn't in order to not make
connection strings dependent on compilation options.

> A small nit, this line should say NULL
> 
> + /* line is guaranteed by OpenSSL to be NUL terminated */

The variable is terminated by the NUL character so I believe this is actually
correct, if you look around in the source tree you'll find many more
references.

> On Thu, Mar 13, 2025 at 5:07 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> 
>>> On 13 Mar 2025, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 
>>> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>>>> Adding the PG prefix to the envvar name addresses my collision
>>>> concern, but I think Tom's comment upthread [1] was saying that we
>>>> should not provide any envvar at all:
>>> 
>>>>> I think it might be safer if we only accepted it as a connection
>>>>> parameter and not via an environment variable.
>>> 
>>>> Is the addition of the PG prefix enough to address that concern too?
>>> 
>>> Indeed, I was advocating for *no* environment variable.  The PG prefix
>>> does not comfort me.
>> 
>> Attached is a rebased version which fixes the test failure under autoconf (I
>> had missed git adding the configure file..) and Windows where the backslashes
>> weren't escaped properly.  It also removes the environment variable and has
>> documentation touchups.

--
Daniel Gustafsson

  19. Re: Adding support for SSLKEYLOGFILE in the frontend

    Peter Eisentraut <peter@eisentraut.org> — 2025-03-14T14:27:31Z

    On 13.03.25 19:31, Tom Lane wrote:
> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>> Adding the PG prefix to the envvar name addresses my collision
>> concern, but I think Tom's comment upthread [1] was saying that we
>> should not provide any envvar at all:
> 
>>> I think it might be safer if we only accepted it as a connection
>>> parameter and not via an environment variable.
> 
>> Is the addition of the PG prefix enough to address that concern too?
> 
> Indeed, I was advocating for *no* environment variable.  The PG prefix
> does not comfort me.

It seems to me that the environment variable would be the most useful 
way to use this feature, for example if you want to debug an existing 
program that you can't or don't want to change.

As was mentioned earlier, libcurl uses an environment variable for this. 
  Moreover, the format originated in the NSS library, which also uses an 
environment variable.

So we are here constructing a higher level of security that others don't 
seem to have found the need for.

It's also possible that we should consider the SSLKEYLOGFILE environment 
variable some kind of quasi-standard like PAGER, and we should be using 
exactly that environment variable name like everyone else.

  20. Re: Adding support for SSLKEYLOGFILE in the frontend

    vignesh C <vignesh21@gmail.com> — 2025-03-16T13:16:28Z

    On Fri, 14 Mar 2025 at 03:38, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>
>
> > On 13 Mar 2025, at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Jacob Champion <jacob.champion@enterprisedb.com> writes:
> >> Adding the PG prefix to the envvar name addresses my collision
> >> concern, but I think Tom's comment upthread [1] was saying that we
> >> should not provide any envvar at all:
> >
> >>> I think it might be safer if we only accepted it as a connection
> >>> parameter and not via an environment variable.
> >
> >> Is the addition of the PG prefix enough to address that concern too?
> >
> > Indeed, I was advocating for *no* environment variable.  The PG prefix
> > does not comfort me.
>
> Attached is a rebased version which fixes the test failure under autoconf (I
> had missed git adding the configure file..) and Windows where the backslashes
> weren't escaped properly.  It also removes the environment variable and has
> documentation touchups.

I noticed that Peter's comments from [1] is not yet addressed. I have
changed the commitfest entry status to Waiting on Author, please
address them and update the status to Needs review.
[1] - https://www.postgresql.org/message-id/68b66b6d-cc59-44f8-bdd2-248d50055740%40eisentraut.org

Regards.
Vignesh

  21. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-16T13:48:58Z

    > On 14 Mar 2025, at 15:27, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 13.03.25 19:31, Tom Lane wrote:
>> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>>> Adding the PG prefix to the envvar name addresses my collision
>>> concern, but I think Tom's comment upthread [1] was saying that we
>>> should not provide any envvar at all:
>>>> I think it might be safer if we only accepted it as a connection
>>>> parameter and not via an environment variable.
>>> Is the addition of the PG prefix enough to address that concern too?
>> Indeed, I was advocating for *no* environment variable.  The PG prefix
>> does not comfort me.
> 
> It seems to me that the environment variable would be the most useful way to use this feature, for example if you want to debug an existing program that you can't or don't want to change.
> 
> As was mentioned earlier, libcurl uses an environment variable for this.  Moreover, the format originated in the NSS library, which also uses an environment variable.
> 
> So we are here constructing a higher level of security that others don't seem to have found the need for.

IIRC the reasoning has been that if a rogue user can inject an environment
variable into your session and read your files it's probably game over anyways.

> It's also possible that we should consider the SSLKEYLOGFILE environment variable some kind of quasi-standard like PAGER, and we should be using exactly that environment variable name like everyone else.

If we would use the same as others, it would make it harder to do fine-grained
debugging of a session

--
Daniel Gustafsson

  22. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-17T15:48:02Z

    On Sun, Mar 16, 2025 at 6:49 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> IIRC the reasoning has been that if a rogue user can inject an environment
> variable into your session and read your files it's probably game over anyways.

(Personally I'm no longer as convinced by this line of argument as I
once was...)

> > It's also possible that we should consider the SSLKEYLOGFILE environment variable some kind of quasi-standard like PAGER, and we should be using exactly that environment variable name like everyone else.
>
> If we would use the same as others, it would make it harder to do fine-grained
> debugging of a session

It also brings up the possibility of two (or more?) separate parts of
the client writing keys simultaneously to the same file through
separate file descriptors, which doesn't seem very fun.

--Jacob

  23. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-18T12:42:44Z

    > On 17 Mar 2025, at 16:48, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Sun, Mar 16, 2025 at 6:49 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> IIRC the reasoning has been that if a rogue user can inject an environment
>> variable into your session and read your files it's probably game over anyways.
> 
> (Personally I'm no longer as convinced by this line of argument as I
> once was...)

Since there is disagreement over this, we should either 1) go ahead with the
latest patch without an env var and revisit the discussion during v19; 2)
adding the env var back into the patch as PGSSLKEYLOGFILE or; 3) postponing all
of this till v19?

Personally I think this feature has enough value even without the env var to
not postpone it, especially since adding an env var in 19 will still be
backwards compatible.  I would go for option 1 to stay on the safe side and
allow time for proper discussion, any other thoughts?

--
Daniel Gustafsson

  24. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-18T13:15:50Z

    On Tue, 18 Mar 2025 at 13:43, Daniel Gustafsson <daniel@yesql.se> wrote:
> Since there is disagreement over this, we should either 1) go ahead with the
> latest patch without an env var and revisit the discussion during v19; 2)
> adding the env var back into the patch as PGSSLKEYLOGFILE or; 3) postponing all
> of this till v19?

Let's do 1.

nit about the actual code: let's put the new option next to the other
ssl options in PQconninfoOptions instead of all the way at the end.

  25. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-18T15:17:17Z

    On Tue, Mar 18, 2025 at 5:43 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> Personally I think this feature has enough value even without the env var to
> not postpone it, especially since adding an env var in 19 will still be
> backwards compatible.  I would go for option 1 to stay on the safe side and
> allow time for proper discussion, any other thoughts?

+1

--Jacob

  26. Re: Adding support for SSLKEYLOGFILE in the frontend

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-20T09:39:05Z

    Hello,

It seems there's rough consensus on proceeding with a connection param
and no environment variable.  TBH it's not very clear to me that an
envvar is a great way to drive this, even if there weren't security
considerations at play, just considering the case of a multithreaded
program that opens two connections ... reading that log file is going to
be super fun.

In initialize_SSL(), the test for conn->sslkeylogfile is inside the
#ifdef for the existance of the SSL function.  I think it's better to
log a message (probably just a warning) that says "this feature is not
supported with this TLS library" rather than doing nothing.  Silently
failing to act is just painful for the user who then has to go to our
source file to figure out why the setting isn't taking effect.

Thanks,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

  27. Re: Adding support for SSLKEYLOGFILE in the frontend

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-03-20T10:58:14Z

    On 20/03/2025 11:39, Álvaro Herrera wrote:
> Hello,
> 
> It seems there's rough consensus on proceeding with a connection param
> and no environment variable.  TBH it's not very clear to me that an
> envvar is a great way to drive this, even if there weren't security
> considerations at play, just considering the case of a multithreaded
> program that opens two connections ... reading that log file is going to
> be super fun.

I believe the usual way to use SSLKEYLOGFILE is indeed to append all 
keys to the same file. That's how I use, at least. I'm not sure if 
openssl has some locking on it, but I've never had a problem with having 
data from different connections mixed up. The lines are not that long, 
it probably just relies on write(2) being atomic enough.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  28. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-20T13:11:12Z

    On Mon, 17 Mar 2025 at 16:48, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Sun, Mar 16, 2025 at 6:49 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> > IIRC the reasoning has been that if a rogue user can inject an environment
> > variable into your session and read your files it's probably game over anyways.
>
> (Personally I'm no longer as convinced by this line of argument as I
> once was...)

I'm not saying there's no attack possible here (although I cannot
think of one), but we allow configuring every other SSL option using
an env var^1. So if there is an attack possible, why would that only
apply to being able to control the sslkeylogfile as opposed to e.g.
sslmode or sslrootcert.

^1 except for "sslpassword", which is weird because that seems exactly
like one of the options you might not want to store in a connection
string for security reasons.

  29. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-20T16:58:47Z

    On Thu, Mar 20, 2025 at 3:58 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> I'm not sure if openssl has some locking on it,

OpenSSL leaves it up to the application (us). OpenSSL 3.5 will
apparently add a builtin implementation, which from a quick skim does
use locking. As another datapoint, libcurl's implementation appears to
rely on implicit flockfile().

--Jacob

  30. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-03-20T16:58:56Z

    On Thu, Mar 20, 2025 at 6:11 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I'm not saying there's no attack possible here (although I cannot
> think of one), but we allow configuring every other SSL option using
> an env var^1. So if there is an attack possible, why would that only
> apply to being able to control the sslkeylogfile as opposed to e.g.
> sslmode or sslrootcert.

(First off: I was specifically referring to the "if X and Y, then it's
game over, so might as well Z too" line of argument. I think that kind
of reasoning causes problems in OSS security.)

I don't think the argument against an envvar is being made from the
point of view of consistency. That's not very helpful in security
contexts, where we'd rather be inconsistently good/better than
consistently bad. (And existing clients that allow an adversary to
control the PGSSLROOTCERT environment variable are broken, and have
been since, what, 2009? I'm not going to spend energy on that.)

The argument is about net-new behavior: is it okay for a new
environment variable to silently log key material for all our
connections to an arbitrary location on disk? I feel less strongly
about it than Tom does, looks like, but the additional risk is
definitely not zero. Maybe we could declare a prefix of environment
variables to be security-critical or something, to help that concern
in the future.

OpenSSL 3.5 will add native support for the SSLKEYLOGFILE envvar, but
I think they've locked it behind both a build-time option and a
default-off configuration setting. And there are moving discussions
[1] on whether they might provide a setting to centrally disable the
logging callback used in this patch, too. I think Tom's concern has
merit, and I think we should move cautiously here.

Thanks,
--Jacob

[1] https://github.com/openssl/openssl/issues/26282

  31. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-26T22:28:44Z

    > On 20 Mar 2025, at 10:39, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> In initialize_SSL(), the test for conn->sslkeylogfile is inside the
> #ifdef for the existance of the SSL function.  I think it's better to
> log a message (probably just a warning) that says "this feature is not
> supported with this TLS library" rather than doing nothing.  Silently
> failing to act is just painful for the user who then has to go to our
> source file to figure out why the setting isn't taking effect.

The only cases when the function isn't defined are the two oldest LibreSSL
versions we support, but even with a LibreSSL version that does have the
function the code is dead since LibreSSL only implements stubs for OpenSSL
compatibility.  This is documented in our docs, but we might as well help the
user further by logging a warning as you suggest.  The attached v10 adds a
version for the two cases when key logging won't happen (in reality it will be
just one case for LibreSSL but with this we can handle a purpose built OpenSSL
without the callback).

--
Daniel Gustafsson

  32. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-01T20:22:07Z

    I took another pass at this one and did a few small tweaks, so I would to take
it for another spin across CI before looking at committing it.  There are no
functionality changes, only polish.

--
Daniel Gustafsson

  33. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-03T15:51:11Z

    > On 1 Apr 2025, at 22:22, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
> I took another pass at this one and did a few small tweaks, so I would to take
> it for another spin across CI before looking at committing it.  There are no
> functionality changes, only polish.

Committed, after another round of testing and looking.

--
Daniel Gustafsson

  34. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-09T18:41:39Z

    Hello,

On Thu, Apr 3, 2025 at 8:51 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> Committed, after another round of testing and looking.

I think we may want to consider marking sslkeylogfile as a debug
option (that is, opt->dispchar = "D") in fe-connect.c. Besides being a
true "debug option", this would also prevent a relatively unprivileged
user of postgres_fdw or dblink from logging the backend connection's
keys. WDYT?

--Jacob

  35. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-09T18:45:06Z

    > On 9 Apr 2025, at 20:41, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> Hello,
> 
> On Thu, Apr 3, 2025 at 8:51 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> Committed, after another round of testing and looking.
> 
> I think we may want to consider marking sslkeylogfile as a debug
> option (that is, opt->dispchar = "D") in fe-connect.c. Besides being a
> true "debug option", this would also prevent a relatively unprivileged
> user of postgres_fdw or dblink from logging the backend connection's
> keys. WDYT?

I think that sounds like a good idea, unless anyone thinks otherwise I'll go
ahead and make it so.

--
Daniel Gustafsson

  36. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-04-14T20:16:38Z

    > On 9 Apr 2025, at 20:45, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 9 Apr 2025, at 20:41, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
>> 
>> Hello,
>> 
>> On Thu, Apr 3, 2025 at 8:51 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>> Committed, after another round of testing and looking.
>> 
>> I think we may want to consider marking sslkeylogfile as a debug
>> option (that is, opt->dispchar = "D") in fe-connect.c. Besides being a
>> true "debug option", this would also prevent a relatively unprivileged
>> user of postgres_fdw or dblink from logging the backend connection's
>> keys. WDYT?
> 
> I think that sounds like a good idea, unless anyone thinks otherwise I'll go
> ahead and make it so.

Just to close the loop, this was done yesterday as 2970c75dd982.

--
Daniel Gustafsson

  37. Re: Adding support for SSLKEYLOGFILE in the frontend

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-04-14T20:39:52Z

    On Mon, Apr 14, 2025 at 1:16 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> Just to close the loop, this was done yesterday as 2970c75dd982.

Thanks!

--Jacob

  38. Re: Adding support for SSLKEYLOGFILE in the frontend

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-26T20:49:08Z

    On 03.04.25 17:51, Daniel Gustafsson wrote:
>> On 1 Apr 2025, at 22:22, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>> I took another pass at this one and did a few small tweaks, so I would to take
>> it for another spin across CI before looking at committing it.  There are no
>> functionality changes, only polish.
> 
> Committed, after another round of testing and looking.

A few more observations on this code:

What is the meaning of this:

+   old_umask = umask(077);
+   fd = open(conn->sslkeylogfile, O_WRONLY | O_APPEND | O_CREAT, 0600);
+   umask(old_umask);

If open() already specifies the file mode, do we still need umask()? 
Maybe I'm missing something.

Also, we usually use symbols for the modes.

+       libpq_append_conn_error(conn, "could not open ssl keylog file 
%s: %s",
+                               conn->sslkeylogfile, pg_strerror(errno));

pg_strerror() is not thread-safe, so it shouldn't be used here. 
Actually, pg_strerror() is not meant for direct use at all.  Probably 
easiest to just use %m.

Also, I can't actually trigger these errors.  This call just sticks the 
errors into the connection structure, but if the connection is otherwise 
successful, nothing will print the error.  Maybe the best we can do is 
print the error to stderr, like similarly in initialize_SSL().

  39. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-06-26T21:06:50Z

    > On 26 Jun 2025, at 22:49, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 03.04.25 17:51, Daniel Gustafsson wrote:
>>> On 1 Apr 2025, at 22:22, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> 
>>> I took another pass at this one and did a few small tweaks, so I would to take
>>> it for another spin across CI before looking at committing it.  There are no
>>> functionality changes, only polish.
>> Committed, after another round of testing and looking.
> 
> A few more observations on this code:

Thanks for review!

> What is the meaning of this:
> 
> +   old_umask = umask(077);
> +   fd = open(conn->sslkeylogfile, O_WRONLY | O_APPEND | O_CREAT, 0600);
> +   umask(old_umask);
> 
> If open() already specifies the file mode, do we still need umask()? Maybe I'm missing something.

No, I think that's a leftover from a previous version which I missed.  I can't
see that being required.

> Also, we usually use symbols for the modes.
> 
> +       libpq_append_conn_error(conn, "could not open ssl keylog file %s: %s",
> +                               conn->sslkeylogfile, pg_strerror(errno));
> 
> pg_strerror() is not thread-safe, so it shouldn't be used here. Actually, pg_strerror() is not meant for direct use at all.  Probably easiest to just use %m.
> 
> Also, I can't actually trigger these errors.  This call just sticks the errors into the connection structure, but if the connection is otherwise successful, nothing will print the error.  Maybe the best we can do is print the error to stderr, like similarly in initialize_SSL().

Thats a good point, a successful connection does not inspect the errormessage.
I'll propose changes for these comments in the morning when coffee has been
had.

--
Daniel Gustafsson

  40. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-06-27T09:15:02Z

    > On 26 Jun 2025, at 23:06, Daniel Gustafsson <daniel@yesql.se> wrote:

> I'll propose changes for these comments in the morning when coffee has been
> had.

The attached moves to logging on stderr along with a test for this, and also
removes the WARNING prefix that was added to the other stderr loggings since
that seems out of place compared to other stderr loggings in libpq (the
sslpassword trunction WARNING is clearly warranted, these were not).  The umask
call is removed as it's not required, and a little bit of whitespace cleanup
from the original commit performed.  Re macros for file modes, this is for
fopen and not open though, or am I missing something?

--
Daniel Gustafsson

  41. Re: Adding support for SSLKEYLOGFILE in the frontend

    Peter Eisentraut <peter@eisentraut.org> — 2025-06-29T10:56:40Z

    On 27.06.25 11:15, Daniel Gustafsson wrote:
>> On 26 Jun 2025, at 23:06, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> I'll propose changes for these comments in the morning when coffee has been
>> had.
> 
> The attached moves to logging on stderr along with a test for this, and also
> removes the WARNING prefix that was added to the other stderr loggings since
> that seems out of place compared to other stderr loggings in libpq (the
> sslpassword trunction WARNING is clearly warranted, these were not).

Hmm, I thought the WARNING prefixes were good.  I think these are similar to

"WARNING: password file \"%s\" is not a plain file\n"

in that something during the connection setup is being ignored.

Otherwise, if you just write something but don't tag it with something 
like warning or error, it's not clear how the user is supposed to 
interpret it.  Is it a progress message, is it something bad, etc.?

> The umask
> call is removed as it's not required, and a little bit of whitespace cleanup
> from the original commit performed.  Re macros for file modes, this is for
> fopen and not open though, or am I missing something?

It looks like the conventions around the code are mixed.  0600 is clear 
enough that we can keep it.

I don't think this is necessary:

+	errno = 0;

  42. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-07-01T08:47:06Z

    > On 29 Jun 2025, at 12:56, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
> On 27.06.25 11:15, Daniel Gustafsson wrote:
>>> On 26 Jun 2025, at 23:06, Daniel Gustafsson <daniel@yesql.se> wrote:
>>> I'll propose changes for these comments in the morning when coffee has been
>>> had.
>> The attached moves to logging on stderr along with a test for this, and also
>> removes the WARNING prefix that was added to the other stderr loggings since
>> that seems out of place compared to other stderr loggings in libpq (the
>> sslpassword trunction WARNING is clearly warranted, these were not).
> 
> Hmm, I thought the WARNING prefixes were good.  I think these are similar to
> 
> "WARNING: password file \"%s\" is not a plain file\n"
> 
> in that something during the connection setup is being ignored.
> 
> Otherwise, if you just write something but don't tag it with something like warning or error, it's not clear how the user is supposed to interpret it.  Is it a progress message, is it something bad, etc.?

Fair.  As this is a connection debugging tool and not for regular use I wasn't
thinking of them as warnings per se (I was more classifying them as the "out of
memory" errors which are printed without a prefix), but I also don't mind
adding them back.  Done in v2 with other comments addressed as well.

--
Daniel Gustafsson

  43. Re: Adding support for SSLKEYLOGFILE in the frontend

    Peter Eisentraut <peter@eisentraut.org> — 2025-07-05T08:56:44Z

    On 01.07.25 10:47, Daniel Gustafsson wrote:
>> On 29 Jun 2025, at 12:56, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 27.06.25 11:15, Daniel Gustafsson wrote:
>>>> On 26 Jun 2025, at 23:06, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>> I'll propose changes for these comments in the morning when coffee has been
>>>> had.
>>> The attached moves to logging on stderr along with a test for this, and also
>>> removes the WARNING prefix that was added to the other stderr loggings since
>>> that seems out of place compared to other stderr loggings in libpq (the
>>> sslpassword trunction WARNING is clearly warranted, these were not).
>>
>> Hmm, I thought the WARNING prefixes were good.  I think these are similar to
>>
>> "WARNING: password file \"%s\" is not a plain file\n"
>>
>> in that something during the connection setup is being ignored.
>>
>> Otherwise, if you just write something but don't tag it with something like warning or error, it's not clear how the user is supposed to interpret it.  Is it a progress message, is it something bad, etc.?
> 
> Fair.  As this is a connection debugging tool and not for regular use I wasn't
> thinking of them as warnings per se (I was more classifying them as the "out of
> memory" errors which are printed without a prefix), but I also don't mind
> adding them back.  Done in v2 with other comments addressed as well.

This patch version looks good to me.

  44. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-07-10T07:33:53Z

    > On 5 Jul 2025, at 10:56, Peter Eisentraut <peter@eisentraut.org> wrote:

> This patch version looks good to me.

Thanks for review, I'll get it committed shortly.

--
Daniel Gustafsson

  45. Re: Adding support for SSLKEYLOGFILE in the frontend

    Daniel Gustafsson <daniel@yesql.se> — 2025-07-12T19:14:44Z

    > On 10 Jul 2025, at 09:33, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 5 Jul 2025, at 10:56, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>> This patch version looks good to me.
> 
> Thanks for review, I'll get it committed shortly.

This was committed yesterday ahead of the beta release.

--
Daniel Gustafsson