Thread: Re: Adding support for SSLKEYLOGFILE in the frontend

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

> 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

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

> 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 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

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

> 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

> 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

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

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

> 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, 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

> 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

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.

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

> 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
wantto 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

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

> 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

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.

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

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)

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)

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.

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

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

> 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

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

> 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