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/
Attachments:
0002-Add-tests-for-sslinfo.patchapplication/octet-stream; name=0002-Add-tests-for-sslinfo.patch; x-unix-mode=0644Download+131-1
0001-Extend-configure_test_server_for_ssl-to-add-extensio.patchapplication/octet-stream; name=0001-Extend-configure_test_server_for_ssl-to-add-extensio.patch; x-unix-mode=0644Download+23-9
Daniel Gustafsson <daniel@yesql.se> 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
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote:
Daniel Gustafsson <daniel@yesql.se> 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
On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote:
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote:
Daniel Gustafsson <daniel@yesql.se> 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/
Attachments:
v2-0001-Extend-configure_test_server_for_ssl-to-add-exten.patchapplication/octet-stream; name=v2-0001-Extend-configure_test_server_for_ssl-to-add-exten.patch; x-unix-mode=0644Download+28-15
v2-0002-Add-tests-for-sslinfo.patchapplication/octet-stream; name=v2-0002-Add-tests-for-sslinfo.patch; x-unix-mode=0644Download+131-1
On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote:
On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote:
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote:
Daniel Gustafsson <daniel@yesql.se> 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
On 17 Jun 2021, at 09:29, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, May 20, 2021 at 08:40:48PM +0200, Daniel Gustafsson wrote:
On 19 May 2021, at 21:05, Andrew Dunstan <andrew@dunslane.net> wrote:
On 5/19/21 1:01 PM, Dagfinn Ilmari Mannsåker wrote:
Daniel Gustafsson <daniel@yesql.se> 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/
Daniel Gustafsson <daniel@yesql.se> writes:
On 17 Jun 2021, at 09:29, Michael Paquier <michael@paquier.xyz> 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
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
On 27 Nov 2021, at 20:27, Tom Lane <tgl@sss.pgh.pa.us> 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/
Attachments:
v3-0001-Extend-configure_test_server_for_ssl-to-add-exten.patchapplication/octet-stream; name=v3-0001-Extend-configure_test_server_for_ssl-to-add-exten.patch; x-unix-mode=0644Download+28-15
v3-0002-Add-tests-for-sslinfo.patchapplication/octet-stream; name=v3-0002-Add-tests-for-sslinfo.patch; x-unix-mode=0644Download+210-2
v3-0003-Use-test-specific-temp-path-for-keys-during-SSL-t.patchapplication/octet-stream; name=v3-0003-Use-test-specific-temp-path-for-keys-during-SSL-t.patch; x-unix-mode=0644Download+42-50
Daniel Gustafsson <daniel@yesql.se> 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
Attachments:
v3-0004-nitpicks.patchtext/x-diff; charset=us-ascii; name=v3-0004-nitpicks.patchDownload+2-7
On 29 Nov 2021, at 23:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> 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/
On 30 Nov 2021, at 00:16, Daniel Gustafsson <daniel@yesql.se> wrote:
On 29 Nov 2021, at 23:50, Tom Lane <tgl@sss.pgh.pa.us> 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/