Обсуждение: pg_hba_lookup function to get all matching pg_hba.conf entries

Hi Hackers,

This is the patch adds a new function called pg_hba_lookup to get all
matching entries
from the pg_hba.conf for the providing input data.The rows are
displayed from the other
the hba conf entries are matched.

This is an updated version of previous failure attempt to create a
catalog view for the
pg_hba.conf [1]. The view is not able to handle the SQL queries properly because
keywords that are present in database and user columns.


currently the following two types are added:

pg_hba_lookup(database, user)
pg_hba_lookup(database, user, address, hostname)


How it works:

With the provided input data, it tries to match the entries of
pg_hba.conf and populate the
result set with all matching entries.

With the recent Tomlane's commit
1e24cf645d24aab3ea39a9d259897fd0cae4e4b6 of "Don't leave pg_hba and
pg_ident data lying around in running backends" [2], the parsed hba
conf entries are not available in the backend side. Temporarily I just
reverted this patch for the
proof of concept purpose. Once we agree with the approach, I will try
to find out a solution
to the same.


How is it useful:

With the output of this view, administrator can identify the lines
that are matching for the given
criteria easily without going through the file.


Record format:

Column name | datatype
-------------------------------
line_number | int4
type              | text
database      | jsonb
user              | jsonb
address        | inet
hostname     | text
method         | text
options          | jsonb

Please suggest me for any column additions or data type changes that
are required.


Example output:

postgres=# select pg_hba_lookup('postgres','all');
                     pg_hba_lookup
-------------------------------------------------------
 (84,local,"[""all""]","[""all""]",,,trust,{})
 (86,host,"[""all""]","[""all""]",127.0.0.1,,trust,{})
 (88,host,"[""all""]","[""all""]",::1,,trust,{})

Here I attached a proof of concept patch for the same.

Any suggestions/comments on this proposed approach?

[1] http://www.postgresql.org/message-id/F40B0968DB0A904DA78A924E633BE78645FE29@SYDEXCHTMP2.au.fjanz.com

[2] http://www.postgresql.org/message-id/E1ZAQuy-00072J-7G@gemulon.postgresql.org

Regards,
Hari Babu
Fujitsu Australia

On Mon, Sep 7, 2015 at 4:34 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
>
>>
>> postgres=# select pg_hba_lookup('postgres','all');
>>                      pg_hba_lookup
>> -------------------------------------------------------
>>  (84,local,"[""all""]","[""all""]",,,trust,{})
>>  (86,host,"[""all""]","[""all""]",127.0.0.1,,trust,{})
>>  (88,host,"[""all""]","[""all""]",::1,,trust,{})
>>
>> Here I attached a proof of concept patch for the same.
>>
>> Any suggestions/comments on this proposed approach?
>>
>
> If I understand well to your proposal, the major benefit is in impossibility
> to enter pg_hba keywords - so you don't need to analyse if parameter is
> keyword or not? It has sense, although It can be hard to do image about
> pg_hba conf from these partial views.

From the function output, it is little bit difficult to map the
pg_hba.conf file.
Because of problems in processing keywords in where clause of a view, I changed
from view to function.

Is there any possibility with rule or something, that the where clause
details can be passed
as function arguments to get the data?

> There can be other way - theoretically we can have a function pg_hba_debug
> with similar API like pg_hba_conf. The result will be a content of
> pg_hba.conf with information about result of any rule.


The output of pg_hba_debug function looks like, the entry of
pg_hba.conf and the result
match for the given input data.

Regards,
Hari Babu
Fujitsu Australia

On 7/21/15 5:15 AM, Haribabu Kommi wrote:
> With the output of this view, administrator can identify the lines
> that are matching for the given
> criteria easily without going through the file.

How is this useful?  I could see the use if you want to debug cases of
user foo on host bar says they can't connect, but you can't impersonate
them to verify it.  But then all you need is a function with a scalar
result, not a result set.

On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 7/21/15 5:15 AM, Haribabu Kommi wrote:
>> With the output of this view, administrator can identify the lines
>> that are matching for the given
>> criteria easily without going through the file.
>
> How is this useful?  I could see the use if you want to debug cases of
> user foo on host bar says they can't connect, but you can't impersonate
> them to verify it.  But then all you need is a function with a scalar
> result, not a result set.

Do you mean the function should return true or false based on the connection
status with the provided arguments?

I also feel difficult to understand the function result as compared to a view.

Regards,
Hari Babu
Fujitsu Australia

On 11/16/15 2:37 AM, Haribabu Kommi wrote:
> On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
>> On 7/21/15 5:15 AM, Haribabu Kommi wrote:
>>> With the output of this view, administrator can identify the lines
>>> that are matching for the given
>>> criteria easily without going through the file.
>>
>> How is this useful?  I could see the use if you want to debug cases of
>> user foo on host bar says they can't connect, but you can't impersonate
>> them to verify it.  But then all you need is a function with a scalar
>> result, not a result set.
> 
> Do you mean the function should return true or false based on the connection
> status with the provided arguments?
> 
> I also feel difficult to understand the function result as compared to a view.

An hba lookup is essentially a lookup by user name, database name,
client address, yielding an authentication method (possibly with
parameters).  So I think this function should work that way as well:
arguments are user name, database name, and so on, and the return value
is an authentication method.  Maybe it would be some kind of record,
with line number and some parameters.

That would address the use case I put forth above.  I don't know whether
that's what you were going for.

On Tue, Nov 17, 2015 at 9:37 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 11/16/15 2:37 AM, Haribabu Kommi wrote:
>> On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
>>> On 7/21/15 5:15 AM, Haribabu Kommi wrote:
>>>> With the output of this view, administrator can identify the lines
>>>> that are matching for the given
>>>> criteria easily without going through the file.
>>>
>>> How is this useful?  I could see the use if you want to debug cases of
>>> user foo on host bar says they can't connect, but you can't impersonate
>>> them to verify it.  But then all you need is a function with a scalar
>>> result, not a result set.
>>
>> Do you mean the function should return true or false based on the connection
>> status with the provided arguments?
>>
>> I also feel difficult to understand the function result as compared to a view.
>
> An hba lookup is essentially a lookup by user name, database name,
> client address, yielding an authentication method (possibly with
> parameters).  So I think this function should work that way as well:
> arguments are user name, database name, and so on, and the return value
> is an authentication method.  Maybe it would be some kind of record,
> with line number and some parameters.
>
> That would address the use case I put forth above.  I don't know whether
> that's what you were going for.

Thanks. Here I attached the poc patch that returns authentication method of the
first matched hba entry in pg_hba.conf with the given input values.
Currently these
functions returns text type. Based on the details required to be
printed, it can
be changed.

postgres=# select pg_hba_lookup('all', 'all');
 pg_hba_lookup
---------------
 trust
(1 row)

comments for the approach?

Regards,
Hari Babu
Fujitsu Australia

On Wed, Nov 25, 2015 at 7:18 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-11-25 8:05 GMT+01:00 Haribabu Kommi <kommi.haribabu@gmail.com>:
>>
>>
>> Thanks. Here I attached the poc patch that returns authentication method
>> of the
>> first matched hba entry in pg_hba.conf with the given input values.
>> Currently these
>> functions returns text type. Based on the details required to be
>> printed, it can
>> be changed.
>>
>> postgres=# select pg_hba_lookup('all', 'all');
>>  pg_hba_lookup
>> ---------------
>>  trust
>> (1 row)
>>
>> comments for the approach?
>
>
> From my perspective, it shows too less informations.
>
> What I am expecting:
>
> 1. line num of choosed rule
> 2. some tracing - via NOTICE, what and why some rules was skipped.

Here I attached the patch with the suggested changes.
Along with line number, I kept the options column also with authentication
options as a jsonb datatype.

Example output:

postgres=# select pg_hba_lookup('test','all','::1');
NOTICE:  Skipped 84 Hba line, because of non matching IP.
NOTICE:  Skipped 86 Hba line, because of non matching database.
NOTICE:  Skipped 87 Hba line, because of non matching role.
 pg_hba_lookup
---------------
 (89,trust,{})
(1 row)

comments?

Regards,
Hari Babu
Fujitsu Australia

> >> Here I attached the patch with the suggested changes.
> >> Along with line number, I kept the options column also with authentication
> >> options as a jsonb datatype.
> >>
> >> Example output:
> >>
> >> postgres=# select pg_hba_lookup('test','all','::1');
> >> NOTICE:  Skipped 84 Hba line, because of non matching IP.
> >> NOTICE:  Skipped 86 Hba line, because of non matching database.
> >> NOTICE:  Skipped 87 Hba line, because of non matching role.
> >>  pg_hba_lookup
> >> ---------------
> >>  (89,trust,{})
> >> (1 row)
> >>
> >> comments?

I don't like this interface.  It's nice for psql, but everybody else is
going to lose.  I think these should be reported in the SRF result set
as well; perhaps add a "mode" column that says "skipped" for such rows,
and "matched" for the one that, uh, matches.  (Please try calling your
function with "select * from" which should give nicer output.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Dec 4, 2015 at 8:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> >> Here I attached the patch with the suggested changes.
>> >> Along with line number, I kept the options column also with authentication
>> >> options as a jsonb datatype.
>> >>
>> >> Example output:
>> >>
>> >> postgres=# select pg_hba_lookup('test','all','::1');
>> >> NOTICE:  Skipped 84 Hba line, because of non matching IP.
>> >> NOTICE:  Skipped 86 Hba line, because of non matching database.
>> >> NOTICE:  Skipped 87 Hba line, because of non matching role.
>> >>  pg_hba_lookup
>> >> ---------------
>> >>  (89,trust,{})
>> >> (1 row)
>> >>
>> >> comments?
>
> I don't like this interface.  It's nice for psql, but everybody else is
> going to lose.  I think these should be reported in the SRF result set
> as well; perhaps add a "mode" column that says "skipped" for such rows,
> and "matched" for the one that, uh, matches.  (Please try calling your
> function with "select * from" which should give nicer output.)
>

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)


In the above case, all the columns are displayed. Based on the
feedback we can keep
the required columns. I didn't yet removed the NOTICE messages in the
attached version.
Are they still required?


Regards,
Hari Babu
Fujitsu Australia

On Fri, Dec 4, 2015 at 7:45 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> this tracing can be implemented to main pg_hba processing. When you are
> connect from some specific client - and you can see, why you cannot to
> connect to Postgres

The trace messages that are going to print doesn't come to client until the
connection gets successful. The traces may not useful for the clients
to find out
why the connection is failing. But it may be useful for administrators.
How about the attached patch?

[kommih@localhost bin]$ ./psql postgres -h ::1
psql (9.6devel)
Type "help" for help.

postgres=#

ServerLog:
NOTICE:  Skipped 84 pg_hba line, because of host connection type.
NOTICE:  Skipped 86 pg_hba line, because of non matching IP.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

> The trace messages that are going to print doesn't come to client until the
> connection gets successful. The traces may not useful for the clients
> to find out
> why the connection is failing. But it may be useful for administrators.
> How about the attached patch?
> 
> [kommih@localhost bin]$ ./psql postgres -h ::1
> psql (9.6devel)
> Type "help" for help.
> 
> postgres=#
> 
> ServerLog:
> NOTICE:  Skipped 84 pg_hba line, because of host connection type.
> NOTICE:  Skipped 86 pg_hba line, because of non matching IP.

That's going to be way too noisy.  Some applications open dozens of
connections per second -- imagine a dozen NOTICEs per each connection
established.  It's going to fill any disk you install as the server log
partition ...

I can imagine worse nightmares, but this one's a pretty ugly one.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

What happens if a "reject" line is matched?  I hope the lookup
would terminate there.

What does it mean to query for "all"?  Do you have database and user
named "all"?  Because otherwise that seems wrong to me; you should be
able to query for specific databases/users, but not for special
keywords; maybe I am wrong and there is a use case for this, in which
case please state what it is.

I see three problems in your code.  One is that the translation of
auth_method enum to text should be a separate function, not the SQL
function layer; another is that the code to put keywords as JSON object
values is way too repetitive; the other is that messing with the JSON
API like that is not nice.  (I don't think we're closed to doing that,
but that would be a separate discussion).  I think this patch should
just use the "push value" interface rather than expose add_jsonb.

(I assume the usage of JSON rather than a regular array was already
discussed and JSON was chosen for some reason.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Pavel Stehule wrote:

> It should be disabled by default
> 
> only when you have some problems, then you can enable it

That still seems mostly unworkable to me.  Are we going to tell DBAs to
set PGOPTIONS when they have some pg_hba problem?

What's the issue with calling the function when you want to research
some problem?  Presumably that's the whole point of the function.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Dec 5, 2015 at 3:31 AM, Alvaro Herrera <alvherre@2ndquadrant.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.

> What happens if a "reject" line is matched?  I hope the lookup
> would terminate there.

whenever any line matches with the given arguments, the function stops
processing further lines.

> What does it mean to query for "all"?  Do you have database and user
> named "all"?  Because otherwise that seems wrong to me; you should be
> able to query for specific databases/users, but not for special
> keywords; maybe I am wrong and there is a use case for this, in which
> case please state what it is.

The 'all' is just passed as a database and user name. In my configuration
I just put every database to match. so just for a test i did that way. There is
no special handling for keywords.

> I see three problems in your code.  One is that the translation of
> auth_method enum to text should be a separate function, not the SQL
> function layer;

Moved into a different function.

>another is that the code to put keywords as JSON object
> values is way too repetitive; the other is that messing with the JSON
> API like that is not nice.  (I don't think we're closed to doing that,
> but that would be a separate discussion).  I think this patch should
> just use the "push value" interface rather than expose add_jsonb.
>
> (I assume the usage of JSON rather than a regular array was already
> discussed and JSON was chosen for some reason.)

Repetitive jsonb object code is moved into a function and used those functions.
Changed all jsonb calls into push value functions.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila <amit.kapila16@gmail.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

On Wed, Dec 9, 2015 at 5:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Another bigger issue I see in the above part of code is that it doesn't
> seem to be safe to call load_hba() at that place in PostgresMain() as
> currently load_hba() is using a context created from PostmasterContext
> to perform the parsing and some other stuff, the PostmasterContext
> won't be available at that time.  It is deleted immediately after
> InitPostgres
> is completed.  So either we need to make PostmasterContext don't go
> away after InitPostgres() or load_hba shouldn't use it and rather use
> CurrentMemroyContext similar to ProcessConfigFile or may be use
> TopMemoryContext instead of PostmasterContext if possible.  I think
> this needs some more thoughts.
>
> Apart from above, this patch doesn't seem to work on Windows or
> for EXEC_BACKEND builds as we are loading the hba file in a
> temporary context (PostmasterContext, refer PerformAuthentication)
> which won't be alive for the backends life.  This works on linux because
> of fork/exec mechanism which allows to inherit the parsed file
> (parsed_hba_lines). This is slightly tricky problem to solve and we
> have couple of options (a) use TopMemoryContext instead of Postmaster
> Context to load hba; (b) Use CurrentMemoryContext (c) pass the parsed
> hba file for Windows/Exec_Backend using save_backend_variables/
> restore_backend_variables mechanism or if you have any other idea.
> If you don't have any better idea, then you can evaluate above ideas
> and see which one makes more sense.

Reverting the context release patch is already provided in the first
mail of this
thread [1]. Forgot to mention about the same in further mails.

Here I attached the same patch. This patch needs to be applied first before
pg_hba_lookup patch. I tested it in windows version also.

[1] - http://www.postgresql.org/message-id/CAJrrPGfFyf45mfK7uB+QHWhXn_tTMkNRVhTuDefQzuZZRwEeQg@mail.gmail.com

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

> Reverting the context release patch is already provided in the first
> mail of this
> thread [1]. Forgot to mention about the same in further mails.
> 
> Here I attached the same patch. This patch needs to be applied first before
> pg_hba_lookup patch. I tested it in windows version also.

So if you change the file and reload repeatedly, we leak all the memory
allocated for HBA lines in TopMemoryContext?  This doesn't sound great.
Perhaps we need a dedicated context which can be reset at will so that
it can be refilled with the right info when we reload the file.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Dec 10, 2015 at 4:22 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Haribabu Kommi wrote:
>
>> Reverting the context release patch is already provided in the first
>> mail of this
>> thread [1]. Forgot to mention about the same in further mails.
>>
>> Here I attached the same patch. This patch needs to be applied first before
>> pg_hba_lookup patch. I tested it in windows version also.
>
> So if you change the file and reload repeatedly, we leak all the memory
> allocated for HBA lines in TopMemoryContext?  This doesn't sound great.
> Perhaps we need a dedicated context which can be reset at will so that
> it can be refilled with the right info when we reload the file.

No. There is no leaks associated with pg_hba.conf parsing. we already have
a memory context called "hba parser context" allocated from Postmaster
context. The "revert_hba_context_release_in_backend" patch changes it to
TopMemoryContext. The memory required for parsing and storing parsed
hba lines is obtained from this context.


Regards,
Hari Babu
Fujitsu Australia

On Thu, Dec 10, 2015 at 2:29 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Dec 10, 2015 at 6:46 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Thu, Dec 10, 2015 at 4:22 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>> > Haribabu Kommi wrote:
>> >
>> >> Reverting the context release patch is already provided in the first
>> >> mail of this
>> >> thread [1]. Forgot to mention about the same in further mails.
>> >>
>> >> Here I attached the same patch. This patch needs to be applied first
>> >> before
>> >> pg_hba_lookup patch. I tested it in windows version also.
>> >
>> > So if you change the file and reload repeatedly, we leak all the memory
>> > allocated for HBA lines in TopMemoryContext?  This doesn't sound great.
>> > Perhaps we need a dedicated context which can be reset at will so that
>> > it can be refilled with the right info when we reload the file.
>>
>> No. There is no leaks associated with pg_hba.conf parsing. we already have
>> a memory context called "hba parser context" allocated from Postmaster
>> context. The "revert_hba_context_release_in_backend" patch changes it to
>> TopMemoryContext. The memory required for parsing and storing parsed
>> hba lines is obtained from this context.
>>
>
> tokenize_file() is called before creation of hba parser context, so below
> change would be problem.
>
> *** 386,392 **** tokenize_file(const char *filename, FILE *file,
>
>   MemoryContext linecxt;
>
>   MemoryContext oldcxt;
>
>
>
> ! linecxt = AllocSetContextCreate(CurrentMemoryContext,
>
>   "tokenize file cxt",
>
>   ALLOCSET_DEFAULT_MINSIZE,
>
>   ALLOCSET_DEFAULT_INITSIZE,
>
> --- 386,392 ----
>
>   MemoryContext linecxt;
>
>   MemoryContext oldcxt;
>
>
>
> ! linecxt = AllocSetContextCreate(TopMemoryContext,
>
>   "tokenize file cxt",
>
>   ALLOCSET_DEFAULT_MINSIZE,
>
>   ALLOCSET_DEFAULT_INITSIZE,
>
>
> How about creating "hba parser context" and "ident parser context"
> at the beginning of their respective functions and don't change
> anything in tokenize_file()?

The tokenize file cxt is deleted after a successful load of pg_hba.conf or
pg_ident.conf files. we don't need this memory once the pg_hba.conf
or pg_ident file is loaded, because of this reason, it is created as a
separate context and deleted later.

Regards,
Hari Babu
Fujitsu Australia

On Thu, Dec 10, 2015 at 4:33 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Dec 10, 2015 at 9:51 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Thu, Dec 10, 2015 at 2:29 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>
>> > How about creating "hba parser context" and "ident parser context"
>> > at the beginning of their respective functions and don't change
>> > anything in tokenize_file()?
>>
>> The tokenize file cxt is deleted after a successful load of pg_hba.conf or
>> pg_ident.conf files. we don't need this memory once the pg_hba.conf
>> or pg_ident file is loaded, because of this reason, it is created as a
>> separate context and deleted later.
>>
>
> What about the error case?

Yes, One error case is possible when the length of the string crosses
the MAX_LINE size.
If we allocate the tokenize file cxt inside CurrentMemoryContext (i.e
MessageContext)
instead of TopMemoryContext, it will automatically freed later in case
if exists.


Regards,
Hari Babu
Fujitsu Australia

Hi,

I've reviewed the patch today, after re-reading the whole discussion.

Overall I'm quite happy with the design - a function is certainly better 
for the use-case. Not just because of the keyword handling issues, but 
because the checks happen in a particular order and terminate once a 
match is found, and that's very difficult to do with a view. So +1 to 
the function approach. Also +1 to abandoning the NOTICE idea and just 
generating rows.

The one unsolved issue is what to do with 1e24cf64. My understanding is 
that the current patch still requires reverting of that patch, but my 
feeling is TL won't be particularly keen about doing that. Or am I 
missing something?

The current patch (v6) also triggers a few warnings during compilation, 
about hostname/address being unitialized in pg_hba_lookup(). That 
happens because 'address' is only set when (! PG_ARGISNULL(2)). Fixing 
it is as simple as
    char    *address = NULL;    char    *hostname = NULL;

at the beginning of the function (this seems correct to me).

The current patch also does not handle 'all' keywords correctly - it 
apparently just compares the values as strings, which is incorrect. For 
example this
    SELECT * FROM pg_hba_lookup('all', 'all')

matches this pg_hba.conf line
    local    all    all    trust

That's clearly incorrect, as Alvaro pointed out.

I'm also wondering whether we really need three separate functions in 
pg_proc.
    pg_hba_lookup(database, user)    pg_hba_lookup(database, user, address)    pg_hba_lookup(database, user, address,
ssl_inuse)

Clearly, that's designed to match the local/host/hostssl/hostnossl cases 
available in pg_hba. But why not to simply use default values instead?
    pg_hba_lookup(database TEXT, user TEXT,                  address TEXT DEFAULT NULL,                  ssl_inuse
BOOLEANDEFAULT NULL)
 


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Dec 16, 2015 at 8:19 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> Hi,
>
> I've reviewed the patch today, after re-reading the whole discussion.

Thanks for the review.

> The one unsolved issue is what to do with 1e24cf64. My understanding is that
> the current patch still requires reverting of that patch, but my feeling is
> TL won't be particularly keen about doing that. Or am I missing something?

Until pg_hba_lookup function, the parsed hba lines are not used in the backend.
These are used only postmaster process for the authentication. As the parsed
hba lines occupy extra memory in the backend process which is of no use.
Because of this reason TL has changed it to PostmasterContext instead of
TopMemoryContext.

> The current patch (v6) also triggers a few warnings during compilation,
> about hostname/address being unitialized in pg_hba_lookup(). That happens
> because 'address' is only set when (! PG_ARGISNULL(2)). Fixing it is as
> simple as
>
>     char    *address = NULL;
>     char    *hostname = NULL;
>
> at the beginning of the function (this seems correct to me).

corrected.

> The current patch also does not handle 'all' keywords correctly - it
> apparently just compares the values as strings, which is incorrect. For
> example this
>
>     SELECT * FROM pg_hba_lookup('all', 'all')
>
> matches this pg_hba.conf line
>
>     local    all    all    trust
>
> That's clearly incorrect, as Alvaro pointed out.

In the above case, the 'all' is taken as a database and user names.
The pg_hba line contains the keyword of 'all' as database and user.
This line can match with any database and user names provided
by the user. Because of this reason, it matches with the first line
of pg_hba.conf.

I feel it is fine. Please let me know if you are expecting a different
behavior.

> I'm also wondering whether we really need three separate functions in
> pg_proc.
>
>     pg_hba_lookup(database, user)
>     pg_hba_lookup(database, user, address)
>     pg_hba_lookup(database, user, address, ssl_inuse)
>
> Clearly, that's designed to match the local/host/hostssl/hostnossl cases
> available in pg_hba. But why not to simply use default values instead?
>
>     pg_hba_lookup(database TEXT, user TEXT,
>                   address TEXT DEFAULT NULL,
>                   ssl_inuse BOOLEAN DEFAULT NULL)
>

Function is changed to accept default values.

Apart from the above, added a local memory context to allocate the memory
required for forming tuple for each line. This context resets for every hba line
to avoid consuming unnecessary memory for scenarios of huge pg_hba.conf
files.

In the revert_hba_context_release_in_backend patch, apart from reverting
the commit - 1e24cf64. In tokenize_file function, changed the new context
allocation from CurrentMemoryContext instead of TopMemoryContext.

Patch apply process:
1. revert_hba_context_release_in_backend_2.patch
2. pg_hba_lookup_poc_v7.patch

Regards,
Hari Babu
Fujitsu Australia

On Wed, Dec 23, 2015 at 8:54 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Wed, Dec 16, 2015 at 9:33 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>>
>> Function is changed to accept default values.
>>
>> Apart from the above, added a local memory context to allocate the memory
>> required for forming tuple for each line. This context resets for every
>> hba line
>> to avoid consuming unnecessary memory for scenarios of huge pg_hba.conf
>> files.
>>
>> In the revert_hba_context_release_in_backend patch, apart from reverting
>> the commit - 1e24cf64. In tokenize_file function, changed the new context
>> allocation from CurrentMemoryContext instead of TopMemoryContext.
>>
>> Patch apply process:
>> 1. revert_hba_context_release_in_backend_2.patch
>> 2. pg_hba_lookup_poc_v7.patch
>
>
> Hello,
>
> 1. Have you considered re-loading the HBA file upon call to this function in
> a local context instead of keeping it in the backends memory?  I do not
> expect that the revert of 1e24cf645d24aab3ea39a9d259897fd0cae4e4b6 would be
> accepted, as the commit message refers to potential security problems with
> keeping this data in backend memory:
>
>     ... This saves a
>     probably-usually-negligible amount of space per running backend.  It
> also
>     avoids leaving potentially-security-sensitive data lying around in
> memory
>     in processes that don't need it.  You'd have to be unusually paranoid to
>     think that that amounts to a live security bug, so I've not gone so far
> as
>     to forcibly zero the memory; but there surely isn't a good reason to
> keep
>     this data around.

Yes, it is possible to load the file locally whenever the lookup
function is called.
Only thing i am considering is performance impact because of huge file load
whenever the function is called.

> 2. I also wonder why JSONB arrays for database/user instead of TEXT[]?

When I first tried this functionality as a view, it became very
difficult to deal with
keyword database and user names, so at that time, it was thought to use jsonb
instead of text[], thinking of easy to handle the keywords. But later decided to
drop the view approach itself. I can change it if others also feel the same.

> 3. What happens with special keywords for database column like
> sameuser/samerole/samegroup and for special values in the user column?

There is no special handling for the keywords in this approach. Based on the
inputs to the function, it checks for the matched line in all hba lines.

For example, if a configuration line contains 'all' for database and user names,
then if user gives any database name and user name this line will be matched
and returned.

> 4. Would it be possible to also include the raw unparsed line from the HBA
> file?  Just the line number is probably enough when you have access to the
> host, but to show the results to someone else you might need to copy the raw
> line manually.  Not a big deal anyway.

IMO as we are already showing all line information in columns
separately, it may not
be looks good.

> 5. Some tests demonstrating possible output would be really nice to have.

Do you mean regression tests? In case of install check case, the results are
based on the server configuration that is running. It may be difficult to write
tests to pass in all scenarios. Because of this reason i didn't add them.


Regards,
Hari Babu
Fujitsu Australia

On Wed, Dec 23, 2015 at 8:56 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Dec 23, 2015 at 8:54 PM, Shulgin, Oleksandr <oleksandr.shulgin@zalando.de> wrote:
>> 5. Some tests demonstrating possible output would be really nice to have.
>
> Do you mean regression tests? In case of install check case, the results are
> based on the server configuration that is running. It may be difficult to write
> tests to pass in all scenarios. Because of this reason i didn't add them.

We have an infrastructure in src/test/perl to set up servers with
specific configurations and write regression tests. You should just
use that,

(Moved this patch to next CF as discussion is still actively going on).
-- 
Michael

"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:
> 1. Have you considered re-loading the HBA file upon call to this function
> in a local context instead of keeping it in the backends memory?

Aside from the security questions, please consider that this feature should
work similarly to the current implementation of the pg_file_settings view,
namely it tells you about what is *currently* in the on-disk files, not
necessarily what is the active setting in the postmaster's memory.
A backend could not be entirely sure about the postmaster's state anyway;
and even if it could be, one of the major applications for features like
this is testing manual changes to the files before you SIGHUP the
postmaster.  So re-reading the files on each usage is a Good Thing, IMO,
even if it sounds inefficient.

> 2. I also wonder why JSONB arrays for database/user instead of TEXT[]?

Yes, that seems rather random to me too.
        regards, tom lane

On Thu, Dec 24, 2015 at 2:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:
>> 1. Have you considered re-loading the HBA file upon call to this function
>> in a local context instead of keeping it in the backends memory?
>
> Aside from the security questions, please consider that this feature should
> work similarly to the current implementation of the pg_file_settings view,
> namely it tells you about what is *currently* in the on-disk files, not
> necessarily what is the active setting in the postmaster's memory.
> A backend could not be entirely sure about the postmaster's state anyway;
> and even if it could be, one of the major applications for features like
> this is testing manual changes to the files before you SIGHUP the
> postmaster.  So re-reading the files on each usage is a Good Thing, IMO,
> even if it sounds inefficient.
>
>> 2. I also wonder why JSONB arrays for database/user instead of TEXT[]?
>
> Yes, that seems rather random to me too.

Here I attached updated patch with the following changes,
- Local loading of HBA file to show the authentication data
- Changed database and user types are text[]

Regards,
Hari Babu
Fujitsu Australia

On Mon, Dec 28, 2015 at 9:09 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Thu, Dec 24, 2015 at 5:16 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Thu, Dec 24, 2015 at 2:37 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > "Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:
>> >> 1. Have you considered re-loading the HBA file upon call to this
>> >> function
>> >> in a local context instead of keeping it in the backends memory?
>> >
>> > Aside from the security questions, please consider that this feature
>> > should
>> > work similarly to the current implementation of the pg_file_settings
>> > view,
>> > namely it tells you about what is *currently* in the on-disk files, not
>> > necessarily what is the active setting in the postmaster's memory.
>> > A backend could not be entirely sure about the postmaster's state
>> > anyway;
>> > and even if it could be, one of the major applications for features like
>> > this is testing manual changes to the files before you SIGHUP the
>> > postmaster.  So re-reading the files on each usage is a Good Thing, IMO,
>> > even if it sounds inefficient.
>> >
>> >> 2. I also wonder why JSONB arrays for database/user instead of TEXT[]?
>> >
>> > Yes, that seems rather random to me too.
>>
>> Here I attached updated patch with the following changes,
>> - Local loading of HBA file to show the authentication data
>> - Changed database and user types are text[]
>
>
> Still this requires a revert of the memory context handling commit for
> load_hba() and load_ident().  I think you can get around the problem by
> changing these functions to work with CurrentMemoryContext and set it
> explicitly to the newly allocated PostmasterContext in
> PerformAuthentication().  In your function you could then create a temporary
> context to be discarded before leaving the function.

Thanks for the review. I didn't understand your point clearly.

In the attached patch, load_hba uses PostmasterContext if it is present,
otherwise CurretMemoryContext. PostmasterContext is present only
in the backend start phase.

> I still think you should not try to re-implement check_hba(), but extend
> this function with means to report line skip reasons as per your
> requirements.  Having an optional callback function might be a good fit (a
> possible use case is logging the reasons line by line).

check_hba function is enhanced to fill the hba line details with
reason for mismatch.
In check_hba function whenever a mismatch is found, the fill_hbaline function is
called to frame the tuple and inserted into tuple store.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Dec 30, 2015 at 1:07 AM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
>
> This is close enough, but what I actually mean by "a callback" is more or
> less like the attached version.

Thanks for the changes.

> While at it, I've also added some trivial code to preserve keyword quoting
> in database and user fields, as well as added netmask output parameter; also
> documentation is extended a little.

Thanks for the documentation changes and regarding the quoting, in any system
catalog table, the quoted objects are represented without quotes as below.

postgres=> select datname from pg_database;
    datname
---------------
 postgres
 template1
 template0
 test_user2_db
 TEST_USER1_DB
 test_user2_dB
(6 rows)

Adding quotes to pg_hba_lookup function makes it different from others.
The issues regarding the same is already discussed in [1].

select a.database[1], b.datname from pg_hba_lookup('postgres','kommih','::1')
                            as a, pg_database as b where a.database[1]
= b.datname;

The queries like above are not possible with quoted output. It is very
rare that the
pg_hba_lookup function used in join operations, but still it is better
to provide
data without quotes. so I reverted these changes in the attached latest patch.

> The biggest question for me is the proper handling of memory contexts for
> HBA and ident files data.  I think it makes sense to release them explicitly
> because with the current state of affairs, we have dangling pointers in
> parsed_{hba,ident}_{context,lines} after release of PostmasterContext.  The
> detailed comment in postgres.c around
> MemoryContextDelete(PostmasterContext); suggests that it's not easy already
> to keep track of the all things that might be affected by this cleanup step:
>
> /*
> * If the PostmasterContext is still around, recycle the space; we don't
> * need it anymore after InitPostgres completes.  Note this does not trash
> * *MyProcPort, because ConnCreate() allocated that space with malloc()
> * ... else we'd need to copy the Port data first.  Also, subsidiary data
> * such as the username isn't lost either; see ProcessStartupPacket().
> */
>
> Not sure if we need any advanced machinery here like some sort of cleanup
> hooks list?  For now I've added discard_{hba,ident}() functions and call
> them explicitly where appropriate.

The added functions properly frees the hba and ident contexts once its use is
finished. I removed the discard function calls in PerformAuthentication function
in EXEC_BACKEND mode, as these are called once the PerformAuthenication
function finishes.

The discard hba and ident context patch is separated from
pg_hba_lookup patch for
easier understanding. The pg_hba_lookup patch needs to be applied on top of
discard_hba_and_ident_cxt patch.

[1] http://www.postgresql.org/message-id/CAFj8pRARzDScocMK30gyYdOgiwuTUcZ7EvE-bBg+wV2Wg5EQRQ@mail.gmail.com

Regards,
Hari Babu
Fujitsu Australia

On Wed, Dec 30, 2015 at 9:48 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Wed, Dec 30, 2015 at 4:31 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>>
>> Adding quotes to pg_hba_lookup function makes it different from others.
>> The issues regarding the same is already discussed in [1].
>>
>> select a.database[1], b.datname from
>> pg_hba_lookup('postgres','kommih','::1')
>>                             as a, pg_database as b where a.database[1]
>> = b.datname;
>>
>> The queries like above are not possible with quoted output. It is very
>> rare that the
>> pg_hba_lookup function used in join operations, but still it is better
>> to provide
>> data without quotes. so I reverted these changes in the attached latest
>> patch.
>
>
> That's a good point.  I wonder that maybe instead of re-introducing quotes
> we could somehow make the unquoted keywords that have special meaning stand
> out, e.g:
>
> database  | {$sameuser}
> user_name | {$all}
>
> That should make it obvious which of the values are placeholders and doesn't
> interfere with joining database or user catalogs (while I would call
> "sameuser" a very unlikely name for a database, "all" might be not that
> unlikely name for a user, e.g. someone called like "Albert L. Lucky" could
> use that as a login name).

It is not only the problem with joins, the following two cases works
without quotes only.
With quotes the query doesn't match with the database name.

select * from pg_hba_lookup('Test', 'kommih','127.0.0.1') where
database = '{"Test"}';
select * from pg_hba_lookup('Test', 'kommih','127.0.0.1') where
database = '{Test}';


Regards,
Hari Babu
Fujitsu Australia

On Thu, Dec 31, 2015 at 10:47 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Dec 30, 2015 at 9:48 PM, Shulgin, Oleksandr
> <oleksandr.shulgin@zalando.de> wrote:
>> On Wed, Dec 30, 2015 at 4:31 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
>> wrote:
>>>
>>>
>>> Adding quotes to pg_hba_lookup function makes it different from others.
>>> The issues regarding the same is already discussed in [1].
>>>
>>> select a.database[1], b.datname from
>>> pg_hba_lookup('postgres','kommih','::1')
>>>                             as a, pg_database as b where a.database[1]
>>> = b.datname;
>>>
>>> The queries like above are not possible with quoted output. It is very
>>> rare that the
>>> pg_hba_lookup function used in join operations, but still it is better
>>> to provide
>>> data without quotes. so I reverted these changes in the attached latest
>>> patch.
>>
>>
>> That's a good point.  I wonder that maybe instead of re-introducing quotes
>> we could somehow make the unquoted keywords that have special meaning stand
>> out, e.g:
>>
>> database  | {$sameuser}
>> user_name | {$all}
>>
>> That should make it obvious which of the values are placeholders and doesn't
>> interfere with joining database or user catalogs (while I would call
>> "sameuser" a very unlikely name for a database, "all" might be not that
>> unlikely name for a user, e.g. someone called like "Albert L. Lucky" could
>> use that as a login name).
>
> It is not only the problem with joins, the following two cases works
> without quotes only.
> With quotes the query doesn't match with the database name.
>
> select * from pg_hba_lookup('Test', 'kommih','127.0.0.1') where
> database = '{"Test"}';
> select * from pg_hba_lookup('Test', 'kommih','127.0.0.1') where
> database = '{Test}';

Hi, Do you have any further comments on the patch that needs to be
taken care?

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

> Hi, Do you have any further comments on the patch that needs to be
> taken care?

I do.  I think the jsonb functions you added should be added to one of
the json .c files instead, since they seem of general applicability.
But actually, I don't think you have ever replied to the question of why
are you using JSON in the first place; isn't a normal array suitable?

The callback stuff is not documented in check_hba() at all.  Can you
please add an explanation just above the function, so that people trying
to use it know what can the callback be used for?  Also a few lines
above the callback itself would be good.  Also, the third argument of
check_hba() is a translatable message so you should enclose it in _() so
that it is picked up for translation.  The "skipped"/"matched" words
(and maybe others?) need to be marked similarly.

That "Failed" in the errmsg in pg_hba_lookup() should be lowercase.

Moving to next CF.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Feb 2, 2016 at 8:57 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Haribabu Kommi wrote:
>
>> Hi, Do you have any further comments on the patch that needs to be
>> taken care?
>
> I do.  I think the jsonb functions you added should be added to one of
> the json .c files instead, since they seem of general applicability.

moved these functions into jsonb_util.c file.

> But actually, I don't think you have ever replied to the question of why
> are you using JSON in the first place; isn't a normal array suitable?

It was discussed and told to use JSON for options instead of array in [1],
because of that reason I changed.

> The callback stuff is not documented in check_hba() at all.  Can you
> please add an explanation just above the function, so that people trying
> to use it know what can the callback be used for?  Also a few lines
> above the callback itself would be good.

Added some details in explaining the call back function.

>Also, the third argument of
> check_hba() is a translatable message so you should enclose it in _() so
> that it is picked up for translation.  The "skipped"/"matched" words
> (and maybe others?) need to be marked similarly.

Changed mode column (skipped/matched) and reason for mismatch details
are enclosed in _() for translation. Do you find any other details needs to be
enclosed?

> That "Failed" in the errmsg in pg_hba_lookup() should be lowercase.

corrected.

> Moving to next CF.

Thanks. updated patch is attached with the above corrections.
This patch needs to be applied on top discard_hba_and_ident_cxt patch
that is posted earlier.

[1] - http://www.postgresql.org/message-id/5547DB0A.2020904@gmx.net

Regards,
Hari Babu
Fujitsu Australia

On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
>This patch needs to be applied on top discard_hba_and_ident_cxt patch
>that is posted earlier.

Here I attached a re-based patch to the latest head with inclusion of
discard_hba_ident_cxt patch for easier review as a single patch.

Regards,
Hari Babu
Fujitsu Australia

On 3/3/16 12:16 AM, Haribabu Kommi wrote:
> On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>>
>> This patch needs to be applied on top discard_hba_and_ident_cxt patch
>> that is posted earlier.
> 
> Here I attached a re-based patch to the latest head with inclusion of
> discard_hba_ident_cxt patch for easier review as a single patch.

Alex, Scott, do you have an idea of when you'll be able to review this
new version?

It applies with a minor conflict (caused by pg_control_* commit):

$ git apply -3 ../other/pg_hba_lookup_poc_v13.patch
error: patch failed: src/include/utils/builtins.h:1151
Falling back to three-way merge...
Applied patch to 'src/include/utils/builtins.h' with conflicts.
U src/include/utils/builtins.h

Thanks,
-- 
-David
david@pgmasters.net

On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Tue, Mar 15, 2016 at 7:23 PM, David Steele <david@pgmasters.net> wrote:
>>
>> On 3/3/16 12:16 AM, Haribabu Kommi wrote:
>> > On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi
>> > <kommi.haribabu@gmail.com> wrote:
>> >>
>> >> This patch needs to be applied on top discard_hba_and_ident_cxt patch
>> >> that is posted earlier.
>> >
>> > Here I attached a re-based patch to the latest head with inclusion of
>> > discard_hba_ident_cxt patch for easier review as a single patch.
>>
>> Alex, Scott, do you have an idea of when you'll be able to review this
>> new version?
>
>
> The new version applies with some fuzziness to the current master and
> compiles cleanly.
>
> Some comments:
>
> +/* Context to use with hba_line_callback function. */
> +typedef struct
> +{
> +   MemoryContext memcxt;
> +   TupleDesc   tupdesc;
> +   Tuplestorestate *tuple_store;
> +}  HbalineContext;
>
> Rather "with *lookup_hba_line_callback*", as hba_line_callback() is a
> generic one.

Fine. I will change the function and context names.

> + line_number |  mode   | type  | database | user_name |  address  |
> netmask                 | hostname | method | options |          reason
>
+-------------+---------+-------+----------+-----------+-----------+-----------------------------------------+----------+--------+---------+--------------------------
> +          84 | skipped | local | {all}    | {all}     |           |
> |          | trust  | {}      | connection type mismatch
> +          86 | skipped | host  | {all}    | {all}     | 127.0.0.1 |
> 255.255.255.255                         |          | trust  | {}      | IP
> address mismatch
> +          88 | matched | host  | {all}    | {all}     | ::1       |
> ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff |          | trust  | {}      |
>
> Hm... now I'm not sure if we really need the "mode" column.  It should be
> clear that we skipped every line that had a non-NULL "reason".  I guess we
> could remove "mode" and rename "reason" to "skip_reason"?

Ok. Lets hear from others also regarding the same.

> Still remains an issue of representing special keywords in database and
> user_name fields, but there was no consensus about that though.

How about adding keyword_database and keyword_user columns to listing
out the keywords.  These columns will be populated only when the hba line
contains any keywords.


Regards,
Hari Babu
Fujitsu Australia

On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Thu, Mar 17, 2016 at 2:12 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
>> <oleksandr.shulgin@zalando.de> wrote:
>> >
>> > Some comments:
>> >
>> > +/* Context to use with hba_line_callback function. */
>> > +typedef struct
>> > +{
>> > +   MemoryContext memcxt;
>> > +   TupleDesc   tupdesc;
>> > +   Tuplestorestate *tuple_store;
>> > +}  HbalineContext;
>> >
>> > Rather "with *lookup_hba_line_callback*", as hba_line_callback() is a
>> > generic one.
>>
>> Fine. I will change the function and context names.
>
>
> You mean change context name and correct the comment?  I didn't suggest to
> change the function name.

Changed the context name and the comment only.

>> > Still remains an issue of representing special keywords in database and
>> > user_name fields, but there was no consensus about that though.
>>
>> How about adding keyword_database and keyword_user columns to listing
>> out the keywords.  These columns will be populated only when the hba line
>> contains any keywords.
>
>
> Hm... that could work too.

Here I attached patch with the added two keyword columns.
During the testing with different IP comparison methods like 'samehost' or
'samenet', the address details are not displayed. Is there any need of
showing the IP compare method also?

Regards,
Hari Babu
Fujitsu Australia

On Fri, Mar 18, 2016 at 7:46 PM, Shulgin, Oleksandr
<oleksandr.shulgin@zalando.de> wrote:
> On Fri, Mar 18, 2016 at 7:53 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr
>> <oleksandr.shulgin@zalando.de> wrote:
>> >
>> > You mean change context name and correct the comment?  I didn't suggest
>> > to
>> > change the function name.
>>
>> Changed the context name and the comment only.
>
> Check.
>
> +} lookup_hba_line_context;
> ^^^^^ but why TAB here?

corrected. I am not sure why pg_indent is adding a tab here.

>> >> > Still remains an issue of representing special keywords in database
>> >> > and
>> >> > user_name fields, but there was no consensus about that though.
>> >>
>> >> How about adding keyword_database and keyword_user columns to listing
>> >> out the keywords.  These columns will be populated only when the hba
>> >> line
>> >> contains any keywords.
>> >
>> >
>> > Hm... that could work too.
>>
>> Here I attached patch with the added two keyword columns.
>
> + if (!tok->quoted && strcmp(tok->string, "all") == 0)
>
> token_is_keyword(tok, "all") ?

updated.

>> During the testing with different IP comparison methods like 'samehost' or
>> 'samenet', the address details are not displayed. Is there any need of
>> showing the IP compare method also?
>
> Do you mean return "samehost/samenet/all" in the yet another keyword_address
> out parameter or something like that?

Yes, Currently other than IP/hostname, if the user specifies
samehost/samenet/all
keywords, currently these are not displayed. I feel adding another column of
keyword_address is worth.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

> > Check.
> >
> > +} lookup_hba_line_context;
> > ^^^^^ but why TAB here?
> 
> corrected. I am not sure why pg_indent is adding a tab here.

It's because lookup_hba_line_context is not listed in typedefs.list.
I suggest adding it and all other new typedefs you add, and rerunning
pgindent, as the lack of those may affect other places where those names
appear.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Haribabu Kommi wrote:
>
>> > Check.
>> >
>> > +} lookup_hba_line_context;
>> > ^^^^^ but why TAB here?
>>
>> corrected. I am not sure why pg_indent is adding a tab here.
>
> It's because lookup_hba_line_context is not listed in typedefs.list.
> I suggest adding it and all other new typedefs you add, and rerunning
> pgindent, as the lack of those may affect other places where those names
> appear.

Thanks for the details. I added the new typedef into typedefs.list file.
Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Mar 21, 2016 at 3:31 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Haribabu Kommi wrote:
>>
>>> > Check.
>>> >
>>> > +} lookup_hba_line_context;
>>> > ^^^^^ but why TAB here?
>>>
>>> corrected. I am not sure why pg_indent is adding a tab here.
>>
>> It's because lookup_hba_line_context is not listed in typedefs.list.
>> I suggest adding it and all other new typedefs you add, and rerunning
>> pgindent, as the lack of those may affect other places where those names
>> appear.
>
> Thanks for the details. I added the new typedef into typedefs.list file.
> Updated patch is attached.

This patch is still marked "needs review".  If it's ready to go, one
of the reviewers should mark it "ready for committer".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/5/16 9:52 PM, Robert Haas wrote:

> On Mon, Mar 21, 2016 at 3:31 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>> Haribabu Kommi wrote:
>>>
>>>>> Check.
>>>>>
>>>>> +} lookup_hba_line_context;
>>>>> ^^^^^ but why TAB here?
>>>>
>>>> corrected. I am not sure why pg_indent is adding a tab here.
>>>
>>> It's because lookup_hba_line_context is not listed in typedefs.list.
>>> I suggest adding it and all other new typedefs you add, and rerunning
>>> pgindent, as the lack of those may affect other places where those names
>>> appear.
>>
>> Thanks for the details. I added the new typedef into typedefs.list file.
>> Updated patch is attached.
> 
> This patch is still marked "needs review".  If it's ready to go, one
> of the reviewers should mark it "ready for committer".

Can one of the reviewers decide if this is ready to commit?  I fear it
will be pushed to the next CF otherwise.  I don't think the committers
have time to make that determination today...

-- 
-David
david@pgmasters.net

On Fri, Apr 8, 2016 at 3:24 PM, David Steele <david@pgmasters.net> wrote:
> On 4/5/16 9:52 PM, Robert Haas wrote:
>> On Mon, Mar 21, 2016 at 3:31 AM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>>> On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera
>>> <alvherre@2ndquadrant.com> wrote:
>>>> Haribabu Kommi wrote:
>>>>
>>>>>> Check.
>>>>>>
>>>>>> +} lookup_hba_line_context;
>>>>>> ^^^^^ but why TAB here?
>>>>>
>>>>> corrected. I am not sure why pg_indent is adding a tab here.
>>>>
>>>> It's because lookup_hba_line_context is not listed in typedefs.list.
>>>> I suggest adding it and all other new typedefs you add, and rerunning
>>>> pgindent, as the lack of those may affect other places where those names
>>>> appear.
>>>
>>> Thanks for the details. I added the new typedef into typedefs.list file.
>>> Updated patch is attached.
>>
>> This patch is still marked "needs review".  If it's ready to go, one
>> of the reviewers should mark it "ready for committer".
>
> Can one of the reviewers decide if this is ready to commit?  I fear it
> will be pushed to the next CF otherwise.  I don't think the committers
> have time to make that determination today...

Well, it's not getting committed unless some committer determines that
it is ready to commit.  As far as 9.6 goes, that committer will not be
me; my commit bit is starting to smoke, and anything I try to do in
the next few hours is likely to have an unacceptable error rate.  I am
beat.  But I think we have a good release to look forward to.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 8, 2016 at 3:24 PM, David Steele <david@pgmasters.net> wrote:
>> Can one of the reviewers decide if this is ready to commit?  I fear it
>> will be pushed to the next CF otherwise.  I don't think the committers
>> have time to make that determination today...

> Well, it's not getting committed unless some committer determines that
> it is ready to commit.

I took a quick look; IMO it is certainly not ready to commit.

* Why is the IP address parameter declared as "text" and not "inet"?
Why is it optional --- it doesn't seem to me that it can have a useful
default value?

* Docs claim that ssl_inuse is of type text, when it's really bool,
and that the result is record when it's really setof record.

* While I agree that allowing ssl_inuse to default to false is probably
okay, I wonder how well this function signature will cope if we ever add
more things that pg_hba matching depends on.

* The patch seems mighty invasive to the auth code, which is not really
a place where we want a lot of churn and opportunity for mistakes.  Is
there another way to do this?  Do we really *need* a line-by-line report
of why specific lines didn't match?

* I'm a tad suspicious of the memory management, in particular the
random-looking rearrangement of where PostmasterContext gets created
and deleted.  It'd likely be better to fix things so that load_hba
doesn't have a hard-wired reference to PostmasterContext in the first
place, but is told which context to allocate under.


More generally, I'm not convinced about the use-case for this patch.
What problem is it supposed to help in dealing with, exactly?  Not syntax
errors in the hba file, evidently, since it doesn't make any attempt to
instrument the file parser.  And it's not very clear that it'll help
with "I can't connect", either, because if you can't connect you're
not going to be running this function.

If people actually need more help in figuring out why the hba line matcher
selected the line it did, I'm inclined to think maybe what's needed is a
logging option that would print a verbose trace of match/no-match
decisions.  More or less the same data this returns, really, but directed
to the postmaster log.  That way you'd be able to see it even when you're
not getting let into the database.

On the whole, though, I wonder if we shouldn't just tweak log_connections
so that it records which pg_hba line was matched.  (We already have
logging about which line was matched on an auth failure.)  I'm not really
convinced that a SQL function like this is going to be very helpful.
        regards, tom lane

On Fri, Apr 8, 2016 at 4:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Apr 8, 2016 at 3:24 PM, David Steele <david@pgmasters.net> wrote:
>>> Can one of the reviewers decide if this is ready to commit?  I fear it
>>> will be pushed to the next CF otherwise.  I don't think the committers
>>> have time to make that determination today...
>
>> Well, it's not getting committed unless some committer determines that
>> it is ready to commit.
>
> I took a quick look; IMO it is certainly not ready to commit.

OK, marked Returned with Feedback.  Hopefully the patch authors will
find your feedback useful.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Apr 9, 2016 at 6:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> More generally, I'm not convinced about the use-case for this patch.
> What problem is it supposed to help in dealing with, exactly?  Not syntax
> errors in the hba file, evidently, since it doesn't make any attempt to
> instrument the file parser.  And it's not very clear that it'll help
> with "I can't connect", either, because if you can't connect you're
> not going to be running this function.

Apologies for replying an old thread.

The main reason behind of this patch is for the administrators to control
and verify the authentication mechanism that was deployed for several
users in the database is correctly picking the assigned hba config or not?

I feel this SQL function is useful for administrators and not for normal
users.

If anyone is not against to the above use case, i will update the patch based
on the review comments and post it later.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Sat, Apr 9, 2016 at 6:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More generally, I'm not convinced about the use-case for this patch.
>> What problem is it supposed to help in dealing with, exactly?  Not syntax
>> errors in the hba file, evidently, since it doesn't make any attempt to
>> instrument the file parser.  And it's not very clear that it'll help
>> with "I can't connect", either, because if you can't connect you're
>> not going to be running this function.

> Apologies for replying an old thread.

> The main reason behind of this patch is for the administrators to control
> and verify the authentication mechanism that was deployed for several
> users in the database is correctly picking the assigned hba config or not?

That doesn't really answer my question: what is a concrete use-case for
this function?  Reproducing the same behavior that would happen during
a login attempt does not seem terribly useful from here, because you
could simply attempt to log in, instead.  As I said upthread, maybe we
need a bit more logging in the authentication logic, but that doesn't
seem to lead to wanting a SQL function.

What I actually think we could use is something like the pg_file_settings
view, but for pg_hba.conf.  In particular, pg_file_settings has a specific
charter of being able to detect syntax errors in not-yet-loaded config
files.  That seems like clearly useful functionality to me, but we don't
have it for pg_hba.conf (and this patch didn't add it).
        regards, tom lane