Re: Support for NSS as a libpq TLS backend

> On 24 Feb 2021, at 01:11, Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> 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.

Yeah, I was clearly undercaffeinated and forgot to re-run the tests on OpenSSL
after some hackery. Fixed.

> 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?)

I initially had a restart => 'yes' parameter which turned too repetetive since
nearly all calls wants a restart. When I switched it to an opt-out I missed to
update the tests. Fixed.

> 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.)

Agreed, removed.

--
Daniel Gustafsson https://vmware.com/