Re: Adding support for SSLKEYLOGFILE in the frontend

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(dot)becs(at)gmail(dot)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(at)yesql(dot)se> wrote:
> >
> > > On 20 Feb 2025, at 04:36, Abhishek Chanda <abhishek(dot)becs(at)gmail(dot)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