Re: pg_hba_lookup function to get all matching pg_hba.conf entries

On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Few assorted comments:

Thanks for the review.

> 1.
> + /*
> + * SQL-accessible SRF to return all the settings from the pg_hba.conf
> + * file.
> + */
> + Datum
> + pg_hba_lookup_2args(PG_FUNCTION_ARGS)
> + {
> + return pg_hba_lookup(fcinfo);
> + }
> +
> + /*
> + * SQL-accessible SRF to return all the settings from the pg_hba.conf
> + * file.
> + */
> + Datum
> + pg_hba_lookup_3args(PG_FUNCTION_ARGS)
> + {
> + return pg_hba_lookup(fcinfo);
> + }
>
> I think it is better to have check on number of args in the
> above functions similar to what we have in ginarrayextract_2args.

ginarrayextract_2args is an deprecated function that checks and returns
error if user is using with two arguments. But in pg_hba_lookup function,
providing two argument is a valid scenario. The check can be added only
to verify whether the provided number of arguments are two or not. Is it
really required?

> 2.
> +
> + /*
> + * Reload authentication config files too to refresh
> + * pg_hba_conf view data.
> + */
> + if (!load_hba())
> + {
> + ereport(DEBUG1,
> + (errmsg("Falure in reloading pg_hba.conf, pg_hba_conf view may show stale
> information")));
> + load_hba_failure = true;
> + }
> +
> + load_hba_failure = false;
>
> Won't the above code set load_hba_failure as false even in
> failure case.

Fixed.

> 3.
> + Datum
> + pg_hba_lookup(PG_FUNCTION_ARGS)
> + {
> + char *user;
> + char *database;
> + char *address;
> + char *hostname;
> + bool ssl_inuse = false;
> + struct sockaddr_storage addr;
> + hba_lookup_args_mode args_mode = TWO_ARGS_MODE; /* Minimum number of
> arguments */
> +
> + /*
> + * We must use the Materialize mode to be safe against HBA file reloads
> + * while the cursor is open. It's also more efficient than having to look
> + * up our current position in the parsed list every time.
> + */
> + ReturnSetInfo *rsi = (ReturnSetInfo *)fcinfo->resultinfo;
> +
> + if (!superuser())
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + (errmsg("only superuser can view pg_hba.conf settings"))));
>
> To be consistent with other similar messages, it is better to
> start this message with "must be superuser ..", refer other
> similar superuser checks in xlogfuncs.c

Updated as "must be superuser to view".

Attached updated patch after taking care of review comments.

Regards,
Hari Babu
Fujitsu Australia