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: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Nathan Bossart <nathandbossart@gmail.com>
Date: 2022-11-03T11:55:05Z
Lists: pgsql-hackers 
On Wed, Nov 02, 2022 at 09:06:02PM +0800, Julien Rouhaud wrote:
> Maybe one alternative approach would be to keep modifying the current test, and
> only do the split when committing the first part?  The split itself should be
> easy and mechanical: just remove everything unneeded (different file names
> including the file name argument from the add_(hba|ident)_line, inclusion
> directive and so on).  Even if the diff is then difficult to read it's not
> really a problem as it should have already been reviewed.

I have not reviewed the test part much yet, TBH.  The path
manipulations because of WIN32 are a bit annoying.  I was wondering if
we could avoid all that by using the basenames, as one option.

>> I have spent some time starting at the logic today of the whole, and
>> GetConfFilesInDir() is really the first thing that stood out.  I am
>> not sure that it makes much sense to keep that in guc-file.c, TBH,
>> once we feed it into hba.c.  Perhaps we could put the refactored
>> routine (and AbsoluteConfigLocation as a side effect) into a new file
>> in misc/?
> 
> Yes I was wondering about it too.  A new fine in misc/ looks sensible.

conffiles.c is the best thing I could come up with, conf.c and
config.c being too generic and these are routines that work on
configuration files, so..

>> Your patch proposes a different alternative, which is to pass down the
>> memory context created in tokenize_auth_file() down to the callers
>> with tokenize_file_with_context() dealing with all the internals.
>> process_included_auth_file() is different extension of that.
>> This may come down to a matter of taste, but could be be cleaner to
>> take an approach similar to tokenize_inc_file() and just create a copy
>> of the AuthToken list coming from a full file and append it to the
>> result rather than passing around the memory context for the lines?
>> This would lead to some simplifications, it seems, at least with the
>> number of arguments passed down across the layers.
> 
> I guess it goes in the same direction as 8fea86830e1 where infrastructure to
> copy AuthTokens was added, I'm fine either way.

I won't hide that I am trying to make the changes a maximum
incremental for this thread so as the final part is easy-ish.

>> The addition of a check for the depth in two places seems unnecessary,
>> and it looks like we should do this kind of check in only one place.
> 
> I usually prefer to add a maybe unnecessary depth check since it's basically
> free rather than risking an unfriendly stack size error (including in possible
> later refactoring), but no objection to get rid of it.

The depth check is a good idea, though I guess that there is an
argument for having it in only one code path, not two.

>> I have not tried yet, but if we actually move the AllocateFile() and
>> FreeFile() calls within tokenize_auth_file(), it looks like we may be
>> able to get to a simpler logic without the need of the with_context()
>> flavor (even no process_included_auth_file required)?  That could make
>> the interface easier to follow as a whole, while limiting the presence
>> of AllocateFile() and FreeFile() to a single code path, impacting
>> open_inc_file() that relies on what the patch uses for
>> SecondaryAuthFile and IncludedAuthFile (we could attempt to use the
>> same error message everywhere as well, as one could expect that
>> expanded and included files have different names which is enough to
>> guess which one is an inclusion and which one is a secondary).
> 
> You meant tokenize_auth_file_internal?  Yes possibly, in general I tried
> to avoid changing too much the existing code to ease the patch acceptance, but
> I agree it would make things simpler.

I *guess* that my mind implied tokenize_auth_file() as of
yesterday's.
--
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