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