Re: Using AF_UNIX sockets always for tests on Windows
On Sun, 21 Jan 2024 at 18:01, vignesh C wrote: > > On Wed, 22 Mar 2023 at 09:59, Thomas Munro wrote: > > > > Thanks Tom, Andres, Juan José, Andrew for your feedback. I agree that > > it's a better "OS harmonisation" outcome if we can choose both ways on > > both OSes. I will leave the 0003 patch aside for now due to lack of > > time, but here's a rebase of the first two patches. Since this is > > really just more cleanup-obsolete-stuff background work, I'm going to > > move it to the next CF. > > > > 0001 -- Teach mkdtemp() to care about permissions on Windows. > > Reviewed by Juan José, who confirmed my blind-coded understanding and > > showed an alternative version but didn't suggest doing it that way. > > It's a little confusing that NULL "attributes" means something > > different from NULL "descriptor", so I figured I should highlight that > > difference more clearly with some new comments. I guess one question > > is "why should we expect the calling process to have the default > > access token?" > > > > 0002 -- Use AF_UNIX for pg_regress. This one removes a couple of > > hundred Windows-only lines that set up SSPI stuff, and some comments > > about a signal handling hazard that -- as far as I can tell by reading > > manuals -- was never really true. > > > > 0003 -- TAP testing adjustments needs some more work based on feedback > > already given, not included in this revision. > > The patch does not apply anymore: > patch -p1 < v3-0002-Always-use-AF_UNIX-sockets-in-pg_regress-on-Windo.patch > patching file src/test/regress/pg_regress.c > ... > Hunk #6 FAILED at 781. > ... > 1 out of 10 hunks FAILED -- saving rejects to file > src/test/regress/pg_regress.c.rej With no update to the thread and the patch not applying I'm marking this as returned with feedback. Please feel free to resubmit to the next CF when there is a new version of the patch. Regards, Vignesh
Re: Using AF_UNIX sockets always for tests on Windows
On Wed, 22 Mar 2023 at 09:59, Thomas Munro wrote: > > Thanks Tom, Andres, Juan José, Andrew for your feedback. I agree that > it's a better "OS harmonisation" outcome if we can choose both ways on > both OSes. I will leave the 0003 patch aside for now due to lack of > time, but here's a rebase of the first two patches. Since this is > really just more cleanup-obsolete-stuff background work, I'm going to > move it to the next CF. > > 0001 -- Teach mkdtemp() to care about permissions on Windows. > Reviewed by Juan José, who confirmed my blind-coded understanding and > showed an alternative version but didn't suggest doing it that way. > It's a little confusing that NULL "attributes" means something > different from NULL "descriptor", so I figured I should highlight that > difference more clearly with some new comments. I guess one question > is "why should we expect the calling process to have the default > access token?" > > 0002 -- Use AF_UNIX for pg_regress. This one removes a couple of > hundred Windows-only lines that set up SSPI stuff, and some comments > about a signal handling hazard that -- as far as I can tell by reading > manuals -- was never really true. > > 0003 -- TAP testing adjustments needs some more work based on feedback > already given, not included in this revision. The patch does not apply anymore: patch -p1 < v3-0002-Always-use-AF_UNIX-sockets-in-pg_regress-on-Windo.patch patching file src/test/regress/pg_regress.c ... Hunk #6 FAILED at 781. ... 1 out of 10 hunks FAILED -- saving rejects to file src/test/regress/pg_regress.c.rej Regards, Vignesh
Re: Using AF_UNIX sockets always for tests on Windows
Thanks Tom, Andres, Juan José, Andrew for your feedback. I agree that
it's a better "OS harmonisation" outcome if we can choose both ways on
both OSes. I will leave the 0003 patch aside for now due to lack of
time, but here's a rebase of the first two patches. Since this is
really just more cleanup-obsolete-stuff background work, I'm going to
move it to the next CF.
0001 -- Teach mkdtemp() to care about permissions on Windows.
Reviewed by Juan José, who confirmed my blind-coded understanding and
showed an alternative version but didn't suggest doing it that way.
It's a little confusing that NULL "attributes" means something
different from NULL "descriptor", so I figured I should highlight that
difference more clearly with some new comments. I guess one question
is "why should we expect the calling process to have the default
access token?"
0002 -- Use AF_UNIX for pg_regress. This one removes a couple of
hundred Windows-only lines that set up SSPI stuff, and some comments
about a signal handling hazard that -- as far as I can tell by reading
manuals -- was never really true.
0003 -- TAP testing adjustments needs some more work based on feedback
already given, not included in this revision.
From e7fac7a15ed0eda6516e7fa0917c06e005341b00 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Wed, 7 Sep 2022 07:35:11 +1200
Subject: [PATCH v3 1/2] Make mkdtemp() more secure on Windows.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Our POSIX mkdtemp() implementation in src/port/mkdtemp.c code would
create directories with default permissions on Windows. Fix, using the
native Windows API instead of mkdir().
This function is currently used by pg_regress's make_temp_sockdir().
Reviewed-by: Juan José Santamaría Flecha
Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
src/port/mkdtemp.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
index 4578e8384c..9d3c4fce71 100644
--- a/src/port/mkdtemp.c
+++ b/src/port/mkdtemp.c
@@ -187,8 +187,35 @@ GETTEMP(char *path, int *doopen, int domkdir)
}
else if (domkdir)
{
+#ifdef WIN32
+ /*
+ * Plain mkdir(path, 0700) would ignore the mode argument, so
+ * we'll use the native Windows API to create the directory. By
+ * setting lpSecurityDescriptor to NULL, we get "the default
+ * security descriptor associated with the access token of the
+ * calling process. [...] By default, the default DACL in the
+ * access token of a process allows access only to the user
+ * represented by the access token."
+ *
+ * Note that a NULL lpSecurityDescriptor is not the same as a NULL
+ * lpSecurityAttributes argument. The latter would mean that the
+ * ACL is inherited from the parent directory, which would
+ * probably work out the same if it's the TMP directory, but by a
+ * different route.
+ */
+ SECURITY_ATTRIBUTES sa = {
+.nLength = sizeof(SECURITY_ATTRIBUTES),
+.lpSecurityDescriptor = NULL,
+.bInheritHandle = false
+ };
+
+ if (CreateDirectory(path, &sa))
+return 1;
+ _dosmaperr(GetLastError());
+#else
if (mkdir(path, 0700) >= 0)
return 1;
+#endif
if (errno != EEXIST)
return 0;
}
--
2.39.2
From 43ae66dc965d807dded8d434b7a1ea0b3f12e986 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 19 Aug 2022 11:28:38 +1200
Subject: [PATCH v3 2/2] Always use AF_UNIX sockets in pg_regress on Windows.
Since we can now rely on AF_UNIX sockets working on supported Windows
versions (10+), we can remove some Windows-only code for setting up
secure TCP in pg_regress.
Previously, we thought the socket cleanup code was unsafe, so we made
Unix-domain sockets an option with a "use-at-your-own-risk" note. On
closer inspection, the concerns about signal handlers don't seem to
apply here. (initdb.c has similar concerns but needs separate
investigation.)
Previously, commit f6dc6dd5 secured temporary installations using TCP/IP
on Windows, while commit be76a6d3 used file system permissions for Unix.
Now that our src/port/mkdtemp.c file creates non-world-accessible
directories on Windows, we can just do the same on Windows.
Note that this doesn't affect the TAP tests, which continue to use
TCP/IP on Windows by default.
Discussion: https://postgr.es/m/CA%2BhUKGK30uLx9dpgkYwomgH0WVLUHytkChDgf3iUM2zp0pf_nA%40mail.gmail.com
---
src/test/regress/pg_regress.c | 274 --
1 file changed, 32 insertions(+), 242 deletions(-)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..9c3251ca3b 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -55,6 +55,20 @@ char *host_platform = HOST_TUPLE;
static char *shellprog = SHELLPROG;
#endif
+/*
+ * The name of the environment variable that controls where we put temporary
+ * files, to overrid
Re: Using AF_UNIX sockets always for tests on Windows
Hello, On Fri, Dec 2, 2022 at 1:03 AM Thomas Munro wrote: > > 1. Teach mkdtemp() to make a non-world-accessible directory. This is > required to be able to make a socket that other processes can't > connect to, to match the paranoia level used on Unix. This was > written just by reading documentation, because I am not a Windows > user, so I would be grateful for a second opinion and/or testing from > a Windows hacker, which would involve testing with two different > users. The idea is that Windows' mkdir() is completely ignoring the > permissions (we can see in the mingw headers that it literally throws > away the mode argument), so we shouldn't use that, but native > CreateDirectory() when given a pointer to a SECURITY_ATTRIBUTES with > lpSecurityDesciptor set to NULL should only allow the current user to > access the object (directory). Does this really work, and would it be > better to create some more explicit private-keep-out > SECURITY_ATTRIBUTE, and how would that look? > A directory created with a NULL SECURITY_ATTRIBUTES inherits the ACL from its parent directory [1]. In this case, its parent is the designated temporary location, which already should have a limited access. You can create an explicit DACL for that directory, PFA a patch for so. This is just an example, not something that I'm proposing as a committable alternative. I'm fairly sure that filesystem permissions must be enough to stop > another OS user from connecting, because it's clear from documentation > that AF_UNIX works on Windows by opening the file and reading some > magic "reparse" data from inside it, so if you can't see into the > containing directory, you can't do it. This patch is the one the rest > are standing on, because the tests should match Unix in their level of > security. > Yes, this is correct. > > Only the first patch is modified, but I'm including all of them so they go through the cfbot. [1] https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea Regards, Juan José Santamaría Flecha v2-0004-Doc-Abstract-AF_UNIX-sockets-don-t-work-on-Windows-a.patch Description: Binary data v2-0003-WIP-Stop-using-TCP-in-TAP-tests-on-Windows.patch Description: Binary data v2-0002-WIP-Always-use-Unix-domain-sockets-in-pg_regress-on-.patch Description: Binary data v2-0001-WIP-Make-mkdtemp-more-secure-on-Windows.patch Description: Binary data
Re: Using AF_UNIX sockets always for tests on Windows
On 2023-01-04 We 07:13, vignesh C wrote: > On Fri, 2 Dec 2022 at 18:08, Andrew Dunstan wrote: >> >> On 2022-12-01 Th 21:10, Andres Freund wrote: >>> Hi, >>> >>> On 2022-12-01 20:56:18 -0500, Tom Lane wrote: Andres Freund writes: > On 2022-12-01 20:30:36 -0500, Tom Lane wrote: >> If we remove that, won't we have a whole lot of code that's not >> tested at all on any platform, ie all the TCP-socket code? > There's some coverage via the auth and ssl tests. But I agree it's an > issue. But to me the fix for that seems to be to add a dedicated test for > that, rather than relying on windows to test our socket code - that's > quite a > few separate code paths from the tcp support of other platforms. IMO that's not the best way forward, because you'll always have nagging questions about whether a single-purpose test covers everything that needs coverage. >>> Still seems better than not having any coverage in our development >>> environments... >>> >>> I think the best place to be in would be to be able to run the whole test suite using either TCP or UNIX sockets, on any platform (with stuff like the SSL test overriding the choice as needed). >>> I agree that that's useful. But it seems somewhat independent from the >>> majority of the proposed changes. To be able to test force-tcp-everywhere we >>> don't need e.g. code for setting sspi auth in pg_regress etc - it's afaik >>> just needed so there's a secure way of running tests at all on windows. >>> >>> I think 0003 should be "trimmed" to only change the default for >>> $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever >>> wants to, can then add a new environment variable to force tap tests to use >>> tcp. >>> >> Not sure if it's useful here, but a few months ago I prepared patches to >> remove the config-auth option of pg_regress, which struck me as more >> than odd, and replace it with a perl module. I didn't get around to >> finishing them, but the patches as of then are attached. >> >> I agree that having some switch that says "run everything with TCP" or >> "run (almost) everything with Unix sockets" would be good. > The patch does not apply on top of HEAD as in [1], please post a rebased > patch: > === Applying patches on top of PostgreSQL commit ID > bf03cfd162176d543da79f9398131abc251ddbb9 === > === applying patch > ./0001-Do-config_auth-in-perl-code-for-TAP-tests-and-vcregr.patch > patching file contrib/basebackup_to_shell/t/001_basic.pl > Hunk #1 FAILED at 21. > 1 out of 1 hunk FAILED -- saving rejects to file > contrib/basebackup_to_shell/t/001_basic.pl.rej > patching file src/bin/pg_basebackup/t/010_pg_basebackup.pl > Hunk #1 FAILED at 29. > 1 out of 1 hunk FAILED -- saving rejects to file > src/bin/pg_basebackup/t/010_pg_basebackup.pl.rej > Hunk #3 FAILED at 461. > 1 out of 3 hunks FAILED -- saving rejects to file > src/test/perl/PostgreSQL/Test/Cluster.pm.rej > > [1] - http://cfbot.cputube.org/patch_41_4033.log > What I posted was not intended as a replacement for Thomas' patches, or indeed meant as a CF item at all. So really we're waiting on Thomas to post a response to Tom's and Andres' comments upthread. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Using AF_UNIX sockets always for tests on Windows
On Fri, 2 Dec 2022 at 18:08, Andrew Dunstan wrote: > > > On 2022-12-01 Th 21:10, Andres Freund wrote: > > Hi, > > > > On 2022-12-01 20:56:18 -0500, Tom Lane wrote: > >> Andres Freund writes: > >>> On 2022-12-01 20:30:36 -0500, Tom Lane wrote: > If we remove that, won't we have a whole lot of code that's not > tested at all on any platform, ie all the TCP-socket code? > >>> There's some coverage via the auth and ssl tests. But I agree it's an > >>> issue. But to me the fix for that seems to be to add a dedicated test for > >>> that, rather than relying on windows to test our socket code - that's > >>> quite a > >>> few separate code paths from the tcp support of other platforms. > >> IMO that's not the best way forward, because you'll always have > >> nagging questions about whether a single-purpose test covers > >> everything that needs coverage. > > Still seems better than not having any coverage in our development > > environments... > > > > > >> I think the best place to be in would be to be able to run the whole test > >> suite using either TCP or UNIX sockets, on any platform (with stuff like > >> the > >> SSL test overriding the choice as needed). > > I agree that that's useful. But it seems somewhat independent from the > > majority of the proposed changes. To be able to test force-tcp-everywhere we > > don't need e.g. code for setting sspi auth in pg_regress etc - it's afaik > > just needed so there's a secure way of running tests at all on windows. > > > > I think 0003 should be "trimmed" to only change the default for > > $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever > > wants to, can then add a new environment variable to force tap tests to use > > tcp. > > > > Not sure if it's useful here, but a few months ago I prepared patches to > remove the config-auth option of pg_regress, which struck me as more > than odd, and replace it with a perl module. I didn't get around to > finishing them, but the patches as of then are attached. > > I agree that having some switch that says "run everything with TCP" or > "run (almost) everything with Unix sockets" would be good. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID bf03cfd162176d543da79f9398131abc251ddbb9 === === applying patch ./0001-Do-config_auth-in-perl-code-for-TAP-tests-and-vcregr.patch patching file contrib/basebackup_to_shell/t/001_basic.pl Hunk #1 FAILED at 21. 1 out of 1 hunk FAILED -- saving rejects to file contrib/basebackup_to_shell/t/001_basic.pl.rej patching file src/bin/pg_basebackup/t/010_pg_basebackup.pl Hunk #1 FAILED at 29. 1 out of 1 hunk FAILED -- saving rejects to file src/bin/pg_basebackup/t/010_pg_basebackup.pl.rej Hunk #3 FAILED at 461. 1 out of 3 hunks FAILED -- saving rejects to file src/test/perl/PostgreSQL/Test/Cluster.pm.rej [1] - http://cfbot.cputube.org/patch_41_4033.log Regards, Vignesh
Re: Using AF_UNIX sockets always for tests on Windows
On 2022-12-01 Th 21:10, Andres Freund wrote:
> Hi,
>
> On 2022-12-01 20:56:18 -0500, Tom Lane wrote:
>> Andres Freund writes:
>>> On 2022-12-01 20:30:36 -0500, Tom Lane wrote:
If we remove that, won't we have a whole lot of code that's not
tested at all on any platform, ie all the TCP-socket code?
>>> There's some coverage via the auth and ssl tests. But I agree it's an
>>> issue. But to me the fix for that seems to be to add a dedicated test for
>>> that, rather than relying on windows to test our socket code - that's quite
>>> a
>>> few separate code paths from the tcp support of other platforms.
>> IMO that's not the best way forward, because you'll always have
>> nagging questions about whether a single-purpose test covers
>> everything that needs coverage.
> Still seems better than not having any coverage in our development
> environments...
>
>
>> I think the best place to be in would be to be able to run the whole test
>> suite using either TCP or UNIX sockets, on any platform (with stuff like the
>> SSL test overriding the choice as needed).
> I agree that that's useful. But it seems somewhat independent from the
> majority of the proposed changes. To be able to test force-tcp-everywhere we
> don't need e.g. code for setting sspi auth in pg_regress etc - it's afaik
> just needed so there's a secure way of running tests at all on windows.
>
> I think 0003 should be "trimmed" to only change the default for
> $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever
> wants to, can then add a new environment variable to force tap tests to use
> tcp.
>
Not sure if it's useful here, but a few months ago I prepared patches to
remove the config-auth option of pg_regress, which struck me as more
than odd, and replace it with a perl module. I didn't get around to
finishing them, but the patches as of then are attached.
I agree that having some switch that says "run everything with TCP" or
"run (almost) everything with Unix sockets" would be good.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
From 5c7fad30aa2db2d47d0cc3869f132ca9d14a8814 Mon Sep 17 00:00:00 2001
From: Andrew Dunstan
Date: Tue, 26 Apr 2022 10:49:26 -0400
Subject: [PATCH 1/2] Do config_auth in perl code for TAP tests and
vcregress.pl.
That means there is no longer any need to call pg_regress --config-auth
to set up Windows authentication. Instead a simple perl function call
does the work.
---
contrib/basebackup_to_shell/t/001_basic.pl | 2 +-
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 2 +-
src/bin/pg_ctl/t/001_start_stop.pl | 4 +-
src/bin/pg_dump/t/010_dump_connstr.pl| 16 ++-
src/bin/pg_rewind/t/RewindTest.pm| 2 +-
src/test/perl/PostgreSQL/Test/Cluster.pm | 7 +-
src/test/perl/PostgreSQL/Test/ConfigAuth.pm | 115 +++
src/test/recovery/t/001_stream_rep.pl| 2 +-
src/tools/msvc/vcregress.pl | 8 +-
9 files changed, 135 insertions(+), 23 deletions(-)
create mode 100644 src/test/perl/PostgreSQL/Test/ConfigAuth.pm
diff --git a/contrib/basebackup_to_shell/t/001_basic.pl b/contrib/basebackup_to_shell/t/001_basic.pl
index 350d42079a..b5de23b48f 100644
--- a/contrib/basebackup_to_shell/t/001_basic.pl
+++ b/contrib/basebackup_to_shell/t/001_basic.pl
@@ -21,7 +21,7 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
# Make sure pg_hba.conf is set up to allow connections from backupuser.
# This is only needed on Windows machines that don't use UNIX sockets.
$node->init('allows_streaming' => 1,
- 'auth_extra' => [ '--create-role', 'backupuser' ]);
+ 'auth_extra' => [ extra_roles => 'backupuser' ]);
$node->append_conf('postgresql.conf',
"shared_preload_libraries = 'basebackup_to_shell'");
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 056fcf3597..a0f4fd7b92 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -29,7 +29,7 @@ umask(0077);
# Initialize node without replication settings
$node->init(extra => ['--data-checksums'],
- auth_extra => [ '--create-role', 'backupuser' ]);
+ auth_extra => [ extra_roles => 'backupuser' ]);
$node->start;
my $pgdata = $node->data_dir;
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index fdffd76d99..c92b06ad85 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -4,6 +4,7 @@
use strict;
use warnings;
+use PostgreSQL::Test::ConfigAuth;
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
@@ -20,8 +21,7 @@ command_exit_is([ 'pg_ctl', 'start', '-D', "$tempdir/nonexistent" ],
command_ok([ 'pg_ctl', 'initdb', '-D', "$tempdir/data", '-o', '-N' ],
'pg_ctl initdb');
-command_ok([ $ENV{PG_REGRESS}, '--config-auth', "$tempdir/data" ],
- 'configure authentication');
+config_auth("$tempdir/data");
my $node_
Re: Using AF_UNIX sockets always for tests on Windows
Hi, On 2022-12-01 20:56:18 -0500, Tom Lane wrote: > Andres Freund writes: > > On 2022-12-01 20:30:36 -0500, Tom Lane wrote: > >> If we remove that, won't we have a whole lot of code that's not > >> tested at all on any platform, ie all the TCP-socket code? > > > There's some coverage via the auth and ssl tests. But I agree it's an > > issue. But to me the fix for that seems to be to add a dedicated test for > > that, rather than relying on windows to test our socket code - that's quite > > a > > few separate code paths from the tcp support of other platforms. > > IMO that's not the best way forward, because you'll always have > nagging questions about whether a single-purpose test covers > everything that needs coverage. Still seems better than not having any coverage in our development environments... > I think the best place to be in would be to be able to run the whole test > suite using either TCP or UNIX sockets, on any platform (with stuff like the > SSL test overriding the choice as needed). I agree that that's useful. But it seems somewhat independent from the majority of the proposed changes. To be able to test force-tcp-everywhere we don't need e.g. code for setting sspi auth in pg_regress etc - it's afaik just needed so there's a secure way of running tests at all on windows. I think 0003 should be "trimmed" to only change the default for $use_unix_sockets on windows and to remove PG_TEST_USE_UNIX_SOCKETS. Whoever wants to, can then add a new environment variable to force tap tests to use tcp. Greetings, Andres Freund
Re: Using AF_UNIX sockets always for tests on Windows
Andres Freund writes: > On 2022-12-01 20:30:36 -0500, Tom Lane wrote: >> If we remove that, won't we have a whole lot of code that's not >> tested at all on any platform, ie all the TCP-socket code? > There's some coverage via the auth and ssl tests. But I agree it's an > issue. But to me the fix for that seems to be to add a dedicated test for > that, rather than relying on windows to test our socket code - that's quite a > few separate code paths from the tcp support of other platforms. IMO that's not the best way forward, because you'll always have nagging questions about whether a single-purpose test covers everything that needs coverage. I think the best place to be in would be to be able to run the whole test suite using either TCP or UNIX sockets, on any platform (with stuff like the SSL test overriding the choice as needed). I doubt ripping out the existing Windows-only support for TCP-based testing is a good step in that direction. regards, tom lane
Re: Using AF_UNIX sockets always for tests on Windows
Hi, On 2022-12-01 20:30:36 -0500, Tom Lane wrote: > Thomas Munro writes: > > Commit f5580882 established that all supported computers have AF_UNIX. > > One of the follow-up consequences that was left unfinished is that we > > could simplify our test harness code to make it the same on all > > platforms. Currently we have hundreds of lines of C and perl to use > > secure TCP connections instead for the benefit of defunct Windows > > versions. Here's a patch set for that. > > If we remove that, won't we have a whole lot of code that's not > tested at all on any platform, ie all the TCP-socket code? There's some coverage via the auth and ssl tests. But I agree it's an issue. But to me the fix for that seems to be to add a dedicated test for that, rather than relying on windows to test our socket code - that's quite a few separate code paths from the tcp support of other platforms. Greetings, Andres Freund
Re: Using AF_UNIX sockets always for tests on Windows
Thomas Munro writes: > Commit f5580882 established that all supported computers have AF_UNIX. > One of the follow-up consequences that was left unfinished is that we > could simplify our test harness code to make it the same on all > platforms. Currently we have hundreds of lines of C and perl to use > secure TCP connections instead for the benefit of defunct Windows > versions. Here's a patch set for that. If we remove that, won't we have a whole lot of code that's not tested at all on any platform, ie all the TCP-socket code? regards, tom lane
Using AF_UNIX sockets always for tests on Windows
Hi,
Commit f5580882 established that all supported computers have AF_UNIX.
One of the follow-up consequences that was left unfinished is that we
could simplify our test harness code to make it the same on all
platforms. Currently we have hundreds of lines of C and perl to use
secure TCP connections instead for the benefit of defunct Windows
versions. Here's a patch set for that. These patches and some
discussion of them were buried in the recent
clean-up-after-recently-dropped-obsolete-systems thread[1], and I
didn't want to lose track of them. I think they would need review and
testing from a Windows-based hacker to make progress. The patches
are:
1. Teach mkdtemp() to make a non-world-accessible directory. This is
required to be able to make a socket that other processes can't
connect to, to match the paranoia level used on Unix. This was
written just by reading documentation, because I am not a Windows
user, so I would be grateful for a second opinion and/or testing from
a Windows hacker, which would involve testing with two different
users. The idea is that Windows' mkdir() is completely ignoring the
permissions (we can see in the mingw headers that it literally throws
away the mode argument), so we shouldn't use that, but native
CreateDirectory() when given a pointer to a SECURITY_ATTRIBUTES with
lpSecurityDesciptor set to NULL should only allow the current user to
access the object (directory). Does this really work, and would it be
better to create some more explicit private-keep-out
SECURITY_ATTRIBUTE, and how would that look?
I'm fairly sure that filesystem permissions must be enough to stop
another OS user from connecting, because it's clear from documentation
that AF_UNIX works on Windows by opening the file and reading some
magic "reparse" data from inside it, so if you can't see into the
containing directory, you can't do it. This patch is the one the rest
are standing on, because the tests should match Unix in their level of
security.
2. Always use AF_UNIX for pg_regress. Remove a bunch of
no-longer-needed sspi auth stuff. Remove comments that worried about
signal handler safety (referring here to real Windows signals, not
fake PostgreSQL signals that are a backend-only concept). By my
reading of the Windows documentation and our code, there is no real
concern here, so the remove_temp() stuff should be fine, as I have
explained in a new comment. But I have not tested this signal safety
claim, not being a Windows user. I added an assertion that should
hold if I am right. If you run this on Windows and interrupt
pg_regress with ^C, does it hold?
3. Use AF_UNIX for TAP tests too.
4. In passing, remove our documentation's claim that Linux's
"abstract" AF_UNIX namespace is available on Windows. It does not
work at all, according to all reports (IMHO it seems like an
inherently insecure interface that other OSes would be unlikely to
adopt).
Note that this thread is not about libpq, which differs from Unix by
defaulting to host=localhost rather than AF_UNIX IIRC. That's a
user-facing policy decision I'm not touching; this thread is just
about cleaning up old test infrastructure of interest to hackers.
[1]
https://www.postgresql.org/message-id/flat/CA%2BhUKGJ3LHeP9w5Fgzdr4G8AnEtJ%3Dz%3Dp6hGDEm4qYGEUX5B6fQ%40mail.gmail.com
From 62b1cdbdc848f96eef02ed97f14be9c1f4557539 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Wed, 7 Sep 2022 07:35:11 +1200
Subject: [PATCH 1/4] WIP: Make mkdtemp() more secure on Windows.
Our POSIX mkdtemp() implementation in src/port/mkdtemp.c code would
create directories with default permissions on Windows. Fix, using
native Windows API instead of mkdir().
---
src/port/mkdtemp.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/port/mkdtemp.c b/src/port/mkdtemp.c
index 8809957dcd..8116317435 100644
--- a/src/port/mkdtemp.c
+++ b/src/port/mkdtemp.c
@@ -187,8 +187,20 @@ GETTEMP(char *path, int *doopen, int domkdir)
}
else if (domkdir)
{
+#ifdef WIN32
+ SECURITY_ATTRIBUTES sa = {
+.nLength = sizeof(SECURITY_ATTRIBUTES),
+.lpSecurityDescriptor = NULL,
+.bInheritHandle = false
+ };
+
+ if (CreateDirectory(path, &sa))
+return 1;
+ _dosmaperr(GetLastError());
+#else
if (mkdir(path, 0700) >= 0)
return 1;
+#endif
if (errno != EEXIST)
return 0;
}
--
2.38.1
From 388719a6fbb45896ff87794ed4bfdbc0f0aac329 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 19 Aug 2022 11:28:38 +1200
Subject: [PATCH 2/4] WIP: Always use Unix-domain sockets in pg_regress on
Windows.
Since we can now rely on Unix-domain sockets working on supported
Windows versions (10+), we can remove a source of instability and a
difference between Unix and Windows in pg_regress.
Previously, we thought the socket cleanup code was unsafe, so we made
Unix-domain sockets an option with a "use-at-your-own-risk" note. On
closer inspection, the concerns about signal handlers don't seem to
apply here. (initdb.c h
