Re: SSL Tests for sslinfo extension
> On 30 Nov 2021, at 00:16, Daniel Gustafsson wrote: >> On 29 Nov 2021, at 23:50, Tom Lane wrote: >> Otherwise I think it's good to go, so I marked it RFC. > > Great! I'll take another look over it tomorrow and will go ahead with it > then. I applied your nitpick diff and took it for another spin in CI, and pushed it. Thanks for review! -- Daniel Gustafsson https://vmware.com/
Re: SSL Tests for sslinfo extension
> On 29 Nov 2021, at 23:50, Tom Lane wrote: > > Daniel Gustafsson writes: >> Agreed. The attached v3 covers the issuer and extension function to at least >> some degree. In order to reliably test the extension I added a new cert >> with a >> CA extension. > > I have two remaining trivial nitpicks, for which I attach an 0004 > delta patch: the README change was fat-fingered slightly, and some > of the commentary about the key file seems now obsolete. Ah yes, thanks. > Otherwise I think it's good to go, so I marked it RFC. Great! I'll take another look over it tomorrow and will go ahead with it then. -- Daniel Gustafsson https://vmware.com/
Re: SSL Tests for sslinfo extension
Daniel Gustafsson writes: > Agreed. The attached v3 covers the issuer and extension function to at least > some degree. In order to reliably test the extension I added a new cert with > a > CA extension. I have two remaining trivial nitpicks, for which I attach an 0004 delta patch: the README change was fat-fingered slightly, and some of the commentary about the key file seems now obsolete. Otherwise I think it's good to go, so I marked it RFC. regards, tom lane diff --git a/src/test/ssl/README b/src/test/ssl/README index ca30f9329a..7e60700652 100644 --- a/src/test/ssl/README +++ b/src/test/ssl/README @@ -12,14 +12,12 @@ TCP connections on localhost. Any user on the same host is able to log in to the test server while the tests are running. Do not run this suite on a multi-user system where you don't trust all local users! -NOTE: You must have given the --enable-tap-tests argument to configure. -Also, to use "make installcheck", you must have built and installed -contrib/sslinfo in addition to the core code. - Running the tests = NOTE: You must have given the --enable-tap-tests argument to configure. +Also, to use "make installcheck", you must have built and installed +contrib/sslinfo in addition to the core code. Run make check diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl index 61b117e6c2..cf2e8dde0f 100644 --- a/src/test/ssl/t/003_sslinfo.pl +++ b/src/test/ssl/t/003_sslinfo.pl @@ -37,9 +37,6 @@ my $common_connstr; # The client's private key must not be world-readable, so take a copy # of the key stored in the code tree and update its permissions. -# -# This changes ssl/client.key to ssl/client_tmp.key etc for the rest -# of the tests. my $client_tmp_key = "${PostgreSQL::Test::Utils::tmp_check}/client_ext.key"; copy("ssl/client_ext.key", $client_tmp_key) or die "couldn't copy ssl/client_ext.key to $client_tmp_key for permissions change: $!";
Re: SSL Tests for sslinfo extension
> On 27 Nov 2021, at 20:27, Tom Lane wrote: > I don't have any problem with this structurally, but I do have a > few nitpicks: Thanks for reviewing! > * I think the error message added in 0001 should complain about > missing password "encryption" not "encoding", no? Doh, of course. > * 0002 hasn't been updated for the great PostgresNode renaming. Fixed. > * 0002 needs to extend src/test/ssl/README to mention that > "make installcheck" requires having installed contrib/sslinfo, > analogous to similar comments in (eg) src/test/recovery/README. Good point, I copied over the wording from recovery/README and adapted for SSL since I think it was well written as is. (Consistency is also a good benefit.) > * 0002 writes a temporary file in the source tree. This is bad; > for one thing I bet it fails under VPATH, but in any case there > is no reason to risk it. Put it in the tmp_check directory instead > (cf temp kdc files in src/test/kerberos/t/001_auth.pl). That's > safer and you needn't worry about cleaning it up. Fixed, and see below. > * Hmm ... now I notice that you borrowed the key-file-copying logic > from the 001 and 002 tests, but it's just as bad practice there. > We should fix them too. Well spotted, I hadn't thought about that but in hindsight it's quite obviously bad. I've done this in a 0003 patch in this series which also comes with the IMO benefit of a tighter coupling between the key filename used in the test with what's in the repo by removing the _tmp suffix. To avoid concatenating with the long tmp_check path variable everywhere, I went with a lookup HASH to make it easier on the eye and harder to mess up should we change tmp path at some point. There might be ways which are more like modern Perl, but I wasn't able to think of one off the bat. > * I ran a code-coverage check and it shows that this doesn't test > ssl_issuer_field() or any of the following functions in sslinfo.c. > I think at least ssl_extension_info() is complicated enough to > deserve a test. Agreed. The attached v3 covers the issuer and extension function to at least some degree. In order to reliably test the extension I added a new cert with a CA extension. -- Daniel Gustafsson https://vmware.com/ v3-0001-Extend-configure_test_server_for_ssl-to-add-exten.patch Description: Binary data v3-0002-Add-tests-for-sslinfo.patch Description: Binary data v3-0003-Use-test-specific-temp-path-for-keys-during-SSL-t.patch Description: Binary data
Re: SSL Tests for sslinfo extension
On Sat, Nov 27, 2021 at 02:27:19PM -0500, Tom Lane wrote: > I think testing sslinfo in src/test/ssl is fine, while putting its test > inside contrib/ would be dangerous, because then the test would be run > by default. src/test/ssl is not run by default because the server it > starts is potentially accessible by other local users, and AFAICS the > same has to be true for an sslinfo test. Ah, indeed, good point. I completely forgot that we'd better control this stuff with PG_TEST_EXTRA. -- Michael signature.asc Description: PGP signature
Re: SSL Tests for sslinfo extension
Daniel Gustafsson writes: > On 17 Jun 2021, at 09:29, Michael Paquier wrote: >> Hmm. Adding internal dependencies between the tests should be avoided >> if we can. What would it take to move those TAP tests to >> contrib/sslinfo instead? This is while keeping in mind that there was >> a patch aimed at refactoring the SSL test suite for NSS. > It would be quite invasive as we currently don't provide the SSLServer test > harness outside of src/test/ssl, and given how tailored it is today I'm not > sure doing so without a rewrite would be a good idea. I think testing sslinfo in src/test/ssl is fine, while putting its test inside contrib/ would be dangerous, because then the test would be run by default. src/test/ssl is not run by default because the server it starts is potentially accessible by other local users, and AFAICS the same has to be true for an sslinfo test. So I don't have any problem with this structurally, but I do have a few nitpicks: * I think the error message added in 0001 should complain about missing password "encryption" not "encoding", no? * 0002 hasn't been updated for the great PostgresNode renaming. * 0002 needs to extend src/test/ssl/README to mention that "make installcheck" requires having installed contrib/sslinfo, analogous to similar comments in (eg) src/test/recovery/README. * 0002 writes a temporary file in the source tree. This is bad; for one thing I bet it fails under VPATH, but in any case there is no reason to risk it. Put it in the tmp_check directory instead (cf temp kdc files in src/test/kerberos/t/001_auth.pl). That's safer and you needn't worry about cleaning it up. * Hmm ... now I notice that you borrowed the key-file-copying logic from the 001 and 002 tests, but it's just as bad practice there. We should fix them too. * I ran a code-coverage check and it shows that this doesn't test ssl_issuer_field() or any of the following functions in sslinfo.c. I think at least ssl_extension_info() is complicated enough to deserve a test. regards, tom lane
Re: SSL Tests for sslinfo extension
> On 17 Jun 2021, at 09:29, Michael Paquier wrote: > > On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote: >>> On 19 May 2021, at 21:05, Andrew Dunstan wrote: >>> >>> On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: Daniel Gustafsson writes: > In order to be able to test extensions with SSL connections, allow > configure_test_server_for_ssl to create any extensions passed as > comma separated list. Each extension is created in all the test > databases which may or may not be useful. Why the comma-separated string, rather than an array reference, i.e. `extensions => [qw(foo bar baz)]`? >> >> No real reason, I just haven't written Perl enough lately to "think in Perl". >> Fixed in the attached. > > Hmm. Adding internal dependencies between the tests should be avoided > if we can. What would it take to move those TAP tests to > contrib/sslinfo instead? This is while keeping in mind that there was > a patch aimed at refactoring the SSL test suite for NSS. It would be quite invasive as we currently don't provide the SSLServer test harness outside of src/test/ssl, and given how tailored it is today I'm not sure doing so without a rewrite would be a good idea. A longer term solution would probably be to teach PostgresNode to provide an instance set up for TLS in case the backend is compiled with TLS support, and use that for things like sslinfo. -- Daniel Gustafsson https://vmware.com/
Re: SSL Tests for sslinfo extension
On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote: > > On 19 May 2021, at 21:05, Andrew Dunstan wrote: > > > > On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: > >> Daniel Gustafsson writes: > >> > >>> In order to be able to test extensions with SSL connections, allow > >>> configure_test_server_for_ssl to create any extensions passed as > >>> comma separated list. Each extension is created in all the test > >>> databases which may or may not be useful. > >> Why the comma-separated string, rather than an array reference, > >> i.e. `extensions => [qw(foo bar baz)]`? > > No real reason, I just haven't written Perl enough lately to "think in Perl". > Fixed in the attached. Hmm. Adding internal dependencies between the tests should be avoided if we can. What would it take to move those TAP tests to contrib/sslinfo instead? This is while keeping in mind that there was a patch aimed at refactoring the SSL test suite for NSS. -- Michael signature.asc Description: PGP signature
Re: SSL Tests for sslinfo extension
> On 19 May 2021, at 21:05, Andrew Dunstan wrote: > > On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: >> Daniel Gustafsson writes: >> >>> In order to be able to test extensions with SSL connections, allow >>> configure_test_server_for_ssl to create any extensions passed as >>> comma separated list. Each extension is created in all the test >>> databases which may or may not be useful. >> Why the comma-separated string, rather than an array reference, >> i.e. `extensions => [qw(foo bar baz)]`? No real reason, I just haven't written Perl enough lately to "think in Perl". Fixed in the attached. >> Also, should it use `CREATE >> EXTENSION .. CASCADE`, in case the specified extensions depend on >> others? Good point. Each extension will have to be in EXTRA_INSTALL as well of course, but we should to CASCADE. > Also, instead of one line per db there should be an inner loop over the > db names. Right, I was lazily using the same approach as for CREATE DATABASE but when the list is used it two places it should be a proper list. Fixed in the attached. -- Daniel Gustafsson https://vmware.com/ v2-0001-Extend-configure_test_server_for_ssl-to-add-exten.patch Description: Binary data v2-0002-Add-tests-for-sslinfo.patch Description: Binary data
Re: SSL Tests for sslinfo extension
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote: > Daniel Gustafsson writes: > >> In order to be able to test extensions with SSL connections, allow >> configure_test_server_for_ssl to create any extensions passed as >> comma separated list. Each extension is created in all the test >> databases which may or may not be useful. > Why the comma-separated string, rather than an array reference, > i.e. `extensions => [qw(foo bar baz)]`? Also, should it use `CREATE > EXTENSION .. CASCADE`, in case the specified extensions depend on > others? > Also, instead of one line per db there should be an inner loop over the db names. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: SSL Tests for sslinfo extension
Daniel Gustafsson writes: > In order to be able to test extensions with SSL connections, allow > configure_test_server_for_ssl to create any extensions passed as > comma separated list. Each extension is created in all the test > databases which may or may not be useful. Why the comma-separated string, rather than an array reference, i.e. `extensions => [qw(foo bar baz)]`? Also, should it use `CREATE EXTENSION .. CASCADE`, in case the specified extensions depend on others? - ilmari -- "The surreality of the universe tends towards a maximum" -- Skud's Law "Never formulate a law or axiom that you're not prepared to live with the consequences of." -- Skud's Meta-Law
SSL Tests for sslinfo extension
While playing around with the recent SSL testharness changes I wrote a test suite for sslinfo as a side effect, which seemed valuable in its own right as we currently have no coverage of this code. The small change needed to the testharness is to support installing modules, which is broken out into 0001 for easier reading. I'll park this in the next commitfest for now. -- Daniel Gustafsson https://vmware.com/ 0002-Add-tests-for-sslinfo.patch Description: Binary data 0001-Extend-configure_test_server_for_ssl-to-add-extensio.patch Description: Binary data