On Mon, 2021-02-22 at 14:31 +0100, Daniel Gustafsson wrote:
> The attached fixes that as well as implements the sslcrldir
> support that was committed recently. The crldir parameter isn't applicable to
> NSS per se since all CRL's are loaded into the NSS database, but it does need
> to be supported for the tests.
>
> The crldir commit also made similar changes to the test harness as I had done
> to support the NSS database, which made these incompatible. To fix that I've
> implemented named parameters in switch_server_cert to make it less magic with
> multiple optional parameters.
The named parameters are a big improvement!
Couple things I've noticed with this patch, back on the OpenSSL side.
In SSL::Backend::OpenSSL's set_server_conf() implementation:
> + my $sslconf =
> + "ssl_ca_file='$params->{cafile}.crt'\n"
> + . "ssl_cert_file='$params->{certfile}.crt'\n"
> + . "ssl_key_file='$params->{keyfile}.key'\n"
> + . "ssl_crl_file='$params->{crlfile}'\n";
> + $sslconf .= "ssl_crl_dir='$params->{crldir}'\n" if defined
> $params->{crldir};
> }
this is missing a `return $sslconf` at the end.
In 001_ssltests.pl:
> -set_server_cert($node, 'server-cn-only', 'root+client_ca',
> - 'server-password', 'echo wrongpassword');
> -command_fails(
> - [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
> - 'restart fails with password-protected key file with wrong password');
> -$node->_update_pid(0);
> +# Since the passphrase callbacks operate at different stages in OpenSSL and
> +# NSS we have two separate blocks for them
> +SKIP:
> +{
> + skip "Certificate passphrases aren't checked on server restart in NSS", 2
> + if ($nss);
> +
> + switch_server_cert($node,
> + certfile => 'server-cn-only',
> + cafile => 'root+client_ca',
> + keyfile => 'server-password',
> + nssdatabase => 'server-cn-only.crt__server-password.key.db',
> + passphrase_cmd => 'echo wrongpassword');
> +
> + command_fails(
> + [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
> + 'restart fails with password-protected key file with wrong password');
> + $node->_update_pid(0);
The removal of set_server_cert() in favor of switch_server_cert()
breaks these tests in OpenSSL, because the restart that
switch_server_cert performs will fail as designed. (The new comment
above switch_server_cert() suggests maybe you had a switch in mind to
skip the restart?)
NSS is not affected because we expect the restart to succeed:
> + command_ok(
> + [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
> + 'restart fails with password-protected key file with wrong password');
but I'd argue that that NSS test and the one after it should probably
be removed. We already know restart succeeded; otherwise
switch_server_cert() would have failed. (The test descriptions also
have the old "restart fails" verbiage.)
--Jacob
