On Tue, Sep 20, 2022 at 01:33:09PM +0200, Drouvot, Bertrand wrote:
> I assume (maybe i should not) that if objects starting with / already exist
> there is very good reason(s) behind. Then I don't think that preventing
> their creation in the DDL would help (quite the contrary for the ones that
> really need them).
I have been pondering on this point for the last few weeks, and I'd
like to change my opinion and side with Tom on this one as per the
very unlikeliness of this being a problem in the wild. I have studied
the places that would require restrictions but that was just feeling
adding a bit more bloat into the CREATE/ALTER ROLE paths for what's
aimed at providing a consistent experience for the user across
pg_hba.conf and pg_ident.conf.
> It looks to me that adding a GUC (off by default) to enable/disable the
> regexp usage in the hba could be a fair compromise. It won't block any
> creation starting with a / and won't open more doors (if such objects exist)
> by default.
Enforcing a behavior change in HBA policies with a GUC does not strike
me as a good thing in the long term. I am ready to bet that it would
just sit around for nothing like the compatibility GUCs.
Anyway, I have looked at the patch.
+ List *roles_re;
+ List *databases_re;
+ regex_t hostname_re;
I am surprised by the approach of using separate lists for the regular
expressions and the raw names. Wouldn't it be better to store
everything in a single list but assign an entry type? In this case it
would be either regex or plain string. This would minimize the
footprint of the changes (no extra arguments *_re in the routines
checking for a match on the roles, databases or hosts). And it seems
to me that this would make unnecessary the use of re_num here and
there. The hostname is different, of course, requiring only an extra
field for its type, or something like that.
Perhaps the documentation would gain in clarity if there were more
examples, like a set of comma-separated examples (mix of regex and raw
strings for example, for all the field types that gain support for
regexes)?
-$node->append_conf('postgresql.conf', "log_connections = on\n");
+$node->append_conf(
+ 'postgresql.conf', qq{
+listen_addresses = '127.0.0.1'
+log_connections = on
+});
Hmm. I think that we may need to reconsider the location of the tests
for the regexes with the host name, as the "safe" regression tests
should not switch listen_addresses. One location where we already do
that is src/test/ssl/, so these could be moved there. Keeping the
database and user name parts in src/test/authentication/ is fine.
Something that stood out on a first review is the refactoring of
001_password.pl that can be done independently of the main patch:
- test_role() -> test_conn() to be able to pass down a database name.
- reset_pg_hba() to control the host, db and user parts. The host
part does not really apply after moving the hosts checks to a more
secure location, so I guess that this had better be extended just for
the user and database, keeping host=local all the time.
I am planning to apply 0001 attached independently, reducing the
footprint of 0002, which is your previous patch left untouched
(mostly!).
--
Michael
