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

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

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

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]/messages/by-id/1774813.1736385450@sss.pgh.pa.us 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]: /messages/by-id/1774813.1736385450@sss.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]/messages/by-id/68b66b6d-cc59-44f8-bdd2-248d50055740@eisentraut.org 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]: /messages/by-id/68b66b6d-cc59-44f8-bdd2-248d50055740@eisentraut.org

Regards.
Vignesh