Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag

Hi,

On 9/29/23 8:19 AM, Michael Paquier wrote:
> 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.

Thanks for looking at it!

> 
>> +             uint32 flags,
>>                char *out_dbname)
>>   {
> 
> This may be more adapted with a bits32 for the flags.

Done that way in v2 attached.

> 
>> +# 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.
> 

I think that would make sense to have more flexibility in the worker_spi
module. I think that could be done in a dedicated patch though. I think it makes
more sense to have the current patch "focusing" on this new flag (while adding a test
about it without too much refactoring). What about doing the worker_spi module
re-factoring as a follow up of this one?

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

Done in v2 attached.

> See other tests that want to check the contents of
> the logs from an offset.
> 

Oh right, worth to modify 019_replslot_limit.pl, 002_corrupted.pl and
001_pg_controldata.pl in a separate patch for consistency? (they are using
"(stat $node->logfile)[7]" or "(stat($pg_control))[7]").

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

I did not want initially to add an extra parameter to InitPostgres() but
I agree it would make more sense. So done that way in v2.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com