Re: Allow file inclusion in pg_hba and pg_ident files

On Wed, May 18, 2022 at 03:12:45PM +0800, Julien Rouhaud wrote:
> The cfbot reports that the patch doesn't apply anymore, rebased v7 attached.

+ switch (kind)
+ {
+ case SecondaryAuthFile:
+ msglog = "could not open secondary authentication file \"@%s\" as \"%s\": %m";
+ msgview = "could not open secondary authentication file \"@%s\" as \"%s\": %s";
+ break;
+ case IncludedAuthFile:
+ msglog = "could not open included authentication file \"%s\" as \"%s\": %m";
+ msgview = "could not open included authentication file \"%s\" as \"%s\": %s";
+ break;
+ default:
+ elog(ERROR, "unknown HbaIncludeKind: %d", kind);
+ break;
+ }
I don't think that HbaIncludeKind is necessary, considering that we
could rely on the file name to provide enough context about the type
involved in the error string generated. The "default" clause in the
switch could just be removed, btw, to generate warnings if a new value
is added in the kind enum.

+ /* relative path is relative to dir of calling file */
There are many relatives here. It seems to me that this is the kind
of behavior we should document precisely (and test!). I am
understanding the following for cascading configurations:
- pg_hba.conf has "include_dir path1".
- path1/ has file1.conf that has "include file2.conf". This means
that file2.conf has to be also included in path1/.

postmaster.c and postinit.c do that:
/*
* Load configuration files for client authentication.
*/
if (!load_hba())
{
/*
* It makes no sense to continue if we fail to load the HBA file,
* since there is no way to connect to the database in this case.
*/
ereport(FATAL,
(errmsg("could not load pg_hba.conf")));
}
This could be confusing as a different file may fail to load, while
pg_hba.conf was able to do its work outside an include clause.

How does include_dir work with the ordering of the HBA entries across
multiple files? 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".

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

+# include FILE
+# include_dir FILE
You mean s/FILE/DIRECTORY/ for include_dir, I guess?

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

In 0002, pg_hba_matches() had better have some documentation,
explaining for which purpose this function can be used with a short
example (aka for an address and a role, find the matching set of HBA
rules and report their line and file)?

I am not sure to be a huge fan of this implementation, actually. The
function is shaped so as the user provides in input the arguments to
fill hbaPort with, passing it down to check_hba(). This could bite
easily in the future if some of the internal fields filled in by the
HBA load and used by the HBA check change over time, particularly if
this stuff has no tests to provide some validation, though we
discussed that a couple of months ago. Perhaps we should think
harder on this point.

+ char tmp[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:255.255.255.255/128")];
Okay. This is the same way of doing things as network.c or
inet_cidr_ntop.c. Shouldn't we centralize the definition of such a
maximum size instead?

+ if (!load_hba())
+ ereport(ERROR,
+ (errcode(ERRCODE_CONFIG_FILE_ERROR),
+ errmsg("Invalidation auth configuration file")));
This error message sounds wrong to me. It would be more consistent to
write that as "could not load authentication file" or such.
postinit.c and postmaster.c do that (these error strings become
partially confusing with the possibility to include extra auth files,
actually, on a separate note).

+ if (PG_ARGISNULL(1))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("parameter role is mandatory")));
+ port->user_name = text_to_cstring(PG_GETARG_TEXT_PP(1));
This could be moved at the beginning of the function, before
processing the first argument of the function (address) in details.
--
Michael