Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 29 Sep 2023 15:19:47 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag
Message-ID: <ZRZsg2pqrCQ9dlsn@paquier.xyz>
References: <bcc36259-7850-4882-97ef-d6b905d2fc51@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="IptVjffxY4qoX+Y5"
Content-Disposition: inline
In-Reply-To: <bcc36259-7850-4882-97ef-d6b905d2fc51@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZRZsg2pqrCQ9dlsn%40paquier.xyz>
Precedence: bulk


--IptVjffxY4qoX+Y5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--IptVjffxY4qoX+Y5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUWbIMACgkQnvQgOdby
QH0OZw//QYfGWM70T9ynnnzTaRi6iJ69zgSUhCRQ522XT528X8CB+YMZ62SIxi64
lCSh6ddyPTw83kgP2navMDIoh7vvsBVllxxIaMCfJ/TVEs3d0Mpq14mDetZ+SMyH
WWXZ7bBrwJth/FilbtvKqApp2JdKtY+tB0SF9NB0PX+YhCc2eebs/UgJD3HP2yrM
rOZVskdMkiBrCS5XgyJd7j8QVyM0riZFN8FUAj+2sB8d7nK2/yG+BHrLiwfAHlFF
5PfmLUk2+R2txSsqCDzNN4KOMcCOhManmoAHidGfaQGWoy4NmUiocXyhG/AWwan4
QfTRhf79LSvIR98T7n0lpbrS7X4YknNiowpSSfVoaWEx9KIAAGf0CzV6O2HPzAuN
EwPOgH/NdxwWxo0LmA1Sg7GFIbcO0yba82queuc28MdYE8IfdoYGh8qYDDJ+xUUA
Wxbe5tJvoFrJQrLWYeejFfurkvVdfRxGQhCnwecXBXrbEELi1nTqOj4AkLO0Kd6T
c1KtyZGbYD+9YByCuKViPwTKlUEbGXtb6hQvkd5LGIOjLT8gYFgk3G5tsac82V9o
h2Azi0Uk391NNoE/X42IGdBKujHieO7rAj4pvSFdQW+v7r9FE+7MoRLeKwIaaGyH
WebBH1hU6OlY9mZSsgzfBp5CZ14m4pRhw8eWp64kKciZenIMDeY=
=43V4
-----END PGP SIGNATURE-----

--IptVjffxY4qoX+Y5--