Re: Allow file inclusion in pg_hba and pg_ident files

On Thu, Nov 03, 2022 at 08:55:05PM +0900, Michael Paquier wrote:
> On Wed, Nov 02, 2022 at 09:06:02PM +0800, Julien Rouhaud wrote:
>>> 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.

So, I have looked at this specific part, and I think that we should
try to solve this issue on its own as the error message leading to a
failure at startup is confusing now, giving:
FATAL: exceeded maxAllocatedDescs (166) while trying to open file \"blah\".

It took me some time to ponder about what kind of interface we should
use for this job, but I think that it comes down to three things:
- The depth ought to be checked just before doing AllocateFile() for a
file.
- We should have one code path calling AllocateFile().
- The same interface should be used for hba.c and hbafuncs.c.

I have settled down to the following in the attached patch 0002:
open_auth_file(const char *filename, int elevel, int depth, char
**err_msg)

More checks could be added in this code path (guc-file.l does more
with immediate recursions, for example), but the depth level should be
enough anyway. I am aware of the fact that this reduces a bit the
information provided for what's called a "secondary" file in your
patch set, but I tend toward moving into something that's
minimalistic. Currently, we provide the file and its '@' shortcut,
and we don't mention at all the file where it comes from. This is a
minimal problem now because we only have pg_hba.conf to worry about
for most users, still I am wondering whether it would be better in the
long run to complete that with an error context that mentions the
file(s) where this comes from and report a full chain of the events
that led to this opening failure. At the end, I think that 0002 moves
us closer toward the goal of this thread. Not been a fan of
HbaIncludeKind since the patch was proposed, TBH, as well.

Another thing is that this removes the need for open_inc_file(), my
opinion is that it is overcomplicated, and actually it seems like
under the non-strict mode (aka include_if_exists), we would generate a
LOG to mention that a configuration file is skipped, but there would
be nothing in err_msg for the TokenizedAuthLine, which is a mistake I
guess?

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

While thinking more about the logic, I saw that your approach to use
the same MemoryContext "linecxt" and pass it down to the various
layers with tokenize_file_with_context() has the advantage to remove
the need to copy a list of TokenizedAuthLine coming from other
included files and/or dirs, which could easily lead to bugs especially
for the sourcefile if someone is not careful enough. There is no need
to touch the existing tokenize_inc_file() that copies a set of
AuthTokens to an existing list in TokenizedAuthLine, of course.

Should we do without tokenize_file_with_context() though? There is an
argument for making everything go through tokenize_auth_file(), having
a MemoryContext as argument (aka if NULL create it, if not use it),
while still returning the memory context used? Similarly, it looks
like we should have no need for process_included_authfile() at the
end.

I have applied as a1a7bb8 the refactoring of the directory logic for
configuration files, and noticed that this could also be used for
tokenize_inc_file(). This reduces the whole patch by ~15%.

Attached is a set of three patches:
- 0001 changes tokenize_inc_file() to use AbsoluteConfigLocation().
AbsoluteConfigLocation() uses a static buffer and a MAXPGPATH, but
we'd rather change it to use a palloc()+strcpy() instead and remove
the static restriction? What do you think? The same applies for the
case where we use DataDir, actually, and it seems like there is no
point in this path-length restriction in this code path.
- 0002 invents the interface to open auth files and check for their
depths, simplifying the main patch a bit as there is no need to track
the depth level here and there anymore.
- 0003 is the rebased patch, simplified after the other changes. The
bulk of the patch is in its TAP test.
--
Michael