Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
Haribabu Kommi writes: > On Sat, Apr 9, 2016 at 6:36 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Sat, Apr 9, 2016 at 6:36 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Apr 8, 2016 at 4:36 PM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Apr 8, 2016 at 3:24 PM, David Steele 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
Robert Haas writes: > On Fri, Apr 8, 2016 at 3:24 PM, David Steele 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Apr 8, 2016 at 3:24 PM, David Steele wrote: > On 4/5/16 9:52 PM, Robert Haas wrote: >> On Mon, Mar 21, 2016 at 3:31 AM, Haribabu Kommi >> wrote: >>> On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera >>> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Mon, Mar 21, 2016 at 3:31 AM, Haribabu Kommi wrote: > On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Mon, Mar 21, 2016 at 2:00 PM, Alvaro Herrera 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 pg-hba-lookup-21-03-2016_1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Mar 18, 2016 at 7:46 PM, Shulgin, Oleksandr wrote: > On Fri, Mar 18, 2016 at 7:53 AM, Haribabu Kommi > wrote: >> >> On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr >> 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 pg-hba-lookup-21-03-2016.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Mar 18, 2016 at 7:53 AM, Haribabu Kommi wrote: > > On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr > 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? > >> > 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") ? > 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? -- Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Mar 17, 2016 at 2:12 AM, Haribabu Kommi
wrote:
> On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
> 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.
> 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.
--
Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
wrote:
> On Tue, Mar 15, 2016 at 7:23 PM, David Steele wrote:
>>
>> On 3/3/16 12:16 AM, Haribabu Kommi wrote:
>> > On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi
>> > 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 |
> ::::::: | | 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr
wrote:
> On Thu, Mar 17, 2016 at 2:12 AM, Haribabu Kommi
> wrote:
>>
>> On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
>> 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
pg-hba-lookup-18-03-2016.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Tue, Mar 15, 2016 at 7:23 PM, David Steele wrote:
> On 3/3/16 12:16 AM, Haribabu Kommi wrote:
> > On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi
> 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.
+ 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 |
::::::: | | 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"?
Still remains an issue of representing special keywords in database and
user_name fields, but there was no consensus about that though.
--
Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi 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 pg_hba_lookup_poc_v13.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Tue, Feb 2, 2016 at 8:57 AM, Alvaro Herrera 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.2020...@gmx.net Regards, Hari Babu Fujitsu Australia pg_hba_lookup_poc_v12.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 31, 2015 at 10:47 AM, Haribabu Kommi
wrote:
> On Wed, Dec 30, 2015 at 9:48 PM, Shulgin, Oleksandr
> wrote:
>> On Wed, Dec 30, 2015 at 4:31 AM, Haribabu Kommi
>> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 30, 2015 at 9:48 PM, Shulgin, Oleksandr
wrote:
> On Wed, Dec 30, 2015 at 4:31 AM, Haribabu Kommi
> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 30, 2015 at 4:31 AM, Haribabu Kommi
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).
--
Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 30, 2015 at 1:07 AM, Shulgin, Oleksandr
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+wv2wg5e...@mail.gmail.com
Regards,
Hari Babu
Fujitsu Australia
pg_hba_lookup_poc_v11.patch
Description: Binary data
discard_hba_and_ident_cxt.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Tue, Dec 29, 2015 at 4:15 AM, Haribabu Kommi
wrote:
> On Mon, Dec 28, 2015 at 9:09 PM, Shulgin, Oleksandr
> wrote:
> >
> > 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.
>
This is close enough, but what I actually mean by "a callback" is more or
less like the attached version.
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.
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.
--
Alex
From 56b584c25b952d6855d2a3d77eb40755d982efb9 Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin
Date: Tue, 29 Dec 2015 14:51:27 +0100
Subject: [PATCH] WIP: pg_hba_lookup()
---
doc/src/sgml/func.sgml | 39 +++
src/backend/catalog/system_views.sql | 9 +
src/backend/libpq/hba.c | 663 ++-
src/backend/utils/init/postinit.c| 26 +-
src/include/catalog/pg_proc.h| 2 +
src/include/libpq/hba.h | 4 +
src/include/utils/builtins.h | 3 +
7 files changed, 720 insertions(+), 26 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index e08bf60..2594dd5
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT collation for ('foo' COLLATE "de_
*** 16576,16581
--- 16576,16594
text
set parameter and return new value
+
+
+
+ pg_hba_lookup
+
+ pg_hba_lookup(database text,
+ user_name text
+ [, address text]
+ [, ssl_inuse text)
+
+record
+scan pg_hba.conf and return examined entries up to a matching one
+
*** SELECT set_config('log_statement_stats',
*** 16633,16638
--- 16646,16677
+
+ pg_hba_lookup returns a set of records containing the
+ line number, mode, type, database, user_name, address, netmask, hostname,
+ method, options and skip reason. For example, to debug problems with user
+ kommih trying to connect to a database postgres
+ from IPv6-address ::1, one can issue a following query:
+
+ postgres=# SELECT * FROM pg_hba_lookup('postgres', 'kommih', '::1');
+ line_number | mode | type | database | user_name | address | netmask | hostname | method | options | reason
+ -+-+---+--+---+---+-+--++-+--
+ 84 | skipped | local | {all}| {all} | | | | trust | {} | connection type mismatch
+
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Mon, Dec 28, 2015 at 9:09 PM, Shulgin, Oleksandr wrote: > On Thu, Dec 24, 2015 at 5:16 AM, Haribabu Kommi > wrote: >> >> On Thu, Dec 24, 2015 at 2:37 AM, Tom Lane wrote: >> > "Shulgin, Oleksandr" 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 pg_hba_lookup_poc_v9.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 24, 2015 at 5:16 AM, Haribabu Kommi wrote: > On Thu, Dec 24, 2015 at 2:37 AM, Tom Lane wrote: > > "Shulgin, Oleksandr" 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. 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). Thank you. -- Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 24, 2015 at 2:37 AM, Tom Lane wrote: > "Shulgin, Oleksandr" 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 pg_hba_lookup_poc_v8.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
"Shulgin, Oleksandr" 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 23, 2015 at 12:56 PM, Haribabu Kommi wrote: > On Wed, Dec 23, 2015 at 8:54 PM, Shulgin, Oleksandr > wrote: > > > > 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. > Not sure why do you call it huge? Anyway since this is going to be used to debug connection problems, I would expect the performance impact to be insignificant. On the other hand, re-loading the HBA file in the function will enable the DBA to test if a proposed change to the HBA file fixes the client problem w/o making the server to reload the config. > 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. > Now I wonder why are you trying to re-implement the checks found in hba.c's check_hba() instead of extending this function to provide textual reason(s) for skipping/rejection? -- Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 23, 2015 at 8:56 PM, Haribabu Kommi wrote: > On Wed, Dec 23, 2015 at 8:54 PM, Shulgin, Oleksandr > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 23, 2015 at 8:54 PM, Shulgin, Oleksandr wrote: > On Wed, Dec 16, 2015 at 9:33 AM, Haribabu Kommi > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 16, 2015 at 9:33 AM, Haribabu Kommi 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. 2. I also wonder why JSONB arrays for database/user instead of TEXT[]? 3. What happens with special keywords for database column like sameuser/samerole/samegroup and for special values in the user column? 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. 5. Some tests demonstrating possible output would be really nice to have. Cheers! -- Alex
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 16, 2015 at 8:19 AM, Tomas Vondra
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
>
> localallalltrust
>
> 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
revert_hba_context_release_in_backend_2.patch
Description: Binary data
pg_hba_lookup_poc_v7.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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
localallalltrust
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 BOOLEAN DEFAULT NULL)
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 10, 2015 at 4:33 PM, Amit Kapila wrote: > On Thu, Dec 10, 2015 at 9:51 AM, Haribabu Kommi > wrote: >> >> On Thu, Dec 10, 2015 at 2:29 PM, Amit Kapila >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 10, 2015 at 9:51 AM, Haribabu Kommi wrote: > > On Thu, Dec 10, 2015 at 2:29 PM, Amit Kapila 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? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 10, 2015 at 2:29 PM, Amit Kapila wrote: > On Thu, Dec 10, 2015 at 6:46 AM, Haribabu Kommi > wrote: >> >> On Thu, Dec 10, 2015 at 4:22 AM, Alvaro Herrera >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 9, 2015 at 2:35 PM, 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. > Thanks, that is helpful. However I think it is better if you can always keep the link of related patches at end of mail. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 10, 2015 at 6:46 AM, Haribabu Kommi wrote: > > On Thu, Dec 10, 2015 at 4:22 AM, Alvaro Herrera > 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()? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Thu, Dec 10, 2015 at 4:22 AM, Alvaro Herrera 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 9, 2015 at 5:31 PM, Amit Kapila 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_ttmknrvhtudefqzuzzrwe...@mail.gmail.com Regards, Hari Babu Fujitsu Australia revert_hba_context_release_in_backend.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 9, 2015 at 6:48 AM, Haribabu Kommi
wrote:
>
> On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila
wrote:
> >
> > 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?
>
I think we can live without such a check especially because you
already have required check for function args in pg_hba_lookup
function.
> > 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.
>
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.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Dec 9, 2015 at 2:36 AM, Amit Kapila 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
pg_hba_lookup_poc_v6.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Tue, Dec 8, 2015 at 9:41 AM, Haribabu Kommi
wrote:
>
> On Sat, Dec 5, 2015 at 3:31 AM, Alvaro Herrera
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
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Sat, Dec 5, 2015 at 3:31 AM, Alvaro Herrera 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
pg_hba_lookup_poc_v5.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-04 17:34 GMT+01:00 Alvaro Herrera : > 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? > why not - it isn't bad idea. > > What's the issue with calling the function when you want to research > some problem? Presumably that's the whole point of the function. > sometimes you shouldn't set real parameters - I had to solve some issues with IP6/IP4 - and I missed debug info on server side. Regards Pavel > > -- > Álvaro Herrerahttp://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-04 17:16 GMT+01:00 Alvaro Herrera : > 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. > It should be disabled by default only when you have some problems, then you can enable it Regards Pavel > > -- > Álvaro Herrerahttp://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-04 5:48 GMT+01:00 Haribabu Kommi : > On Fri, Dec 4, 2015 at 7:45 AM, Pavel Stehule > 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? > yes, it is only for admin and should be stored to log > > [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 >
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-04 5:33 GMT+01:00 Haribabu Kommi :
> On Fri, Dec 4, 2015 at 8:05 AM, Alvaro Herrera
> 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)
>
the tabular interface is better, and then NOTICEs are not necessary. I
like to see some info why row was skipped in the table.
Regards
Pavel
>
>
> 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
>
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Dec 4, 2015 at 7:45 AM, Pavel Stehule 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 hba_trace.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Fri, Dec 4, 2015 at 8:05 AM, Alvaro Herrera 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
pg_hba_lookup_poc_v4.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
> >> 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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-03 5:53 GMT+01:00 Pavel Stehule :
>
>
> 2015-12-03 5:00 GMT+01:00 Haribabu Kommi :
>
>> On Wed, Nov 25, 2015 at 7:18 PM, Pavel Stehule
>> wrote:
>> >
>> >
>> > 2015-11-25 8:05 GMT+01:00 Haribabu Kommi :
>> >>
>> >>
>> >> 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?
>>
>
> I liked it
>
> The text of notice can be reduced "Skipped xx line, ..." - it have to be
> pg_hba
>
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
Pavel
>
> Pavel
>
>
>>
>> Regards,
>> Hari Babu
>> Fujitsu Australia
>>
>
>
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-12-03 5:00 GMT+01:00 Haribabu Kommi :
> On Wed, Nov 25, 2015 at 7:18 PM, Pavel Stehule
> wrote:
> >
> >
> > 2015-11-25 8:05 GMT+01:00 Haribabu Kommi :
> >>
> >>
> >> 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?
>
I liked it
The text of notice can be reduced "Skipped xx line, ..." - it have to be
pg_hba
Pavel
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Wed, Nov 25, 2015 at 7:18 PM, Pavel Stehule wrote:
>
>
> 2015-11-25 8:05 GMT+01:00 Haribabu Kommi :
>>
>>
>> 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
pg_hba_lookup_poc_v3.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
2015-11-25 8:05 GMT+01:00 Haribabu Kommi :
> On Tue, Nov 17, 2015 at 9:37 AM, Peter Eisentraut wrote:
> > On 11/16/15 2:37 AM, Haribabu Kommi wrote:
> >> On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut
> 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?
>
>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.
Regards
Pavel
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Tue, Nov 17, 2015 at 9:37 AM, Peter Eisentraut wrote:
> On 11/16/15 2:37 AM, Haribabu Kommi wrote:
>> On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut 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
pg_hba_lookup_poc_v2.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On 11/16/15 2:37 AM, Haribabu Kommi wrote: > On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Mon, Nov 16, 2015 at 2:30 PM, Peter Eisentraut 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
Hi
2015-07-21 11:15 GMT+02:00 Haribabu Kommi :
> 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)
>
>
After testing your prototype I am thinking so it is not good way. It is
hard to understand what is result of these functions.
-1 from me
Regards
Pavel
>
> 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/f40b0968db0a904da78a924e633be78645f...@sydexchtmp2.au.fjanz.com
>
> [2]
> http://www.postgresql.org/message-id/e1zaquy-00072j...@gemulon.postgresql.org
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
On Mon, Sep 7, 2015 at 4:34 AM, Pavel Stehule 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
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.
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.
Regards
Pavel
Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries
Hi > Any suggestions/comments on this proposed approach? > This is interesting functionality - The same was requested by one important Czech customer. I'll do review Regards Pavel
