Обсуждение: [PATCH] Add inline comments to the pg_hba_file_rules view
Hi, This patch proposes the column "comment" to the pg_hba_file_rules view. It basically parses the inline comment (if any) of a valid pg_hba.conf entry and displays it in the new column. For such pg_hba entries ... host db jim 127.0.0.1/32 md5 # foo host db jim 127.0.0.1/32 md5 #bar host db jim 127.0.0.1/32 md5 # #foo# ... it returns the following pg_hba_file_rules records: postgres=# SELECT type, database, user_name, address, comment FROM pg_hba_file_rules WHERE user_name[1]='jim'; type | database | user_name | address | comment ------+----------+-----------+-----------+--------- host | {db} | {jim} | 127.0.0.1 | foo host | {db} | {jim} | 127.0.0.1 | bar host | {db} | {jim} | 127.0.0.1 | #foo# (3 rows) This feature can come in quite handy when we need to read important comments from the hba entries without having access to the pg_hba.conf file directly. The patch slightly changes the test 004_file_inclusion.pl to accommodate the new column and the hba comments. Discussion: https://www.postgresql.org/message-id/flat/3fec6550-93b0-b542-b203-b0054aaee83b%40uni-muenster.de Best regards, Jim
Вложения
This is a very useful feature. I applied the patch to the master branch, and both make check and make check-world passed without any issues. Just one comment here, based on the example below, > host db jim 127.0.0.1/32 md5 # #foo# > > ... it returns the following pg_hba_file_rules records: > > postgres=# SELECT type, database, user_name, address, comment > FROM pg_hba_file_rules > WHERE user_name[1]='jim'; > > type | database | user_name | address | comment > ------+----------+-----------+-----------+--------- > host | {db} | {jim} | 127.0.0.1 | #foo# Since "only the first #" and "any leading spaces" are removed, IMO, it can be more accurate to say, Text after the first <literal>#</literal> comment character in the end of a valid <literal>pg_hba.conf</literal> entry, if any Best regards, David
Hi David On 09.09.23 01:52, David Zhang wrote: > This is a very useful feature. I applied the patch to the master > branch, and both make check and make check-world passed without any > issues. > Thanks for reviewing this patch! > > Since "only the first #" and "any leading spaces" are removed, IMO, it > can be more accurate to say, > > Text after the first <literal>#</literal> comment character in the end > of a valid <literal>pg_hba.conf</literal> entry, if any > I agree. v2 attached includes your suggestion. Thanks! Jim
Вложения
On Mon, Sep 04, 2023 at 12:54:15PM +0200, Jim Jones wrote: > The patch slightly changes the test 004_file_inclusion.pl to accommodate the > new column and the hba comments. > > Discussion: https://www.postgresql.org/message-id/flat/3fec6550-93b0-b542-b203-b0054aaee83b%40uni-muenster.de Well, it looks like what I wrote a couple of days ago was perhaps confusing: https://www.postgresql.org/message-id/ZPHAiNp%2ByKMsa/vc%40paquier.xyz https://www.postgresql.org/message-id/ZPE8A7EnUH+ax5kw@paquier.xyz This patch touches hbafuncs.c and the system view pg_hba_file_rules, but I don't think this stuff should touch any of these code paths. That's what I meant in my second message: the SQL portion should be usable for all types of configuration files, even pg_ident.conf and postgresql.conf, and not only pg_hba.conf. A new SQL function returning a SRF made of the comments extracted and the line numbers can be joined with all the system views of the configuration files, like sourcefile and sourceline in pg_settings, etc. -- Michael
Вложения
Hi On 11.09.23 00:33, Michael Paquier wrote: > Well, it looks like what I wrote a couple of days ago was perhaps > confusing: > https://www.postgresql.org/message-id/ZPHAiNp%2ByKMsa/vc%40paquier.xyz > https://www.postgresql.org/message-id/ZPE8A7EnUH+ax5kw@paquier.xyz > > This patch touches hbafuncs.c and the system view pg_hba_file_rules, > but I don't think this stuff should touch any of these code paths. > That's what I meant in my second message: the SQL portion should be > usable for all types of configuration files, even pg_ident.conf and > postgresql.conf, and not only pg_hba.conf. A new SQL function > returning a SRF made of the comments extracted and the line numbers > can be joined with all the system views of the configuration files, > like sourcefile and sourceline in pg_settings, etc. > -- > Michael Thanks for the feedback. I indeed misunderstood what you meant in the other thread, as you explicitly only mentioned hba.c. The change to hbafunc.c was mostly a function call and a new column to the view: comment = GetInlineComment(hba->rawline); if(comment) values[index++] = CStringGetTextDatum(comment); else nulls[index++] = true; Just to make sure I got what you have in mind: you suggest to read the pg_hba.conf a second time via a new (generic) function like pg_read_file() that returns line numbers and their contents (+comments), and the results of this new function would be joined pg_hba_file_rules in SQL. Is that correct? Thanks
On Thu, Sep 14, 2023 at 01:33:04PM +0200, Jim Jones wrote: > Just to make sure I got what you have in mind: you suggest to read the > pg_hba.conf a second time via a new (generic) function like pg_read_file() > that returns line numbers and their contents (+comments), and the results of > this new function would be joined pg_hba_file_rules in SQL. Is that correct? Yes, my suggestion was to define a new set-returning function that takes in input a file path and that returns as one row one comment and its line number from the configuration file. -- Michael
Вложения
On 15.09.23 01:28, Michael Paquier wrote: > Yes, my suggestion was to define a new set-returning function that > takes in input a file path and that returns as one row one comment and > its line number from the configuration file. > -- > Michael Thanks! If reading the file again is an option, perhaps a simple SQL function would suffice? Something along these lines .. CREATE OR REPLACE FUNCTION pg_read_conf_comments(text) RETURNS TABLE (line_number int, comment text) AS $$ SELECT lnum, trim(substring(line, nullif(strpos(line,'#'),0)+1, length(line)-strpos(line,'#') )) AS comment FROM unnest(string_to_array(pg_read_file($1),E'\n')) WITH ORDINALITY hba(line,lnum) WHERE trim(line) !~~ '#%' AND trim(line) <> ''; $$ STRICT LANGUAGE SQL ; .. then we could join it with pg_hba_file_rules (or any other conf file) SELECT type, database, user_name, address, c.comment FROM pg_hba_file_rules h, pg_read_conf_comments(h.file_name) c WHERE user_name[1]='jim' AND h.line_number = c.line_number ; type | database | user_name | address | comment ------+----------+-----------+-----------+--------- host | {db} | {jim} | 127.0.0.1 | foo host | {db} | {jim} | 127.0.0.1 | bar host | {db} | {jim} | 127.0.0.1 | #foo# (3 rows) Is it more or less what you had in mind?
On Fri, Sep 15, 2023 at 09:37:23AM +0200, Jim Jones wrote: > SELECT type, database, user_name, address, c.comment > FROM pg_hba_file_rules h, pg_read_conf_comments(h.file_name) c > WHERE user_name[1]='jim' AND h.line_number = c.line_number ; > > type | database | user_name | address | comment > ------+----------+-----------+-----------+--------- > host | {db} | {jim} | 127.0.0.1 | foo > host | {db} | {jim} | 127.0.0.1 | bar > host | {db} | {jim} | 127.0.0.1 | #foo# > (3 rows) > > > Is it more or less what you had in mind? That was the idea. I forgot about strpos(), but if you do that, do we actually need a function in core to achieve that? There are a few fancy cases with the SQL function you have sent, actually.. strpos() would grep the first '#' character, ignoring quoted areas. -- Michael
Вложения
Hi Michael On 16.09.23 06:18, Michael Paquier wrote: > That was the idea. I forgot about strpos(), but if you do that, do we > actually need a function in core to achieve that? I guess it depends who you ask :) I personally think it would be a good addition to the view, as it would provide a more comprehensive look into the hba file. Yes, the fact that it could possibly be written in SQL sounds silly, but it's IMHO still relevant to have it by default. > There are a few fancy cases with the SQL function you have sent, > actually.. strpos() would grep the first '#' character, ignoring > quoted areas. Yes, you're totally right. I didn't take into account any token surrounded by double quotes containing #. v3 attached addresses this issue. From the following hba: host db jim 192.168.10.1/32 md5 # foo host db jim 192.168.10.2/32 md5 #bar host db jim 192.168.10.3/32 md5 # #foo# host "a#db" "a#user" 192.168.10.4/32 md5 # fancy #hba entry We can get these records from the view: SELECT type, database, user_name, address, comment FROM pg_hba_file_rules WHERE address ~~ '192.168.10.%'; type | database | user_name | address | comment ------+----------+-----------+--------------+------------------ host | {db} | {jim} | 192.168.10.1 | foo host | {db} | {jim} | 192.168.10.2 | bar host | {db} | {jim} | 192.168.10.3 | #foo# host | {a#db} | {a#user} | 192.168.10.4 | fancy #hba entry I am still struggling to find a way to enable this function in separated path without having to read the conf file multiple times, or writing too much redundant code. How many other conf files do you think would profit from this feature? Jim
Вложения
On 04.09.23 11:54, Jim Jones wrote: > This patch proposes the column "comment" to the pg_hba_file_rules view. > It basically parses the inline comment (if any) of a valid pg_hba.conf > entry and displays it in the new column. > > For such pg_hba entries ... > > host db jim 127.0.0.1/32 md5 # foo > host db jim 127.0.0.1/32 md5 #bar > host db jim 127.0.0.1/32 md5 # #foo# I'm skeptical about this. First, there are multiple commenting styles. The end-of-line style is less common in my experience, because pg_hba.conf lines tend to belong. Another style is # foo host db jim 127.0.0.1/32 md5 # bar host db jim 127.0.0.1/32 md5 or even as a block # foo and bar host db jim 127.0.0.1/32 md5 host db jim 127.0.0.1/32 md5 Another potential problem is that maybe people don't want their comments leaked out of the file. Who knows what they have written in there. I think we should leave file comments be file comments. If we want some annotations to be exported to higher-level views, we should make that an intentional and explicit separate feature.
> On 26 Sep 2023, at 15:19, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 04.09.23 11:54, Jim Jones wrote: >> This patch proposes the column "comment" to the pg_hba_file_rules view. It basically parses the inline comment (if any)of a valid pg_hba.conf entry and displays it in the new column. >> For such pg_hba entries ... >> host db jim 127.0.0.1/32 md5 # foo >> host db jim 127.0.0.1/32 md5 #bar >> host db jim 127.0.0.1/32 md5 # #foo# > > I'm skeptical about this. > > First, there are multiple commenting styles. The end-of-line style is less common in my experience, because pg_hba.conflines tend to belong. Another style is > > # foo > host db jim 127.0.0.1/32 md5 > # bar > host db jim 127.0.0.1/32 md5 > > or even as a block > > # foo and bar > host db jim 127.0.0.1/32 md5 > host db jim 127.0.0.1/32 md5 Or even a more complicated one (which I've seen variants of in production) where only horizontal whitespace separates two subsequent lines of comments: # Block comment host db jim 127.0.0.1/32 md5 #end of line multi- #line comment # A new block comment directly following host db jim 127.0.0.1/32 md5 > I think we should leave file comments be file comments. If we want some annotations to be exported to higher-level views,we should make that an intentional and explicit separate feature. +1 -- Daniel Gustafsson
Also a reluctant -1, as the comment-at-EOL style is very rare in my experience over the years of seeing many a pg_hba file.
Hi! On 26.09.23 15:19, Peter Eisentraut wrote: > On 04.09.23 11:54, Jim Jones wrote: >> This patch proposes the column "comment" to the pg_hba_file_rules >> view. It basically parses the inline comment (if any) of a valid >> pg_hba.conf entry and displays it in the new column. >> >> For such pg_hba entries ... >> >> host db jim 127.0.0.1/32 md5 # foo >> host db jim 127.0.0.1/32 md5 #bar >> host db jim 127.0.0.1/32 md5 # #foo# > > I'm skeptical about this. > > First, there are multiple commenting styles. The end-of-line style is > less common in my experience, because pg_hba.conf lines tend to > belong. Another style is > > # foo > host db jim 127.0.0.1/32 md5 > # bar > host db jim 127.0.0.1/32 md5 > > or even as a block > > # foo and bar > host db jim 127.0.0.1/32 md5 > host db jim 127.0.0.1/32 md5 > > Another potential problem is that maybe people don't want their > comments leaked out of the file. Who knows what they have written in > there. I also considered this for a while. That's why I suggested only inline comments. On a second thought, I agree that making only certain types of comments "accessible" by the pg_hba_file_rules view can be misleading and can possibly leak sensible info if misused. > > I think we should leave file comments be file comments. If we want > some annotations to be exported to higher-level views, we should make > that an intentional and explicit separate feature. My first suggestion [1] was to use a different character (other than '#'), but a good point was made, that it would add more complexity to the hba.c, which is already complex enough. My main motivation with this feature is to be able to annotate pg_hba entries in a way that it can be read using the pg_hba_file_rule via SQL - these annotations might contain information like tags, client (application) names or any relevant info regarding the granted access. This info would help me to generate some reports that contain client access information. I can sort of achieve something similar using pg_read_file(),[2] but I thought it would be nice to have it directly from the view. Do you think that this feature is in general not a good idea? Or perhaps a different annotation method would address your concerns? Thank you very much for taking a look into it! Jim 1- https://www.postgresql.org/message-id/flat/3fec6550-93b0-b542-b203-b0054aaee83b@uni-muenster.de 2- https://www.postgresql.org/message-id/b63625ca-580f-14dc-7e7c-f90cd4d95cf7%40uni-muenster.de
> On 26 Sep 2023, at 20:40, Jim Jones <jim.jones@uni-muenster.de> wrote: > Do you think that this feature is in general not a good idea? I wouldn't rule it out as a bad idea per se. As always when dealing with access rules and pg_hba there is a security angle to consider, but I think that could be addressed. > Or perhaps a different annotation method would address your concerns? An annotation syntax specifically for this would address my concern, but the argument that pg_hba (and related code) is border-line too complicated as it is does hold some water. Complexity in code can lead to bugs, but complexity in syntax can lead to misconfigurations or unintentional infosec leaks which is usually more problematic. I would propose to not worry about code and instead just discuss a potential new format for annotations, and only implement parsing and handling once something has been agreed upon. This should be in a new thread however to ensure visibility, since it's beyond the subject of this thread. -- Daniel Gustafsson
Hi Daniel On 27.09.23 10:21, Daniel Gustafsson wrote: > An annotation syntax specifically for this would address my concern, > but the > argument that pg_hba (and related code) is border-line too complicated as it is > does hold some water. Complexity in code can lead to bugs, but complexity in > syntax can lead to misconfigurations or unintentional infosec leaks which is > usually more problematic. Yeah, that's why the possibility to use the normal comments for this feature seemed at first so appealing :) > I would propose to not worry about code and instead just discuss a potential > new format for annotations, and only implement parsing and handling once > something has been agreed upon. This should be in a new thread however to > ensure visibility, since it's beyond the subject of this thread. Sounds good! I will open a new thread as soon as I get back home, so that we can collect some ideas. Thanks Jim