On Tue, Mar 23, 2021 at 12:38:50AM +0100, Daniel Gustafsson wrote:
> Thanks again for reviewing, another version which addresses the remaining
> issues will be posted soon but I wanted to get this out to give further reviews
> something that properly works.
I have been looking at the infrastructure of the tests, patches 0002
(some refactoring) and 0003 (more refactoring with tests for NSS), and
I am a bit confused by its state.
First, I think that the split is not completely clear. For example,
patch 0003 has changes for OpenSSL.pm and Server.pm, but wouldn't it
be better to have all the refactoring infrastructure only in 0002,
with 0003 introducing only the NSS pieces for its internal data and
NSS.pm?
+ keyfile => 'server-password',
+ nssdatabase => 'server-cn-only.crt__server-password.key.db',
+ passphrase_cmd => 'echo secret1',
001_ssltests.pl and 002_scram.pl have NSS-related parameters, which
does not look like a clean separation to me as there are OpenSSL tests
that use some NSS parts, and the main scripts should remain neutral in
terms setting contents, including only variables and callbacks that
should be filled specifically for each SSL implementation, no? Aren't
we missing a second piece here with a set of callbacks for the
per-library test paths then?
+ if (defined($openssl))
+ {
+ copy_files("ssl/server-*.crt", $pgdata);
+ copy_files("ssl/server-*.key", $pgdata);
+ chmod(0600, glob "$pgdata/server-*.key") or die $!;
+ copy_files("ssl/root+client_ca.crt", $pgdata);
+ copy_files("ssl/root_ca.crt", $pgdata);
+ copy_files("ssl/root+client.crl", $pgdata);
+ mkdir("$pgdata/root+client-crldir");
+ copy_files("ssl/root+client-crldir/*",
"$pgdata/root+client-crldir/");
+ }
+ elsif (defined($nss))
+ {
+ RecursiveCopy::copypath("ssl/nss", $pgdata . "/nss") if -e
"ssl/nss";
+ }
This had better be in its own callback, for example.
--
Michael