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-23T06:05:18Z
Lists: pgsql-hackers

Attachments

On Tue, Nov 22, 2022 at 05:20:01PM +0900, Michael Paquier wrote:
> +     /* XXX: this should stick to elevel for some cases? */
> +     ereport(LOG,
> +             (errmsg("skipping missing authentication file \"%s\"",
> +                     inc_fullname)));
> Should we always issue a LOG here?  In some cases we use an elevel of
> DEBUG3.

And here I think that we need to use elevel.  In hba.c, this would
generate a LOG while hbafuncs.c uses DEBUG3, leading to less noise in
the log files.

> So, what do you think about something like the attached?  I have begun
> a lookup at the tests, but I don't have enough material for an actual
> review of this part yet.  Note that I have removed temporarily
> process_included_authfile(), as I was looking at all the code branches
> in details.  The final result ought to include AbsoluteConfigLocation(),
> open_auth_file() and tokenize_auth_file() with an extra missing_ok
> argument, I guess ("strict" as proposed originally is not the usual
> PG-way for ENOENT-ish problems).  process_line should be used only
> when we have no err_msg, meaning that these have been consumed in some
> TokenizedAuthLines already.

This, however, was still brittle in terms of memory handling.
Reloading the server a few hundred times proved that this was leaking
memory in the tokenization.  This becomes quite simple once you switch
to an approach where tokenize_auth_file() uses its own memory context,
while we store all the TokenizedAuthLines in the static context.  It
also occurred to me that we can create and drop the static context
holding the tokens when opening the top-root auth file, aka with a
depth at 0.  It may be a bit simpler to switch to a single-context
approach for all the allocations of tokenize_auth_file(), though I
find the use of a static context much cleaner for the inclusions with
@ files when we expand an existing list.

The patch can be split, again, into more pieces.  Attached 0001
reworks the memory allocation contexts for the tokenization and 0002
is the main feature.  As of things are, I am quite happy with the
shapes of 0001 and 0002.  I have tested quite a bit its robustness,
with valgrind for example, to make sure that there are no leaks in the
postmaster (with[out] -DEXEC_BACKEND).  The inclusion logic is
refactored to be in a state close to your previous patches, see
tokenize_inclusion_file().

Note that the tests are failing for some of the Windows CIs, actually,
due to a difference in some of the paths generated vs the file paths
(aka c:\cirrus vs c:/cirrus, as far as I saw).
--
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