Horton

Thread

Commits

  1. Move some client-specific routines from SSLServer to PostgresNode

  2. Stop requiring an explicit return from perl subroutines

  1. Refactor SSL test framework to support multiple TLS libraries

    Daniel Gustafsson <daniel@yesql.se> — 2021-01-21T09:42:02Z

    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-6BD9336D630A@yesql.se

  2. Re: Refactor SSL test framework to support multiple TLS libraries

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-24T23:02:00Z

    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/

  3. Re: Refactor SSL test framework to support multiple TLS libraries

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-24T23:26:30Z

    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

  4. Re: Refactor SSL test framework to support multiple TLS libraries

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-24T23:49:47Z

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

  5. Re: Refactor SSL test framework to support multiple TLS libraries

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-25T13:25:11Z

    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

  6. Re: Refactor SSL test framework to support multiple TLS libraries

    Michael Paquier <michael@paquier.xyz> — 2021-03-30T06:50:28Z

    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

  7. Re: Refactor SSL test framework to support multiple TLS libraries

    Michael Paquier <michael@paquier.xyz> — 2021-03-30T09:53:49Z

    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

  8. Re: Refactor SSL test framework to support multiple TLS libraries

    Andrew Dunstan <andrew@dunslane.net> — 2021-03-30T13:45:36Z

    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

  9. Re: Refactor SSL test framework to support multiple TLS libraries

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-30T15:15:07Z

    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

  10. Re: Refactor SSL test framework to support multiple TLS libraries

    Daniel Gustafsson <daniel@yesql.se> — 2021-03-30T21:59:17Z

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

  11. Re: Refactor SSL test framework to support multiple TLS libraries

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-03-30T22:14:55Z

    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

  12. Re: Refactor SSL test framework to support multiple TLS libraries

    Michael Paquier <michael@paquier.xyz> — 2021-03-31T01:01:45Z

    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

  13. Re: Refactor SSL test framework to support multiple TLS libraries

    Michael Paquier <michael@paquier.xyz> — 2021-03-31T01:43:00Z

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

  14. Re: Refactor SSL test framework to support multiple TLS libraries

    Michael Paquier <michael@paquier.xyz> — 2021-04-01T01:02:52Z

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