Providing catalog view to pg_hba.conf file - Patch submission

On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi <
vaishnavip@fast.au.fujitsu.com> wrote:

Hi,

In connection to my previous proposal about "providing catalog view to
pg_hba.conf file contents" , I have developed the attached patch .

[Current situation]

Currently, to view the pg_hba.conf file contents, DB admin has to access
the file from database server to read the settings. In case of huge and
multiple hba files, finding the appropriate hba rules which are loaded will
be difficult and take some time.

[What this Patch does]

Functionality of the attached patch is that it will provide a new view
"pg_hba_settings" to admin users. Public access to the view is restricted.
This view will display basic information about HBA setting details of
postgresql cluster. Information to be shown , is taken from parsed hba
lines and not directly read from pg_hba.conf files. Documentation files are
also updated to include details of this new view under "Chapter 47.System
Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"

[Advantage]

Advantage of having this "pg_hba_settings" view is that the admin can
check, what hba rules are loaded in runtime via database connection itself.
And, thereby it will be easy and useful for admin to check all the users
with their privileges in a single view to manage them.

This looks like a useful feature, so make sure you register it on
https://commitfest.postgresql.org/action/commitfest_view?id=22.

I haven't looked at the actual code yet, btu I did notice one thing at a
very quick lookover at the docs - it seems to be completely ignoring the
key/value parameters given on a row, and stops reporting after the auth
method? That seems bad. And also, probably host/mask should be using the
inet style datatypes and not text?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: Magnus Hagander [mailto:magnus@hagander.net]
Sent: Friday, 14 March 2014 9:33 PM
To: Prabakaran, Vaishnavi
Cc: PostgreSQL-development
Subject: Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission

On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi <vaishnavip@fast.au.fujitsu.com<mailto:vaishnavip@fast.au.fujitsu.com>> wrote:
Hi,

In connection to my previous proposal about "providing catalog view to pg_hba.conf file contents" , I have developed the attached patch .

[Current situation]
Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time.

[What this Patch does]
Functionality of the attached patch is that it will provide a new view "pg_hba_settings" to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under "Chapter 47.System Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"

[Advantage]
Advantage of having this "pg_hba_settings" view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them.

This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22.

I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and >stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text?

Agree, am now working on including a new column "configuration_option" to display the key/value parameter set. I will send the updated patch once after adding new column.
Host/mask values are stored as sockaddr_storage structure in parsed_hba_lines, so I have used text datatype to display the hostname.

Thanks & Regards,
Vaishnavi
Fujitsu Australia
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Friday, Mar 14, 2014 at 9:33 PM, Maganus Hagander <magnus@hagander.net<mailto:magnus@hagander.net> > wrote:

Hi,
In connection to my previous proposal about "providing catalog view to pg_hba.conf file contents" , I have developed the attached patch .
[Current situation]
Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time.

[What this Patch does]
Functionality of the attached patch is that it will provide a new view "pg_hba_settings" to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be >>shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under "Chapter 47.System Catalogs". Also , a new note is added in "chapter 19.1 The >>pg_hba.conf File"
[Advantage]
Advantage of having this "pg_hba_settings" view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single >>view to manage them.

This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22.

Sure, I will add it to commitfest.

I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and >stops reporting after the auth method? That seems bad. And also, >probably host/mask should be using the inet style datatypes and not text?

Added new column "configuration_option" to pg_hba_settings view to display the key/value parameter set. Attached the updated patch.

Thanks & Regards,
Vaishnavi
Fujitsu Australia

On Fri, Mar 14, 2014 at 12:30 AM, Prabakaran, Vaishnavi
<vaishnavip@fast.au.fujitsu.com> wrote:

Hi,

In connection to my previous proposal about "providing catalog view to
pg_hba.conf file contents" , I have developed the attached patch .

[...]

[What this Patch does]

Functionality of the attached patch is that it will provide a new view
"pg_hba_settings" to admin users. Public access to the view is restricted.
This view will display basic information about HBA setting details of
postgresql cluster. Information to be shown , is taken from parsed hba
lines and not directly read from pg_hba.conf files. Documentation files are
also updated to include details of this new view under "Chapter 47.System
Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"

A normal user can see all the info the view provide once you GRANT
permissions on it. How much info should a non-superuser see from this
view? currently a non-superuser can't see pg_hba info, now it can.

This function should be superuser only or only show info related for
current_user if it user is not superuser.

Also, i think you should use lowercase values just they are in
pg_hba.conf (ie: local not Local, host not Host, etc)

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi Vaishnavi.

In addition to Jaime's comments about the functionality, here are are
some comments about the code.

Well, they were supposed to be comments about the code, but it turns out
I have comments about the feature as well. We need to figure out what to
do about the database and user columns. Returning an array containing
e.g. "{all}" won't fly. We must distinguish between "all" and "{all}".

I don't have a good solution, other than returning two columns each: one
a string, and one an array, only one of them set for any given record.

+ int
+ GetNumHbaLines(void)
+ {

Please add a blank line before this.

+ /*
+  * Fetches the HbaLine corresponding to linenum variable.
+  * Fill the values required to build a tuple, of view pg_hba_settings, in values pointer.
+  */
+ void
+ GetHbaLineByNum(int linenum, const char **values)
+ {

I suggest naming this function GetHbaValuesByLinenum() and changing the
comment to "Fill in suitable values to build a tuple representing the
HbaLine corresponding to the given linenum".

Actually, maybe it should be hba_getvaluesbyline() for consistency with
the other functions in the file? In that case, the preceding function
should also be renamed to hba_getnumlines().

+ 	/* Retrieving connection type */
+ 	switch (hba->conntype)
+ 	{

The comment should be just "Connection type". Generally speaking, all of
the "Retrieving" comments should be changed similarly. Or just removed.

+ 		case ctLocal:
+ 			values[0] = pstrdup("Local");
+ 			break;

I agree with Jaime: this should be lowercase. And do you really need to
pstrdup() these strings?

+ 	appendStringInfoString(&str, "{");
+ 	foreach(dbcell, hba->databases)
+ 	{
+ 		tok = lfirst(dbcell);
+ 		appendStringInfoString(&str, tok->string);
+ 		appendStringInfoString(&str, ",");
+ 	}
+ 
+ 	/*
+ 	 * For any number of values inserted, separator at the end needs to be
+ 	 * replaced by string terminator
+ 	 */
+ 	if (str.len > 1)
+ 	{
+ 		str.data[str.len - 1] = '\0';
+ 		str.len -= 1;
+ 		appendStringInfoString(&str, "}");
+ 		values[1] = str.data;
+ 	}
+ 	else
+ 		values[1] = NULL;

I don't like this at all. I would write it something like this:

n = 0;

/* hba->conntype stuff */

n++;
if (list_length(hba->databases) != 0)
{
initStringInfo(&str);
appendStringInfoString(&str, "{");

foreach(cell, hba->databases)
{
tok = lfirst(cell);
appendStringInfoString(&str, tok->string);
appendStringInfoString(&str, ",");
}

str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

Note the variable n instead of using 0/1/… indexes into values, and that
I moved the call to initStringInfo from the beginning of the function.

The same applies to the other cases below.

(But this is, of course, all subject to whatever solution is found to
the all/{all} problem.)

/* Retrieving Socket address */
switch (hba->addr.ss_family)
{
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
#ifdef HAVE_IPV6
case AF_INET6:
len = sizeof(struct sockaddr_in6);
break;
#endif
default:
len = sizeof(struct sockaddr_storage);
break;
}
if (getnameinfo((const struct sockaddr *) & hba->addr, len, buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) == 0)
values[3] = pstrdup(buffer);
else
values[3] = NULL;

This should use pg_getnameinfo_all. You don't need the switch, just pass
in .salen. Use a buffer of NI_MAXHOST, not 256. Look for other calls to
pg_getnameinfo_all for examples. (Also split long lines.)

(Note: this pstrdup is OK.)

/* Retrieving socket mask */
switch (hba->mask.ss_family)
{
case AF_INET:

Same.

+ 		case ipCmpMask:
+ 			values[5] = pstrdup("Mask");
+ 			break;

Lowercase, no pstrdup.

+ 		case uaReject:
+ 			values[7] = pstrdup("Reject");
+ 			break;

Same.

+ if ((hba->usermap) && (hba->auth_method == uaIdent || hba->auth_method == uaPeer || hba->auth_method == uaCert || hba->auth_method == uaGSS || hba->auth_method == uaSSPI))

Split across lines.

+ appendStringInfoString(&str, pstrdup(hba->ldapserver));

No need for the many, many pstrdup()s.

+ 	if (str.len > 1)
+ 	{
+ 		str.data[str.len - 1] = '\0';
+ 		str.len -= 1;
+ 		appendStringInfoString(&str, "}");
+ 		values[8] = str.data;
+ 	}
+ 	else
+ 		values[8] = NULL;

This should become:

if (str.len != 0)
{
str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

+ 	/* Retrieving linenumber */
+ 	snprintf(buffer, sizeof(buffer), "%d", hba->linenumber);
+ 	values[9] = pstrdup(buffer);

Use psprintf() and get rid of the static buffer.

+ 	/* stuff done only on the first call of the function */
+ 	if (SRF_IS_FIRSTCALL())
+ 	{
+ 		/* create a function context for cross-call persistence */
+ 		funcctx = SRF_FIRSTCALL_INIT();

This is not a comment about your patch, just a general complaint about
how everyone cargo-cults ALL of these comments for EVERY SINGLE SRF.
Isn't "if (SRF_IS_FIRSTCALL())" _perfectly_ clear, or is it just me?

I'd just as soon rip them all out of this patch.

+ 		TupleDescInitEntry(tupdesc, (AttrNumber) 1, "connection_type",
+ 						   TEXTOID, -1, 0);

(See comments below about column names.)

+ 	else
+ 	{
+ 		/* do when there is no more left */
+ 		SRF_RETURN_DONE(funcctx);
+ 	}

I'd just say SRF_RETURN_DONE(funcctx) at the end without the else block.

+ DATA(insert OID = 3206 (  pg_show_all_hba_settings  PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 "" "{25,1009,1009,25,25,25,25,25,1009,23}" "{o,o,o,o,o,o,o,o,o,o}" "{connection_type,databases,roles,socket_address,socket_mask,compare_method,hostname,authmethod,configuration_options,linenumber}" _null_ show_all_hba_settings _null_ _null_ _null_ ));
+ DESCR("SHOW ALL HBA settings as a function");

OID 3206 is now in use by the #>> operator, so you'll have to pick
another when you resubmit.

I went through the values one by one, and they look OK. But see comments
below about the column names.

The description should be "Return client authentication settings" or so.

+   <para>
+    The view <structname>pg_hba_settings</structname> provides access to
+    useful information about authentication details of Postgresql cluster.
+    For security reasons, access to this view is limited only to superusers. No update, insert or delete operations are allowed in this view. 
+   </para>

I suggest:

The read-only <structname>pg_hba_settings</structname> view provides
access to the client authentication configuration from pg_hba.conf.
Access to this view is limited to superusers.

(So this addresses Jaime's comment: the view is already superuser-only.)

+ <entry><structfield>connection_type</structfield></entry>

I'd call this just "type" to match pg_hba.conf.

+       <entry><structfield>databases</structfield></entry>
+       <entry><type>text[]</type></entry>
+       <entry>List of database names</entry>

I'm really not a fan of returning an array with e.g. "{all}". But I'm
not sure what to do instead. I think the really right thing to do would
be to have two separate columns, one with "all", "sameuser", "samerole",
"replication", or empty; and the other an array of database names.

I'd like to hear what others think about this.

+ <entry><structfield>roles</structfield></entry>

Same problem.

+ <entry><structfield>socket_address</structfield></entry>

Just "address".

+ <entry><structfield>socket_mask</structfield></entry>

Just "mask".

+ <entry><structfield>authmethod</structfield></entry>

Just "method".

+ <entry><structfield>configuration_options</structfield></entry>

Just "options".

+ <entry><structfield>linenumber</structfield></entry>

Should be "line_number", I think.

+    <para>
+     Details of this file can be accessed from pg_hba_settings view. 
+     See <xref linkend="view-pg-hba-settings"> for details.
+    </para>

I suggest "The contents of this file are reflected in the
pg_hba_settings view. See … for details."

Do you have the time to rework the patch along these lines and resubmit
for this commitfest (i.e. in the next few days, certainly sometime this
week)? If not, please let me know and I will move it to the next CF to
give you time to make the changes.

Thank you for your contribution. Although I've asked for a lot of
changes, I think the feature is certainly one we want.

-- Abhijit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

At 2014-06-29 22:25:54 +0530, ams@2ndQuadrant.com wrote:

I think the really right thing to do would be to have two separate
columns, one with "all", "sameuser", "samerole", "replication", or
empty; and the other an array of database names.

After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{"all"}' for '"all"'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.

I think having two columns would work. The columns could be called
"database" and "database_list" and "user" and "user_list" respectively.

The database column may contain one of "all", "sameuser", "samegroup",
"replication", but if it's empty, database_list will contain an array of
database names. Then ("all", {}) and ("", {all}) are easily separated.
Likewise for user and user_list.

I've marked this patch "returned with feedback" and moved it to the
August CF after discussion with Vaishnavi.

-- Abhijit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

I think having two columns would work. The columns could be called
"database" and "database_list" and "user" and "user_list" respectively.

The database column may contain one of "all", "sameuser", "samegroup",
"replication", but if it's empty, database_list will contain an array of
database names. Then ("all", {}) and ("", {all}) are easily separated.
Likewise for user and user_list.

Thanks for the review.

I corrected all the review comments except the one to add two columns
as (database, database_list and user, user_list). I feel this may cause
some confusion to the users.

Here I attached the latest version of the patch.
I will add this patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia

On 1/27/15 1:04 AM, Haribabu Kommi wrote:

On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

I think having two columns would work. The columns could be called
"database" and "database_list" and "user" and "user_list" respectively.

The database column may contain one of "all", "sameuser", "samegroup",
"replication", but if it's empty, database_list will contain an array of
database names. Then ("all", {}) and ("", {all}) are easily separated.
Likewise for user and user_list.

Thanks for the review.

I corrected all the review comments except the one to add two columns
as (database, database_list and user, user_list). I feel this may cause
some confusion to the users.

Here I attached the latest version of the patch.
I will add this patch to the next commitfest.

Apologies if this was covered, but why isn't the IP address an inet instead of text?

Also, what happens if someone reloads the config in the middle of running the SRF? ISTM it'd be better to do something like process all of parsed_hba_lines into a tuplestore. Obviously there's still a race condition there, but at least it's a lot smaller, and AFAIK no worse than the pg_stats views.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

On 1/27/15 1:04 AM, Haribabu Kommi wrote:

Here I attached the latest version of the patch.
I will add this patch to the next commitfest.

Apologies if this was covered, but why isn't the IP address an inet instead
of text?

Corrected the address type as inet instead of text. updated patch is attached.

Also, what happens if someone reloads the config in the middle of running
the SRF?

hba entries are reloaded only in postmaster process, not in every backend.
So there shouldn't be any problem with config file reload. Am i
missing something?

Regards,
Hari Babu
Fujitsu Australia

On Wed, Jan 28, 2015 at 4:46 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby <Jim.Nasby@bluetreble.com>

wrote:

On 1/27/15 1:04 AM, Haribabu Kommi wrote:

Here I attached the latest version of the patch.
I will add this patch to the next commitfest.

Apologies if this was covered, but why isn't the IP address an inet

instead

of text?

Corrected the address type as inet instead of text. updated patch is

attached.

Also, what happens if someone reloads the config in the middle of

running

the SRF?

hba entries are reloaded only in postmaster process, not in every backend.
So there shouldn't be any problem with config file reload. Am i
missing something?

I don't have any comment to disagree with the patch (idea and code).

But I'm thinking about this patch and would not be interesting to have a
FDW to manipulate the hba file? Imagine if we are able to manipulate the
HBA file using INSERT/UPDATE/DELETE.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Timbira: http://www.timbira.com.br
Blog: http://fabriziomello.github.io
Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Github: http://github.com/fabriziomello

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> writes:

But I'm thinking about this patch and would not be interesting to have a
FDW to manipulate the hba file? Imagine if we are able to manipulate the
HBA file using INSERT/UPDATE/DELETE.

Since the HBA file is fundamentally order-dependent, while SQL tables
are fundamentally not, that doesn't seem like a great API match. You
could probably brute-force something that would work, but it would very
much be a case of using a hammer to solve a screwdriver problem.

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

On 1/28/15 12:46 AM, Haribabu Kommi wrote:

Also, what happens if someone reloads the config in the middle of running
the SRF?

hba entries are reloaded only in postmaster process, not in every backend.
So there shouldn't be any problem with config file reload. Am i
missing something?

Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com> writes:

But I'm thinking about this patch and would not be interesting to have a
FDW to manipulate the hba file? Imagine if we are able to manipulate the
HBA file using INSERT/UPDATE/DELETE.

Since the HBA file is fundamentally order-dependent, while SQL tables
are fundamentally not, that doesn't seem like a great API match. You
could probably brute-force something that would work, but it would very
much be a case of using a hammer to solve a screwdriver problem.

Maybe, but my intention is provide an easy way to edit HBA entries. With an
extension or API to edit HBA entries many developers of PostgreSQL tools
(ie. pgadmin, phppgadmin, etc) will be benefited.

Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to
manipulate HBA entries like we did with ALTER SYSTEM. It's just some
thoughts about it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

Timbira: http://www.timbira.com.br
Blog: http://fabriziomello.github.io
Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Github: http://github.com/fabriziomello

On 1/29/15 6:19 AM, Fabr�zio de Royes Mello wrote:

On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello@gmail.com <mailto:fabriziomello@gmail.com>> writes:

But I'm thinking about this patch and would not be interesting to have a
FDW to manipulate the hba file? Imagine if we are able to manipulate the
HBA file using INSERT/UPDATE/DELETE.

Since the HBA file is fundamentally order-dependent, while SQL tables
are fundamentally not, that doesn't seem like a great API match. You
could probably brute-force something that would work, but it would very
much be a case of using a hammer to solve a screwdriver problem.

Maybe, but my intention is provide an easy way to edit HBA entries. With an extension or API to edit HBA entries many developers of PostgreSQL tools (ie. pgadmin, phppgadmin, etc) will be benefited.

Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to manipulate HBA entries like we did with ALTER SYSTEM. It's just some thoughts about it.

Aside from Tom's concern about sets not being a good way to handle this (which I agree with), the idea of "editing" pg_hba.conf via SQL raises all the problems that were brought up when ALTER SYSTEM was being developed. One of the big problems is a question of how you can safely modify a text file that's full of comments and what-not. You'd need to address those issues if you hope to modify pg_hba.conf via SQL.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 30, 2015 at 3:16 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

On 1/29/15 6:19 AM, Fabrízio de Royes Mello wrote:

Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax

to manipulate HBA entries like we did with ALTER SYSTEM. It's just some
thoughts about it.

Aside from Tom's concern about sets not being a good way to handle this

(which I agree with), the idea of "editing" pg_hba.conf via SQL raises all
the problems that were brought up when ALTER SYSTEM was being developed.
One of the big problems is a question of how you can safely modify a text
file that's full of comments and what-not. You'd need to address those
issues if you hope to modify pg_hba.conf via SQL.

I think the big problem you are mentioning can be resolved in
a similar way as we have done for ALTER SYSTEM which is
to have a separate file (.auto.conf) for settings done via
ALTER SYSTEM command, do you see any major problem
with that approach.

Here one thing that is different is the format of pg_hba.conf file
which is quite different from postgresql.conf file, it seems to me
if want to handle this via ALTER SYSTEM we need to have
additional/different syntax.

For Example,

ALTER SYSTEM AUTH TYPE = <type_value> DATABASE = <value> ...
or
ALTER SYSTEM AUTH ...

In second syntax, user can mention the whole record. Internally we
need to validate the syntax and values.

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

On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think the big problem you are mentioning can be resolved in
a similar way as we have done for ALTER SYSTEM which is
to have a separate file (.auto.conf) for settings done via
ALTER SYSTEM command, do you see any major problem
with that approach.

Yes. The contents of postgresql.conf are only mildly order-dependent.
If you put the same setting in more than once, it matters which one is
last. Apart from that, though, it doesn't really matter:
wal_keep_segments=10 means the same thing if it occurs before
max_connections=401 that it means after that. The same is not true of
pg_hba.conf, where the order matters a lot. This makes merging two
files together much less feasible, and much more confusing.

You are also a lot more likely to lock yourself out of the database by
adjusting pg_hba.conf. You can do that by modifying postgresql.conf,
say by putting an invalid combination of parameters in there or
getting it to request more semaphores or more RAM than the system can
accommodate or changing listen_addresses to 127.0.0.1, but there are
lots of things that you can do that carry no such risk. This is much
less true with pg_hba.conf. Even if I had a feature that would let me
modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use
it.

Overall, this seems to me like a can of worms better left unopened.

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

On 1/29/15 9:13 PM, Amit Kapila wrote:

Aside from Tom's concern about sets not being a good way to handle

this (which I agree with), the idea of "editing" pg_hba.conf via SQL
raises all the problems that were brought up when ALTER SYSTEM was being
developed. One of the big problems is a question of how you can safely
modify a text file that's full of comments and what-not. You'd need to
address those issues if you hope to modify pg_hba.conf via SQL.

I think the big problem you are mentioning can be resolved in
a similar way as we have done for ALTER SYSTEM which is
to have a separate file (.auto.conf) for settings done via
ALTER SYSTEM command, do you see any major problem
with that approach.

Yes I do. pg_hba.conf is completely depending on ordering, so there's no
way you can simply toss another file into the mix. It's bad enough that
we do that with postgresql.auto.conf, but at least that's a simple
over-ride. With HBA a single ALTER SYSTEM could activate (or deactivate)
a huge swath of pg_hba.conf. That makes for a system that's fragile, and
since it's security related, dangerous.

I could maybe see an interface where we allowed users to perform
line-level operations on pg_hba.conf via SQL: UPDATE line X, INSERT
BEFORE/AFTER line X, DELETE line X. At least that would preserve the
critical nature of rules ordering.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila <amit.kapila16@gmail.com>

wrote:

I think the big problem you are mentioning can be resolved in
a similar way as we have done for ALTER SYSTEM which is
to have a separate file (.auto.conf) for settings done via
ALTER SYSTEM command, do you see any major problem
with that approach.

Yes. The contents of postgresql.conf are only mildly order-dependent.
If you put the same setting in more than once, it matters which one is
last. Apart from that, though, it doesn't really matter:
wal_keep_segments=10 means the same thing if it occurs before
max_connections=401 that it means after that. The same is not true of
pg_hba.conf, where the order matters a lot.

Do you mean to say that as authentication system uses just the
first record that matches to perform authentication, it could lead
to problems if an order is not maintained? Won't the same
set of problems can occur if user tries to that manually and do
it without proper care of such rules. Now the problem with
command is that user can't see the order in which entries are
being made, but it seems to me that we can provide a view or some
way to user so that the order of entries is visible and the same is
allowed to be manipulated via command.

This makes merging two
files together much less feasible, and much more confusing.

You are also a lot more likely to lock yourself out of the database by
adjusting pg_hba.conf.

I think that could be even possible via Alter User .. password, if the
password is changed then also kind of user can be locked out of
database, basically it is also part of authentication mechanism.

Even if I had a feature that would let me
modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use
it.

Okay, but how about via some server side utility or some other way with
which users don't need to manually edit the file?

It seems to be that some of the other databases like Oracle also provide
a way for users to operate of similar files via commands, although in a
different way [1]http://docs.oracle.com/cd/B28359_01/network.111/b28317/lsnrctl.htm#i553656.

Overall, this seems to me like a can of worms better left unopened.

Sure, I can understand the dangers you want to highlight, however
OTOH it seems to me that providing some way to users with which
they can change things without manually editing file is a good move.

[1]: http://docs.oracle.com/cd/B28359_01/network.111/b28317/lsnrctl.htm#i553656
http://docs.oracle.com/cd/B28359_01/network.111/b28317/lsnrctl.htm#i553656

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

On 01/30/2015 10:01 PM, Amit Kapila wrote:

On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas <robertmhaas@gmail.com
<mailto:robertmhaas@gmail.com>> wrote:

Yes. The contents of postgresql.conf are only mildly order-dependent.
If you put the same setting in more than once, it matters which one is
last. Apart from that, though, it doesn't really matter:
wal_keep_segments=10 means the same thing if it occurs before
max_connections=401 that it means after that. The same is not true of
pg_hba.conf, where the order matters a lot.

Do you mean to say that as authentication system uses just the
first record that matches to perform authentication, it could lead
to problems if an order is not maintained? Won't the same
set of problems can occur if user tries to that manually and do
it without proper care of such rules. Now the problem with
command is that user can't see the order in which entries are
being made, but it seems to me that we can provide a view or some
way to user so that the order of entries is visible and the same is
allowed to be manipulated via command.

We *can*, yes. But the technical issues around that have not been
addressed. Certainly just making the new system view respond to
UPDATE/INSERT/DELETE would not be sufficient.

And then once we address the technical issues, we'll need to address the
security implications.

I think this is worth doing; there's some tremendous utility potential
in having a PostgresQL which can be 100% managed via port 5432,
especially for the emerging world of container-based hosting (Docker et.
al.). However, it's also going to be difficult.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers