Thread: 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-6BD9336D630A@yesql.se
Attachment
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/
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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