Re: Support for NSS as a libpq TLS backend

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Jacob Champion <pchampion(at)vmware(dot)com>
Cc: "hlinnaka(at)iki(dot)fi" <hlinnaka(at)iki(dot)fi>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "andrew(dot)dunstan(at)2ndquadrant(dot)com" <andrew(dot)dunstan(at)2ndquadrant(dot)com>, "sfrost(at)snowman(dot)net" <sfrost(at)snowman(dot)net>, "thomas(dot)munro(at)gmail(dot)com" <thomas(dot)munro(at)gmail(dot)com>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>
Subject: Re: Support for NSS as a libpq TLS backend
Date: 2021-02-24 12:23:32
Message-ID: 04E81B50-ACE9-40D3-9C66-7041F969A958@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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/

Attachment Content-Type Size
v29-0009-nss-Build-infrastructure.patch application/octet-stream 20.5 KB
v29-0008-nss-Support-NSS-in-cryptohash.patch application/octet-stream 6.1 KB
v29-0007-nss-Support-NSS-in-sslinfo.patch application/octet-stream 3.6 KB
v29-0006-nss-Support-NSS-in-pgcrypto.patch application/octet-stream 24.6 KB
v29-0005-nss-Documentation.patch application/octet-stream 33.8 KB
v29-0004-nss-pg_strong_random-support.patch application/octet-stream 1.9 KB
v29-0003-nss-Add-NSS-specific-tests.patch application/octet-stream 52.2 KB
v29-0002-Refactor-SSL-testharness-for-multiple-library.patch application/octet-stream 11.5 KB
v29-0001-nss-Support-libnss-as-TLS-library-in-libpq.patch application/octet-stream 92.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2021-02-24 12:25:54 Re: Single transaction in the tablesync worker?
Previous Message Julien Rouhaud 2021-02-24 12:21:41 Re: REINDEX backend filtering