Thread: Refactoring SSL tests
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/
Attachment
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
> 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/
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
Attachment
> 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/
Attachment
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
> 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/
Attachment
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
> 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/