Re: Refactor SSL test framework to support multiple TLS libraries
On Wed, Mar 31, 2021 at 10:43:00AM +0900, Michael Paquier wrote: > Jacob has just raised this as an issue for an integration with NLS, > because it may be possible that things fail with "SSL error" but a > different error pattern, causing false positives: > https://www.postgresql.org/message-id/e0f0484a1815b26bb99ef9ddc7a110dfd6425931.ca...@vmware.com > > I agree that those matches should be much more picky. We may need to > be careful across all versions of OpenSSL supported though :/ As I got my eyes on that, I am going to begin a new thread with a patch. > With all the comments addressed, with updates to use a single scalar > for all the connection strings and with a proper indentation, I finish > with the attached. Does that look fine? Hearing nothing, I have applied this cleanup patch. I am not sure if I will be able to tackle the remaining issues, aka switching SSLServer.pm to become an OO module and plug OpenSSL-specific things on top of that. -- Michael signature.asc Description: PGP signature
Re: Refactor SSL test framework to support multiple TLS libraries
On Tue, Mar 30, 2021 at 12:15:07PM -0300, Alvaro Herrera wrote:
> The only complain I have is that "the given node" is nonsensical in
> PostgresNode. I suggest to delete the word "given". Also "This is
> expected to fail with a message that matches the regular expression
> $expected_stderr".
Your suggestions are better, indeed.
> The POD doc for connect_fails uses order: ($connstr, $testname,
> $expected_stderr)
> but the routine has:
> + my ($self, $connstr, $expected_stderr, $testname) = @_;
>
> these should match.
Fixed.
> (There's quite an inconsistency in the existing test code about
> expected_stderr being a string or a regex; and some regexes are quite
> generic: just qr/SSL error/. Not this patch responsibility to fix that.)
Jacob has just raised this as an issue for an integration with NLS,
because it may be possible that things fail with "SSL error" but a
different error pattern, causing false positives:
https://www.postgresql.org/message-id/e0f0484a1815b26bb99ef9ddc7a110dfd6425931.ca...@vmware.com
I agree that those matches should be much more picky. We may need to
be careful across all versions of OpenSSL supported though :/
> As I understand, our perlcriticrc no longer requires 'return' at the end
> of routines (commit 0516f94d18c5), so you can omit that.
Fixed. Thanks.
With all the comments addressed, with updates to use a single scalar
for all the connection strings and with a proper indentation, I finish
with the attached. Does that look fine?
--
Michael
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 1e96357d7e..d6e10544bb 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1511,6 +1511,11 @@ the B parameter is also given.
If B is set and this parameter is given, the scalar it references
is set to true if the psql call times out.
+=item connstr => B
+
+If set, use this as the connection string for the connection to the
+backend.
+
=item replication => B
If set, add B to the conninfo string.
@@ -1550,14 +1555,20 @@ sub psql
my $replication = $params{replication};
my $timeout = undef;
my $timeout_exception = 'psql timed out';
- my @psql_params = (
- 'psql',
- '-XAtq',
- '-d',
- $self->connstr($dbname)
- . (defined $replication ? " replication=$replication" : ""),
- '-f',
- '-');
+
+ # Build the connection string.
+ my $psql_connstr;
+ if (defined $params{connstr})
+ {
+ $psql_connstr = $params{connstr};
+ }
+ else
+ {
+ $psql_connstr = $self->connstr($dbname);
+ }
+ $psql_connstr .= defined $replication ? " replication=$replication" : "";
+
+ my @psql_params = ('psql', '-XAtq', '-d', $psql_connstr, '-f', '-');
# If the caller wants an array and hasn't passed stdout/stderr
# references, allocate temporary ones to capture them so we
@@ -1849,6 +1860,51 @@ sub interactive_psql
=pod
+=item $node->connect_ok($connstr, $test_name)
+
+Attempt a connection with a custom connection string. This is expected
+to succeed.
+
+=cut
+
+sub connect_ok
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($self, $connstr, $test_name) = @_;
+ my ($ret, $stdout, $stderr)= $self->psql(
+ 'postgres',
+ "SELECT \$\$connected with $connstr\$\$",
+ connstr => "$connstr",
+ on_error_stop => 0);
+
+ ok($ret == 0, $test_name);
+}
+
+=pod
+
+=item $node->connect_fails($connstr, $expected_stderr, $test_name)
+
+Attempt a connection with a custom connection string. This is expected
+to fail with a message that matches the regular expression
+$expected_stderr.
+
+=cut
+
+sub connect_fails
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+ my ($self, $connstr, $expected_stderr, $test_name) = @_;
+ my ($ret, $stdout, $stderr) = $self->psql(
+ 'postgres',
+ "SELECT \$\$connected with $connstr\$\$",
+ connstr => "$connstr");
+
+ ok($ret != 0, $test_name);
+ like($stderr, $expected_stderr, "$test_name: matches");
+}
+
+=pod
+
=item $node->poll_query_until($dbname, $query [, $expected ])
Run B<$query> repeatedly, until it returns the B<$expected> result
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index adaa1b4e9b..b1a63f279c 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -135,103 +135,94 @@ $common_connstr =
"user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
-test_connect_fails(
- $common_connstr, "sslmode=disable",
+$node->connect_fails(
+ "$common_connstr sslmode=disable",
qr/\Qno pg_hba.conf entry\E/,
"server doesn't accept non-SSL connections");
# Try without a root cert. In sslmode=require, this should work. In verify-ca
# or verify-full mode it should fail.
-test_connect_ok(
- $common_connstr,
- "sslrootcert=invalid sslmode=require",
+$node->connect_ok(
+ "$common_connstr sslrootcert=invalid sslmode=require",
"connect without server root
Re: Refactor SSL test framework to support multiple TLS libraries
On Tue, Mar 30, 2021 at 07:14:55PM -0300, Alvaro Herrera wrote: > On 2021-Mar-30, Daniel Gustafsson wrote: >> This double concatenation could be a single concat, or just use scalar value >> interpolation in the string to make it even more readable. As it isn't using >> the same line broken pattern of the others the concat looks a bit weird as a >> result. > > +1 for using a single scalar. Agreed. I posted things this way to make a lookup at the diffs easier for the eye, but that was not intended for the final patch. -- Michael signature.asc Description: PGP signature
Re: Refactor SSL test framework to support multiple TLS libraries
On 2021-Mar-30, Daniel Gustafsson wrote: > +$node->connect_ok($common_connstr . " " . "user=ssltestuser", > > This double concatenation could be a single concat, or just use scalar value > interpolation in the string to make it even more readable. As it isn't using > the same line broken pattern of the others the concat looks a bit weird as a > result. +1 for using a single scalar. -- Álvaro Herrera Valdivia, Chile
Re: Refactor SSL test framework to support multiple TLS libraries
> On 30 Mar 2021, at 11:53, Michael Paquier wrote: > > On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote: >> The test_*() ones are just wrappers for psql able to use a customized >> connection string. It seems to me that it would make sense to move >> those two into PostgresNode::psql itself and extend it to be able to >> handle custom connection strings? > > Looking at this part, I think that this is a win in terms of future > changes for SSLServer.pm as it would become a facility only in charge > of managing the backend's SSL configuration. This has also the > advantage to make the error handling with psql more consistent with > the other tests. > > So, attached is a patch to do this simplification. The bulk of the > changes is within the tests themselves to adapt to the merge of > $common_connstr and $connstr for the new routines of PostgresNode.pm, > and I have done things this way to ease the patch lookup. Thoughts? LGTM with the findings that Alvaro reported. +$node->connect_ok($common_connstr . " " . "user=ssltestuser", This double concatenation could be a single concat, or just use scalar value interpolation in the string to make it even more readable. As it isn't using the same line broken pattern of the others the concat looks a bit weird as a result. Thanks for picking it up, as I have very limited time for hacking right now. -- Daniel Gustafsson https://vmware.com/
Re: Refactor SSL test framework to support multiple TLS libraries
On 2021-Mar-30, Michael Paquier wrote: > On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote: > > The test_*() ones are just wrappers for psql able to use a customized > > connection string. It seems to me that it would make sense to move > > those two into PostgresNode::psql itself and extend it to be able to > > handle custom connection strings? > > Looking at this part, I think that this is a win in terms of future > changes for SSLServer.pm as it would become a facility only in charge > of managing the backend's SSL configuration. This has also the > advantage to make the error handling with psql more consistent with > the other tests. > > So, attached is a patch to do this simplification. The bulk of the > changes is within the tests themselves to adapt to the merge of > $common_connstr and $connstr for the new routines of PostgresNode.pm, > and I have done things this way to ease the patch lookup. Thoughts? I agree this seems a win. The only complain I have is that "the given node" is nonsensical in PostgresNode. I suggest to delete the word "given". Also "This is expected to fail with a message that matches the regular expression $expected_stderr". The POD doc for connect_fails uses order: ($connstr, $testname, $expected_stderr) but the routine has: + my ($self, $connstr, $expected_stderr, $testname) = @_; these should match. (There's quite an inconsistency in the existing test code about expected_stderr being a string or a regex; and some regexes are quite generic: just qr/SSL error/. Not this patch responsibility to fix that.) As I understand, our perlcriticrc no longer requires 'return' at the end of routines (commit 0516f94d18c5), so you can omit that. -- Álvaro Herrera Valdivia, Chile
Re: Refactor SSL test framework to support multiple TLS libraries
On 3/30/21 5:53 AM, Michael Paquier wrote: > On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote: >> The test_*() ones are just wrappers for psql able to use a customized >> connection string. It seems to me that it would make sense to move >> those two into PostgresNode::psql itself and extend it to be able to >> handle custom connection strings? > Looking at this part, I think that this is a win in terms of future > changes for SSLServer.pm as it would become a facility only in charge > of managing the backend's SSL configuration. This has also the > advantage to make the error handling with psql more consistent with > the other tests. > > So, attached is a patch to do this simplification. The bulk of the > changes is within the tests themselves to adapt to the merge of > $common_connstr and $connstr for the new routines of PostgresNode.pm, > and I have done things this way to ease the patch lookup. Thoughts? Looks reasonable. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Refactor SSL test framework to support multiple TLS libraries
On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
> The test_*() ones are just wrappers for psql able to use a customized
> connection string. It seems to me that it would make sense to move
> those two into PostgresNode::psql itself and extend it to be able to
> handle custom connection strings?
Looking at this part, I think that this is a win in terms of future
changes for SSLServer.pm as it would become a facility only in charge
of managing the backend's SSL configuration. This has also the
advantage to make the error handling with psql more consistent with
the other tests.
So, attached is a patch to do this simplification. The bulk of the
changes is within the tests themselves to adapt to the merge of
$common_connstr and $connstr for the new routines of PostgresNode.pm,
and I have done things this way to ease the patch lookup. Thoughts?
--
Michael
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 1e96357d7e..403ef56d54 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1511,6 +1511,11 @@ the B parameter is also given.
If B is set and this parameter is given, the scalar it references
is set to true if the psql call times out.
+=item connstr => B
+
+If set, use this as the connection string for the connection to the
+backend.
+
=item replication => B
If set, add B to the conninfo string.
@@ -1550,12 +1555,24 @@ sub psql
my $replication = $params{replication};
my $timeout = undef;
my $timeout_exception = 'psql timed out';
+
+ # Build the connection string.
+ my $psql_connstr;
+ if (defined $params{connstr})
+ {
+ $psql_connstr = $params{connstr};
+ }
+ else
+ {
+ $psql_connstr = $self->connstr($dbname);
+ }
+ $psql_connstr .= defined $replication ? " replication=$replication" : "";
+
my @psql_params = (
'psql',
'-XAtq',
'-d',
- $self->connstr($dbname)
- . (defined $replication ? " replication=$replication" : ""),
+ $psql_connstr,
'-f',
'-');
@@ -1849,6 +1866,54 @@ sub interactive_psql
=pod
+=item $node->connect_ok($connstr, $testname)
+
+Attempt a connection to the given node, with a custom connection string.
+This is expected to succeed.
+
+=cut
+
+sub connect_ok
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my ($self, $connstr, $testname) = @_;
+
+ my ($ret, $stdout, $stderr) = $self->psql('postgres',
+ "SELECT \$\$connected with $connstr\$\$",
+ connstr => "$connstr", on_error_stop => 0);
+
+ ok($ret == 0, $testname);
+ return;
+}
+
+=pod
+
+=item $node->connect_fails($connstr, $testname, $expected_stderr)
+
+Attempt a connection to the given node, with a custom connection string.
+This is expected to fail.
+
+=cut
+
+sub connect_fails
+{
+ local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+ my ($self, $connstr, $expected_stderr, $testname) = @_;
+
+ my ($ret, $stdout, $stderr) = $self->psql('postgres',
+ "SELECT \$\$connected with $connstr\$\$",
+ connstr => "$connstr");
+
+ ok($ret != 0, $testname);
+
+ like($stderr, $expected_stderr, "$testname: matches");
+ return;
+}
+
+=pod
+
=item $node->poll_query_until($dbname, $query [, $expected ])
Run B<$query> repeatedly, until it returns the B<$expected> result
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index adaa1b4e9b..dc3b69cb46 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -135,102 +135,102 @@ $common_connstr =
"user=ssltestuser dbname=trustdb sslcert=invalid hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test";
# The server should not accept non-SSL connections.
-test_connect_fails(
- $common_connstr, "sslmode=disable",
+$node->connect_fails(
+ $common_connstr . " " . "sslmode=disable",
qr/\Qno pg_hba.conf entry\E/,
"server doesn't accept non-SSL connections");
# Try without a root cert. In sslmode=require, this should work. In verify-ca
# or verify-full mode it should fail.
-test_connect_ok(
- $common_connstr,
+$node->connect_ok(
+ $common_connstr . " " .
"sslrootcert=invalid sslmode=require",
"connect without server root cert sslmode=require");
-test_connect_fails(
- $common_connstr,
+$node->connect_fails(
+ $common_connstr . " " .
"sslrootcert=invalid sslmode=verify-ca",
qr/root certificate file "invalid" does not exist/,
"connect without server root cert sslmode=verify-ca");
-test_connect_fails(
- $common_connstr,
+$node->connect_fails(
+ $common_connstr . " " .
"sslrootcert=invalid sslmode=verify-full",
qr/root certificate file "invalid" does not exist/,
"connect without server root cert sslmode=verify-full");
# Try with wrong root cert, should fail. (We're using the client CA as the
# root, but the server's key is signed by the server CA.)
-test_connect_fails($common_connstr,
+$node->connect_fails($common_connstr . " " .
"sslrootcert=ssl/client_ca.crt sslmode=require",
qr/SSL error/, "connect with wrong server root cert sslmode=require");
Re: Refactor SSL test framework to support multiple TLS libraries
On Thu, Mar 25, 2021 at 09:25:11AM -0400, Andrew Dunstan wrote: > The thing is that SSLServer isn't currently constructed in an OO > fashion. Typically, OO modules in perl don't export anything, and all > access is via the class (for the constructor or static methods) or > instances, as in > > my $instance = MyClass->new(); > $instance->mymethod(); > > In such a module you should not see lines using Exporter or defining > @Export. > > So probably the first step in this process would be to recast SSLServer > as an OO type module, without subclassing it, and then create a subclass > along the lines Alvarro suggests. It seems that it does not make sense to transform all the contents of SSLServer to become an OO module. So it looks necessary to me to split things, with one part being the OO module managing the server configuration. So, first, we have some helper routines that should not be within the module: - copy_files() - test_connect_fails() - test_connect_ok() The test_*() ones are just wrappers for psql able to use a customized connection string. It seems to me that it would make sense to move those two into PostgresNode::psql itself and extend it to be able to handle custom connection strings? copy_files() is more generic than that. Wouldn't it make sense to move that to TestLib.pm instead? Second, the routines managing the server setup itself: - a new() routine to create and register a node removing the duplicated initialization setup in 001 and 002. - switch_server_cert(), with a split on set_server_cert() as that looks cleaner. - configure_hba_for_ssl() - install_certificates() (present inside Daniel's patch) - Something to copy the keys from the tree. Patch v2 from upthread does mostly that, but it seems to me that we should integrate better with PostgresNode to manage the backend node, no? > Incidentally, I'm not sure why we need to break SSLServer into > SSL::Server - are we expecting to create other children of the SSL > namespace? Agreed. -- Michael signature.asc Description: PGP signature
Re: Refactor SSL test framework to support multiple TLS libraries
On 3/24/21 7:49 PM, Daniel Gustafsson wrote: >> On 25 Mar 2021, at 00:26, Alvaro Herrera wrote: >> >> On 2021-Mar-25, Daniel Gustafsson wrote: >> >>> Attached is a v2 which addresses the comments raised on the main NSS >>> thread, as >>> well as introduces named parameters for the server cert function to make the >>> test code easier to read. >> I don't like this patch. I think your SSL::Server::OpenSSL and >> SSL::Server::NSS packages should be doing "use parent SSL::Server"; >> having SSL::Server grow a line to "use" its subclass >> SSL::Server::OpenSSL and import its get_new_openssl_backend() method >> seems to go against the grain. > I'm far from skilled at Perl module inheritance but that makes sense, I'll > take > a stab at that after some sleep and coffee. > The thing is that SSLServer isn't currently constructed in an OO fashion. Typically, OO modules in perl don't export anything, and all access is via the class (for the constructor or static methods) or instances, as in my $instance = MyClass->new(); $instance->mymethod(); In such a module you should not see lines using Exporter or defining @Export. So probably the first step in this process would be to recast SSLServer as an OO type module, without subclassing it, and then create a subclass along the lines Alvarro suggests. If this is all strange to you, I can help a bit. Incidentally, I'm not sure why we need to break SSLServer into SSL::Server - are we expecting to create other children of the SSL namespace? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Refactor SSL test framework to support multiple TLS libraries
> On 25 Mar 2021, at 00:26, Alvaro Herrera wrote: > > On 2021-Mar-25, Daniel Gustafsson wrote: > >> Attached is a v2 which addresses the comments raised on the main NSS thread, >> as >> well as introduces named parameters for the server cert function to make the >> test code easier to read. > > I don't like this patch. I think your SSL::Server::OpenSSL and > SSL::Server::NSS packages should be doing "use parent SSL::Server"; > having SSL::Server grow a line to "use" its subclass > SSL::Server::OpenSSL and import its get_new_openssl_backend() method > seems to go against the grain. I'm far from skilled at Perl module inheritance but that makes sense, I'll take a stab at that after some sleep and coffee. -- Daniel Gustafsson https://vmware.com/
Re: Refactor SSL test framework to support multiple TLS libraries
On 2021-Mar-25, Daniel Gustafsson wrote: > Attached is a v2 which addresses the comments raised on the main NSS thread, > as > well as introduces named parameters for the server cert function to make the > test code easier to read. I don't like this patch. I think your SSL::Server::OpenSSL and SSL::Server::NSS packages should be doing "use parent SSL::Server"; having SSL::Server grow a line to "use" its subclass SSL::Server::OpenSSL and import its get_new_openssl_backend() method seems to go against the grain. -- Álvaro Herrera39°49'30"S 73°17'W
Re: Refactor SSL test framework to support multiple TLS libraries
Attached is a v2 which addresses the comments raised on the main NSS thread, as well as introduces named parameters for the server cert function to make the test code easier to read. -- Daniel Gustafsson https://vmware.com/ v2-0001-Refactor-SSL-testharness-for-multiple-library.patch Description: Binary data
Refactor SSL test framework to support multiple TLS libraries
In an attempt to slice off as much non-NSS specific changes as possible from the larger libnss patch proposed in [0], the attached patch contains the ssl test harness refactoring to support multiple TLS libraries. The changes are mostly a refactoring to hide library specific setup in their own modules, but also extend set_server_cert() to support password command which cleans up the TAP tests from hands-on setup and teardown. cheers ./daniel [0] https://postgr.es/m/fab21fc8-0f62-434f-aa78-6bd9336d6...@yesql.se 0001-Refactor-library-specific-SSL-test-setup-into-separa.patch Description: Binary data
