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/

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 Herrera 39�49'30"S 73�17'W

On 25 Mar 2021, at 00:26, Alvaro Herrera <alvherre@alvh.no-ip.org> 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/

On 3/24/21 7:49 PM, Daniel Gustafsson wrote:

On 25 Mar 2021, at 00:26, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

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

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

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

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

On 30 Mar 2021, at 11:53, Michael Paquier <michael@paquier.xyz> 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/

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

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

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:
/messages/by-id/e0f0484a1815b26bb99ef9ddc7a110dfd6425931.camel@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

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:
/messages/by-id/e0f0484a1815b26bb99ef9ddc7a110dfd6425931.camel@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