Re: Support for NSS as a libpq TLS backend

> On 24 Feb 2021, at 01:11, Jacob Champion <pchampion@vmware.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/