Re: Allow file inclusion in pg_hba and pg_ident files

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"?

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

>> The set of HBA rules are very sensitive to their
>> ordering, and ReadDir() depends on readdir(), which provides a list
>> of files depending on its FS implementation.  That does not seem like
>> a sane concept to me.  I can this this order (tracked by rule_number)
>> being strictly enforced when it comes to the loading of files, though,
>> so I would recommend to focus on the implementation of "include"
>> rather than "include_dir".
> 
> 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.

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

>> +                   /*
>> +                    * Only parse files with names ending in ".conf".
>> +                    * Explicitly reject files starting with ".". This
>> +                    * excludes things like "." and "..", as well as typical
>> +                    * hidden files, backup files, and editor debris.
>> +                    */
>> I don't think that there is any need to restrict that to files ending
>> with .conf.  We don't do that for postgresql.conf's include, for one.
> 
> I'm confused.  Are you talking about postgresql.conf's include or include_dir
> option?  I'm pretty sure that I borrowed this logic from
> src/backend/utils/misc/guc-file.l / ParseConfigDirectory(), which implements
> the include_dir logic for the postgresql.conf file.

Ah, ParseConfigDirectory() does that for the sake of avoiding any
debris files.  The reasoning (2a0c81a, aka
https://www.postgresql.org/message-id/4EC341E1.1060908%402ndQuadrant.com)
relates to debris files like the ones created by editors and such.  So
you are right to do so.

>> Finally I also added 0003, which is a POC for a new pg_hba_matches()
>> function, that can help DBA to understand why their configuration isn't
>> working as they expect.  This only to start the discussion on that topic, the
>> code is for now really hackish, as I don't know how much this is wanted
>> and/or if some other behavior would be better, and there's also no
>> documentation or test.  The function for now only takes an optional inet
>> (null means unix socket), the target role and an optional ssl flag and
>> returns the file, line and raw line matching if any, or null.  For instance:
>>
>> =# select * from pg_hba_matches('127.0.0.1'::inet, 'postgres');
>> -[ RECORD 1  ]-----------------------------------------------------------------
>> file_name | /tmp/pgbuild/toto.conf
>> line_num  | 5
>> raw_line  | host    all            all            127.0.0.1/32           trust
> 
> I will put more details and this example in the commit message if you want, but
> if no one is interested in that feature I'm ok with discarding it.

Oh, OK, I was not really following, then.  I'd be fine to revisit this
part if necessary.
--
Michael