pgsql: Add TAP tests for contrib/sslinfo

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 private key 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=ssltestuser sslcert=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.pm line 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 private key 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=ssltestuser sslcert=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.pm line 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/

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 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.
--
Michael

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/

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

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.

+When passing a path from the Perl test code to PostgreSQL, like for example
+the path to an SSL certificate, it must be converted for use on the host
+system by using PostgreSQL::Test::Utils::perl2host.

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. It would be good
to add some examples of what that is, I guess. Until now, I think
that we have seen:
- Tablespace paths in SQL queries.
- Paths passed down as function arguments, processed in system calls
by frontends (like pg_basebackup and its tablespace map).
- Paths part of connection strings, processed in system calls in
libpq.
--
Michael

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

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

Greetings,

Andres Freund

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 not private 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/

Hi,

On 2021-12-02 15:59:44 -0500, Andrew Dunstan wrote:

On 12/2/21 14:51, Andres Freund wrote:

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.

I meant whether the tests as a whole are expected to work with msvc after the
current set of fixes - because they don't for me. Not whether perl2host is the
problem. Sorry for the confusion...

Greetings,

Andres Freund

Hi,

On 2021-12-02 22:07:17 +0100, Daniel Gustafsson wrote:

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.

I really want to get the CI stuff merged so stuff like this is easier going
forward... FWIW, I have a new colleague working on installing more of the
optional dependencies for windows - but we can add those later.

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 not private 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:

Hm. Isn't the problem that we are not adding enough of the other path
delimiters? The /client.key seems fine, the problem is that all the other
delimiters were swallowed. Or is that what you mean?

I think the difference might be that the most of the other tests do not pass
such paths to postgres via postgresql.conf (based on a *very* cursory grep). I
assume the guc machinery treats the \ as an escape, and that's why you're
seeing c:cirrussrc... instead of c:\cirrus\src\...

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.

And this works because it uses *forward* slashes instad of backward slashes,
which then do not get escaped by the guc machinery.

Greetings,

Andres Freund