Horton

Re: Allow file inclusion in pg_hba and pg_ident files

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Aleksander Alekseev <aleksander@timescale.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Nathan Bossart <nathandbossart@gmail.com>
Date: 2022-03-25T11:18:31Z
Lists: pgsql-hackers 
On Thu, Mar 24, 2022 at 04:08:38PM +0800, Julien Rouhaud wrote:
> On Thu, Mar 24, 2022 at 04:50:31PM +0900, Michael Paquier wrote:
>> And so, this first part has been applied as of d4781d8.  I'll try to
>> look at 0002 shortly.
> 
> Thanks!

Now looking at 0002.  The changes in hba.c are straight-forward,
that's a nice read.

 	if (!field) { \
-		ereport(LOG, \
+		ereport(elevel, \
 				(errcode(ERRCODE_CONFIG_FILE_ERROR), \
 				 errmsg("missing entry in file \"%s\" at end of line %d", \
 						IdentFileName, line_num))); \
+		*err_msg = psprintf("missing entry at end of line"); \
 		return NULL; \
 	} \
I think that we'd better add to err_msg the line number and the file
name.  This would become particularly important once the facility to
include files gets added.  We won't use IdentFileName for this
purpose, but at least we would know which areas to change.  Also, even
if the the view proposes line_number, there is an argument in favor of
consistency here.

+select count(*) >= 0 as ok from pg_ident_file_mappings;

I'd really like to see more tests for this stuff (pg_hba_file_rules is
not a great example in this area), where we could rely on something
for *nix platforms.  Windows would provide some valid coverage as we
enable it by default for SSPI in pg_regress but that's not great for
the average Joe.  One thing I thought about first is that
src/test/authentication/ does not test peer authentication at all, 
which would map nicely with pg_ident.conf and some user mappings.  So
we could have a new test there, that depends on how the backend reacts
when calling getpeereid(), making the results conditional.  This would
be nice in the long-term.

As of a set of tests, I think that for now I would add two things, for
a total of four tests:
- Stick some queries on pg_ident_file_mappings only for Windows in
some of the tests of src/test/authentication/, say 001_password.pl.
One test should test for some valid fields.  Another idea I have here
is to add some junk to pg_ident.conf, reload and check that an error
is generated (the missing field case on a given line).
- Do the same for src/test/kerberos/, with one positive and one
negative test with some junk in the ident conf file.

A last idea is to abuse of the fact that pg_ident is loaded even if we
don't use it: aka we could add some right and junky contents in
pg_ident.conf, then check its validity.  Using one of the existing
tests may not be right, particularly if we finish by extending it, so
I would move that to a new fresh test script.

+    a.pg_usernamee,
[...]
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>pg_username</structfield> <type>text</type>

pg_usernamee sounds a bit weird as attribute name for the view.  We
could stick to a simple postgres_user_name, or postgres_name, for
example.  Perhaps that's just a typo in the function output and you
intended to use pg_username?

+   /* Free parse_hba_line memory */
+   MemoryContextSwitchTo(oldcxt);
+   MemoryContextDelete(identcxt);
Incorrect comment, this should be parse_ident_line.
--
Michael

Commits

  1. Add TAP tests for include directives in HBA end ident files

  2. Introduce variables for initial and max nesting depth on configuration files

  3. Add support for file inclusions in HBA and ident configuration files

  4. Add missing initialization in tokenize_expand_file() for output list

  5. Rework memory contexts in charge of HBA/ident tokenization

  6. Add error context callback when tokenizing authentication files

  7. Invent open_auth_file() in hba.c to refactor authentication file opening

  8. Use AbsoluteConfigLocation() when building an included path in hba.c

  9. Move code related to configuration files in directories to new file

  10. doc: Fix some descriptions related to pg_ident_file_mappings

  11. Add rule_number to pg_hba_file_rules and map_number to pg_ident_file_mappings

  12. Refactor code handling the names of files loaded in hba.c

  13. Make consistent a couple of log messages when parsing HBA files

  14. Use hba_file/ident_file GUCs rather than pg_hba.conf/pg_ident.conf in logs

  15. Fix path reference when parsing pg_ident.conf for pg_ident_file_mappings

  16. Add system view pg_ident_file_mappings

  17. Modify query on pg_hba_file_rules to check for errors in regression tests

  18. Refactor code related to pg_hba_file_rules() into new file