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
