Re: pg_hba_file_settings view patch

On Tue, Jan 17, 2017 at 10:19 AM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
> On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>> +/* 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.

That one:
+ Assert(noptions <= MAX_OPTIONS);
+ if (noptions)
+ return PointerGetDatum(
+ construct_array(options, noptions, TEXTOID, -1, false, 'i'));

>> =# \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 field is converted into
> 3 columns in the view.

Hm. We could as well consider cidr and use just one column. But still,
the use of inet as a data type in a system view looks like a wrong
choice to me. Or we could actually just use text... Opinions from
others are welcome here of course.

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

Thanks. Now that works.

> Updated patch attached.

This begins to look good. I have found a couple of minor issues.

+ <para>
+ The <structname>pg_hba_rules</structname> view can be read only by
+ superusers.
+ </para>
This is not true anymore.

+ <entry>
+ Line number within client authentication configuration file
+ the current value was set at
+ </entry>
I'd tune that without a past sentence. Saying just pg_hba.conf would
be fine perhaps?

+ <row>
+ <entry><structfield>database</structfield></entry>
+ <entry><structfield>text[]</structfield></entry>
+ <entry>List of database name</entry>
+ </row>
This should be plural, database nameS.

+ <entry>
+ List of keyword address names,
+ name can be all, samehost and samenet
+ </entry>
Phrasing looks weird to me, what about "List of keyword address names,
whose values can be all, samehost or samenet", with <literal> markups.

+postgres=# select line_number, type, database, user_name, auth_method
from pg_hba_rules;
Nit: this could be upper-cased.

+static Datum
+getauthmethod(UserAuth auth_method)
+{
+ ...
+ default:
+ elog(ERROR, "unexpected authentication method in parsed HBA entry");
+ break;
+ }
I think that you should also remove the default clause here to catchup
errors at compilation.

+ 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");
+ }
You could go without the default clause here as well.
--
Michael