Thread: pgsql: Add TAP tests for contrib/sslinfo
Add TAP tests for contrib/sslinfo This adds rudimentary coverage of the sslinfo extension into the SSL test harness. The output is validated by comparing with pg_stat_ssl to provide some level of test stability should the underlying certs be slightly altered. A new cert is added to provide an extension to test against. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Andrew Dunstan <andrew@dunslane.net> Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> Discussion: https://postgr.es/m/E23F9811-0C77-45DA-912F-D809AB140741@yesql.se Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ae81776a23f78babc9707e22f95dea15aa2dbcd2 Modified Files -------------- src/test/ssl/Makefile | 2 + src/test/ssl/README | 2 + src/test/ssl/conf/client_ext.config | 16 +++++ src/test/ssl/ssl/client_ext.crt | 21 ++++++ src/test/ssl/ssl/client_ext.key | 28 ++++++++ src/test/ssl/sslfiles.mk | 2 +- src/test/ssl/t/003_sslinfo.pl | 134 ++++++++++++++++++++++++++++++++++++ 7 files changed, 204 insertions(+), 1 deletion(-)
> On 30 Nov 2021, at 11:50, Daniel Gustafsson <dgustafsson@postgresql.org> wrote: > > Add TAP tests for contrib/sslinfo This just failed on fairywren with the below: not ok 1 - certificate authorization succeeds with correct client cert in PEM format # Failed test 'certificate authorization succeeds with correct client cert in PEM format' # at t/003_sslinfo.pl line 71. # got: '2' # expected: '0' connection error: 'psql: error: connection to server at "127.0.0.1", port 60695 failed: certificate present, but not privatekey file "/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/ssl/tmp_check/client_ext.key"' while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=127.0.0.1 user=ssltestusersslcert=ssl/client_ext.crt sslkey=/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/ssl/tmp_check/client_ext.key-f - -v ON_ERROR_STOP=1' at /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Cluster.pmline 1807. ..which is odd, since the key was copied to tmp_check earlier in the test and passed that. The 001 and 002 tests do the same kind of copying and there it worked, so it seems a tad odd. Does anyone have any ideas on insights here? -- Daniel Gustafsson https://vmware.com/
> On 30 Nov 2021, at 17:40, Daniel Gustafsson <daniel@yesql.se> wrote: > >> On 30 Nov 2021, at 11:50, Daniel Gustafsson <dgustafsson@postgresql.org> wrote: >> >> Add TAP tests for contrib/sslinfo > > This just failed on fairywren with the below: > > not ok 1 - certificate authorization succeeds with correct client cert in PEM format > > # Failed test 'certificate authorization succeeds with correct client cert in PEM format' > # at t/003_sslinfo.pl line 71. > # got: '2' > # expected: '0' > connection error: 'psql: error: connection to server at "127.0.0.1", port 60695 failed: certificate present, but not privatekey file "/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/ssl/tmp_check/client_ext.key"' > while running 'psql -XAtq -d sslrootcert=ssl/root+server_ca.crt sslmode=require dbname=certdb hostaddr=127.0.0.1 user=ssltestusersslcert=ssl/client_ext.crt sslkey=/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/ssl/tmp_check/client_ext.key-f - -v ON_ERROR_STOP=1' at /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Cluster.pmline 1807. > > ..which is odd, since the key was copied to tmp_check earlier in the test and > passed that. The 001 and 002 tests do the same kind of copying and there it > worked, so it seems a tad odd. Does anyone have any ideas on insights here? Scratch that, all the copying for tests 001 through 003 had failed. I clearly need another coffee. The question still stands though, does anyone have any ideas on what could've happened as I'm currently drawing a blank? -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> Scratch that, all the copying for tests 001 through 003 had failed. I clearly
> need another coffee.
> The question still stands though, does anyone have any ideas on what could've
> happened as I'm currently drawing a blank?
Dunno, but it strikes me that the libpq code issuing this error is not up
to our usual quality standards. It's just assuming that the stat()
failure is ENOENT, and I have a sneaking suspicion that that's not so.
I'm inclined to suggest that we start by changing that code
to look like, say,
if (stat(fnbuf, &buf) != 0)
{
if (errno == ENOENT)
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("certificate present, but not private key file \"%s\"\n"),
fnbuf);
else
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not stat private key file \"%s\": %m\n"),
fnbuf);
return -1;
}
and see what we learn.
regards, tom lane
> On 30 Nov 2021, at 21:55, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> Scratch that, all the copying for tests 001 through 003 had failed. I clearly >> need another coffee. >> The question still stands though, does anyone have any ideas on what could've >> happened as I'm currently drawing a blank? > > Dunno, but it strikes me that the libpq code issuing this error is not up > to our usual quality standards. It's just assuming that the stat() > failure is ENOENT, and I have a sneaking suspicion that that's not so. > > I'm inclined to suggest that we start by changing that code > to look like, say, > > if (stat(fnbuf, &buf) != 0) > ... > > and see what we learn. That seems like a change worthy of doing regardless, so +1 on trying this. We can't use %m in frontend though can we? Isn't using strerror_r() like in the attached what we need to do? If so I can go ahead and do that, and it shouldn't make the buildfarm any worse off than it is, and ideally we'll get clues as to why msys is happy to copy the files with Perl but not read them from C. -- Daniel Gustafsson https://vmware.com/
Attachment
Daniel Gustafsson <daniel@yesql.se> writes:
> That seems like a change worthy of doing regardless, so +1 on trying this. We
> can't use %m in frontend though can we?
Yes we can, since v12 or whenever we started using our own snprintf always.
regards, tom lane
> On 30 Nov 2021, at 22:36, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <daniel@yesql.se> writes: >> That seems like a change worthy of doing regardless, so +1 on trying this. We >> can't use %m in frontend though can we? > > Yes we can, since v12 or whenever we started using our own snprintf always. Oh, interesting, I hadn't realized that. I'll go do that instead then. -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> Oh, interesting, I hadn't realized that. I'll go do that instead then.
... okay, so all we learned is that it really is an ENOENT failure.
At this point my guess is that the test is copying the key file
to the wrong place because of an MSys path issue. I don't know
that topic well enough to debug it, though.
regards, tom lane
> On 1 Dec 2021, at 07:19, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Nov 30, 2021 at 11:34:21PM -0500, Tom Lane wrote:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>> Oh, interesting, I hadn't realized that. I'll go do that instead then.
>>
>> ... okay, so all we learned is that it really is an ENOENT failure.
>>
>> At this point my guess is that the test is copying the key file
>> to the wrong place because of an MSys path issue. I don't know
>> that topic well enough to debug it, though.
>
> Daniel has pinged me about this issue. From what I can see, c113d8ad
> has changed 001_ssltests.pl so as the keys are not anymore relative
> paths with *_tmp* names but absolute paths with the same key name, so
> it seems to me that you should sprinkle some perl2host() calls in the
> ${PostgreSQL::Test::Utils::tmp_check} paths to allow Msys to
> understand them. At quick glance, it looks like this is the first
> time we'd pass down absolute paths to libpq within connection
> strings in the TAP tests.
I think that's a very plausible explanation, I'm currently experimenting with a
patch that I will soon apply hoping that it will remedy the msys issue. If it
doesn't, I'll revert to copying inside ssl/ as before to return to the
drawingboard for a proper fix.
--
Daniel Gustafsson https://vmware.com/
> On 1 Dec 2021, at 12:49, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 1 Dec 2021, at 07:19, Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Tue, Nov 30, 2021 at 11:34:21PM -0500, Tom Lane wrote:
>>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>> Oh, interesting, I hadn't realized that. I'll go do that instead then.
>>>
>>> ... okay, so all we learned is that it really is an ENOENT failure.
>>>
>>> At this point my guess is that the test is copying the key file
>>> to the wrong place because of an MSys path issue. I don't know
>>> that topic well enough to debug it, though.
>>
>> Daniel has pinged me about this issue. From what I can see, c113d8ad
>> has changed 001_ssltests.pl so as the keys are not anymore relative
>> paths with *_tmp* names but absolute paths with the same key name, so
>> it seems to me that you should sprinkle some perl2host() calls in the
>> ${PostgreSQL::Test::Utils::tmp_check} paths to allow Msys to
>> understand them. At quick glance, it looks like this is the first
>> time we'd pass down absolute paths to libpq within connection
>> strings in the TAP tests.
>
> I think that's a very plausible explanation, I'm currently experimenting with a
> patch that I will soon apply hoping that it will remedy the msys issue. If it
> doesn't, I'll revert to copying inside ssl/ as before to return to the
> drawingboard for a proper fix.
Looks like perl2host() was the missing piece, fairywren turned green with
commit c3b34a0ff4. Thanks for pointing me in the right direction, I will draft
a small paragraph on this to the TAP test README for other to learn from.
--
Daniel Gustafsson https://vmware.com/
> On 1 Dec 2021, at 20:49, Daniel Gustafsson <daniel@yesql.se> wrote: > Looks like perl2host() was the missing piece, fairywren turned green with > commit c3b34a0ff4. Thanks for pointing me in the right direction, I will draft > a small paragraph on this to the TAP test README for other to learn from. Would it make sense to add something like the attached to the Portability section in the src/test/perl/README? It definitely would've helped me with this particular issue, but that's admittedly a pretty limited samplesize. -- Daniel Gustafsson https://vmware.com/
Attachment
Daniel Gustafsson <daniel@yesql.se> writes:
> Would it make sense to add something like the attached to the Portability
> section in the src/test/perl/README? It definitely would've helped me with
> this particular issue, but that's admittedly a pretty limited samplesize.
I agree that some docs about this would be nice, but I think the rules
for when to use perl2host are more complex than what you suggest here.
Perhaps Andrew can clarify.
regards, tom lane
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Dec 01, 2021 at 04:04:09PM -0500, Tom Lane wrote:
>> I agree that some docs about this would be nice, but I think the rules
>> for when to use perl2host are more complex than what you suggest here.
>> Perhaps Andrew can clarify.
> It seems to me that the rule applies to any paths passed down to the C
> code of Postgres, then processed by a system call.
I'm a little gun-shy of any sweeping statements here, because of
bad experience (27ab1981e, b35a67bc0). It's as important to be
clear about when *not* to use perl2host as when to do so.
regards, tom lane
On 12/2/21 14:51, Andres Freund wrote: > Hi, > > On 2021-12-01 20:49:21 +0100, Daniel Gustafsson wrote: >> Looks like perl2host() was the missing piece, fairywren turned green with >> commit c3b34a0ff4. > Does that work with MSVC? I rebased my CI patch ontop of this, and it fails on > windows: > https://cirrus-ci.com/task/6093088335593472?logs=ssl_test#L5 > No, it's a no-op everywhere but MSys. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 2 Dec 2021, at 20:51, Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-12-01 20:49:21 +0100, Daniel Gustafsson wrote: >> Looks like perl2host() was the missing piece, fairywren turned green with >> commit c3b34a0ff4. > > Does that work with MSVC? I rebased my CI patch ontop of this, and it fails on > windows: > https://cirrus-ci.com/task/6093088335593472?logs=ssl_test#L5 I was under the impression that it did, but it seems that my Appveyor scripts had a bug which hid that =( Sorry for that. This seems to be another case of Perl being perfectly happy to copy and manage the files, but Postgres not being able to read them since we'd otherwise die() a lot sooner. The error message is the same as when the msys directory wasn't fixed with perl2host: [04:05:47.204] # got: 'psql: error: connection to server at "127.0.0.1", port 59360 failed: certificate present, but notprivate key file "C:cirrussrctestssltmp_checktmp_test_AIn7/client.key" Looking at other tests, we pass a tempdir() to postgres just fine, so that clearly works. The one difference here is that the filename is added with "/client.key", so I have a feeling this is a delimiter issue where Windows expects a backslash? In src/test/recovery/t/025_stuck_on_old_timeline.pl we do the following, which IMO would be neat if perl2host (or a similar function) would do for us: my $archivedir_primary = $node_primary->archive_dir; $archivedir_primary =~ s!\\!/!g if $PostgreSQL::Test::Utils::windows_os; $node_primary->append_conf( 'postgresql.conf', qq( archive_command = '"$perlbin" "$FindBin::RealBin/cp_history_files" "%p" "$archivedir_primary/%f"' wal_keep_size=128MB )); I'll fix it, or revert if I can't make it work. Sorry for the breakage. -- Daniel Gustafsson https://vmware.com/
> On 2 Dec 2021, at 22:21, Andres Freund <andres@anarazel.de> wrote: > And this works because it uses *forward* slashes instad of backward slashes, > which then do not get escaped by the guc machinery. Correct. Applying the same fix to the src/test/ssl/t/00X (per the attached) makes the ssl tests pass in MSVC when using your Cirrus CI file from the "Add CI" thread. -- Daniel Gustafsson https://vmware.com/
Attachment
> On 2 Dec 2021, at 23:54, Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-12-02 23:34:06 +0100, Daniel Gustafsson wrote: >>> On 2 Dec 2021, at 22:21, Andres Freund <andres@anarazel.de> wrote: >> >>> And this works because it uses *forward* slashes instad of backward slashes, >>> which then do not get escaped by the guc machinery. >> >> Correct. Applying the same fix to the src/test/ssl/t/00X (per the attached) >> makes the ssl tests pass in MSVC when using your Cirrus CI file from the "Add >> CI" thread. > > Cool. I guess you're coming to commit something like that? Thats my plan. Tomorrow is rapidly becoming today here though so I'd prefer to tend to it first thing in the morning to not risk (more) breakage while being unable to deal with it. -- Daniel Gustafsson https://vmware.com/
> On 2 Dec 2021, at 23:54, Andres Freund <andres@anarazel.de> wrote: > Cool. I guess you're coming to commit something like that? After another successful pass with MSVC in the CI setup I've pushed this now. -- Daniel Gustafsson https://vmware.com/