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

On Tue, Dec 8, 2015 at 9:41 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:
>
> On Sat, Dec 5, 2015 at 3:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
> > Haribabu Kommi wrote:
> >
> >> How about as follows?
> >>
> >> postgres=# select * from pg_hba_lookup('all','all','::1');
> >> line_number | type | database | user | address | hostname |
method | options | mode
> >>
-------------+-------+----------+---------+-----------+----------+--------+---------+---------
> >> 84 | local | ["all"] | ["all"] | | |
trust | {} | skipped
> >> 86 | host | ["all"] | ["all"] | 127.0.0.1 | |
trust | {} | skipped
> >> 88 | host | ["all"] | ["all"] | ::1 | |
trust | {} | matched
> >> (3 rows)
> >
> > What did you do to the whitespace when posting that table? I had to
> > reformat it pretty heavily to understand what you had.
> > Anyway, I think the "mode" column should be right after the line number.
> > I assume the "reason" for skipped lines is going to be somewhere in the
> > table too.
>
> when i try to copy paste the output from psql, it didn't come properly, so
> I adjusted the same to looks properly, but after sending mail, it look
ugly.
>
> Added reason column also as the last column of the table and moved the
mode
> as the second column.
>

Few assorted comments:

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.

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.

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

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com