Horton

Thread

Commits

  1. Simplify TAP tests of pg_dump for connection strings

  2. Blind attempt to fix SSPI-auth case in 010_dump_connstr.pl.

  1. Where is SSPI auth username determined for TAP tests?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-29T20:36:51Z

    bowerbird is failing the pg_dump regression tests with a lot of

FATAL:  SSPI authentication failed for user "regress_postgres"

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.

			regards, tom lane

  2. Re: Where is SSPI auth username determined for TAP tests?

    Michael Paquier <michael@paquier.xyz> — 2019-06-30T01:42:22Z

    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

  3. Re: Where is SSPI auth username determined for TAP tests?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-30T16:09:18Z

    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

  4. Re: Where is SSPI auth username determined for TAP tests?

    Michael Paquier <michael@paquier.xyz> — 2019-07-03T06:20:24Z

    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

  5. Re: Where is SSPI auth username determined for TAP tests?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-07-03T13:53:14Z

    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

  6. Re: Where is SSPI auth username determined for TAP tests?

    Michael Paquier <michael@paquier.xyz> — 2019-07-04T02:35:21Z

    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