Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
Date
Msg-id ZRZsg2pqrCQ9dlsn@paquier.xyz
Whole thread Raw
In response to Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag  ("Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>)
Responses Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
List pgsql-hackers
On Thu, Sep 28, 2023 at 02:37:02PM +0200, Drouvot, Bertrand wrote:
> This patch allows the role provided in BackgroundWorkerInitializeConnection()
> and BackgroundWorkerInitializeConnectionByOid() to lack login authorization.

Interesting.  Yes, there would be use cases for that, I suppose.

> +             uint32 flags,
>               char *out_dbname)
>  {

This may be more adapted with a bits32 for the flags.

> +# Ask the background workers to connect with this role with the flag in place.
> +$node->append_conf(
> +    'postgresql.conf', q{
> +worker_spi.role = 'nologrole'
> +worker_spi.bypass_login_check = true
> +});
> +$node->restart;
> +
> +# An error message should not be issued.
> +ok( !$node->log_contains(
> +        "role \"nologrole\" is not permitted to log in", $log_start),
> +    "nologrole allowed to connect if BGWORKER_BYPASS_ROLELOGINCHECK is set");
> +
>  done_testing();

It would be cheaper to use a dynamic background worker for such tests.
Something that I've been tempted to do in this module is to extend the
amount of data that's given to bgw_main_arg when launching a worker
with worker_spi_launch().  How about extending the SQL function so as
it is possible to give in input a role name (or a regrole), a database
name (or a database OID) and a text[] for the flags?  This would
require a bit more refactoring, but this would be benefitial to show
or one can pass down a full structure from the registration to the
main() routine.  On top of that, it would make the addition of the new
GUCs worker_spi.bypass_login_check and worker_spi.role unnecessary.

> +# return the size of logfile of $node in bytes
> +sub get_log_size
> +{
> +    my ($node) = @_;
> +
> +    return (stat $node->logfile)[7];
> +}

Just use -s here.  See other tests that want to check the contents of
the logs from an offset.

> - * Allow bypassing datallowconn restrictions when connecting to database
> + * Allow bypassing datallowconn restrictions and login check when connecting
> + * to database
>   */
> -#define BGWORKER_BYPASS_ALLOWCONN 1
> +#define BGWORKER_BYPASS_ALLOWCONN 0x0001
> +#define BGWORKER_BYPASS_ROLELOGINCHECK 0x0002

The structure of the patch is inconsistent.  These flags are in
bgworker.h, but they are used also by InitPostgres().  Perhaps a
second boolean flag would be OK rather than a second set of flags for
InitPostgres() mapping with the bgworker set.
--
Michael

Attachment

pgsql-hackers by date:

Previous
From: vignesh C
Date:
Subject: Re: Invalidate the subscription worker in cases where a user loses their superuser status
Next
From: Alexander Korotkov
Date:
Subject: Re: Index range search optimization