Where is SSPI auth username determined for TAP tests?

On Sat, Jun 29, 2019 at 04:36:51PM -0400, Tom Lane wrote:

I think this is likely a consequence of ca129e58c0 having modified
010_dump_connstr.pl to use "regress_postgres" not "postgres" as the
bootstrap superuser name in the source cluster. I suppose I overlooked
some dependency on the user name that only affects SSPI ... but what?
I don't see anything about the destination cluster configuration (which
already used a nondefault superuser name) that I didn't replicate
in the source cluster configuration.

Didn't you get trapped with something similar to what has been fixed
in d9f543e? If you want pg_hba.conf to be correctly set up for SSPI
on Windows, you should pass "auth_extra => ['--create-role',
'regress_postgres']" to the init() method initializing the node.

Looking at the commit...
 my $node = get_new_node('main');
-$node->init(extra => [ '--locale=C', '--encoding=LATIN1' ]);
+$node->init(extra =>
+     [ '-U', $src_bootstrap_super, '--locale=C', '--encoding=LATIN1' ]);
[...]
 $node->run_log(
     [
        $ENV{PG_REGRESS}, '--config-auth',
	$node->data_dir,  '--create-role',
-       "$dbname1,$dbname2,$dbname3,$dbname4"
+       "$username1,$username2,$username3,$username4"
     ]);

This part is wrong and just needs to be updated to as
$src_bootstrap_super also gets its role added in --create-role, which
would set up pg_hba.conf as you would like.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Sat, Jun 29, 2019 at 04:36:51PM -0400, Tom Lane wrote:

I think this is likely a consequence of ca129e58c0 having modified
010_dump_connstr.pl to use "regress_postgres" not "postgres" as the
bootstrap superuser name in the source cluster. I suppose I overlooked
some dependency on the user name that only affects SSPI ... but what?

Didn't you get trapped with something similar to what has been fixed
in d9f543e? If you want pg_hba.conf to be correctly set up for SSPI
on Windows, you should pass "auth_extra => ['--create-role',
'regress_postgres']" to the init() method initializing the node.

After further study, I think the root issue here is that pg_regress.c's
config_sspi_auth() has no provision for non-default bootstrap superuser
names --- it makes a mapping entry for (what should be) the default
superuser name whether the cluster is using that or not. I now see that
010_dump_connstr.pl is hacking around that by doing

my $envar_node = get_new_node('destination_envar');
$envar_node->init(extra =>
[ '-U', $dst_bootstrap_super, '--locale=C', '--encoding=LATIN1' ]);
$envar_node->run_log(
[
$ENV{PG_REGRESS}, '--config-auth',
$envar_node->data_dir, '--create-role',
"$dst_bootstrap_super,$restore_super"
]);

that is, it's explicitly listing the non-default bootstrap superuser
among the roles to be "created". This is all pretty weird and
undocumented ...

We could apply the same hack on the source node, but I wonder if it
wouldn't be better to make this less of a kluge. I'm tempted to
propose that "pg_regress --config-auth --user XXX" should understand
XXX as the bootstrap superuser name, and then we could clean up
010_dump_connstr.pl by using that.

regards, tom lane

On Sun, Jun 30, 2019 at 12:09:18PM -0400, Tom Lane wrote:

We could apply the same hack on the source node, but I wonder if it
wouldn't be better to make this less of a kluge. I'm tempted to
propose that "pg_regress --config-auth --user XXX" should understand
XXX as the bootstrap superuser name, and then we could clean up
010_dump_connstr.pl by using that.

I have been reviewing that part, and the part to split the bootstrap
user from the set of extra roles created looks fine to me. Now, it
seems to me that you can simplify 010_dump_connstr.pl as per the
attached because PostgresNode::Init can take care of --auth-config
part with the correct options using auth_extra. What do you think
about the cleanup attached?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

I have been reviewing that part, and the part to split the bootstrap
user from the set of extra roles created looks fine to me. Now, it
seems to me that you can simplify 010_dump_connstr.pl as per the
attached because PostgresNode::Init can take care of --auth-config
part with the correct options using auth_extra. What do you think
about the cleanup attached?

I haven't checked that this actually works, but it looks plausible,
and I agree it's simpler/easier.

regards, tom lane

On Wed, Jul 03, 2019 at 09:53:14AM -0400, Tom Lane wrote:

I haven't checked that this actually works, but it looks plausible,
and I agree it's simpler/easier.

Thanks, committed. While testing on Windows, I have been trapped by
the fact that IPC::Run mishandles double quotes, causing the tests to
fail for the environment variable part because of a mismatching
pg_hba.conf entry. The difference is that with we run pg_regress
--config-auth using IPC::Run::run on HEAD but the patch switches to
system(). So I have finished by removing the double-quote handling
from the restore user name which makes the whole test suite more
consistent. The patch has at the end the advantage of removing in
pg_ident.conf the entry related to the OS user running the scripts,
which makes the environment more restricted by default.
--
Michael