> 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
