Re: pg_hba_file_settings view patch

On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > Could you hold on a bit to commit that? I'd like to look at it in more
> > details. At quick glance, there is for example no need to use
> > CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
> > C routine itself. And memset() can be used in fill_hba_line for the
> > error code path.
>
> And here we go.
>

Thanks for the review.

> +<programlisting>
> +postgres=# select * from pg_hba_rules;
> [... large example ...]
> +
> +</programlisting>
> It would be nice to reduce the width of this example. That's not going
> to be friendly with the generated html.
>

Added a small example.

+ switch (hba->conntype)
> + {
> + case ctLocal:
> + values[index] = CStringGetTextDatum("local");
> + break;
> + case ctHost:
> + values[index] = CStringGetTextDatum("host");
> + break;
> + case ctHostSSL:
> + values[index] = CStringGetTextDatum("hostssl");
> + break;
> + case ctHostNoSSL:
> + values[index] = CStringGetTextDatum("hostnossl");
> + break;
> + default:
> + elog(ERROR, "unexpected connection type in parsed HBA
> entry");
> + break;
> + }
> Here let's remove the break clause and let compilers catch problem
> when they show up.
>

Removed.

> + if (hba->pamservice)
> + {
> + initStringInfo(&str);
> + appendStringInfoString(&str, "pamservice=");
> + appendStringInfoString(&str, hba->pamservice);
> + options[noptions++] = CStringGetTextDatum(str.data);
> + }
> There is a bunch of code duplication here. I think that it would make
> more sense to encapsulate that into a routine, at least let's use
> appendStringInfo and let's group the two calls together.
>

Use a new function to reduce the repeated lines of code.

> +/* LDAP supports 10 currently, keep this well above the most any
> method needs */
> +#define MAX_OPTIONS 12
> Er, why? There is an assert already, that should be enough.
>

Which Assert? This macro is used to verify that the maximum number
of authentication options that are possible for a single hba line.

> =# \d pg_hba_rules
> View "pg_catalog.pg_hba_rules"
> Column | Type | Collation | Nullable | Default
> ------------------+---------+-----------+----------+---------
> line_number | integer | | |
> type | text | | |
> keyword_database | text[] | | |
> database | text[] | | |
> keyword_user | text[] | | |
> user_name | text[] | | |
> keyword_address | text | | |
> address | inet | | |
> netmask | inet | | |
> hostname | text | | |
> method | text | | |
> options | text[] | | |
> error | text | | |
> keyword_database and database map actually to the same thing if you
> refer to a raw pg_hba.conf file because they have the same meaning for
> user. You could simplify the view and just remove keyword_database,
> keyword_user and keyword_address. This would simplify your patch as
> well with all hte mumbo-jumbo to see if a string is a dedicated
> keyword or not. In its most simple shape pg_hba_rules should show to
> the user as an exact map of the entries of the raw file.
>

I removed keyword_database and keyword_user columns where the data
in those columns can easily represent with the database and user columns.
But for address filed can contains keywords such as "same host" and etc and
also a hostname also. Because of this reason, this filed is converted into
3 columns in the view.

I have copied the example file of pg_hba.conf, reloaded it:
> https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
> And then the output result gets corrupted by showing up free()'d results:
> null | null | \x7F\x7F\x7F\x7F\x7F
>

There was a problem in resetting the error string, working with attached
patch.

> + if (err_msg)
> + {
> + /* type */
> + index++;
> + nulls[index] = true;
> [... long sequence ...]
> Please let's use MemSet here and remove this large chunk of code..
>

Done.

> + if (!superuser())
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + (errmsg("must be superuser to view pg_hba.conf
> settings"))));
> Access to the function is already revoked, so what's the point of this
> superuser check?
>

Removed.

>
> + tupdesc = CreateTemplateTupleDesc(NUM_PG_HBA_LOOKUP_ATTS, false);
> + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "line_number",
> + INT4OID, -1, 0);
> There is no need to list all the columns here. You can just use
> get_call_result_type() and be done with it as the types and columns
> names are already listed in the pg_proc entry of the new function.]

Done.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia