Horton

Thread

Commits

  1. SSL TAP test backend library independence refactoring

  1. Refactoring SSL tests

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-02T13:26:02Z

    As part of the NSS patchset (and Secure Transport before that), I had to
refactor the SSL tests to handle different SSL libraries.  The current tests
and test module is quite tied to how OpenSSL works wrt setting up the server,
the attached refactors this and abstracts the OpenSSL specifics more like how
the rest of the codebase is set up.

The tight coupling is of course not a problem right now, but I think this patch
has more benefits making it a candidate for going in regardless of the fate of
the NSS patchset.  This is essentially the 0002 patch from that patchset with
additional cleanup and documentation:

* switch_server_cert takes a set of named parameters rather than a fixed set
with defaults depending on each other, which made adding ssl_passphrase to it
cumbersome. It also adds readability IMO.

* SSLServer is renamed SSL::Server, which in turn use SSL::Backend::X where X
is the backend pointed to by with_ssl.  Each backend will implement its own
module which is responsible for setting up keys/certs and to resolve sslkey
values to their full paths.  The idea is that the namespace will also allow for
an SSL::Client in the future when we implment running client tests against
different servers etc.

* The modules are POD documented.

* While not related to the refactor per se, the hardcoded number of planned
tests is removed in favor of calling done_testing().

With this, adding a new SSL library is quite straightforward, I've done the
legwork to test that =)

I opted for avoiding too invasive changes leaving the tests somewhat easy to
compare to back branches.

Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
I'm happy to take pointers on how to improve that.

--
Daniel Gustafsson		https://vmware.com/

  2. Re: Refactoring SSL tests

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-02T16:09:35Z

    On 2/2/22 08:26, Daniel Gustafsson wrote:
> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
> I'm happy to take pointers on how to improve that.


It feels a bit odd to me from a perl POV. I think it needs to more along
the lines of standard OO patterns. I'll take a stab at that based on
this, might be a few days.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  3. Re: Refactoring SSL tests

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-02T19:50:05Z

    > On 2 Feb 2022, at 17:09, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2/2/22 08:26, Daniel Gustafsson wrote:

>> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
>> I'm happy to take pointers on how to improve that.
> 
> It feels a bit odd to me from a perl POV. I think it needs to more along
> the lines of standard OO patterns. I'll take a stab at that based on
> this, might be a few days.

That would be great, thanks!

--
Daniel Gustafsson		https://vmware.com/

  4. Re: Refactoring SSL tests

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-07T16:29:54Z

    On 2/2/22 14:50, Daniel Gustafsson wrote:
>> On 2 Feb 2022, at 17:09, Andrew Dunstan <andrew@dunslane.net> wrote:
>> On 2/2/22 08:26, Daniel Gustafsson wrote:
>>> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
>>> I'm happy to take pointers on how to improve that.
>> It feels a bit odd to me from a perl POV. I think it needs to more along
>> the lines of standard OO patterns. I'll take a stab at that based on
>> this, might be a few days.
> That would be great, thanks!
>

Here's the result of that surgery.  It's a little incomplete in that it
needs some POD adjustment, but I think the code is right - it passes
testing for me.

One of the advantages of this, apart from being more idiomatic, is that
by avoiding the use of package level variables you can have two
SSL::Server objects, one for OpenSSL and (eventually) one for NSS. This
was the original motivation for the recent install_path additions to
PostgreSQL::Test::Cluster, so it complements that work nicely.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  5. Re: Refactoring SSL tests

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-08T14:24:13Z

    > On 7 Feb 2022, at 17:29, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2/2/22 14:50, Daniel Gustafsson wrote:

>>> On 2 Feb 2022, at 17:09, Andrew Dunstan <andrew@dunslane.net> wrote:
>>> On 2/2/22 08:26, Daniel Gustafsson wrote:
>>>> Thoughts?  I'm fairly sure there are many crimes against Perl in this patch,
>>>> I'm happy to take pointers on how to improve that.
>>> It feels a bit odd to me from a perl POV. I think it needs to more along
>>> the lines of standard OO patterns. I'll take a stab at that based on
>>> this, might be a few days.
>> That would be great, thanks!
> 
> Here's the result of that surgery.  It's a little incomplete in that it
> needs some POD adjustment, but I think the code is right - it passes
> testing for me.

Confirmed, it passes all tests for me as well.

> One of the advantages of this, apart from being more idiomatic, is that
> by avoiding the use of package level variables you can have two
> SSL::Server objects, one for OpenSSL and (eventually) one for NSS. This
> was the original motivation for the recent install_path additions to
> PostgreSQL::Test::Cluster, so it complements that work nicely.

Agreed, this version is a clear improvement over my attempt. Thanks!

The attached v2 takes a stab at fixing up the POD sections.

--
Daniel Gustafsson		https://vmware.com/

  6. Re: Refactoring SSL tests

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-08T15:46:29Z

    On 2/8/22 09:24, Daniel Gustafsson wrote:
>
> The attached v2 takes a stab at fixing up the POD sections.


There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
that's my fault:


+  my $backend = SSL::backend::OpenSSL->new();


Also, I think we should document that SSL::Server::new() takes an
optional flavor parameter, in the absence of which it uses
$ENV{with_ssl}, and that 'openssl' is the only currently supported value.


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

  7. Re: Refactoring SSL tests

    Daniel Gustafsson <daniel@yesql.se> — 2022-02-09T13:11:30Z

    > On 8 Feb 2022, at 16:46, Andrew Dunstan <andrew@dunslane.net> wrote:

> There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
> that's my fault:
> 
> +  my $backend = SSL::backend::OpenSSL->new();

Fixed.

> Also, I think we should document that SSL::Server::new() takes an
> optional flavor parameter, in the absence of which it uses
> $ENV{with_ssl}, and that 'openssl' is the only currently supported value.

Good point, done in the attached.

--
Daniel Gustafsson		https://vmware.com/

  8. Re: Refactoring SSL tests

    Andrew Dunstan <andrew@dunslane.net> — 2022-02-09T13:28:59Z

    On 2/9/22 08:11, Daniel Gustafsson wrote:
>> On 8 Feb 2022, at 16:46, Andrew Dunstan <andrew@dunslane.net> wrote:
>> There a capitalization typo in SSL/Backend/OpenSSL.pm - looks like
>> that's my fault:
>>
>> +  my $backend = SSL::backend::OpenSSL->new();
> Fixed.
>
>> Also, I think we should document that SSL::Server::new() takes an
>> optional flavor parameter, in the absence of which it uses
>> $ENV{with_ssl}, and that 'openssl' is the only currently supported value.
> Good point, done in the attached.



LGTM


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  9. Re: Refactoring SSL tests

    Daniel Gustafsson <daniel@yesql.se> — 2022-03-26T21:08:25Z

    > On 9 Feb 2022, at 14:28, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2/9/22 08:11, Daniel Gustafsson wrote:

>> Good point, done in the attached.
> 
> LGTM

Now that the recent changes to TAP and SSL tests have settled, I took another
pass at this.  After rebasing and fixing and polishing and taking it for
multiple spins on the CI I've pushed this today to master.

--
Daniel Gustafsson		https://vmware.com/