public inbox for [email protected]  

help / color / mirror / Atom feed

From: Jacob Champion <[email protected]>
To: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Support for NSS as a libpq TLS backend
Date: Wed, 24 Feb 2021 00:11:55 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<YCOJ7P6yfrvrbaY/@paquier.xyz>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (181+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Support for NSS as a libpq TLS backend
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox