Re: Allow file inclusion in pg_hba and pg_ident files

On Tue, May 24, 2022 at 10:44:05AM +0900, Michael Paquier wrote:
> On Mon, May 23, 2022 at 07:43:08PM +0800, Julien Rouhaud wrote:
> > That being said, I'd gladly drop that enum and only handle a single error
> > message, as the rest of the error context (including the owning file name and
> > line) should provide enough information to users.
> > 
> > If so, should I use "included authentication file" everywhere, or use something
> > else?
> 
> With the file name provided the context, "authentication file"?

So you mean having an error message like that (having an "include myconf"
in the HBA file):

LOG:  could not open authentication file "myconf" as "/path/to/myconf": No such file or directory
LOG:  pg_hba.conf was not reloaded

This would be somewhat consistent with how it's done for the postgresql.conf,
assuming that we do fix that hardcoded "pg_hba.conf":

LOG:  could not open configuration file "/path/to/toto": No such file or directory
LOG:  configuration file "/path/to/postgresql.conf" contains errors; no changes were applied

With this message it's clear that the file that couldn't be opened is an
included file.

However those two cases are slightly different, and technically the "secondary
file" is not an authentication file, just a list of tokens, so I'm still a bit
worried about ambiguity here.

> > The detail should be provided in the logs so it should disambiguate the
> > situation.  While you're mentioning this message, AFAICS it can already be
> > entirely wrong as-is, as it doesn't take into account the hba_file
> > configuration:
> > 
> > ALTER SYSTEM SET hba_file = '/tmp/myfile';
> 
> Hmm, indeed.  And we load the GUCs before the HBA rules, obviously.
> This could be made less confusing.  Do you think that this can be
> improved independently of the main patch, building the include
> structure on top of it?

Sure, I can split that in another commit.

After a bit more digging, I think that this comes from the fact that there's no
"official" name for this file.  Even the documentation just says "the
pg_hba.conf file" [1].  So using pg_hba.conf can either means explicitly
$PGDATA/pg_hba.conf or the instance's HBA file in general, whatever its
location.

I think it would be good to improve this, including in the doc, but I'm
assuming it's entirely for HEAD only, including the error messages?

If so, should I also change the doc to replace "pg_hba.conf" with something
else when it's not referring to the file default name?

I'm thinking of using "HBA file" to replace pg_hba.conf, and using
"authentication file" when it can be either the "HBA file" and the "User Name
Maps file", would that be ok?

> > But the same problem already exists for the postgresql.conf's include_dir.
> > 
> > Having an hba rule masking another isn't really better than having a GUC
> > value overloaded by another one.  Usually people rely on a sane naming (like
> > 01_file.conf and so on) to get a predictable behavior, and we even document
> > that very thoroughly for the postgresql.conf part.  Improving the
> > documentation, as you already pointed out a bit above, should be enough?
> 
> Ah.  ParseConfigDirectory() does a qsort() before processing each
> file included in a folder.  Unless I am missing something, your patch
> reads the entries in the directory, but does not sort the files by
> name to ensure a strict ordering of them.
> 
> While on it, it looks like you should have sanity checks similar to
> ParseConfigDirectory() for CRLFs & friends, as of:
> if (strspn(includedir, " \t\r\n") == strlen(includedir))
> 
> All this logic is very similar, so this could be perhaps refactored.

Indeed, I will move that in a common function to be consistent.

> >> +      <para>
> >> +       Rule number, in priority order, of this rule if the rule is valid,
> >> +       otherwise null
> >> +      </para></entry>
> >> This is a very important field.  I think that this explanation should
> >> be extended and explain why relying on this number counts (aka this is
> >> the order of the rules loaded across files).  Btw, this could be added
> >> as a separate patch, even if this maps to the line number when it
> >> comes to the ordering.
> > 
> > Agreed, I will improve the documentation to outline the importance of that
> > information.
> > 
> > Do you mean a separate patch to ease review or to eventually commit both
> > separately?  FWIW I don't think that adding any form of inclusion in the hba
> > files should be done without a way for users to check the results.  Any test
> > facility would also probably rely on this field.
> 
> Yeah, agreed that rule_number is important for the sake of the
> inclusions.  Still, I was wondering if rule_number makes sense on its
> own, meaning that we could add it before working on the inclusion
> logic.  Having it without the inclusion is less interesting if you
> have the line numbers to order the HBA rules, of course, as this is
> enough to guess the priority of the HBA entries.

Ah I see.  Sure I could split it in another commit too, but this isn't
backpatchable (and pg15 isn't branched yet anyway) so I'm not sure how useful
that is.  I will go head and do that anyway, it will be easy to merge the
commits if needed.


[1] https://www.postgresql.org/docs/current/auth-pg-hba-conf.html