[PATCH] Add inline comments to the pg_hba_file_rules view

Started by Jim Jonesover 2 years ago15 messages

jim.jones@uni-muenster.de

over 2 years ago

1 attachment(s)

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:
/messages/by-id/3fec6550-93b0-b542-b203-b0054aaee83b@uni-muenster.de

Best regards,
Jim

Attachments:

v1-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchtext/x-patch; charset=UTF-8; name=v1-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchDownload

From bb795fae29a0f714c590f94176a4675d7ae85a2f Mon Sep 17 00:00:00 2001
From: Jim Jones <jim.jones@uni-muenster.de>
Date: Mon, 4 Sep 2023 12:32:04 +0200
Subject: [PATCH v1] Add inline comments to the pg_hba_file_rules view

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. The patch slightly changes the
test 004_file_inclusion.pl to accomodate the new column and the hba comments.
The view's documentation at system-views.sgml is extended accordingly.

The function GetInlineComment() was added to conffiles.c to avoid adding more
complexity directly to hba.c. It also enables this feature to be used by other
configuration files, if necessary.
---
 doc/src/sgml/system-views.sgml                |  9 +++++
 src/backend/utils/adt/hbafuncs.c              | 11 +++++-
 src/backend/utils/misc/conffiles.c            | 26 +++++++++++++
 src/include/catalog/pg_proc.dat               |  6 +--
 src/include/libpq/hba.h                       |  1 +
 src/include/utils/conffiles.h                 |  1 +
 .../authentication/t/004_file_inclusion.pl    | 39 ++++++++++++++++---
 src/test/regress/expected/rules.out           |  3 +-
 8 files changed, 85 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 2b35c2f91b..d4951372a5 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -1090,6 +1090,15 @@
       </para></entry>
      </row>
 
+    <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>comment</structfield> <type>text</type>
+      </para>
+      <para>
+       Text after the <literal>#</literal> comment character in the end of a valid <literal>pg_hba.conf</literal> entry, if any
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>error</structfield> <type>text</type>
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 73d3ad1dad..929678e97e 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -22,6 +22,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
+#include "utils/conffiles.h"
 
 
 static ArrayType *get_hba_options(HbaLine *hba);
@@ -159,7 +160,7 @@ get_hba_options(HbaLine *hba)
 }
 
 /* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS	 11
+#define NUM_PG_HBA_FILE_RULES_ATTS	 12
 
 /*
  * fill_hba_line
@@ -191,6 +192,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 	const char *addrstr;
 	const char *maskstr;
 	ArrayType  *options;
+	char       *comment;
 
 	Assert(tupdesc->natts == NUM_PG_HBA_FILE_RULES_ATTS);
 
@@ -346,6 +348,13 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 			values[index++] = PointerGetDatum(options);
 		else
 			nulls[index++] = true;
+
+		/* comments */
+		comment = GetInlineComment(hba->rawline);
+		if(comment)
+			values[index++] = CStringGetTextDatum(comment);
+		else
+			nulls[index++] = true;
 	}
 	else
 	{
diff --git a/src/backend/utils/misc/conffiles.c b/src/backend/utils/misc/conffiles.c
index 376a5c885b..0f4169dea3 100644
--- a/src/backend/utils/misc/conffiles.c
+++ b/src/backend/utils/misc/conffiles.c
@@ -25,6 +25,32 @@
 #include "storage/fd.h"
 #include "utils/conffiles.h"
 
+/*
+ * GetInlineComment
+ *
+ * This function returns comments of a given config file line,
+ * if there is any. A comment is any text after the # character.
+ */
+char *
+GetInlineComment(char *line)
+{
+	size_t len;
+	char *comment = strchr(line, '#');
+
+	if(!comment)
+		return NULL;
+
+	/* ignoring '#' character, as we don't need it in the comment itself. */
+	comment++;
+	len = strlen(comment);
+
+	/* trim leading and trailing whitespaces */
+	while(isspace(comment[len - 1])) --len;
+	while(*comment && isspace(*comment)) ++comment, --len;
+
+	return pnstrdup(comment, len);
+}
+
 /*
  * AbsoluteConfigLocation
  *
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..360f71e8ef 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6244,9 +6244,9 @@
 { oid => '3401', descr => 'show pg_hba.conf rules',
   proname => 'pg_hba_file_rules', prorows => '1000', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,error}',
+  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text,text}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,comment,error}',
   prosrc => 'pg_hba_file_rules' },
 { oid => '6250', descr => 'show pg_ident.conf mappings',
   proname => 'pg_ident_file_mappings', prorows => '1000', proretset => 't',
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 189f6d0df2..5cb7224712 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -135,6 +135,7 @@ typedef struct HbaLine
 	char	   *radiusidentifiers_s;
 	List	   *radiusports;
 	char	   *radiusports_s;
+	char	   *comments;
 } HbaLine;
 
 typedef struct IdentLine
diff --git a/src/include/utils/conffiles.h b/src/include/utils/conffiles.h
index e3868fbc23..b60adae5fa 100644
--- a/src/include/utils/conffiles.h
+++ b/src/include/utils/conffiles.h
@@ -23,5 +23,6 @@ extern char **GetConfFilesInDir(const char *includedir,
 								const char *calling_file,
 								int elevel, int *num_filenames,
 								char **err_msg);
+extern char *GetInlineComment(char *line);
 
 #endif							/* CONFFILES_H */
diff --git a/src/test/authentication/t/004_file_inclusion.pl b/src/test/authentication/t/004_file_inclusion.pl
index 55d28ad586..47902b0188 100644
--- a/src/test/authentication/t/004_file_inclusion.pl
+++ b/src/test/authentication/t/004_file_inclusion.pl
@@ -63,6 +63,22 @@ sub add_hba_line
 	# Increment pg_hba_file_rules.rule_number and save it.
 	$globline = ++$line_counters{'hba_rule'};
 
+	my $comment;
+	my $comment_position = index($entry, "#");
+
+	# In case the pg_hba entry has a comment, we store it in $comment
+	# and remove it from $entry. The content of $comment will be pushed
+	# into @tokens separataely. This avoids having whitespaces in $comment
+	# being interpreted as element separator by split(). The leading
+	# and trailing whitespaces in $comment are removed to reproduce the
+	# behaviour of GetInlineComment(char *line).
+	if($comment_position != -1)
+	{
+		$comment = substr($entry, $comment_position+1);
+		$comment =~ s/^\s+|\s+$//g;
+		$entry = substr($entry, 0, $comment_position-1);
+	}
+
 	# Generate the expected pg_hba_file_rules line
 	@tokens = split(/ /, $entry);
 	$tokens[1] = '{' . $tokens[1] . '}';    # database
@@ -72,6 +88,16 @@ sub add_hba_line
 	push @tokens, '';
 	push @tokens, '';
 
+	# Append comment if applicable, otherwise append empty string
+	if(not defined $comment)
+	{
+		push @tokens, '';
+	}
+	else
+	{
+		push @tokens, $comment;
+	}
+
 	# Final line expected, output of the SQL query.
 	$line = "";
 	$line .= "\n" if ($globline > 1);
@@ -167,15 +193,15 @@ mkdir("$data_dir/hba_inc_if");
 mkdir("$data_dir/hba_pos");
 
 # First, make sure that we will always be able to connect.
-$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust');
+$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust    #   #comment containing leading/trailing whitespaces and "#"   ');
 
 # "include".  Note that as $hba_file is located in $data_dir/subdir1,
 # pg_hba_pre.conf is located at the root of the data directory.
 $hba_expected .=
-  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf");
+  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf#foo");
 $hba_expected .=
   add_hba_line($node, 'pg_hba_pre.conf', "local pre all reject");
-$hba_expected .= add_hba_line($node, "$hba_file", "local all all reject");
+$hba_expected .= add_hba_line($node, "$hba_file", "local all all reject #bar");
 add_hba_line($node, "$hba_file", "include ../hba_pos/pg_hba_pos.conf");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "local pos all reject");
@@ -184,7 +210,7 @@ $hba_expected .=
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "include pg_hba_pos2.conf");
 $hba_expected .=
-  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject");
+  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject #bar!#");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos3 all reject");
 
@@ -216,7 +242,7 @@ $hba_expected .= "\n"
   . $line_counters{'hba_rule'} . "|"
   . basename($hba_file) . "|"
   . $line_counters{$hba_file}
-  . '|local|{db1,db3}|{all}|reject||';
+  . '|local|{db1,db3}|{all}|reject|||';
 
 note "Generating ident structure with include directives";
 
@@ -279,7 +305,8 @@ my $contents = $node->safe_psql(
   user_name,
   auth_method,
   options,
-  error
+  error,
+  comment
  FROM pg_hba_file_rules ORDER BY rule_number;));
 is($contents, $hba_expected, 'check contents of pg_hba_file_rules');
 
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5058be5411..6a6c6f9edd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1348,8 +1348,9 @@ pg_hba_file_rules| SELECT rule_number,
     netmask,
     auth_method,
     options,
+    comment,
     error
-   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, error);
+   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, comment, error);
 pg_ident_file_mappings| SELECT map_number,
     file_name,
     line_number,
-- 
2.34.1

David Zhang

david.zhang@highgo.ca

over 2 years ago

In reply to: Jim Jones (#1)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: David Zhang (#2)

1 attachment(s)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

Attachments:

v2-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchtext/x-patch; charset=UTF-8; name=v2-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchDownload

From a817d8b8fc437ef20f9b1f397764895e6d1f2008 Mon Sep 17 00:00:00 2001
From: Jim Jones <jim.jones@uni-muenster.de>
Date: Sat, 9 Sep 2023 22:22:09 +0200
Subject: [PATCH v2] Add inline comments to the pg_hba_file_rules view

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. The patch slightly changes the
test 004_file_inclusion.pl to accomodate the new column and the hba comments.
The view's documentation at system-views.sgml is extended accordingly.

The function GetInlineComment() was added to conffiles.c to avoid adding more
complexity directly to hba.c. It also enables this feature to be used by other
configuration files, if necessary.
---
 doc/src/sgml/system-views.sgml                |  9 +++++
 src/backend/utils/adt/hbafuncs.c              | 11 +++++-
 src/backend/utils/misc/conffiles.c            | 26 +++++++++++++
 src/include/catalog/pg_proc.dat               |  6 +--
 src/include/libpq/hba.h                       |  1 +
 src/include/utils/conffiles.h                 |  1 +
 .../authentication/t/004_file_inclusion.pl    | 39 ++++++++++++++++---
 src/test/regress/expected/rules.out           |  3 +-
 8 files changed, 85 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 2b35c2f91b..68f9857de0 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -1090,6 +1090,15 @@
       </para></entry>
      </row>
 
+    <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>comment</structfield> <type>text</type>
+      </para>
+      <para>
+       Text after the first <literal>#</literal> comment character in the end of a valid <literal>pg_hba.conf</literal> entry, if any
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>error</structfield> <type>text</type>
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 73d3ad1dad..929678e97e 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -22,6 +22,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
+#include "utils/conffiles.h"
 
 
 static ArrayType *get_hba_options(HbaLine *hba);
@@ -159,7 +160,7 @@ get_hba_options(HbaLine *hba)
 }
 
 /* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS	 11
+#define NUM_PG_HBA_FILE_RULES_ATTS	 12
 
 /*
  * fill_hba_line
@@ -191,6 +192,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 	const char *addrstr;
 	const char *maskstr;
 	ArrayType  *options;
+	char       *comment;
 
 	Assert(tupdesc->natts == NUM_PG_HBA_FILE_RULES_ATTS);
 
@@ -346,6 +348,13 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 			values[index++] = PointerGetDatum(options);
 		else
 			nulls[index++] = true;
+
+		/* comments */
+		comment = GetInlineComment(hba->rawline);
+		if(comment)
+			values[index++] = CStringGetTextDatum(comment);
+		else
+			nulls[index++] = true;
 	}
 	else
 	{
diff --git a/src/backend/utils/misc/conffiles.c b/src/backend/utils/misc/conffiles.c
index 376a5c885b..0f4169dea3 100644
--- a/src/backend/utils/misc/conffiles.c
+++ b/src/backend/utils/misc/conffiles.c
@@ -25,6 +25,32 @@
 #include "storage/fd.h"
 #include "utils/conffiles.h"
 
+/*
+ * GetInlineComment
+ *
+ * This function returns comments of a given config file line,
+ * if there is any. A comment is any text after the # character.
+ */
+char *
+GetInlineComment(char *line)
+{
+	size_t len;
+	char *comment = strchr(line, '#');
+
+	if(!comment)
+		return NULL;
+
+	/* ignoring '#' character, as we don't need it in the comment itself. */
+	comment++;
+	len = strlen(comment);
+
+	/* trim leading and trailing whitespaces */
+	while(isspace(comment[len - 1])) --len;
+	while(*comment && isspace(*comment)) ++comment, --len;
+
+	return pnstrdup(comment, len);
+}
+
 /*
  * AbsoluteConfigLocation
  *
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..360f71e8ef 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6244,9 +6244,9 @@
 { oid => '3401', descr => 'show pg_hba.conf rules',
   proname => 'pg_hba_file_rules', prorows => '1000', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,error}',
+  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text,text}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,comment,error}',
   prosrc => 'pg_hba_file_rules' },
 { oid => '6250', descr => 'show pg_ident.conf mappings',
   proname => 'pg_ident_file_mappings', prorows => '1000', proretset => 't',
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 189f6d0df2..5cb7224712 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -135,6 +135,7 @@ typedef struct HbaLine
 	char	   *radiusidentifiers_s;
 	List	   *radiusports;
 	char	   *radiusports_s;
+	char	   *comments;
 } HbaLine;
 
 typedef struct IdentLine
diff --git a/src/include/utils/conffiles.h b/src/include/utils/conffiles.h
index e3868fbc23..b60adae5fa 100644
--- a/src/include/utils/conffiles.h
+++ b/src/include/utils/conffiles.h
@@ -23,5 +23,6 @@ extern char **GetConfFilesInDir(const char *includedir,
 								const char *calling_file,
 								int elevel, int *num_filenames,
 								char **err_msg);
+extern char *GetInlineComment(char *line);
 
 #endif							/* CONFFILES_H */
diff --git a/src/test/authentication/t/004_file_inclusion.pl b/src/test/authentication/t/004_file_inclusion.pl
index 55d28ad586..47902b0188 100644
--- a/src/test/authentication/t/004_file_inclusion.pl
+++ b/src/test/authentication/t/004_file_inclusion.pl
@@ -63,6 +63,22 @@ sub add_hba_line
 	# Increment pg_hba_file_rules.rule_number and save it.
 	$globline = ++$line_counters{'hba_rule'};
 
+	my $comment;
+	my $comment_position = index($entry, "#");
+
+	# In case the pg_hba entry has a comment, we store it in $comment
+	# and remove it from $entry. The content of $comment will be pushed
+	# into @tokens separataely. This avoids having whitespaces in $comment
+	# being interpreted as element separator by split(). The leading
+	# and trailing whitespaces in $comment are removed to reproduce the
+	# behaviour of GetInlineComment(char *line).
+	if($comment_position != -1)
+	{
+		$comment = substr($entry, $comment_position+1);
+		$comment =~ s/^\s+|\s+$//g;
+		$entry = substr($entry, 0, $comment_position-1);
+	}
+
 	# Generate the expected pg_hba_file_rules line
 	@tokens = split(/ /, $entry);
 	$tokens[1] = '{' . $tokens[1] . '}';    # database
@@ -72,6 +88,16 @@ sub add_hba_line
 	push @tokens, '';
 	push @tokens, '';
 
+	# Append comment if applicable, otherwise append empty string
+	if(not defined $comment)
+	{
+		push @tokens, '';
+	}
+	else
+	{
+		push @tokens, $comment;
+	}
+
 	# Final line expected, output of the SQL query.
 	$line = "";
 	$line .= "\n" if ($globline > 1);
@@ -167,15 +193,15 @@ mkdir("$data_dir/hba_inc_if");
 mkdir("$data_dir/hba_pos");
 
 # First, make sure that we will always be able to connect.
-$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust');
+$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust    #   #comment containing leading/trailing whitespaces and "#"   ');
 
 # "include".  Note that as $hba_file is located in $data_dir/subdir1,
 # pg_hba_pre.conf is located at the root of the data directory.
 $hba_expected .=
-  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf");
+  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf#foo");
 $hba_expected .=
   add_hba_line($node, 'pg_hba_pre.conf', "local pre all reject");
-$hba_expected .= add_hba_line($node, "$hba_file", "local all all reject");
+$hba_expected .= add_hba_line($node, "$hba_file", "local all all reject #bar");
 add_hba_line($node, "$hba_file", "include ../hba_pos/pg_hba_pos.conf");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "local pos all reject");
@@ -184,7 +210,7 @@ $hba_expected .=
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "include pg_hba_pos2.conf");
 $hba_expected .=
-  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject");
+  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject #bar!#");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos3 all reject");
 
@@ -216,7 +242,7 @@ $hba_expected .= "\n"
   . $line_counters{'hba_rule'} . "|"
   . basename($hba_file) . "|"
   . $line_counters{$hba_file}
-  . '|local|{db1,db3}|{all}|reject||';
+  . '|local|{db1,db3}|{all}|reject|||';
 
 note "Generating ident structure with include directives";
 
@@ -279,7 +305,8 @@ my $contents = $node->safe_psql(
   user_name,
   auth_method,
   options,
-  error
+  error,
+  comment
  FROM pg_hba_file_rules ORDER BY rule_number;));
 is($contents, $hba_expected, 'check contents of pg_hba_file_rules');
 
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5058be5411..6a6c6f9edd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1348,8 +1348,9 @@ pg_hba_file_rules| SELECT rule_number,
     netmask,
     auth_method,
     options,
+    comment,
     error
-   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, error);
+   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, comment, error);
 pg_ident_file_mappings| SELECT map_number,
     file_name,
     line_number,
-- 
2.34.1

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Jim Jones (#1)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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: /messages/by-id/3fec6550-93b0-b542-b203-b0054aaee83b@uni-muenster.de

Well, it looks like what I wrote a couple of days ago was perhaps
confusing:
/messages/by-id/ZPHAiNp+yKMsa/vc@paquier.xyz
/messages/by-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

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: Michael Paquier (#4)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

On 11.09.23 00:33, Michael Paquier wrote:

Well, it looks like what I wrote a couple of days ago was perhaps
confusing:
/messages/by-id/ZPHAiNp+yKMsa/vc@paquier.xyz
/messages/by-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

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Jim Jones (#5)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: Michael Paquier (#6)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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 ;

Is it more or less what you had in mind?

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Jim Jones (#7)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: Michael Paquier (#8)

1 attachment(s)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

Attachments:

v3-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchtext/x-patch; charset=UTF-8; name=v3-0001-Add-inline-comments-to-the-pg_hba_file_rules-view.patchDownload

From 47f55bab0a8e8af286e6be2f40d218f25a5066c9 Mon Sep 17 00:00:00 2001
From: Jim Jones <jim.jones@uni-muenster.de>
Date: Wed, 20 Sep 2023 00:04:05 +0200
Subject: [PATCH v3] Add inline comments to the pg_hba_file_rules view

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. The patch slightly changes the
test 004_file_inclusion.pl to accomodate the new column and the hba comments.
The view's documentation at system-views.sgml is extended accordingly.

The function GetInlineComment() was added to conffiles.c to avoid adding more
complexity directly to hba.c. It also enables this feature to be used by other
configuration files, if necessary.
---
 doc/src/sgml/system-views.sgml                |  9 +++
 src/backend/utils/adt/hbafuncs.c              | 11 ++-
 src/backend/utils/misc/conffiles.c            | 41 ++++++++++
 src/include/catalog/pg_proc.dat               |  6 +-
 src/include/libpq/hba.h                       |  1 +
 src/include/utils/conffiles.h                 |  1 +
 .../authentication/t/004_file_inclusion.pl    | 74 +++++++++++++++++--
 src/test/regress/expected/rules.out           |  3 +-
 8 files changed, 135 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 2b35c2f91b..68f9857de0 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -1090,6 +1090,15 @@
       </para></entry>
      </row>
 
+    <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>comment</structfield> <type>text</type>
+      </para>
+      <para>
+       Text after the first <literal>#</literal> comment character in the end of a valid <literal>pg_hba.conf</literal> entry, if any
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>error</structfield> <type>text</type>
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 73d3ad1dad..929678e97e 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -22,6 +22,7 @@
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/guc.h"
+#include "utils/conffiles.h"
 
 
 static ArrayType *get_hba_options(HbaLine *hba);
@@ -159,7 +160,7 @@ get_hba_options(HbaLine *hba)
 }
 
 /* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS	 11
+#define NUM_PG_HBA_FILE_RULES_ATTS	 12
 
 /*
  * fill_hba_line
@@ -191,6 +192,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 	const char *addrstr;
 	const char *maskstr;
 	ArrayType  *options;
+	char       *comment;
 
 	Assert(tupdesc->natts == NUM_PG_HBA_FILE_RULES_ATTS);
 
@@ -346,6 +348,13 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
 			values[index++] = PointerGetDatum(options);
 		else
 			nulls[index++] = true;
+
+		/* comments */
+		comment = GetInlineComment(hba->rawline);
+		if(comment)
+			values[index++] = CStringGetTextDatum(comment);
+		else
+			nulls[index++] = true;
 	}
 	else
 	{
diff --git a/src/backend/utils/misc/conffiles.c b/src/backend/utils/misc/conffiles.c
index 376a5c885b..feda49bf31 100644
--- a/src/backend/utils/misc/conffiles.c
+++ b/src/backend/utils/misc/conffiles.c
@@ -25,6 +25,47 @@
 #include "storage/fd.h"
 #include "utils/conffiles.h"
 
+/*
+ * GetInlineComment
+ *
+ * This function returns comments of a given config file line,
+ * if there is any. A comment is any text after the # character,
+ * as long as it is not located after a opeining '"'.
+ */
+char *
+GetInlineComment(char *line)
+{
+
+	size_t nq = 0;
+	char *comment;
+
+	for (int i = 0; i < strlen(line); i++) {
+
+		if (line[i] == '"')
+			nq++;
+		else if (line[i] == '#' && nq % 2 == 0)
+		{
+			size_t len = strlen(line);
+			comment = (char *) palloc(len * sizeof(char *));
+			memcpy(comment,&line[i+1],len-i);
+
+			/* trim leading and trailing whitespaces */
+			while (*comment && isspace((unsigned char) *comment))
+				comment++;
+			len = strlen(comment);
+			while (len > 0 && isspace((unsigned char) comment[len - 1]))
+				len--;
+			comment[len] = '\0';
+
+			return comment;
+		}
+
+	}
+
+	return NULL;
+
+ }
+
 /*
  * AbsoluteConfigLocation
  *
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9805bc6118..360f71e8ef 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6244,9 +6244,9 @@
 { oid => '3401', descr => 'show pg_hba.conf rules',
   proname => 'pg_hba_file_rules', prorows => '1000', proretset => 't',
   provolatile => 'v', prorettype => 'record', proargtypes => '',
-  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text}',
-  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
-  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,error}',
+  proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text,text}',
+  proargmodes => '{o,o,o,o,o,o,o,o,o,o,o,o}',
+  proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,comment,error}',
   prosrc => 'pg_hba_file_rules' },
 { oid => '6250', descr => 'show pg_ident.conf mappings',
   proname => 'pg_ident_file_mappings', prorows => '1000', proretset => 't',
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 189f6d0df2..5cb7224712 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -135,6 +135,7 @@ typedef struct HbaLine
 	char	   *radiusidentifiers_s;
 	List	   *radiusports;
 	char	   *radiusports_s;
+	char	   *comments;
 } HbaLine;
 
 typedef struct IdentLine
diff --git a/src/include/utils/conffiles.h b/src/include/utils/conffiles.h
index e3868fbc23..b60adae5fa 100644
--- a/src/include/utils/conffiles.h
+++ b/src/include/utils/conffiles.h
@@ -23,5 +23,6 @@ extern char **GetConfFilesInDir(const char *includedir,
 								const char *calling_file,
 								int elevel, int *num_filenames,
 								char **err_msg);
+extern char *GetInlineComment(char *line);
 
 #endif							/* CONFFILES_H */
diff --git a/src/test/authentication/t/004_file_inclusion.pl b/src/test/authentication/t/004_file_inclusion.pl
index 55d28ad586..199c824e33 100644
--- a/src/test/authentication/t/004_file_inclusion.pl
+++ b/src/test/authentication/t/004_file_inclusion.pl
@@ -63,8 +63,56 @@ sub add_hba_line
 	# Increment pg_hba_file_rules.rule_number and save it.
 	$globline = ++$line_counters{'hba_rule'};
 
+	# It is possible to have the character '#' in several elements of
+	# a hba entry if they're wrapped with double quotes. So we first check
+	# if a '#' comes after an opening '"', and if so we ignore it. If a '#'
+	# is located after a closing '"' or if there is no previous '"' in the
+	# string, we consider it as the beginning of an inline comment.
+
+	my $comment_position = -1;
+	my $nq = 0;
+
+	for my $i (0..length($entry)-1){
+
+		my $char = substr($entry, $i, 1);
+
+		if($char eq '"')
+		{
+			$nq = $nq + 1;
+		} elsif ($char eq '#' && $nq % 2 == 0)
+		{
+			$comment_position = $i;
+			last;
+		}
+
+	}
+
+	# In case the pg_hba entry has a comment, we store it in $comment
+	# and remove it from $entry. The content of $comment will be pushed
+	# into @tokens separataely. This avoids having whitespaces in $comment
+	# being interpreted as element separator by split(). The leading
+	# and trailing whitespaces in $comment are removed to reproduce the
+	# behaviour of GetInlineComment(char *line).
+
+	my $comment;
+
+	if($comment_position != -1)
+	{
+		$comment = substr($entry, $comment_position+1);
+		$comment =~ s/^\s+|\s+$//g;
+		$entry = substr($entry, 0, $comment_position);
+	}
+
 	# Generate the expected pg_hba_file_rules line
 	@tokens = split(/ /, $entry);
+
+	# Remove surrounding double quotes.
+	foreach (@tokens)
+	{
+		$_ =~ s/^"|"$//g;
+	}
+
+	# Wrapping database and user name with {}
 	$tokens[1] = '{' . $tokens[1] . '}';    # database
 	$tokens[2] = '{' . $tokens[2] . '}';    # user_name
 
@@ -72,6 +120,16 @@ sub add_hba_line
 	push @tokens, '';
 	push @tokens, '';
 
+	# Append comment, if any. Otherwise append empty string
+	if(not defined $comment)
+	{
+		push @tokens, '';
+	}
+	else
+	{
+		push @tokens, $comment;
+	}
+
 	# Final line expected, output of the SQL query.
 	$line = "";
 	$line .= "\n" if ($globline > 1);
@@ -167,15 +225,18 @@ mkdir("$data_dir/hba_inc_if");
 mkdir("$data_dir/hba_pos");
 
 # First, make sure that we will always be able to connect.
-$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust');
+$hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust    #   # First, make sure that we will always be able to connect.   ');
+
+# Add database and user names containing # by wrapping them with double quotes.
+$hba_expected .= add_hba_line($node, "$hba_file", 'local "fancy#dbname" "fancy#username" trust# Add database and user names containing # by wrapping them with double quotes.');
 
 # "include".  Note that as $hba_file is located in $data_dir/subdir1,
 # pg_hba_pre.conf is located at the root of the data directory.
 $hba_expected .=
-  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf");
+  add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf#foo");
 $hba_expected .=
   add_hba_line($node, 'pg_hba_pre.conf', "local pre all reject");
-$hba_expected .= add_hba_line($node, "$hba_file", "local all all reject");
+$hba_expected .= add_hba_line($node, "$hba_file", 'local all "all" reject #bar');
 add_hba_line($node, "$hba_file", "include ../hba_pos/pg_hba_pos.conf");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "local pos all reject");
@@ -184,7 +245,7 @@ $hba_expected .=
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "include pg_hba_pos2.conf");
 $hba_expected .=
-  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject");
+  add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos2 all reject #bar!#");
 $hba_expected .=
   add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', "local pos3 all reject");
 
@@ -216,7 +277,7 @@ $hba_expected .= "\n"
   . $line_counters{'hba_rule'} . "|"
   . basename($hba_file) . "|"
   . $line_counters{$hba_file}
-  . '|local|{db1,db3}|{all}|reject||';
+  . '|local|{db1,db3}|{all}|reject|||';
 
 note "Generating ident structure with include directives";
 
@@ -279,7 +340,8 @@ my $contents = $node->safe_psql(
   user_name,
   auth_method,
   options,
-  error
+  error,
+  comment
  FROM pg_hba_file_rules ORDER BY rule_number;));
 is($contents, $hba_expected, 'check contents of pg_hba_file_rules');
 
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5058be5411..6a6c6f9edd 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1348,8 +1348,9 @@ pg_hba_file_rules| SELECT rule_number,
     netmask,
     auth_method,
     options,
+    comment,
     error
-   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, error);
+   FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, comment, error);
 pg_ident_file_mappings| SELECT map_number,
     file_name,
     line_number,
-- 
2.34.1

#10

Peter Eisentraut

peter@eisentraut.org

over 2 years ago

In reply to: Jim Jones (#1)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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.

#11

Daniel Gustafsson

daniel@yesql.se

over 2 years ago

In reply to: Peter Eisentraut (#10)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

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.

--
Daniel Gustafsson

#12

Greg Sabino Mullane

htamfids@gmail.com

over 2 years ago

In reply to: Jim Jones (#1)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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.

#13

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: Peter Eisentraut (#10)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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-
/messages/by-id/3fec6550-93b0-b542-b203-b0054aaee83b@uni-muenster.de
2-
/messages/by-id/b63625ca-580f-14dc-7e7c-f90cd4d95cf7@uni-muenster.de

#14

Daniel Gustafsson

daniel@yesql.se

over 2 years ago

In reply to: Jim Jones (#13)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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

#15

Jim Jones

jim.jones@uni-muenster.de

over 2 years ago

In reply to: Daniel Gustafsson (#14)

Re: [PATCH] Add inline comments to the pg_hba_file_rules view

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