Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Nov 28, 2022 at 04:12:40PM +0900, Michael Paquier wrote: > Attached is a rebased patch of the rest. With everything we have > dealt with in this CF, perhaps it would be better to mark this entry > as committed and switch to a new thread where the negative TAP tests > could be discussed? For now, I have marked the CF entry as committed. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Sun, Nov 27, 2022 at 07:04:46PM +0800, Julien Rouhaud wrote:
> And here's the rebased patch for the TAP tests. I will switch the CF entry to
> Needs Review.
I have been looking at that, and applied the part 1 of the test for
the positive tests to cover the basic ground I'd like to see covered
for this feature. I have done quite a few changes to this part, like
reworking the way the incrementation of the counters is done for the
rule/map numbers and the line numbers of each file. The idea to use a
global dictionary to track the current number of lines was rather
clear, so I have kept that. add_{hba,ident}_line() rely on the
files included in a directory to be ordered in the same way as the
backend, and I am fine to live with that.
+# Normalize the data directory for Windows
+$data_dir =~ s/\/\.\//\//g;# reduce /./ to /
+$data_dir =~ s/\/\//\//g; # reduce // to /
+$data_dir =~ s/\/$//; # remove trailing /
+$data_dir =~ s/\\/\//g;# change \ to /
The mysticism around the conversion of the data directory path is
something I'd like to avoid, or I suspect that we are going to have a
hard time debugging any issues in this area. For the positive cases
committed, I have gone through the approach of using the base name of
the configuration file to avoid those compatibility issues. Wouldn't
it better to do the same for the expected error messages, switching to
an approach where we only check for the configuration file name in all
these regexps?
+# Generate the expected output for the auth file view error reporting (file
+# name, file line, error), for the given array of error conditions, as
+# generated generated by add_hba_line/add_ident_line with an err_str.
+sub generate_log_err_rows
+{
generate_log_err_rows() does not strike me as a good interface. The
issues I am pointed at here is it depends directly on the result
returned by add_hba_line() or add_ident_line() when the caller passes
down an error string. As far as I get it, this interface is done
as-is to correctly increment the line number of one file, and I'd like
to believe that add_{hba,ident}_line() should be kept designed so as
they only return the expected matching entry in their catalog views,
without any code path returning something else (in your case the
triplet [ $filename, $fileline, $err_str ]). That makes the whole
harder to understand.
Attached is a rebased patch of the rest. With everything we have
dealt with in this CF, perhaps it would be better to mark this entry
as committed and switch to a new thread where the negative TAP tests
could be discussed? It looks like the part for the error pattern
checks compared with the logs could be the easiest portion of what's
remaining.
--
Michael
From adb076649534bf9ef3020c8a000ed7fd269b13b7 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 28 Nov 2022 16:08:15 +0900
Subject: [PATCH v24] Add regression tests for file inclusion in HBA end ident
configuration files
This covers the error tests.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya@jrouhaud
---
.../authentication/t/004_file_inclusion.pl| 454 +-
1 file changed, 452 insertions(+), 2 deletions(-)
diff --git a/src/test/authentication/t/004_file_inclusion.pl b/src/test/authentication/t/004_file_inclusion.pl
index c420f3ebca..4a49d55c6a 100644
--- a/src/test/authentication/t/004_file_inclusion.pl
+++ b/src/test/authentication/t/004_file_inclusion.pl
@@ -33,6 +33,10 @@ my %line_counters = ('hba_rule' => 0, 'ident_rule' => 0);
# the general hba rule number as an include directive generates no data
# in pg_hba_file_rules.
#
+# If an err_str is provided, it returns an arrayref containing the provided
+# filename, the current line number in that file and the provided err_str. The
+# err_str has to be a valid regex string.
+#
# This function returns the entry of pg_hba_file_rules expected when this
# is loaded by the backend.
sub add_hba_line
@@ -40,6 +44,7 @@ sub add_hba_line
my $node = shift;
my $filename = shift;
my $entry= shift;
+ my $err_str = shift;
my $globline;
my $fileline;
my @tokens;
@@ -58,11 +63,27 @@ sub add_hba_line
$fileline = ++$line_counters{$filename};
# Include directive, that does not generate a view entry.
- return '' if ($entry =~ qr/^include/);
+ if ($entry =~ qr/^include/)
+ {
+ if (defined $err_str)
+ {
+ return [ $filename, $fileline, $err_str ];
+ }
+ else
+ {
+ return '';
+ }
+ }
# Increment pg_hba_file_rules.rule_number and save it.
$globline = ++$line_counters{'hba_rule'};
+ # If caller provided an err_str, just returns the needed metadata
+ if (defined $err_str)
+ {
+ return [ $filename, $fileline, $err_str ];
+ }
+
# Generate the expected pg_hba_file_rules line
@tokens= split(/ /, $entry);
$tokens[1] = '{' . $tokens[1] . '}';# database
@@ -91,6 +112,10 @@ sub add_hba_line
# the general map number as an include directive
Re: Allow file inclusion in pg_hba and pg_ident files
On Sun, Nov 27, 2022 at 09:49:31PM +0900, Ian Lawrence Barwick wrote: > Thanks for the quick update! FWIW, I do intend to tackle this last part ASAP, as the last piece to commit to get the full picture in the tree. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
2022年11月27日(日) 20:04 Julien Rouhaud : > > On Sun, Nov 27, 2022 at 03:39:44PM +0800, Julien Rouhaud wrote: > > Le dim. 27 nov. 2022 à 15:31, Ian Lawrence Barwick > > > > > > > > I'm trying to reconcile open CommitFest entries with their actual > > > status; the entry for this: > > > > > > https://commitfest.postgresql.org/40/3558/ > > > > > > shows as "Waiting on Author", but looks like it's all been committed; > > > is there anything > > > left to do on this? > > > > > > > right the CF entry is out of date. there's still the additional tap test > > that needs to be taken care of. I sent a new version recently that works on > > windows CI, so I guess the correct status should now be needs review, > > although the latest patchset probably doesn't apply anymore since the first > > patch has been committed. I'm traveling today I'll try to send a rebased > > version in the evening > > And here's the rebased patch for the TAP tests. I will switch the CF entry to > Needs Review. Thanks for the quick update! Regards Ian Barwick
Re: Allow file inclusion in pg_hba and pg_ident files
On Sun, Nov 27, 2022 at 03:39:44PM +0800, Julien Rouhaud wrote:
> Le dim. 27 nov. 2022 à 15:31, Ian Lawrence Barwick
>
> >
> > I'm trying to reconcile open CommitFest entries with their actual
> > status; the entry for this:
> >
> > https://commitfest.postgresql.org/40/3558/
> >
> > shows as "Waiting on Author", but looks like it's all been committed;
> > is there anything
> > left to do on this?
> >
>
> right the CF entry is out of date. there's still the additional tap test
> that needs to be taken care of. I sent a new version recently that works on
> windows CI, so I guess the correct status should now be needs review,
> although the latest patchset probably doesn't apply anymore since the first
> patch has been committed. I'm traveling today I'll try to send a rebased
> version in the evening
And here's the rebased patch for the TAP tests. I will switch the CF entry to
Needs Review.
>From 8c0c6f9f477c8f1a28d475267f3bdca59c84ed86 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Thu, 24 Nov 2022 14:20:22 +0800
Subject: [PATCH v23] Add regression tests for file inclusion in HBA end ident
configuration files
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya@jrouhaud
---
src/test/authentication/meson.build | 1 +
.../authentication/t/004_file_inclusion.pl| 722 ++
2 files changed, 723 insertions(+)
create mode 100644 src/test/authentication/t/004_file_inclusion.pl
diff --git a/src/test/authentication/meson.build
b/src/test/authentication/meson.build
index c2b48c43c9..cfc23fa213 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -7,6 +7,7 @@ tests += {
't/001_password.pl',
't/002_saslprep.pl',
't/003_peer.pl',
+ 't/004_file_inclusion.pl',
],
},
}
diff --git a/src/test/authentication/t/004_file_inclusion.pl
b/src/test/authentication/t/004_file_inclusion.pl
new file mode 100644
index 00..1be807c0a2
--- /dev/null
+++ b/src/test/authentication/t/004_file_inclusion.pl
@@ -0,0 +1,722 @@
+
+# Copyright (c) 2021-2022, PostgreSQL Global Development Group
+
+# Set of tests for authentication and pg_hba.conf inclusion.
+# This test can only run with Unix-domain sockets.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Time::HiRes qw(usleep);
+use IPC::Runqw(pump finish timer);
+use Data::Dumper;
+
+if (!$use_unix_sockets)
+{
+ plan skip_all =>
+ "authentication tests cannot run without Unix-domain sockets";
+}
+
+# stores the current line counter for each file. hba_rule and ident_rule are
+# fake file names used for the global rule number for each auth view.
+my %cur_line = ('hba_rule' => 1, 'ident_rule' => 1);
+
+my $hba_file = 'subdir1/pg_hba_custom.conf';
+my $ident_file = 'subdir2/pg_ident_custom.conf';
+
+# Initialize primary node
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->start;
+
+my $data_dir = $node->data_dir;
+
+# Normalize the data directory for Windows
+$data_dir =~ s/\/\.\//\//g;# reduce /./ to /
+$data_dir =~ s/\/\//\//g; # reduce // to /
+$data_dir =~ s/\/$//; # remove trailing /
+$data_dir =~ s/\\/\//g;# change \ to /
+
+
+# Add the given payload to the given relative HBA file of the given node.
+# This function maintains the %cur_line metadata, so it has to be called in the
+# expected inclusion evaluation order in order to keep it in sync.
+#
+# If the payload starts with "include" or "ignore", the function doesn't
+# increase the general hba rule number.
+#
+# If an err_str is provided, it returns an arrayref containing the provided
+# filename, the current line number in that file and the provided err_str. The
+# err_str has to be a valid regex string.
+# Otherwise it only returns the line number of the payload in the wanted file.
+# This function has to be called in the expected inclusion evaluation order to
+# keep the %cur_line information in sync.
+sub add_hba_line
+{
+ my $node = shift;
+ my $filename = shift;
+ my $payload = shift;
+ my $err_str = shift;
+ my $globline;
+ my $fileline;
+ my @tokens;
+ my $line;
+
+ # Append the payload to the given file
+ $node->append_conf($filename, $payload);
+
+ # Get the current %cur_line counter for the file
+ if (not defined $cur_line{$filename})
+ {
+ $cur_line{$filename} = 1;
+ }
+ $fileline = $cur_line{$filename}++;
+
+ # Include directive, don't generate an underlying pg_hba_file_rules line
+ # but make sure we incremented the %cur_line counter.
+ # Also ignore line beginning with "ignore", for content of files that
+ # should not being included
+ if ($payload =~ qr/^(include|ignore)/)
+ {
+ if (defined $err_str)
+ {
+
Re: Allow file inclusion in pg_hba and pg_ident files
Le dim. 27 nov. 2022 à 15:31, Ian Lawrence Barwick > > I'm trying to reconcile open CommitFest entries with their actual > status; the entry for this: > > https://commitfest.postgresql.org/40/3558/ > > shows as "Waiting on Author", but looks like it's all been committed; > is there anything > left to do on this? > right the CF entry is out of date. there's still the additional tap test that needs to be taken care of. I sent a new version recently that works on windows CI, so I guess the correct status should now be needs review, although the latest patchset probably doesn't apply anymore since the first patch has been committed. I'm traveling today I'll try to send a rebased version in the evening >
Re: Allow file inclusion in pg_hba and pg_ident files
2022年11月25日(金) 11:25 Julien Rouhaud : > > On Fri, Nov 25, 2022 at 07:41:59AM +0900, Michael Paquier wrote: > > On Thu, Nov 24, 2022 at 05:07:24PM +0800, Julien Rouhaud wrote: > > > So I went with CONF_FILE_START_DEPTH and CONF_FILE_MAX_DEPTH. Attached > > > v22 > > > that fixes it in all the places I found. > > > > Sounds fine. Added one comment, fixed one comment, and applied. > > Thanks! > > Thanks a lot! Hi I'm trying to reconcile open CommitFest entries with their actual status; the entry for this: https://commitfest.postgresql.org/40/3558/ shows as "Waiting on Author", but looks like it's all been committed; is there anything left to do on this? Thanks Ian Barwick
Re: Allow file inclusion in pg_hba and pg_ident files
On Fri, Nov 25, 2022 at 07:41:59AM +0900, Michael Paquier wrote: > On Thu, Nov 24, 2022 at 05:07:24PM +0800, Julien Rouhaud wrote: > > So I went with CONF_FILE_START_DEPTH and CONF_FILE_MAX_DEPTH. Attached v22 > > that fixes it in all the places I found. > > Sounds fine. Added one comment, fixed one comment, and applied. > Thanks! Thanks a lot!
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Nov 24, 2022 at 05:07:24PM +0800, Julien Rouhaud wrote: > So I went with CONF_FILE_START_DEPTH and CONF_FILE_MAX_DEPTH. Attached v22 > that fixes it in all the places I found. Sounds fine. Added one comment, fixed one comment, and applied. Thanks! -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Thu, Nov 24, 2022 at 02:37:23PM +0800, Julien Rouhaud wrote:
> On Thu, Nov 24, 2022 at 02:07:21PM +0900, Michael Paquier wrote:
> > On Wed, Nov 23, 2022 at 03:56:50PM +0800, Julien Rouhaud wrote:
> > > The depth 0 is getting used quite a lot now, maybe we should have a
> > > define for
> > > it to make it easier to grep, like TOP_LEVEL_AUTH_FILE or something like
> > > that?
> > > And also add a define for the magical 10 for the max inclusion depth, for
> > > both
> > > auth files and GUC files while at it?
> >
> > Sounds like a good idea to me, and it seems to me that this had better
> > be unified between the GUCs (see ParseConfigFp() that hardcodes a
> > depth of 0) and hba.c. It looks like they could be added to
> > conffiles.h, as of CONF_FILE_START_{LEVEL,DEPTH} and
> > CONF_FILE_MAX_{LEVEL,DEPTH}. Would you like to send a patch?
So I went with CONF_FILE_START_DEPTH and CONF_FILE_MAX_DEPTH. Attached v22
that fixes it in all the places I found.
> > Now, to the tests..
> >
> > > Mmm, I haven't looked deeply so I'm not sure if the perl podules are
> > > aware of
> > > it or not, but maybe we could somehow detect the used delimiter at the
> > > beginning after normalizing the directory, and use a $DELIM rather than a
> > > plain
> > > "/"?
> >
> > I am not sure. Could you have a look and see if you can get the CI
> > back to green? The first thing I would test is to switch the error
> > patterns to be regexps based on the basenames rather than the full
> > paths (tweaking the queries on the system views to do htat), so as we
> > avoid all this business with slash and backslash transformations.
Apparently just making sure that the $node->data_dir consistently uses forward
slashes is enough to make the CI happy, for VS 2019 [1] and MinGW64 [2], so
done this way with an extra normalization step.
[1] https://cirrus-ci.com/task/4944203946917888
[2] https://cirrus-ci.com/task/6070103853760512
>From 879cf469d00d9274b67b80eb5fe47dfccf03022d Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Thu, 24 Nov 2022 16:57:53 +0800
Subject: [PATCH v22 1/2] Introduce macros for initial/maximum depth levels for
configuration files
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya@jrouhaud
---
src/backend/commands/extension.c | 4 +++-
src/backend/libpq/hba.c | 8
src/backend/utils/misc/guc-file.l | 5 +++--
src/backend/utils/misc/guc.c | 8 +---
src/include/utils/conffiles.h | 3 +++
5 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 806d6056ab..de01b792b9 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -60,6 +60,7 @@
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/conffiles.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -515,7 +516,8 @@ parse_extension_control_file(ExtensionControlFile *control,
* Parse the file content, using GUC's file parsing code. We need not
* check the return value since any errors will be thrown at ERROR
level.
*/
- (void) ParseConfigFp(file, filename, 0, ERROR, , );
+ (void) ParseConfigFp(file, filename, CONF_FILE_START_DEPTH, ERROR,
,
+);
FreeFile(file);
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 862ec18e91..8f1a0c4c73 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -620,7 +620,7 @@ free_auth_file(FILE *file, int depth)
FreeFile(file);
/* If this is the last cleanup, remove the tokenization context */
- if (depth == 0)
+ if (depth == CONF_FILE_START_DEPTH)
{
MemoryContextDelete(tokenize_context);
tokenize_context = NULL;
@@ -650,7 +650,7 @@ open_auth_file(const char *filename, int elevel, int depth,
* avoid dumping core due to stack overflow if an include file loops
back
* to itself. The maximum nesting depth is pretty arbitrary.
*/
- if (depth > 10)
+ if (depth > CONF_FILE_MAX_DEPTH)
{
ereport(elevel,
(errcode_for_file_access(),
@@ -684,7 +684,7 @@ open_auth_file(const char *filename, int elevel, int depth,
* tokenization. This will be closed with this file when coming back to
* this level of cleanup.
*/
- if (depth == 0)
+ if (depth == CONF_FILE_START_DEPTH)
{
/*
* A context may be present, but assume that it has been
eliminated
@@ -762,7 +762,7 @@ tokenize_auth_file(const char *filename, FILE *file, List
**tok_lines,
initStringInfo();
- if (depth == 0)
+ if (depth == CONF_FILE_START_DEPTH)
*tok_lines = NIL;
while
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Nov 24, 2022 at 02:07:21PM +0900, Michael Paquier wrote:
> On Wed, Nov 23, 2022 at 03:56:50PM +0800, Julien Rouhaud wrote:
> > The depth 0 is getting used quite a lot now, maybe we should have a define
> > for
> > it to make it easier to grep, like TOP_LEVEL_AUTH_FILE or something like
> > that?
> > And also add a define for the magical 10 for the max inclusion depth, for
> > both
> > auth files and GUC files while at it?
>
> Sounds like a good idea to me, and it seems to me that this had better
> be unified between the GUCs (see ParseConfigFp() that hardcodes a
> depth of 0) and hba.c. It looks like they could be added to
> conffiles.h, as of CONF_FILE_START_{LEVEL,DEPTH} and
> CONF_FILE_MAX_{LEVEL,DEPTH}. Would you like to send a patch?
Agreed, and yes I will take care of that shortly.
> > The comment seems a bit ambiguous as with "loading the top..." you probably
> > meant something like "loading the file in memory" rather than "(re)loading
> > the
> > configuration". Maybe s/loading/opening/?
>
> Right. I have used "opening" at the end.
>
> I have worked on all that, did more polishing to the docs, the
> comments and some tiny bits of the logic, and applied both 0001 and
> 0002.
Thanks a lot!
> Now, to the tests..
>
> > Mmm, I haven't looked deeply so I'm not sure if the perl podules are aware
> > of
> > it or not, but maybe we could somehow detect the used delimiter at the
> > beginning after normalizing the directory, and use a $DELIM rather than a
> > plain
> > "/"?
>
> I am not sure. Could you have a look and see if you can get the CI
> back to green? The first thing I would test is to switch the error
> patterns to be regexps based on the basenames rather than the full
> paths (tweaking the queries on the system views to do htat), so as we
> avoid all this business with slash and backslash transformations.
I'm also working on it! Hopefully I should be able to come up with a fix soon.
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 23, 2022 at 03:56:50PM +0800, Julien Rouhaud wrote:
> The depth 0 is getting used quite a lot now, maybe we should have a define for
> it to make it easier to grep, like TOP_LEVEL_AUTH_FILE or something like that?
> And also add a define for the magical 10 for the max inclusion depth, for both
> auth files and GUC files while at it?
Sounds like a good idea to me, and it seems to me that this had better
be unified between the GUCs (see ParseConfigFp() that hardcodes a
depth of 0) and hba.c. It looks like they could be added to
conffiles.h, as of CONF_FILE_START_{LEVEL,DEPTH} and
CONF_FILE_MAX_{LEVEL,DEPTH}. Would you like to send a patch?
> The comment seems a bit ambiguous as with "loading the top..." you probably
> meant something like "loading the file in memory" rather than "(re)loading the
> configuration". Maybe s/loading/opening/?
Right. I have used "opening" at the end.
I have worked on all that, did more polishing to the docs, the
comments and some tiny bits of the logic, and applied both 0001 and
0002. Now, to the tests..
> Mmm, I haven't looked deeply so I'm not sure if the perl podules are aware of
> it or not, but maybe we could somehow detect the used delimiter at the
> beginning after normalizing the directory, and use a $DELIM rather than a
> plain
> "/"?
I am not sure. Could you have a look and see if you can get the CI
back to green? The first thing I would test is to switch the error
patterns to be regexps based on the basenames rather than the full
paths (tweaking the queries on the system views to do htat), so as we
avoid all this business with slash and backslash transformations.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
Sorry for the very late answer, I had quite a lot of other things going on
recently. And thanks for taking care of the patchset!
On Wed, Nov 23, 2022 at 03:05:18PM +0900, Michael Paquier wrote:
> On Tue, Nov 22, 2022 at 05:20:01PM +0900, Michael Paquier wrote:
> > + /* XXX: this should stick to elevel for some cases? */
> > + ereport(LOG,
> > + (errmsg("skipping missing authentication file \"%s\"",
> > + inc_fullname)));
> > Should we always issue a LOG here? In some cases we use an elevel of
> > DEBUG3.
>
> And here I think that we need to use elevel. In hba.c, this would
> generate a LOG while hbafuncs.c uses DEBUG3, leading to less noise in
> the log files.
Yeah I agree the LOG message is only interesting if you're reloading the conf.
I actually thought that this is what I did sorry about that.
> > So, what do you think about something like the attached? I have begun
> > a lookup at the tests, but I don't have enough material for an actual
> > review of this part yet. Note that I have removed temporarily
> > process_included_authfile(), as I was looking at all the code branches
> > in details. The final result ought to include AbsoluteConfigLocation(),
> > open_auth_file() and tokenize_auth_file() with an extra missing_ok
> > argument, I guess ("strict" as proposed originally is not the usual
> > PG-way for ENOENT-ish problems). process_line should be used only
> > when we have no err_msg, meaning that these have been consumed in some
> > TokenizedAuthLines already.
>
> This, however, was still brittle in terms of memory handling.
> Reloading the server a few hundred times proved that this was leaking
> memory in the tokenization.
Oh, nice catch.
> This becomes quite simple once you switch
> to an approach where tokenize_auth_file() uses its own memory context,
> while we store all the TokenizedAuthLines in the static context. It
> also occurred to me that we can create and drop the static context
> holding the tokens when opening the top-root auth file, aka with a
> depth at 0. It may be a bit simpler to switch to a single-context
> approach for all the allocations of tokenize_auth_file(), though I
> find the use of a static context much cleaner for the inclusions with
> @ files when we expand an existing list.
Agreed.
The depth 0 is getting used quite a lot now, maybe we should have a define for
it to make it easier to grep, like TOP_LEVEL_AUTH_FILE or something like that?
And also add a define for the magical 10 for the max inclusion depth, for both
auth files and GUC files while at it?
> The patch can be split, again, into more pieces. Attached 0001
> reworks the memory allocation contexts for the tokenization and 0002
> is the main feature. As of things are, I am quite happy with the
> shapes of 0001 and 0002. I have tested quite a bit its robustness,
> with valgrind for example, to make sure that there are no leaks in the
> postmaster (with[out] -DEXEC_BACKEND). The inclusion logic is
> refactored to be in a state close to your previous patches, see
> tokenize_inclusion_file().
Yep I saw that. I don't have much to say about the patch, it looks good to me.
The only nitpicking I have is:
+/*
+ * Memory context holding the list of TokenizedAuthLines when parsing
+ * HBA or ident config files. This is created when loading the top HBA
+ + or ident file (depth of 0).
+ */
+static MemoryContext tokenize_context = NULL;
The comment seems a bit ambiguous as with "loading the top..." you probably
meant something like "loading the file in memory" rather than "(re)loading the
configuration". Maybe s/loading/opening/?
>
> Note that the tests are failing for some of the Windows CIs, actually,
> due to a difference in some of the paths generated vs the file paths
> (aka c:\cirrus vs c:/cirrus, as far as I saw).
Mmm, I haven't looked deeply so I'm not sure if the perl podules are aware of
it or not, but maybe we could somehow detect the used delimiter at the
beginning after normalizing the directory, and use a $DELIM rather than a plain
"/"?
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Nov 22, 2022 at 05:20:01PM +0900, Michael Paquier wrote:
> + /* XXX: this should stick to elevel for some cases? */
> + ereport(LOG,
> + (errmsg("skipping missing authentication file \"%s\"",
> + inc_fullname)));
> Should we always issue a LOG here? In some cases we use an elevel of
> DEBUG3.
And here I think that we need to use elevel. In hba.c, this would
generate a LOG while hbafuncs.c uses DEBUG3, leading to less noise in
the log files.
> So, what do you think about something like the attached? I have begun
> a lookup at the tests, but I don't have enough material for an actual
> review of this part yet. Note that I have removed temporarily
> process_included_authfile(), as I was looking at all the code branches
> in details. The final result ought to include AbsoluteConfigLocation(),
> open_auth_file() and tokenize_auth_file() with an extra missing_ok
> argument, I guess ("strict" as proposed originally is not the usual
> PG-way for ENOENT-ish problems). process_line should be used only
> when we have no err_msg, meaning that these have been consumed in some
> TokenizedAuthLines already.
This, however, was still brittle in terms of memory handling.
Reloading the server a few hundred times proved that this was leaking
memory in the tokenization. This becomes quite simple once you switch
to an approach where tokenize_auth_file() uses its own memory context,
while we store all the TokenizedAuthLines in the static context. It
also occurred to me that we can create and drop the static context
holding the tokens when opening the top-root auth file, aka with a
depth at 0. It may be a bit simpler to switch to a single-context
approach for all the allocations of tokenize_auth_file(), though I
find the use of a static context much cleaner for the inclusions with
@ files when we expand an existing list.
The patch can be split, again, into more pieces. Attached 0001
reworks the memory allocation contexts for the tokenization and 0002
is the main feature. As of things are, I am quite happy with the
shapes of 0001 and 0002. I have tested quite a bit its robustness,
with valgrind for example, to make sure that there are no leaks in the
postmaster (with[out] -DEXEC_BACKEND). The inclusion logic is
refactored to be in a state close to your previous patches, see
tokenize_inclusion_file().
Note that the tests are failing for some of the Windows CIs, actually,
due to a difference in some of the paths generated vs the file paths
(aka c:\cirrus vs c:/cirrus, as far as I saw).
--
Michael
From 7949bda7adcb378f8355f06325d4fade56077337 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 23 Nov 2022 14:39:07 +0900
Subject: [PATCH v21 1/2] Rework memory contexts in charge of HBA/ident
tokenization
The list of TokenizedAuthLines generated for the tokens are now stored
in a static context called tokenize_context, where only all the parsed
elements are stored. This context is stored when opening the top-root
of an authentication file, and is cleaned up once we are done with it
with a new routine called free_auth_file(). One call of
open_auth_file() should have one matching call of free_auth_file().
Rather than having tokenize_auth_file() return a memory context that
includes all the records, this now creates and drops one memory context
each time the function is called. This will simplify recursive calls to
this routine for the upcoming inclusion record logic.
While on it, rename tokenize_inc_file() to tokenize_expand_file() as
this would conflict with the upcoming patch that will add inclusion
records for HBA/ident files.
Reloading HBA/indent configuration in a tight loop shows no leaks, as of
one type of test done (with and without -DEXEC_BACKEND).
---
src/include/libpq/hba.h | 5 +-
src/backend/libpq/hba.c | 152 +++
src/backend/utils/adt/hbafuncs.c | 12 +--
3 files changed, 119 insertions(+), 50 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index a84a5f0961..90c51ad6fa 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -179,7 +179,8 @@ extern IdentLine *parse_ident_line(TokenizedAuthLine *tok_line, int elevel);
extern bool pg_isblank(const char c);
extern FILE *open_auth_file(const char *filename, int elevel, int depth,
char **err_msg);
-extern MemoryContext tokenize_auth_file(const char *filename, FILE *file,
- List **tok_lines, int elevel, int depth);
+extern void free_auth_file(FILE *file, int depth);
+extern void tokenize_auth_file(const char *filename, FILE *file,
+ List **tok_lines, int elevel, int depth);
#endif /* HBA_H */
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index abdebeb3f8..9064ad6714 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -76,6 +76,13 @@ typedef struct
#define token_is_keyword(t, k) (!t->quoted && strcmp(t->string, k) == 0)
#define
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Nov 17, 2022 at 11:33:05AM +0900, Michael Paquier wrote:
> By the way, I am wondering whether process_included_authfile() is
> the most intuitive interface here. The only thing that prevents a
> common routine to process the include commands is the failure on
> GetConfFilesInDir(), where we need to build a TokenizedAuthLine when
> the others have already done so tokenize_file_with_context(). Could
> it be cleaner to have a small routine like makeTokenizedAuthLine()
> that gets reused when we fail scanning a directory to build a
> TokenizedAuthLine, in combination with a small-ish routine working on
> a directory like ParseConfigDirectory() but for HBA/ident? Or we
> could just drop process_included_authfile() entirely? On failure,
> this would make the code do a next_line all the time for all the
> include clauses.
I have been waiting for your reply for some time, so I have taken some
to look at this patch by myself and hacked on it.
At the end, the thing I was not really happy about is the
MemoryContext used to store the set of TokenizedAuthLines. I have
looked at a couple of approaches, like passing around the context as
you do, but at the end there is something I found annoying: we may
tokenize a file in the line context of a different file, storing in
much more data than just the TokenizedAuthLines. Then I got a few
steps back and began using a static memory context that only stored
the TokenizedAuthLines, switching to it in one place as of the end of
tokenize_auth_file() when inserting an item. This makes hbafuncs.c a
bit less aware of the memory context, but I think that we could live
with that with a cleaner tokenization interface. That's close to what
GUCs do, in some way. Note that this may be better as an independent
patch, actually, as it impacts the current @ inclusions.
I have noticed a bug in the logic of include_if_exists: we should
ignore only files on ENOENT, but complain for other errnos after
opening the file.
The docs and the sample files have been tweaked a bit, giving a
cleaner separation between the main record types and the inclusion
ones.
+ /* XXX: this should stick to elevel for some cases? */
+ ereport(LOG,
+ (errmsg("skipping missing authentication file \"%s\"",
+ inc_fullname)));
Should we always issue a LOG here? In some cases we use an elevel of
DEBUG3.
So, what do you think about something like the attached? I have begun
a lookup at the tests, but I don't have enough material for an actual
review of this part yet. Note that I have removed temporarily
process_included_authfile(), as I was looking at all the code branches
in details. The final result ought to include AbsoluteConfigLocation(),
open_auth_file() and tokenize_auth_file() with an extra missing_ok
argument, I guess ("strict" as proposed originally is not the usual
PG-way for ENOENT-ish problems). process_line should be used only
when we have no err_msg, meaning that these have been consumed in some
TokenizedAuthLines already.
--
Michael
From c585c8d4dffb99f6abb4129cd9adf27849fa0ce0 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 14 Nov 2022 13:47:15 +0900
Subject: [PATCH v20 1/2] Allow file inclusion in pg_hba and pg_ident files.
pg_hba.conf file now has support for "include", "include_dir" and
"include_if_exists" directives, which work similarly to the same directives in
the postgresql.conf file.
Many regression tests added to cover both the new directives, but also error
detection for the whole pg_hba / pg_ident files.
Catversion is bumped.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/include/catalog/pg_proc.dat | 12 +-
src/backend/libpq/hba.c | 233 ++-
src/backend/libpq/pg_hba.conf.sample | 25 +-
src/backend/libpq/pg_ident.conf.sample| 15 +-
src/backend/utils/adt/hbafuncs.c | 39 +-
.../authentication/t/004_file_inclusion.pl| 657 ++
src/test/regress/expected/rules.out | 6 +-
doc/src/sgml/client-auth.sgml | 86 ++-
doc/src/sgml/system-views.sgml| 23 +-
9 files changed, 1020 insertions(+), 76 deletions(-)
create mode 100644 src/test/authentication/t/004_file_inclusion.pl
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index f15aa2dbb1..f9301b2627 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6161,16 +6161,16 @@
{ oid => '3401', descr => 'show pg_hba.conf rules',
proname => 'pg_hba_file_rules', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
- proallargtypes => '{int4,int4,text,_text,_text,text,text,text,_text,text}',
- proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
- proargnames => '{rule_number,line_number,type,database,user_name,address,netmask,auth_method,options,error}',
+
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 16, 2022 at 10:53:02AM +0800, Julien Rouhaud wrote: > While being the same inclusion infrastructure, it's likely that people will > have different usage. I'm assuming that for GUCs the main usage is to have > your automation tool put one of your template conf for instance > (small_vm.conf, > big_vm.conf or something like that) in an included directory, so you don't > really need a lot of them. You can also rely on ALTER SYSTEM to avoid > manually > handling configuration files entirely. > > For authentication there are probably very different pattern. The use case I > had when writing this patch is some complex application that relies on many > services, each service having dedicated role and authentication, and services > can be enabled or disabled dynamically pretty much anytime. I had to write > code to merge possibly new entries with existing pg_hba/pg_ident configuration > files. With this feature it would be much easier (and robust) to simply have > a > main pg_hba/pg_ident that includes some directory, and have the service > enable/disable simply creates/removes a dedicated file for each service, and > we > then would usually have at least a dozen files there. > > I'm assuming people doing multi-tenant can also have similar usage. So you main application for HBA/ident is include_dir/.. For GUCs, we would just skip the directory if it has no files, but the directory has to exist. Your patch is behaving the same. >> but that does not >> mean, it seems to me, that there should be an inconsistency in the way >> we process those commands because one has implemented a feature but >> not the other. On the contrary, I'd rather try to make them >> consistent. > > You mean stopping at the first error, even if it's only for the view > reporting? > That will make the reload consistent, but the view will be a bit useless then. Yep, I meant to stop the generation of the TokenizedAuthLines at the first inclusion error by letting tokenize_auth_file() return a status to be able to stop the recursions. But after some second-thoughts pondering about this, I see why I am wrong and why you are right. As you say, stopping the generation of the TokenizedAuthLines would just limit the amount of data reported at once in the view, the golden rule for HBA/ident being that we would reload nothing as long as there is at least one error reported when parsing things. So giving more control to tokenize_auth_file() to stop a recursion, say by making it return a boolean status, makes little sense. > It would be nice to have the information that an "include_if_exists" file > didn't exist, but having a log-level message in the "error" column is a clear > POLA violation. People will probably just do something like > > SELECT file_name, line_number, error FROM pg_hba_file_rules WHERE error IS NOT > NULL; > > and report an error if any row is found. Having to parse the error field to > know if that's really an error or not is going to be a huge foot-gun. Maybe > we > could indeed report the problem in err_msg but for include_if_exists display > it > in some other column of the view? Hmm. One possibility would be to add a direct mention to the "include", "include_dir" and "include_if_exists" clauses through pg_hba_file_rules.type? I don't see how to do that without making the patch much more invasive as per the existing separation between tokenization and Hba/IdentLine filling, though, and perhaps the error provided with the full path to the file would provide enough context for one to know if the failure is happening on an included file for database/user lists or a full file from an "include" clause. It also means that include_if_exists remains transparent all the time. Simpler may be for the best here, at the end. By the way, I am wondering whether process_included_authfile() is the most intuitive interface here. The only thing that prevents a common routine to process the include commands is the failure on GetConfFilesInDir(), where we need to build a TokenizedAuthLine when the others have already done so tokenize_file_with_context(). Could it be cleaner to have a small routine like makeTokenizedAuthLine() that gets reused when we fail scanning a directory to build a TokenizedAuthLine, in combination with a small-ish routine working on a directory like ParseConfigDirectory() but for HBA/ident? Or we could just drop process_included_authfile() entirely? On failure, this would make the code do a next_line all the time for all the include clauses. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Le mer. 16 nov. 2022 à 13:01, Ian Lawrence Barwick a
écrit :
> 2022年11月14日(月) 14:41 Michael Paquier :
> >
> > On Sat, Nov 12, 2022 at 04:13:53PM +0800, Julien Rouhaud wrote:
> > > It's looks good to me. I agree that file name and line number should
> be enough
> > > to diagnose any unexpected error.
> >
> > Thanks for checking. I have looked at 0001 and 0002 again with a
> > fresh mind, and applied both of them this morning.
>
> Hi
>
> Just a quick note to mention that 0003 adds a new test ("
> 004_file_inclusion.pl")
> but doesn't seem to include it in the "meson.build" file.
>
ah thanks! Michael also mentioned that previously but as we were focusing
on preliminary refactoring patches I didn't check if if was fixed in the
patches sent recently, but I will definitely take care of it for the next
round
>
Re: Allow file inclusion in pg_hba and pg_ident files
2022年11月14日(月) 14:41 Michael Paquier :
>
> On Sat, Nov 12, 2022 at 04:13:53PM +0800, Julien Rouhaud wrote:
> > It's looks good to me. I agree that file name and line number should be
> > enough
> > to diagnose any unexpected error.
>
> Thanks for checking. I have looked at 0001 and 0002 again with a
> fresh mind, and applied both of them this morning.
Hi
Just a quick note to mention that 0003 adds a new test ("004_file_inclusion.pl")
but doesn't seem to include it in the "meson.build" file.
Regards
Ian Barwick
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Nov 15, 2022 at 08:46:55AM +0900, Michael Paquier wrote: > On Mon, Nov 14, 2022 at 03:47:27PM +0800, Julien Rouhaud wrote: > > > > If you have an include_dir directive and multiple files have wrong > > permission > > (or maybe broken symlink or something like that), you will get multiple > > errors > > when trying to process that single directive. I think it's friendlier to > > report as much detail as we can, so users can make sure they fix everything > > rather than iterating over the first error. That's especially helpful if > > the > > fix is done in some external tooling (puppet or whatever) rather than > > directly > > on the server. > > Hmm, Okay. Well, this would include only errors on I/O or permission > problems for existing files.. I have not seen deployments that have > dozens of sub-files for GUCs with an included dir, but perhaps I lack > of experience on user histories. While being the same inclusion infrastructure, it's likely that people will have different usage. I'm assuming that for GUCs the main usage is to have your automation tool put one of your template conf for instance (small_vm.conf, big_vm.conf or something like that) in an included directory, so you don't really need a lot of them. You can also rely on ALTER SYSTEM to avoid manually handling configuration files entirely. For authentication there are probably very different pattern. The use case I had when writing this patch is some complex application that relies on many services, each service having dedicated role and authentication, and services can be enabled or disabled dynamically pretty much anytime. I had to write code to merge possibly new entries with existing pg_hba/pg_ident configuration files. With this feature it would be much easier (and robust) to simply have a main pg_hba/pg_ident that includes some directory, and have the service enable/disable simply creates/removes a dedicated file for each service, and we then would usually have at least a dozen files there. I'm assuming people doing multi-tenant can also have similar usage. > > I think that the problem is that we have the same interface for processing > > the > > files on a startup/reload and for filling the views, so if we want the > > views to > > be helpful and report all errors we have to also allow a bogus "include" to > > continue in the reload case too. The same problem doesn't exists for GUCs, > > so > > a slightly different behavior there might be acceptable. > > Well, there is as well the point that we don't have yet a view for > GUCs that does the equivalent of HBA and ident, Yes that's what I meant by "the same problem doesn't exists for GUCs". > but that does not > mean, it seems to me, that there should be an inconsistency in the way > we process those commands because one has implemented a feature but > not the other. On the contrary, I'd rather try to make them > consistent. You mean stopping at the first error, even if it's only for the view reporting? That will make the reload consistent, but the view will be a bit useless then. > As things are in the patch, the only difference between > "include_if_exists" and "include" is that the latter would report some > information if the file goes missing, the former generates a LOG entry > about the file being skipped without something in the system view. > Now, wouldn't it be useful for the end-user to report that a file is > skipped as an effect of "include_if_exists" in the system views? If > not, then what's the point of having this clause to begin with? I don't really get the argument "the proposed monitoring view doesn't give this information, so the feature isn't needed". Also, unless I'm missing something the other difference between "include" and "include_if_exists" is that the first one will refuse to reload the conf (or start the server) if the file is missing, while the other one will? > My > opinion is that both clauses are useful, still on the ground of > consistency both clauses should work the same as for GUCs. Both > should report something in err_msg even if that's just about a file > being skipped, though I agree that this could be considered confusing > as well for an if_exists clause (does not look like a big deal to me > based on the debuggability gain). It would be nice to have the information that an "include_if_exists" file didn't exist, but having a log-level message in the "error" column is a clear POLA violation. People will probably just do something like SELECT file_name, line_number, error FROM pg_hba_file_rules WHERE error IS NOT NULL; and report an error if any row is found. Having to parse the error field to know if that's really an error or not is going to be a huge foot-gun. Maybe we could indeed report the problem in err_msg but for include_if_exists display it in some other column of the view?
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Nov 14, 2022 at 03:47:27PM +0800, Julien Rouhaud wrote: > On Mon, Nov 14, 2022 at 02:40:37PM +0900, Michael Paquier wrote: >> If the caller passes NULL for *linectx as the initial line context, >> just create it as we do now. If *linectx is not NULL, just reuse it. >> That may be cleaner than returning the created MemoryContext as >> returned result from tokenize_auth_file(). > > I originally used two functions as it's a common pattern (e.g. ReadBuffer / > ReadBufferExtended or all the *_internal versions of functions) and to avoid > unnecessary branch, but I agree that here having an extra branch is unlikely > to > make any measurable difference. It would only matter on -DEXEC_BACKEND / > win32, and in that case the extra overhead (over an already expensive new > backend mechanism) is way more than that, so +1 to keep things simple here. Oh, Okay. So that was your intention here. >> This aggregates all the error messages for all the files included in a >> given repository. As the patch stands, it seems to me that we would >> get errors related to an include_dir clause for two cases: >> - The specified path does not exist, in which case we have only one >> err_msg to consume and report back. >> - Multiple failures in opening and/or reading included files. >> In the second case, aggregating the reports would provide a full set >> of information, but that's not something a user would be able to act >> on directly as this is system-related. Or there is a case to know a >> full list of files in the case of multiple files that cannot be read >> because of permission issues? We may be fine with just the first >> system error here. Note that in the case where files can be read and >> opened, these would have their own TokenizedAuthLines for each line >> parsed, meaning one line in the SQL views once translated to an >> HbaLine or an IdentLine. > > If you have an include_dir directive and multiple files have wrong permission > (or maybe broken symlink or something like that), you will get multiple errors > when trying to process that single directive. I think it's friendlier to > report as much detail as we can, so users can make sure they fix everything > rather than iterating over the first error. That's especially helpful if the > fix is done in some external tooling (puppet or whatever) rather than directly > on the server. Hmm, Okay. Well, this would include only errors on I/O or permission problems for existing files.. I have not seen deployments that have dozens of sub-files for GUCs with an included dir, but perhaps I lack of experience on user histories. > I think that the problem is that we have the same interface for processing the > files on a startup/reload and for filling the views, so if we want the views > to > be helpful and report all errors we have to also allow a bogus "include" to > continue in the reload case too. The same problem doesn't exists for GUCs, so > a slightly different behavior there might be acceptable. Well, there is as well the point that we don't have yet a view for GUCs that does the equivalent of HBA and ident, but that does not mean, it seems to me, that there should be an inconsistency in the way we process those commands because one has implemented a feature but not the other. On the contrary, I'd rather try to make them consistent. As things are in the patch, the only difference between "include_if_exists" and "include" is that the latter would report some information if the file goes missing, the former generates a LOG entry about the file being skipped without something in the system view. Now, wouldn't it be useful for the end-user to report that a file is skipped as an effect of "include_if_exists" in the system views? If not, then what's the point of having this clause to begin with? My opinion is that both clauses are useful, still on the ground of consistency both clauses should work the same as for GUCs. Both should report something in err_msg even if that's just about a file being skipped, though I agree that this could be considered confusing as well for an if_exists clause (does not look like a big deal to me based on the debuggability gain). -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Mon, Nov 14, 2022 at 02:40:37PM +0900, Michael Paquier wrote:
> On Sat, Nov 12, 2022 at 04:13:53PM +0800, Julien Rouhaud wrote:
> > It's looks good to me. I agree that file name and line number should be
> > enough
> > to diagnose any unexpected error.
>
> Thanks for checking. I have looked at 0001 and 0002 again with a
> fresh mind, and applied both of them this morning.
Thanks a lot!
> This makes the remaining bits of the patch much easier to follow in
> hba.c. Here are more comments after a closer review of the whole for
> the C logic.
Agreed.
> -MemoryContext
> -tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
> - int elevel, int depth)
> +static void
> +tokenize_file_with_context(MemoryContext linecxt, const char *filename,
>
> I really tend to prefer having one routine rather than two for the
> tokenization entry point. Switching to the line context after setting
> up the callback is better, and tokenize_file_with_context() does so.
> Anyway, what do you think about having one API that gains a
> "MemoryContext *" argument, as of the following:
> void tokenize_auth_file(const char *filename, FILE *file,
> List **tok_lines,
> int depth, int elevel, MemoryContext *linectx)
>
> If the caller passes NULL for *linectx as the initial line context,
> just create it as we do now. If *linectx is not NULL, just reuse it.
> That may be cleaner than returning the created MemoryContext as
> returned result from tokenize_auth_file().
I originally used two functions as it's a common pattern (e.g. ReadBuffer /
ReadBufferExtended or all the *_internal versions of functions) and to avoid
unnecessary branch, but I agree that here having an extra branch is unlikely to
make any measurable difference. It would only matter on -DEXEC_BACKEND /
win32, and in that case the extra overhead (over an already expensive new
backend mechanism) is way more than that, so +1 to keep things simple here.
> +/* Cumulate errors if any. */
> +if (err_msg)
> +{
> +if (err_buf.len > 0)
> +appendStringInfoChar(_buf, '\n');
> +appendStringInfoString(_buf, err_msg);
> +}
>
> This aggregates all the error messages for all the files included in a
> given repository. As the patch stands, it seems to me that we would
> get errors related to an include_dir clause for two cases:
> - The specified path does not exist, in which case we have only one
> err_msg to consume and report back.
> - Multiple failures in opening and/or reading included files.
> In the second case, aggregating the reports would provide a full set
> of information, but that's not something a user would be able to act
> on directly as this is system-related. Or there is a case to know a
> full list of files in the case of multiple files that cannot be read
> because of permission issues? We may be fine with just the first
> system error here. Note that in the case where files can be read and
> opened, these would have their own TokenizedAuthLines for each line
> parsed, meaning one line in the SQL views once translated to an
> HbaLine or an IdentLine.
If you have an include_dir directive and multiple files have wrong permission
(or maybe broken symlink or something like that), you will get multiple errors
when trying to process that single directive. I think it's friendlier to
report as much detail as we can, so users can make sure they fix everything
rather than iterating over the first error. That's especially helpful if the
fix is done in some external tooling (puppet or whatever) rather than directly
on the server.
>
> This line of thoughts brings an interesting point, actually: there is
> an inconsistency between "include_if_exists" and "include" compared to
> the GUC processing. As of the patch, if we use "include" on a file
> that does not exist, the tokenization logic would jump over it and
> continue processing the follow-up entries anyway. This is a different
> policy than the GUCs, where we would immediately stop looking at
> parameters after an "include" if it fails because its file does not
> exist, working as a immediate stop in the processing. The difference
> that the patch brings between "include_if_exists" and "include" is
> that we report an error in one case but not the other, still skip the
> files in both cases and move on with the rest. Hence my question,
> shouldn't we do like the GUC processing for the hba and ident files,
> aka stop immediately when we fail to find a file on an "include"
> clause? This would be equivalent to doing a "break" in
> tokenize_file_with_context() after failing an include file.
I think that the problem is that we have the same interface for processing the
files on a startup/reload and for filling the views, so if we want the views to
be helpful and report all errors we have to also allow a bogus "include" to
continue in the reload case too. The same problem doesn't
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Nov 12, 2022 at 04:13:53PM +0800, Julien Rouhaud wrote:
> It's looks good to me. I agree that file name and line number should be
> enough
> to diagnose any unexpected error.
Thanks for checking. I have looked at 0001 and 0002 again with a
fresh mind, and applied both of them this morning.
This makes the remaining bits of the patch much easier to follow in
hba.c. Here are more comments after a closer review of the whole for
the C logic.
-MemoryContext
-tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
- int elevel, int depth)
+static void
+tokenize_file_with_context(MemoryContext linecxt, const char *filename,
I really tend to prefer having one routine rather than two for the
tokenization entry point. Switching to the line context after setting
up the callback is better, and tokenize_file_with_context() does so.
Anyway, what do you think about having one API that gains a
"MemoryContext *" argument, as of the following:
void tokenize_auth_file(const char *filename, FILE *file,
List **tok_lines,
int depth, int elevel, MemoryContext *linectx)
If the caller passes NULL for *linectx as the initial line context,
just create it as we do now. If *linectx is not NULL, just reuse it.
That may be cleaner than returning the created MemoryContext as
returned result from tokenize_auth_file().
+/* Cumulate errors if any. */
+if (err_msg)
+{
+if (err_buf.len > 0)
+appendStringInfoChar(_buf, '\n');
+appendStringInfoString(_buf, err_msg);
+}
This aggregates all the error messages for all the files included in a
given repository. As the patch stands, it seems to me that we would
get errors related to an include_dir clause for two cases:
- The specified path does not exist, in which case we have only one
err_msg to consume and report back.
- Multiple failures in opening and/or reading included files.
In the second case, aggregating the reports would provide a full set
of information, but that's not something a user would be able to act
on directly as this is system-related. Or there is a case to know a
full list of files in the case of multiple files that cannot be read
because of permission issues? We may be fine with just the first
system error here. Note that in the case where files can be read and
opened, these would have their own TokenizedAuthLines for each line
parsed, meaning one line in the SQL views once translated to an
HbaLine or an IdentLine.
This line of thoughts brings an interesting point, actually: there is
an inconsistency between "include_if_exists" and "include" compared to
the GUC processing. As of the patch, if we use "include" on a file
that does not exist, the tokenization logic would jump over it and
continue processing the follow-up entries anyway. This is a different
policy than the GUCs, where we would immediately stop looking at
parameters after an "include" if it fails because its file does not
exist, working as a immediate stop in the processing. The difference
that the patch brings between "include_if_exists" and "include" is
that we report an error in one case but not the other, still skip the
files in both cases and move on with the rest. Hence my question,
shouldn't we do like the GUC processing for the hba and ident files,
aka stop immediately when we fail to find a file on an "include"
clause? This would be equivalent to doing a "break" in
tokenize_file_with_context() after failing an include file.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Nov 10, 2022 at 10:29:40AM +0900, Michael Paquier wrote: > > FWIW, I have been playing with the addition of a ErrorContextCallback > in tokenize_auth_file(), and this addition leads to a really nice > result. With this method, it is possible to know the full chain of > events leading to a failure when tokenizing included files, which is > not available now in the logs when reloading the server. > > We could extend it to have more verbose information by passing more > arguments to tokenize_auth_file(), still I'd like to think that just > knowing the line number and the full path to the file is more than > enough once you know the full chain of events. 0001 and 0002 ought to > be merged together, but I am keeping these separate to show how simple > the addition of the ErrorContextCallback is. It's looks good to me. I agree that file name and line number should be enough to diagnose any unexpected error.
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 09, 2022 at 12:09:01PM +0800, Julien Rouhaud wrote:
> On Wed, Nov 09, 2022 at 09:51:17AM +0900, Michael Paquier wrote:
>> Julien, please note that this is waiting on author for now. What do
>> you think about the now-named v18-0001 and the addition of an
>> ErrorContextCallback to provide more information about the list of
>> included files on error?
>
> Yes, I'm unfortunately fully aware that it's waiting on me. I've been a bit
> busy this week with $work but I will try to go back to it as soon as I can,
> hopefully this week!
FWIW, I have been playing with the addition of a ErrorContextCallback
in tokenize_auth_file(), and this addition leads to a really nice
result. With this method, it is possible to know the full chain of
events leading to a failure when tokenizing included files, which is
not available now in the logs when reloading the server.
We could extend it to have more verbose information by passing more
arguments to tokenize_auth_file(), still I'd like to think that just
knowing the line number and the full path to the file is more than
enough once you know the full chain of events. 0001 and 0002 ought to
be merged together, but I am keeping these separate to show how simple
the addition of the ErrorContextCallback is.
--
Michael
From 41ae50e3b5b8e1d9013f337e4574702830dca95a Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 7 Nov 2022 13:35:44 +0900
Subject: [PATCH v19 1/3] Invent open_auth_file() in hba.c, to refactor auth
file opening
This adds a check on the recursion depth when including auth files,
something that has never been done when processing '@' files for
database and user name lists in pg_hba.conf.
---
src/include/libpq/hba.h | 4 +-
src/backend/libpq/hba.c | 100 ++-
src/backend/utils/adt/hbafuncs.c | 18 ++
3 files changed, 79 insertions(+), 43 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 7ad227d34a..a84a5f0961 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -177,7 +177,9 @@ extern int check_usermap(const char *usermap_name,
extern HbaLine *parse_hba_line(TokenizedAuthLine *tok_line, int elevel);
extern IdentLine *parse_ident_line(TokenizedAuthLine *tok_line, int elevel);
extern bool pg_isblank(const char c);
+extern FILE *open_auth_file(const char *filename, int elevel, int depth,
+ char **err_msg);
extern MemoryContext tokenize_auth_file(const char *filename, FILE *file,
- List **tok_lines, int elevel);
+ List **tok_lines, int elevel, int depth);
#endif /* HBA_H */
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index a9f87ab5bf..d8c0b585e5 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -117,7 +117,8 @@ static const char *const UserAuthName[] =
static List *tokenize_inc_file(List *tokens, const char *outer_filename,
- const char *inc_filename, int elevel, char **err_msg);
+ const char *inc_filename, int elevel,
+ int depth, char **err_msg);
static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
int elevel, char **err_msg);
static int regcomp_auth_token(AuthToken *token, char *filename, int line_num,
@@ -414,7 +415,7 @@ regexec_auth_token(const char *match, AuthToken *token, size_t nmatch,
*/
static List *
next_field_expand(const char *filename, char **lineptr,
- int elevel, char **err_msg)
+ int elevel, int depth, char **err_msg)
{
char buf[MAX_TOKEN];
bool trailing_comma;
@@ -431,7 +432,7 @@ next_field_expand(const char *filename, char **lineptr,
/* Is this referencing a file? */
if (!initial_quote && buf[0] == '@' && buf[1] != '\0')
tokens = tokenize_inc_file(tokens, filename, buf + 1,
- elevel, err_msg);
+ elevel, depth + 1, err_msg);
else
tokens = lappend(tokens, make_auth_token(buf, initial_quote));
} while (trailing_comma && (*err_msg == NULL));
@@ -459,6 +460,7 @@ tokenize_inc_file(List *tokens,
const char *outer_filename,
const char *inc_filename,
int elevel,
+ int depth,
char **err_msg)
{
char *inc_fullname;
@@ -468,24 +470,18 @@ tokenize_inc_file(List *tokens,
MemoryContext linecxt;
inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename);
+ inc_file = open_auth_file(inc_fullname, elevel, depth, err_msg);
- inc_file = AllocateFile(inc_fullname, "r");
if (inc_file == NULL)
{
- int save_errno = errno;
-
- ereport(elevel,
-(errcode_for_file_access(),
- errmsg("could not open secondary authentication file \"@%s\" as \"%s\": %m",
- inc_filename, inc_fullname)));
- *err_msg = psprintf("could not open secondary authentication file \"@%s\" as \"%s\": %s",
- inc_filename, inc_fullname, strerror(save_errno));
+ /* error already logged */
pfree(inc_fullname);
return tokens;
}
/* There is possible recursion here if the file contains @ */
-
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Wed, Nov 09, 2022 at 09:51:17AM +0900, Michael Paquier wrote: > On Tue, Nov 08, 2022 at 10:04:16AM +0900, Michael Paquier wrote: > > CF bot unhappy as I have messed up with rules.out. Rebased. I have > > removed the restriction on MAXPGPATH in AbsoluteConfigLocation() in > > 0001, while on it. The absolute paths built on GUC or ident > > inclusions are the same. > > Rebased after 6bbd8b7g that is an equivalent of the previous 0001. Thanks a lot! > Julien, please note that this is waiting on author for now. What do > you think about the now-named v18-0001 and the addition of an > ErrorContextCallback to provide more information about the list of > included files on error? Yes, I'm unfortunately fully aware that it's waiting on me. I've been a bit busy this week with $work but I will try to go back to it as soon as I can, hopefully this week!
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Nov 08, 2022 at 10:04:16AM +0900, Michael Paquier wrote:
> CF bot unhappy as I have messed up with rules.out. Rebased. I have
> removed the restriction on MAXPGPATH in AbsoluteConfigLocation() in
> 0001, while on it. The absolute paths built on GUC or ident
> inclusions are the same.
Rebased after 6bbd8b7g that is an equivalent of the previous 0001.
Julien, please note that this is waiting on author for now. What do
you think about the now-named v18-0001 and the addition of an
ErrorContextCallback to provide more information about the list of
included files on error?
--
Michael
From 50441282e6b298ee737e6e1a490d77baedfe5d69 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 7 Nov 2022 13:35:44 +0900
Subject: [PATCH v18 1/2] Invent open_auth_file() in hba.c, to refactor auth
file opening
This adds a check on the recursion depth when including auth files,
something that has never been done when processing '@' files for
database and user name lists in pg_hba.conf.
---
src/include/libpq/hba.h | 4 +-
src/backend/libpq/hba.c | 100 ++-
src/backend/utils/adt/hbafuncs.c | 18 ++
3 files changed, 79 insertions(+), 43 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 7ad227d34a..a84a5f0961 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -177,7 +177,9 @@ extern int check_usermap(const char *usermap_name,
extern HbaLine *parse_hba_line(TokenizedAuthLine *tok_line, int elevel);
extern IdentLine *parse_ident_line(TokenizedAuthLine *tok_line, int elevel);
extern bool pg_isblank(const char c);
+extern FILE *open_auth_file(const char *filename, int elevel, int depth,
+ char **err_msg);
extern MemoryContext tokenize_auth_file(const char *filename, FILE *file,
- List **tok_lines, int elevel);
+ List **tok_lines, int elevel, int depth);
#endif /* HBA_H */
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index a9f87ab5bf..d8c0b585e5 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -117,7 +117,8 @@ static const char *const UserAuthName[] =
static List *tokenize_inc_file(List *tokens, const char *outer_filename,
- const char *inc_filename, int elevel, char **err_msg);
+ const char *inc_filename, int elevel,
+ int depth, char **err_msg);
static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
int elevel, char **err_msg);
static int regcomp_auth_token(AuthToken *token, char *filename, int line_num,
@@ -414,7 +415,7 @@ regexec_auth_token(const char *match, AuthToken *token, size_t nmatch,
*/
static List *
next_field_expand(const char *filename, char **lineptr,
- int elevel, char **err_msg)
+ int elevel, int depth, char **err_msg)
{
char buf[MAX_TOKEN];
bool trailing_comma;
@@ -431,7 +432,7 @@ next_field_expand(const char *filename, char **lineptr,
/* Is this referencing a file? */
if (!initial_quote && buf[0] == '@' && buf[1] != '\0')
tokens = tokenize_inc_file(tokens, filename, buf + 1,
- elevel, err_msg);
+ elevel, depth + 1, err_msg);
else
tokens = lappend(tokens, make_auth_token(buf, initial_quote));
} while (trailing_comma && (*err_msg == NULL));
@@ -459,6 +460,7 @@ tokenize_inc_file(List *tokens,
const char *outer_filename,
const char *inc_filename,
int elevel,
+ int depth,
char **err_msg)
{
char *inc_fullname;
@@ -468,24 +470,18 @@ tokenize_inc_file(List *tokens,
MemoryContext linecxt;
inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename);
+ inc_file = open_auth_file(inc_fullname, elevel, depth, err_msg);
- inc_file = AllocateFile(inc_fullname, "r");
if (inc_file == NULL)
{
- int save_errno = errno;
-
- ereport(elevel,
-(errcode_for_file_access(),
- errmsg("could not open secondary authentication file \"@%s\" as \"%s\": %m",
- inc_filename, inc_fullname)));
- *err_msg = psprintf("could not open secondary authentication file \"@%s\" as \"%s\": %s",
- inc_filename, inc_fullname, strerror(save_errno));
+ /* error already logged */
pfree(inc_fullname);
return tokens;
}
/* There is possible recursion here if the file contains @ */
- linecxt = tokenize_auth_file(inc_fullname, inc_file, _lines, elevel);
+ linecxt = tokenize_auth_file(inc_fullname, inc_file, _lines, elevel,
+ depth);
FreeFile(inc_file);
pfree(inc_fullname);
@@ -521,6 +517,59 @@ tokenize_inc_file(List *tokens,
return tokens;
}
+/*
+ * open_auth_file
+ * Open the given file.
+ *
+ * filename: the absolute path to the target file
+ * elevel: message logging level
+ * depth: recursion level of the file opened.
+ * err_msg: details about the error.
+ *
+ * Return value is the opened file. On error, returns NULL with details
+ * about the error stored in "err_msg".
+ */
+FILE *
+open_auth_file(const char *filename, int elevel,
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Nov 07, 2022 at 03:07:15PM +0900, Michael Paquier wrote:
> 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.
CF bot unhappy as I have messed up with rules.out. Rebased. I have
removed the restriction on MAXPGPATH in AbsoluteConfigLocation() in
0001, while on it. The absolute paths built on GUC or ident
inclusions are the same.
--
Michael
From f056ccbd604d63185763f774c5105f3208919306 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 8 Nov 2022 09:51:20 +0900
Subject: [PATCH v17 1/3] Expand the use of AbsoluteConfigLocation() in hba.c
The logic in charge of expanding an include file for database and user
names used the same code as AbsoluteConfigLocation() when building the
configuration file to include, so simplify this code. While on it,
remove the restriction to MAXPGPATH, and switch to the same method as
what tokenize_inc_file() used.
---
src/backend/libpq/hba.c| 17 ++---
src/backend/utils/misc/conffiles.c | 12
2 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index e9fc0af7c9..a9f87ab5bf 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -41,6 +41,7 @@
#include "storage/fd.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+#include "utils/conffiles.h"
#include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -466,21 +467,7 @@ tokenize_inc_file(List *tokens,
ListCell *inc_line;
MemoryContext linecxt;
- if (is_absolute_path(inc_filename))
- {
- /* absolute path is taken as-is */
- inc_fullname = pstrdup(inc_filename);
- }
- else
- {
- /* relative path is relative to dir of calling file */
- inc_fullname = (char *) palloc(strlen(outer_filename) + 1 +
- strlen(inc_filename) + 1);
- strcpy(inc_fullname, outer_filename);
- get_parent_directory(inc_fullname);
- join_path_components(inc_fullname, inc_fullname, inc_filename);
- canonicalize_path(inc_fullname);
- }
+ inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename);
inc_file = AllocateFile(inc_fullname, "r");
if (inc_file == NULL)
diff --git a/src/backend/utils/misc/conffiles.c b/src/backend/utils/misc/conffiles.c
index 4a99a1961e..35e2a3790b 100644
--- a/src/backend/utils/misc/conffiles.c
+++ b/src/backend/utils/misc/conffiles.c
@@ -35,15 +35,17 @@
char *
AbsoluteConfigLocation(const char *location, const char *calling_file)
{
- char abs_path[MAXPGPATH];
-
if (is_absolute_path(location))
return pstrdup(location);
else
{
+ char *abs_path;
+
if (calling_file != NULL)
{
- strlcpy(abs_path, calling_file, sizeof(abs_path));
+ abs_path = (char *) palloc0(strlen(calling_file) + 1 +
+ strlen(location) + 1);
+ strcpy(abs_path, calling_file);
get_parent_directory(abs_path);
join_path_components(abs_path, abs_path, location);
canonicalize_path(abs_path);
@@ -51,10 +53,12 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)
else
{
Assert(DataDir);
+ abs_path = (char *) palloc0(strlen(DataDir) + 1 +
+ strlen(location) + 1);
join_path_components(abs_path, DataDir, location);
canonicalize_path(abs_path);
}
- return pstrdup(abs_path);
+ return abs_path;
}
}
--
2.38.1
From e201139be17e525c266240acf762c2b6a9fe6436 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 7 Nov 2022 13:35:44 +0900
Subject: [PATCH v17 2/3] Invent open_auth_file() in hba.c, to refactor auth
file opening
This adds a check on the recursion depth when including auth files,
something that has never been done when processing '@' files for
database and user name lists in pg_hba.conf.
---
src/include/libpq/hba.h | 4 +-
src/backend/libpq/hba.c | 100 ++-
src/backend/utils/adt/hbafuncs.c | 18 ++
3 files changed, 79 insertions(+), 43 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 7ad227d34a..a84a5f0961 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -177,7 +177,9 @@ extern int check_usermap(const char *usermap_name,
extern HbaLine *parse_hba_line(TokenizedAuthLine *tok_line, int elevel);
extern IdentLine *parse_ident_line(TokenizedAuthLine *tok_line, int elevel);
extern
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
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 02, 2022 at 09:06:02PM +0800, Julien Rouhaud wrote: > Maybe one alternative approach would be to keep modifying the current test, > and > only do the split when committing the first part? The split itself should be > easy and mechanical: just remove everything unneeded (different file names > including the file name argument from the add_(hba|ident)_line, inclusion > directive and so on). Even if the diff is then difficult to read it's not > really a problem as it should have already been reviewed. I have not reviewed the test part much yet, TBH. The path manipulations because of WIN32 are a bit annoying. I was wondering if we could avoid all that by using the basenames, as one option. >> I have spent some time starting at the logic today of the whole, and >> GetConfFilesInDir() is really the first thing that stood out. I am >> not sure that it makes much sense to keep that in guc-file.c, TBH, >> once we feed it into hba.c. Perhaps we could put the refactored >> routine (and AbsoluteConfigLocation as a side effect) into a new file >> in misc/? > > Yes I was wondering about it too. A new fine in misc/ looks sensible. conffiles.c is the best thing I could come up with, conf.c and config.c being too generic and these are routines that work on configuration files, so.. >> Your patch proposes a different alternative, which is to pass down the >> memory context created in tokenize_auth_file() down to the callers >> with tokenize_file_with_context() dealing with all the internals. >> process_included_auth_file() is different extension of that. >> This may come down to a matter of taste, but could be be cleaner to >> take an approach similar to tokenize_inc_file() and just create a copy >> of the AuthToken list coming from a full file and append it to the >> result rather than passing around the memory context for the lines? >> This would lead to some simplifications, it seems, at least with the >> number of arguments passed down across the layers. > > I guess it goes in the same direction as 8fea86830e1 where infrastructure to > copy AuthTokens was added, I'm fine either way. I won't hide that I am trying to make the changes a maximum incremental for this thread so as the final part is easy-ish. >> 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. >> 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. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Nov 02, 2022 at 04:46:48PM +0900, Michael Paquier wrote: > On Fri, Oct 28, 2022 at 11:49:54AM +0800, Julien Rouhaud wrote: > > To be honest I'd rather not to. It's excessively annoying to work on those > > tests (I spent multiple days trying to make it as clean and readable as > > possible), and splitting it to only test the current infrastructure will > > need > > some substantial efforts. > > Well, I'd really like to split things as much as possible, beginning > with some solid basics before extending its capabilities. This > reduces the odds of introducing issues in-between, particularly in > areas as sensible as authentication that involves not-yet-logged-in > users. > > > But more importantly, the next commit that will add tests for file inclusion > > will then be totally unmaintainable and unreadable, so that's IMO even > > worse. > > I think it will probably either be the current file overwritten or a new one > > written from scratch if some changes are done in the simplified test, and > > I'm > > not volunteering to do that. > > Not sure about that either. Maybe one alternative approach would be to keep modifying the current test, and only do the split when committing the first part? The split itself should be easy and mechanical: just remove everything unneeded (different file names including the file name argument from the add_(hba|ident)_line, inclusion directive and so on). Even if the diff is then difficult to read it's not really a problem as it should have already been reviewed. > Anyway, the last patch posted on this thread does not apply and the CF > bot fails, so it needed a rebase. I have first noticed that your > patch included some doc fixes independent of this thread, so I have > applied that as e765028 and backpatched it down to 15. Yes I saw that this morning, thanks! > The TAP test > needs to be renamed to 0004, and it was missing from meson.build, > hence the CI was not testing it. Ah right. This is quite annoying that we have to explicitly name every single test file there. For the source code it's hard to not notice a missed file and you will get an error in the CI, but a missed test is just entirely transparent :( > I have spent some time starting at the logic today of the whole, and > GetConfFilesInDir() is really the first thing that stood out. I am > not sure that it makes much sense to keep that in guc-file.c, TBH, > once we feed it into hba.c. Perhaps we could put the refactored > routine (and AbsoluteConfigLocation as a side effect) into a new file > in misc/? Yes I was wondering about it too. A new fine in misc/ looks sensible. > As of HEAD, tokenize_inc_file() is the place where we handle a list of > tokens included with an '@' file, appending the existing set of > AuthTokens into the list we are building by grabbing a copy of these > before deleting the line memory context. > > Your patch proposes a different alternative, which is to pass down the > memory context created in tokenize_auth_file() down to the callers > with tokenize_file_with_context() dealing with all the internals. > process_included_auth_file() is different extension of that. > This may come down to a matter of taste, but could be be cleaner to > take an approach similar to tokenize_inc_file() and just create a copy > of the AuthToken list coming from a full file and append it to the > result rather than passing around the memory context for the lines? > This would lead to some simplifications, it seems, at least with the > number of arguments passed down across the layers. I guess it goes in the same direction as 8fea86830e1 where infrastructure to copy AuthTokens was added, I'm fine either way. > 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. > 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
Re: Allow file inclusion in pg_hba and pg_ident files
On Fri, Oct 28, 2022 at 11:49:54AM +0800, Julien Rouhaud wrote:
> To be honest I'd rather not to. It's excessively annoying to work on those
> tests (I spent multiple days trying to make it as clean and readable as
> possible), and splitting it to only test the current infrastructure will need
> some substantial efforts.
Well, I'd really like to split things as much as possible, beginning
with some solid basics before extending its capabilities. This
reduces the odds of introducing issues in-between, particularly in
areas as sensible as authentication that involves not-yet-logged-in
users.
> But more importantly, the next commit that will add tests for file inclusion
> will then be totally unmaintainable and unreadable, so that's IMO even worse.
> I think it will probably either be the current file overwritten or a new one
> written from scratch if some changes are done in the simplified test, and I'm
> not volunteering to do that.
Not sure about that either.
Anyway, the last patch posted on this thread does not apply and the CF
bot fails, so it needed a rebase. I have first noticed that your
patch included some doc fixes independent of this thread, so I have
applied that as e765028 and backpatched it down to 15. The TAP test
needs to be renamed to 0004, and it was missing from meson.build,
hence the CI was not testing it.
I have spent some time starting at the logic today of the whole, and
GetConfFilesInDir() is really the first thing that stood out. I am
not sure that it makes much sense to keep that in guc-file.c, TBH,
once we feed it into hba.c. Perhaps we could put the refactored
routine (and AbsoluteConfigLocation as a side effect) into a new file
in misc/?
As of HEAD, tokenize_inc_file() is the place where we handle a list of
tokens included with an '@' file, appending the existing set of
AuthTokens into the list we are building by grabbing a copy of these
before deleting the line memory context.
Your patch proposes a different alternative, which is to pass down the
memory context created in tokenize_auth_file() down to the callers
with tokenize_file_with_context() dealing with all the internals.
process_included_auth_file() is different extension of that.
This may come down to a matter of taste, but could be be cleaner to
take an approach similar to tokenize_inc_file() and just create a copy
of the AuthToken list coming from a full file and append it to the
result rather than passing around the memory context for the lines?
This would lead to some simplifications, it seems, at least with the
number of arguments passed down across the layers.
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 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).
Attached are two patches: the first one is a rebase of what you have
posted, and the second one are some changes I did while playing with
the logic. In the second one, except for conffiles.{h.c}, the changes
are just a POC but that's to show the areas that I am planning to
rework, and your tests pass with it. I still need to think about all
that and reconsider the design of the interface that would fit with
the tokenization of the inclusions, without that many subroutines to
do the work as it makes the code harder to follow. Well, that's to
say that I am not staying idle :)
--
Michael
From 2b727d7f011b94952afda81e94b369dc2d60a98f Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 2 Nov 2022 16:42:53 +0900
Subject: [PATCH v15 1/2] Allow file inclusion in pg_hba and pg_ident files.
pg_hba.conf file now has support for "include", "include_dir" and
"include_if_exists" directives, which work similarly to the same directives in
the postgresql.conf file.
This fixes a possible crash if a secondary file tries to include itself as
there's now a nesting depth check in the inclusion code path, same as the
postgresql.conf.
Many regression tests added to cover both the new directives, but also error
detection for the whole pg_hba / pg_ident files.
Catversion is bumped.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/include/catalog/pg_proc.dat | 12 +-
src/include/libpq/hba.h
Re: Allow file inclusion in pg_hba and pg_ident files
On Fri, Oct 28, 2022 at 10:24:23AM +0900, Michael Paquier wrote: > On Thu, Oct 27, 2022 at 12:26:25PM +0800, Julien Rouhaud wrote: > > I am still not completely sure what's the best way to do things here, > so let's do the following: let's keep the patch the way you think is > better for now (I may change my opinion on that but I'll hack that by > myself anyway). Using what you have as a base, could you split the > test and have it in its simplest to ease irs review? It would be able > to stress the buildfarm with a first version of the test and see how > it goes from there, and it is useful by itself IMO as HEAD has zero > coverage for this area. To be honest I'd rather not to. It's excessively annoying to work on those tests (I spent multiple days trying to make it as clean and readable as possible), and splitting it to only test the current infrastructure will need some substantial efforts. But more importantly, the next commit that will add tests for file inclusion will then be totally unmaintainable and unreadable, so that's IMO even worse. I think it will probably either be the current file overwritten or a new one written from scratch if some changes are done in the simplified test, and I'm not volunteering to do that.
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Oct 27, 2022 at 12:26:25PM +0800, Julien Rouhaud wrote: > On Thu, Oct 27, 2022 at 12:08:31PM +0900, Michael Paquier wrote: >> >> Putting things afresh, there are two different things here (sorry I >> need to see that typed ;p): >> 1) How do we want to check reliably the loading of the HBA and ident >> files on errors? > > I guess you meant the failure to load HBA / ident files containing invalid > data? Yeah. >> Hmm. And what if we just gave up on the checks for error patterns in >> pg_hba_file_rules? One part that I was thinking about when typing this part yesterday is that an EXEC_BACKEND build should work in non-WIN32 in TAP even if pg_ident.conf cannot be loaded, but I forgot entirely about the part where we need a user mapping for the SSPI authentication on WIN32, as set by pg_regress. > We discussed this problem in the past (1), and my understanding was that > detecting a -DEXEC_BACKEND/Win32 build and skipping those tests in that case > would be an acceptable solution to make sure there's at least some coverage. > The proposed patch adds such an approach, making sure that the failure is due > to an invalid HBA file. If you changed you mind I can remove that part, but > again I'd like to be sure of what you exactly want before starting to rewrite > stuff. I am still not completely sure what's the best way to do things here, so let's do the following: let's keep the patch the way you think is better for now (I may change my opinion on that but I'll hack that by myself anyway). Using what you have as a base, could you split the test and have it in its simplest to ease irs review? It would be able to stress the buildfarm with a first version of the test and see how it goes from there, and it is useful by itself IMO as HEAD has zero coverage for this area. > I'm not sure what you mean here. The patch does check for all the errors > looking at LOG lines and CONTEXT lines, but to make the regexp easier it > doesn't try to make sure that each CONTEXT line is immediately following the > expected LOG line. Hmm. Perhaps we'd better make sure that the LOG/CONTEXT link is checked? The context includes the line number while a generic sentence, and the LOG provides all the details of the error happening. > That's why the errors are divided in 2 steps: a first step with a single error > using some inclusion, so we can validate that the CONTEXT line is entirely > correct (wrt. line number and such), and then every possible error pattern > where we assume that the CONTEXT line are still following their LOG entry if > they're found. It also has the knowledge of which errors adds a CONTEXT line > and which don't. And that's done twice, for HBA and ident. Okay, so you do check the relationship between both, after all. > The part 3 is just concatenating everything back, for HBA and ident. So > long-term maintenance shouldn't get any harder as there won't be any need for > more steps. We can just keep appending stuff in the 2nd step and all the > tests > should run as expected. Hmm. Okay. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Thu, Oct 27, 2022 at 12:08:31PM +0900, Michael Paquier wrote: > > Putting things afresh, there are two different things here (sorry I > need to see that typed ;p): > 1) How do we want to check reliably the loading of the HBA and ident > files on errors? I guess you meant the failure to load HBA / ident files containing invalid data? > EXEC_BACKEND would reload an entire new thing for > each connection, hence we need some loops to go through that. > 2) How to check the contents of pg_hba_file_rules and > pg_ident_file_mappings? > > There is a dependency between 1) and 2) once we try to check for error > patterns in pg_hba_file_rules, because connections would just not > happen. This is not the case for pg_ident_file_mappings though, so we > could still test for buggy patterns in pg_ident.conf (or any of its > included parts) with some expected content of > pg_ident_file_mappings.error after a successful connection. > > Hmm. And what if we just gave up on the checks for error patterns in > pg_hba_file_rules? We discussed this problem in the past (1), and my understanding was that detecting a -DEXEC_BACKEND/Win32 build and skipping those tests in that case would be an acceptable solution to make sure there's at least some coverage. The proposed patch adds such an approach, making sure that the failure is due to an invalid HBA file. If you changed you mind I can remove that part, but again I'd like to be sure of what you exactly want before starting to rewrite stuff. > One thing that we could do for this part is to > include all the buggy patterns we want to check at once in pg_hba.conf > in its included portions, then scan for all the logs produced after > attempting to start a server as the loading of pg_hba.conf would > produce one LOG line with its CONTEXT for each buggy entry. The patch > checks for error patterns with generate_log_err_rows(), but it looks > like it would make the part 3 of the new test cleaner and easier to > maintain in the long-term. I'm not sure what you mean here. The patch does check for all the errors looking at LOG lines and CONTEXT lines, but to make the regexp easier it doesn't try to make sure that each CONTEXT line is immediately following the expected LOG line. That's why the errors are divided in 2 steps: a first step with a single error using some inclusion, so we can validate that the CONTEXT line is entirely correct (wrt. line number and such), and then every possible error pattern where we assume that the CONTEXT line are still following their LOG entry if they're found. It also has the knowledge of which errors adds a CONTEXT line and which don't. And that's done twice, for HBA and ident. The part 3 is just concatenating everything back, for HBA and ident. So long-term maintenance shouldn't get any harder as there won't be any need for more steps. We can just keep appending stuff in the 2nd step and all the tests should run as expected. [1] https://www.postgresql.org/message-id/YtZLeJPlMutL9heh%40paquier.xyz
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Oct 26, 2022 at 11:32:14PM +0800, Julien Rouhaud wrote: > Have you already done a rebase while working on the patch or are you intending > to take care of it, or should I? Let's no both do the work :) Spoiler alert: I have not done a rebase yet ;) -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Oct 26, 2022 at 11:32:14PM +0800, Julien Rouhaud wrote: > I don't mind taking care of that, but before doing so I'd like to have some > feedback on whether you're ok with my approach (per my initial email about it > at [1]) or if you had some different > ideas on how to do it. Putting things afresh, there are two different things here (sorry I need to see that typed ;p): 1) How do we want to check reliably the loading of the HBA and ident files on errors? EXEC_BACKEND would reload an entire new thing for each connection, hence we need some loops to go through that. 2) How to check the contents of pg_hba_file_rules and pg_ident_file_mappings? There is a dependency between 1) and 2) once we try to check for error patterns in pg_hba_file_rules, because connections would just not happen. This is not the case for pg_ident_file_mappings though, so we could still test for buggy patterns in pg_ident.conf (or any of its included parts) with some expected content of pg_ident_file_mappings.error after a successful connection. Hmm. And what if we just gave up on the checks for error patterns in pg_hba_file_rules? One thing that we could do for this part is to include all the buggy patterns we want to check at once in pg_hba.conf in its included portions, then scan for all the logs produced after attempting to start a server as the loading of pg_hba.conf would produce one LOG line with its CONTEXT for each buggy entry. The patch checks for error patterns with generate_log_err_rows(), but it looks like it would make the part 3 of the new test cleaner and easier to maintain in the long-term. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Oct 26, 2022 at 03:56:07PM +0900, Michael Paquier wrote: > > So, I have spent a good portion of today looking at what you have > here, applying 0001 and 0003 while fixing, rebasing and testing the > whole, discarding 0002 (we could do more for the line number and > source file in terms of the LOGs reported for a regexec failure). Thanks! > Now remains 0004, which is the core of the proposal, and while it > needs a rebase, Have you already done a rebase while working on the patch or are you intending to take care of it, or should I? Let's no both do the work :) > - The TAP test, which is half the size of the patch in line number. > Could it be possible to make it more edible, introducing a basic > infrastructure to check a set of rules in pg_hba.conf and > pg_ident.conf without the inclusion logic? Checks for error patterns > (that I agree we strongly lack tests for) look like something we'd > want to tackle independently of the inclusion logic, and it should be > built on top of a basic test infra, at least. I don't mind taking care of that, but before doing so I'd like to have some feedback on whether you're ok with my approach (per my initial email about it at [1]) or if you had some different ideas on how to do it. [1] https://www.postgresql.org/message-id/20220730080936.atyxodvwlmf2wnoc@jrouhaud
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Oct 26, 2022 at 11:19:48AM +0800, Julien Rouhaud wrote: > That wouldn't be overdoing anymore if we remove the line number / filename > from > the fill_*_line prototypes right? So, I have spent a good portion of today looking at what you have here, applying 0001 and 0003 while fixing, rebasing and testing the whole, discarding 0002 (we could do more for the line number and source file in terms of the LOGs reported for a regexec failure). Now remains 0004, which is the core of the proposal, and while it needs a rebase, I have not been able to spend much time looking at its internals. In order to help with the follow-up steps, I have spotted a few areas that could be done first: - The diffs in guc.h/c for the introduction of GetDirConfFiles() to get a set of files in a directory (got to think a bit more about ParseConfigFp() when it comes to the keywords, but that comes to play with the parsing logic which is different). - The TAP test, which is half the size of the patch in line number. Could it be possible to make it more edible, introducing a basic infrastructure to check a set of rules in pg_hba.conf and pg_ident.conf without the inclusion logic? Checks for error patterns (that I agree we strongly lack tests for) look like something we'd want to tackle independently of the inclusion logic, and it should be built on top of a basic test infra, at least. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Oct 26, 2022 at 11:19:48AM +0800, Julien Rouhaud wrote: > That wouldn't be overdoing anymore if we remove the line number / filename > from > the fill_*_line prototypes right? Yeah, but there is a twist: HbaLine or IdentLine can be passed as NULL when entering in fill_hba_line() or fill_ident_line() in the event of an error when tokenizing the line or when failing the creation of their Line, so attempting to read the line number from either of them when filling in a tuple for their respective view would cause a crash. So, for now, I have taken the minimalistic approach with the addition of the source file name in HbaFile and in TokenizedAuthLine. The former is actually necessary anyway as auth.c wants it in two locations (improved auth_failed() and set_authn_id()). It is still true that the line number in IdentLine remains unused. Hence, do you think that the addition of the source file name and its line number could be useful for the error reporting done in check_ident_usermap()? The contents of the HbaLines are used in auth.c for its own reporting magic, and it looks like this would be the only location where what's in the IdentLines is useful, aka provide more details about what is happening. Once users are able to include ident files, that would likely help in debugging issues. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Oct 25, 2022 at 08:59:57PM +0900, Michael Paquier wrote:
>
> Hmm. I would be tempted to keep track of the file name and the line
> number as well in IdentLine. One reason is that this can become
> useful for debugging. A second is that this can reduce a bit the
> arguments of fill_ident_line() and fill_hba_line() in hbafuncs.c once
> we track these in HbaLine and IdentLine.
Ok, I guess something like the attached v14 is what you want.
> And HEAD is slightly
> overdoing it in its interface for the line number, actually, as we
> pass the line number twice: from {Ident,Hba}Line and the respective
> field from TokenizedAuthLine.
That wouldn't be overdoing anymore if we remove the line number / filename from
the fill_*_line prototypes right?
>From ecc27b9101acf40f4888da8be033c70e6f21358a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Tue, 25 Oct 2022 15:17:27 +0900
Subject: [PATCH v14 1/5] Refactor knowledge of origin file in hba.c
This limits the footprint of HbaFileName and IdentFileName to their
entry loading point, easing the introduction of the inclusion logic.
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/backend/libpq/hba.c | 114 +---
src/include/libpq/hba.h | 3 ++
2 files changed, 63 insertions(+), 54 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index ea92f02a47..56bbe31dfd 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -641,6 +641,7 @@ tokenize_auth_file(const char *filename, FILE *file, List
**tok_lines,
tok_line = (TokenizedAuthLine *)
palloc(sizeof(TokenizedAuthLine));
tok_line->fields = current_line;
+ tok_line->file_name = pstrdup(filename);
tok_line->line_num = line_number;
tok_line->raw_line = pstrdup(buf.data);
tok_line->err_msg = err_msg;
@@ -984,7 +985,7 @@ do { \
errmsg("authentication option \"%s\" is only valid for
authentication methods %s", \
optname, _(validmethods)), \
errcontext("line %d of configuration file \"%s\"", \
- line_num, HbaFileName))); \
+ line_num, file_name))); \
*err_msg = psprintf("authentication option \"%s\" is only valid for
authentication methods %s", \
optname, validmethods); \
return false; \
@@ -1004,7 +1005,7 @@ do { \
errmsg("authentication method \"%s\" requires
argument \"%s\" to be set", \
authname, argname), \
errcontext("line %d of configuration file
\"%s\"", \
- line_num, HbaFileName))); \
+ line_num, file_name))); \
*err_msg = psprintf("authentication method \"%s\" requires
argument \"%s\" to be set", \
authname, argname); \
return NULL; \
@@ -1027,7 +1028,7 @@ do { \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("missing entry at end of line"), \
errcontext("line %d of configuration file
\"%s\"", \
- line_num,
IdentFileName))); \
+ line_num, file_name)));
\
*err_msg = pstrdup("missing entry at end of line"); \
return NULL; \
} \
@@ -1040,7 +1041,7 @@ do { \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("multiple values in ident field"), \
errcontext("line %d of configuration file
\"%s\"", \
- line_num,
IdentFileName))); \
+ line_num, file_name)));
\
*err_msg = pstrdup("multiple values in ident field"); \
return NULL; \
} \
@@ -1063,6 +1064,7 @@ HbaLine *
parse_hba_line(TokenizedAuthLine *tok_line, int elevel)
{
int line_num = tok_line->line_num;
+ char *file_name = tok_line->file_name;
char **err_msg = _line->err_msg;
char *str;
struct addrinfo *gai_result;
@@ -1077,6 +1079,7 @@ parse_hba_line(TokenizedAuthLine *tok_line, int elevel)
HbaLine*parsedline;
parsedline = palloc0(sizeof(HbaLine));
+ parsedline->sourcefile = pstrdup(file_name);
parsedline->linenumber = line_num;
parsedline->rawline = pstrdup(tok_line->raw_line);
@@
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Oct 25, 2022 at 03:08:59PM +0800, Julien Rouhaud wrote:
> On Tue, Oct 25, 2022 at 03:43:21PM +0900, Michael Paquier wrote:
>> Another advantage is that it minimizes the presence of the hardcoded
>> HbaFileName and IdentFileName in hba.c, which is one thing we are
>> trying to achieve here for the inclusion of more files. I found a bit
>> strange that IdentLine had no sourcefile, actually. We track the file
>> number but use it nowhere, and it seems to me that having more
>> symmetry between both would be a good thing.
>
> If IdentLine->linenumber is useless, why not get rid of it rather than
> tracking
> another useless info? Hba is much more structured so we need a more
> specialized struct, but I don't think ident will ever go that way.
Hmm. I would be tempted to keep track of the file name and the line
number as well in IdentLine. One reason is that this can become
useful for debugging. A second is that this can reduce a bit the
arguments of fill_ident_line() and fill_hba_line() in hbafuncs.c once
we track these in HbaLine and IdentLine. And HEAD is slightly
overdoing it in its interface for the line number, actually, as we
pass the line number twice: from {Ident,Hba}Line and the respective
field from TokenizedAuthLine.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Oct 25, 2022 at 03:43:21PM +0900, Michael Paquier wrote: > > Another advantage is that it minimizes the presence of the hardcoded > HbaFileName and IdentFileName in hba.c, which is one thing we are > trying to achieve here for the inclusion of more files. I found a bit > strange that IdentLine had no sourcefile, actually. We track the file > number but use it nowhere, and it seems to me that having more > symmetry between both would be a good thing. If IdentLine->linenumber is useless, why not get rid of it rather than tracking another useless info? Hba is much more structured so we need a more specialized struct, but I don't think ident will ever go that way. > So, the key of the logic is how we are going to organize the > tokenization of the HBA and ident lines through all the inclusions.. > As far as I get it, tokenize_auth_file() is the root call and > tokenize_file_with_context() with its depth is able to work on each > individual file, and it can optionally recurse depending on what's > included. Why do you need to switch to the old context in > tokenize_file_with_context()? Could it be simpler to switch once to > linecxt outside of the internal routine? It just seemed the cleanest way to go. We could do without but then we would have to duplicate MemoryContextSwitchTo calls all over the place, and probably handling an optional memory context creation in the function. > It looks like GetDirConfFiles() is another piece that can be > refactored and reviewed on its own, as we use it in > ParseConfigDirectory()@guc.c. I'm fine with it.
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Oct 24, 2022 at 04:03:03PM +0800, Julien Rouhaud wrote:
> It would also require to bring HbaLine->sourcefile. I'm afraid it would be
> weird to introduce such a refactoring in a separate commit just to pass a
> constant down multiple level of indirection, as all the macro will remain
> specific to either hba or ident anyway.
Putting my hands on it, I am not really afraid of doing that
independently. From what I can see, this is cutting 23kB worth of
diffs from 0002, reducing it from 94K to 71kB.
> I agree that there are quite a lot of s/XXXFileName/file_name/, but those
> aren't complicated, and keeping them in the same commit makes it easy to
> validate that none has been forgotten since the regression tests covering
> those
> messages are in that commit too.
Another advantage is that it minimizes the presence of the hardcoded
HbaFileName and IdentFileName in hba.c, which is one thing we are
trying to achieve here for the inclusion of more files. I found a bit
strange that IdentLine had no sourcefile, actually. We track the file
number but use it nowhere, and it seems to me that having more
symmetry between both would be a good thing.
So, the key of the logic is how we are going to organize the
tokenization of the HBA and ident lines through all the inclusions..
As far as I get it, tokenize_auth_file() is the root call and
tokenize_file_with_context() with its depth is able to work on each
individual file, and it can optionally recurse depending on what's
included. Why do you need to switch to the old context in
tokenize_file_with_context()? Could it be simpler to switch once to
linecxt outside of the internal routine?
It looks like GetDirConfFiles() is another piece that can be
refactored and reviewed on its own, as we use it in
ParseConfigDirectory()@guc.c.
--
Michael
From 7c12a9dd2b23765c0e0d38da8140051a89c45fb4 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 25 Oct 2022 15:17:27 +0900
Subject: [PATCH v13 1/3] Refactor knowledge of origin file in hba.c
This limits the footprint of HbaFileName and IdentFileName to their
entry loading point, easing the introduction of the inclusion logic.
---
src/include/libpq/hba.h | 3 ++
src/backend/libpq/hba.c | 114 +---
2 files changed, 63 insertions(+), 54 deletions(-)
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index cec2e2665f..bf896ac084 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -93,6 +93,7 @@ typedef struct AuthToken
typedef struct HbaLine
{
+ char *sourcefile;
int linenumber;
char *rawline;
ConnType conntype;
@@ -138,6 +139,7 @@ typedef struct HbaLine
typedef struct IdentLine
{
+ char *sourcefile;
int linenumber;
char *usermap;
@@ -157,6 +159,7 @@ typedef struct IdentLine
typedef struct TokenizedAuthLine
{
List *fields; /* List of lists of AuthTokens */
+ char *file_name; /* File name of origin */
int line_num; /* Line number */
char *raw_line; /* Raw line text */
char *err_msg; /* Error message if any */
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index ea92f02a47..6524b60610 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -641,6 +641,7 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
tok_line = (TokenizedAuthLine *) palloc(sizeof(TokenizedAuthLine));
tok_line->fields = current_line;
+ tok_line->file_name = pstrdup(filename);
tok_line->line_num = line_number;
tok_line->raw_line = pstrdup(buf.data);
tok_line->err_msg = err_msg;
@@ -984,7 +985,7 @@ do { \
errmsg("authentication option \"%s\" is only valid for authentication methods %s", \
optname, _(validmethods)), \
errcontext("line %d of configuration file \"%s\"", \
- line_num, HbaFileName))); \
+ line_num, file_name))); \
*err_msg = psprintf("authentication option \"%s\" is only valid for authentication methods %s", \
optname, validmethods); \
return false; \
@@ -1004,7 +1005,7 @@ do { \
errmsg("authentication method \"%s\" requires argument \"%s\" to be set", \
authname, argname), \
errcontext("line %d of configuration file \"%s\"", \
- line_num, HbaFileName))); \
+ line_num, file_name))); \
*err_msg = psprintf("authentication method \"%s\" requires argument \"%s\" to be set", \
authname, argname); \
return NULL; \
@@ -1027,7 +1028,7 @@ do { \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("missing entry at end of line"), \
errcontext("line %d of configuration file \"%s\"", \
- line_num, IdentFileName))); \
+ line_num, file_name))); \
*err_msg = pstrdup("missing entry at end of line"); \
return NULL; \
} \
@@ -1040,7 +1041,7 @@ do { \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("multiple values in ident field"), \
errcontext("line %d of configuration file \"%s\"", \
- line_num, IdentFileName))); \
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Mon, Oct 24, 2022 at 04:13:51PM +0900, Michael Paquier wrote:
> On Mon, Oct 24, 2022 at 01:33:12PM +0800, Julien Rouhaud wrote:
> > v12 attached, fixing multiple conflicts with recent activity.
>
> typedef struct TokenizedAuthLine
> {
> List *fields; /* List of lists of AuthTokens */
> + char *file_name; /* File name *
>
> Hmm. While putting my eyes on this patch set for the first time in a
> few months (sorry!), the addition of file_name to TokenizedAuthLine in
> 0002 stands out. This impacts all the macros used for the validation
> of the ident and HBA lines, leading as well to a lot of bloat in the
> patch with patterns like that in 20~25 places:
> errcontext("line %d of configuration file \"%s\"", \
> - line_num, IdentFileName))); \
> + line_num, file_name))); \
> [...]
> errcontext("line %d of configuration file \"%s\"",
> - line_num, HbaFileName)));
> + line_num, file_name)));
>
> Do you think that it would make sense to split that into its own
> patch? That would move the code toward less references to HbaFileName
> and IdentFileName, which is one step toward what we want for this
> thread. This tokenization of HBA and ident files is already shared,
> so moving this knowledge into TokenizedAuthLine looks helpful
> independently of the rest.
It would also require to bring HbaLine->sourcefile. I'm afraid it would be
weird to introduce such a refactoring in a separate commit just to pass a
constant down multiple level of indirection, as all the macro will remain
specific to either hba or ident anyway.
I agree that there are quite a lot of s/XXXFileName/file_name/, but those
aren't complicated, and keeping them in the same commit makes it easy to
validate that none has been forgotten since the regression tests covering those
messages are in that commit too.
And of course while double checking that none was forgotten I realize that I
missed the new regcomp_auth_token() which introduced a couple new usage of
HbaFileName.
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Oct 24, 2022 at 01:33:12PM +0800, Julien Rouhaud wrote:
> v12 attached, fixing multiple conflicts with recent activity.
typedef struct TokenizedAuthLine
{
List *fields; /* List of lists of AuthTokens */
+ char *file_name; /* File name *
Hmm. While putting my eyes on this patch set for the first time in a
few months (sorry!), the addition of file_name to TokenizedAuthLine in
0002 stands out. This impacts all the macros used for the validation
of the ident and HBA lines, leading as well to a lot of bloat in the
patch with patterns like that in 20~25 places:
errcontext("line %d of configuration file \"%s\"", \
- line_num, IdentFileName))); \
+ line_num, file_name))); \
[...]
errcontext("line %d of configuration file \"%s\"",
- line_num, HbaFileName)));
+ line_num, file_name)));
Do you think that it would make sense to split that into its own
patch? That would move the code toward less references to HbaFileName
and IdentFileName, which is one step toward what we want for this
thread. This tokenization of HBA and ident files is already shared,
so moving this knowledge into TokenizedAuthLine looks helpful
independently of the rest.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Oct 10, 2022 at 10:51:32AM +0800, Julien Rouhaud wrote:
> On Sun, Sep 18, 2022 at 01:06:12AM +0800, Julien Rouhaud wrote:
> > On Tue, Aug 16, 2022 at 02:10:30PM +0800, Julien Rouhaud wrote:
> > >
> > > On Sat, Jul 30, 2022 at 04:09:36PM +0800, Julien Rouhaud wrote:
> > >
> > > >
> > > > - 0001: the rule_number / mapping_number addition in the views in a
> > > > separate
> > > > commit
> > > > - 0002: the main file inclusion patch. Only a few minor bugfix since
> > > > previous version discovered thanks to the tests (a bit more about it
> > > > after),
> > > > and documentation tweaks based on previous discussions
> > > > - 0003: the pg_hba_matches() POC, no changes
v12 attached, fixing multiple conflicts with recent activity.
>From 3361344bc0e907b341df912cf8ffcf28440b2370 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 30 May 2022 10:59:51 +0800
Subject: [PATCH v12 1/3] Add rule_number / mapping_number to the
pg_hba/pg_ident views.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
doc/src/sgml/system-views.sgml | 22 +
src/backend/utils/adt/hbafuncs.c| 50 ++---
src/include/catalog/pg_proc.dat | 11 ---
src/test/regress/expected/rules.out | 10 +++---
4 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 1ca7c3f9bf..4723f712a7 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -991,6 +991,18 @@
+
+
+ rule_number int4
+
+
+ Rule number of this rule among all rules if the rule is valid, otherwise
+ null. This indicates the order in which each rule will be considered
+ until the first matching one, if any, is used to perform authentication
+ with the client.
+
+
+
line_number int4
@@ -1131,6 +1143,16 @@
+
+
+ mapping_number int4
+
+
+ Mapping number, in priority order, of this mapping if the mapping is
+ valid, otherwise null
+
+
+
line_number int4
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index cfdc4d8b39..21a451e391 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -26,10 +26,12 @@
static ArrayType *get_hba_options(HbaLine *hba);
static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba,
const char *err_msg);
+ int rule_number, int lineno,
HbaLine *hba,
+ const char *err_msg);
static void fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
static void fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, IdentLine
*ident, const char *err_msg);
+ int mapping_number, int
lineno, IdentLine *ident,
+ const char *err_msg);
static void fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
@@ -157,7 +159,7 @@ get_hba_options(HbaLine *hba)
}
/* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS 9
+#define NUM_PG_HBA_FILE_RULES_ATTS 10
/*
* fill_hba_line
@@ -165,6 +167,7 @@ get_hba_options(HbaLine *hba)
*
* tuple_store: where to store data
* tupdesc: tuple descriptor for the view
+ * rule_number: unique rule identifier among all valid rules
* lineno: pg_hba.conf line number (must always be valid)
* hba: parsed line data (can be NULL, in which case err_msg should be set)
* err_msg: error message (NULL if none)
@@ -174,7 +177,8 @@ get_hba_options(HbaLine *hba)
*/
static void
fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba, const char *err_msg)
+ int rule_number, int lineno, HbaLine *hba,
+ const char *err_msg)
{
Datum values[NUM_PG_HBA_FILE_RULES_ATTS];
boolnulls[NUM_PG_HBA_FILE_RULES_ATTS];
@@ -193,6 +197,11 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
memset(nulls, 0, sizeof(nulls));
index = 0;
+ /* rule_number */
+ if (err_msg)
+ nulls[index++] = true;
+ else
+ values[index++] = Int32GetDatum(rule_number);
/* line_number */
values[index++] = Int32GetDatum(lineno);
@@ -336,7 +345,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
else
{
/* no parsing result, so set relevant fields to nulls */
- memset([1], true,
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Sun, Sep 18, 2022 at 01:06:12AM +0800, Julien Rouhaud wrote:
> On Tue, Aug 16, 2022 at 02:10:30PM +0800, Julien Rouhaud wrote:
> >
> > On Sat, Jul 30, 2022 at 04:09:36PM +0800, Julien Rouhaud wrote:
> >
> > >
> > > - 0001: the rule_number / mapping_number addition in the views in a
> > > separate
> > > commit
> > > - 0002: the main file inclusion patch. Only a few minor bugfix since
> > > previous version discovered thanks to the tests (a bit more about it
> > > after),
> > > and documentation tweaks based on previous discussions
> > > - 0003: the pg_hba_matches() POC, no changes
v11 attached, fixing a shadowed variable warning.
>From fc8cfc1327e153ec23860aea63cdd81ff053cba2 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 30 May 2022 10:59:51 +0800
Subject: [PATCH v11 1/3] Add rule_number / mapping_number to the
pg_hba/pg_ident views.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
doc/src/sgml/system-views.sgml | 22 +
src/backend/utils/adt/hbafuncs.c| 50 ++---
src/include/catalog/pg_proc.dat | 11 ---
src/test/regress/expected/rules.out | 10 +++---
4 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 1ca7c3f9bf..4723f712a7 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -991,6 +991,18 @@
+
+
+ rule_number int4
+
+
+ Rule number of this rule among all rules if the rule is valid, otherwise
+ null. This indicates the order in which each rule will be considered
+ until the first matching one, if any, is used to perform authentication
+ with the client.
+
+
+
line_number int4
@@ -1131,6 +1143,16 @@
+
+
+ mapping_number int4
+
+
+ Mapping number, in priority order, of this mapping if the mapping is
+ valid, otherwise null
+
+
+
line_number int4
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 9e5794071c..c9be4bff1f 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -26,10 +26,12 @@
static ArrayType *get_hba_options(HbaLine *hba);
static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba,
const char *err_msg);
+ int rule_number, int lineno,
HbaLine *hba,
+ const char *err_msg);
static void fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
static void fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, IdentLine
*ident, const char *err_msg);
+ int mapping_number, int
lineno, IdentLine *ident,
+ const char *err_msg);
static void fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
@@ -157,7 +159,7 @@ get_hba_options(HbaLine *hba)
}
/* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS 9
+#define NUM_PG_HBA_FILE_RULES_ATTS 10
/*
* fill_hba_line
@@ -165,6 +167,7 @@ get_hba_options(HbaLine *hba)
*
* tuple_store: where to store data
* tupdesc: tuple descriptor for the view
+ * rule_number: unique rule identifier among all valid rules
* lineno: pg_hba.conf line number (must always be valid)
* hba: parsed line data (can be NULL, in which case err_msg should be set)
* err_msg: error message (NULL if none)
@@ -174,7 +177,8 @@ get_hba_options(HbaLine *hba)
*/
static void
fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba, const char *err_msg)
+ int rule_number, int lineno, HbaLine *hba,
+ const char *err_msg)
{
Datum values[NUM_PG_HBA_FILE_RULES_ATTS];
boolnulls[NUM_PG_HBA_FILE_RULES_ATTS];
@@ -193,6 +197,11 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
memset(nulls, 0, sizeof(nulls));
index = 0;
+ /* rule_number */
+ if (err_msg)
+ nulls[index++] = true;
+ else
+ values[index++] = Int32GetDatum(rule_number);
/* line_number */
values[index++] = Int32GetDatum(lineno);
@@ -336,7 +345,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
else
{
/* no parsing result, so set relevant fields to nulls */
- memset([1], true, (NUM_PG_HBA_FILE_RULES_ATTS - 2) *
sizeof(bool));
+ memset([2], true,
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Aug 16, 2022 at 02:10:30PM +0800, Julien Rouhaud wrote:
> Hi,
>
> On Sat, Jul 30, 2022 at 04:09:36PM +0800, Julien Rouhaud wrote:
>
> >
> > - 0001: the rule_number / mapping_number addition in the views in a separate
> > commit
> > - 0002: the main file inclusion patch. Only a few minor bugfix since
> > previous version discovered thanks to the tests (a bit more about it
> > after),
> > and documentation tweaks based on previous discussions
> > - 0003: the pg_hba_matches() POC, no changes
v10 attached to fix a few conflict with recent refactoring commits, no other
change.
>From d81715bf9848f1aa310ea199a9774eebcbe372ab Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 30 May 2022 10:59:51 +0800
Subject: [PATCH v10 1/3] Add rule_number / mapping_number to the
pg_hba/pg_ident views.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
doc/src/sgml/system-views.sgml | 22 +
src/backend/utils/adt/hbafuncs.c| 50 ++---
src/include/catalog/pg_proc.dat | 11 ---
src/test/regress/expected/rules.out | 10 +++---
4 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 44aa70a031..1d619427c1 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -991,6 +991,18 @@
+
+
+ rule_number int4
+
+
+ Rule number of this rule among all rules if the rule is valid, otherwise
+ null. This indicates the order in which each rule will be considered
+ until the first matching one, if any, is used to perform authentication
+ with the client.
+
+
+
line_number int4
@@ -1131,6 +1143,16 @@
+
+
+ mapping_number int4
+
+
+ Mapping number, in priority order, of this mapping if the mapping is
+ valid, otherwise null
+
+
+
line_number int4
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 9e5794071c..c9be4bff1f 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -26,10 +26,12 @@
static ArrayType *get_hba_options(HbaLine *hba);
static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba,
const char *err_msg);
+ int rule_number, int lineno,
HbaLine *hba,
+ const char *err_msg);
static void fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
static void fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, IdentLine
*ident, const char *err_msg);
+ int mapping_number, int
lineno, IdentLine *ident,
+ const char *err_msg);
static void fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
@@ -157,7 +159,7 @@ get_hba_options(HbaLine *hba)
}
/* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS 9
+#define NUM_PG_HBA_FILE_RULES_ATTS 10
/*
* fill_hba_line
@@ -165,6 +167,7 @@ get_hba_options(HbaLine *hba)
*
* tuple_store: where to store data
* tupdesc: tuple descriptor for the view
+ * rule_number: unique rule identifier among all valid rules
* lineno: pg_hba.conf line number (must always be valid)
* hba: parsed line data (can be NULL, in which case err_msg should be set)
* err_msg: error message (NULL if none)
@@ -174,7 +177,8 @@ get_hba_options(HbaLine *hba)
*/
static void
fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba, const char *err_msg)
+ int rule_number, int lineno, HbaLine *hba,
+ const char *err_msg)
{
Datum values[NUM_PG_HBA_FILE_RULES_ATTS];
boolnulls[NUM_PG_HBA_FILE_RULES_ATTS];
@@ -193,6 +197,11 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
memset(nulls, 0, sizeof(nulls));
index = 0;
+ /* rule_number */
+ if (err_msg)
+ nulls[index++] = true;
+ else
+ values[index++] = Int32GetDatum(rule_number);
/* line_number */
values[index++] = Int32GetDatum(lineno);
@@ -336,7 +345,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
else
{
/* no parsing result, so set relevant fields to nulls */
- memset([1], true, (NUM_PG_HBA_FILE_RULES_ATTS - 2) *
sizeof(bool));
+ memset([2], true, (NUM_PG_HBA_FILE_RULES_ATTS - 3) *
sizeof(bool));
}
/* error
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Sat, Jul 30, 2022 at 04:09:36PM +0800, Julien Rouhaud wrote:
>
> - 0001: the rule_number / mapping_number addition in the views in a separate
> commit
> - 0002: the main file inclusion patch. Only a few minor bugfix since
> previous version discovered thanks to the tests (a bit more about it after),
> and documentation tweaks based on previous discussions
> - 0003: the pg_hba_matches() POC, no changes
Attached v9, simple rebase after multiple conflicts and part of the patchset
applied.
>From 5430fd3b829b9c7600b61095752d4f7f85d1150d Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 30 May 2022 10:59:51 +0800
Subject: [PATCH v9 1/3] Add rule_number / mapping_number to the
pg_hba/pg_ident views.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
doc/src/sgml/system-views.sgml | 22 +
src/backend/utils/adt/hbafuncs.c| 50 ++---
src/include/catalog/pg_proc.dat | 11 ---
src/test/regress/expected/rules.out | 10 +++---
4 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 44aa70a031..1d619427c1 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -991,6 +991,18 @@
+
+
+ rule_number int4
+
+
+ Rule number of this rule among all rules if the rule is valid, otherwise
+ null. This indicates the order in which each rule will be considered
+ until the first matching one, if any, is used to perform authentication
+ with the client.
+
+
+
line_number int4
@@ -1131,6 +1143,16 @@
+
+
+ mapping_number int4
+
+
+ Mapping number, in priority order, of this mapping if the mapping is
+ valid, otherwise null
+
+
+
line_number int4
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 9e5794071c..c9be4bff1f 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -26,10 +26,12 @@
static ArrayType *get_hba_options(HbaLine *hba);
static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba,
const char *err_msg);
+ int rule_number, int lineno,
HbaLine *hba,
+ const char *err_msg);
static void fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
static void fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, IdentLine
*ident, const char *err_msg);
+ int mapping_number, int
lineno, IdentLine *ident,
+ const char *err_msg);
static void fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
@@ -157,7 +159,7 @@ get_hba_options(HbaLine *hba)
}
/* Number of columns in pg_hba_file_rules view */
-#define NUM_PG_HBA_FILE_RULES_ATTS 9
+#define NUM_PG_HBA_FILE_RULES_ATTS 10
/*
* fill_hba_line
@@ -165,6 +167,7 @@ get_hba_options(HbaLine *hba)
*
* tuple_store: where to store data
* tupdesc: tuple descriptor for the view
+ * rule_number: unique rule identifier among all valid rules
* lineno: pg_hba.conf line number (must always be valid)
* hba: parsed line data (can be NULL, in which case err_msg should be set)
* err_msg: error message (NULL if none)
@@ -174,7 +177,8 @@ get_hba_options(HbaLine *hba)
*/
static void
fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba, const char *err_msg)
+ int rule_number, int lineno, HbaLine *hba,
+ const char *err_msg)
{
Datum values[NUM_PG_HBA_FILE_RULES_ATTS];
boolnulls[NUM_PG_HBA_FILE_RULES_ATTS];
@@ -193,6 +197,11 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
memset(nulls, 0, sizeof(nulls));
index = 0;
+ /* rule_number */
+ if (err_msg)
+ nulls[index++] = true;
+ else
+ values[index++] = Int32GetDatum(rule_number);
/* line_number */
values[index++] = Int32GetDatum(lineno);
@@ -336,7 +345,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc
tupdesc,
else
{
/* no parsing result, so set relevant fields to nulls */
- memset([1], true, (NUM_PG_HBA_FILE_RULES_ATTS - 2) *
sizeof(bool));
+ memset([2], true, (NUM_PG_HBA_FILE_RULES_ATTS - 3) *
sizeof(bool));
}
/* error */
@@ -359,6 +368,7 @@ fill_hba_view(Tuplestorestate *tuple_store, TupleDesc
tupdesc)
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Fri, Aug 05, 2022 at 09:56:29AM +0900, Michael Paquier wrote: > On Tue, Aug 02, 2022 at 07:32:54PM +0900, Michael Paquier wrote: > > As a quick update from my side, I intend to look and apply 0001~0003 > > (not double-checked yet) shortly. > > And a couple of days later, these look fine so done as of 47ab1ac and > 718fe0a. 0002 and 0003 have been merged together. Thanks!
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Aug 02, 2022 at 07:32:54PM +0900, Michael Paquier wrote: > As a quick update from my side, I intend to look and apply 0001~0003 > (not double-checked yet) shortly. And a couple of days later, these look fine so done as of 47ab1ac and 718fe0a. 0002 and 0003 have been merged together. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Jul 30, 2022 at 04:09:36PM +0800, Julien Rouhaud wrote: > I've been working on all of that and came up with the attached v8. > > - 0001: I modified things as discussed previously to report the real auth file > names rather than the hardcoded "pg_ident.conf" and "pg_hba.conf" in the > various log messages > - 0002 and 0003 are minor fixes to error logs more consistent As a quick update from my side, I intend to look and apply 0001~0003 (not double-checked yet) shortly. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Jul 26, 2022 at 1:14 PM Michael Paquier wrote: > > On Tue, Jul 26, 2022 at 01:04:02PM +0800, Julien Rouhaud wrote: > > It doesn't have much impact most of the time. The filename is reported if > > there's an IO error while reading the already opened correct file. The real > > problem is if the hba_file and ident_file are stored in different directory, > > any secondary file (@filename) in the pg_ident.conf would be searched in the > > wrong directory. With the pending file inclusion patchset, the problem is > > immediately visible as the view is reporting the wrong file name. > > Oops, obviously. I'll go and fix that as that's on me. > > > Simple fix attached. I'll add a v15 open item shortly. > > Thanks. Better not to lose track of it. For the archives's sake, this was fixed shortly after as of 27e0ee57f68d27af68967759a2ff61a581f501dc on master and the open item is closed.
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Tue, Jul 19, 2022 at 03:13:12PM +0900, Michael Paquier wrote:
> On Mon, Jul 18, 2022 at 03:11:51PM +0800, Julien Rouhaud wrote:
>
> > I'm not really sure what should be done here. The best compromise I can
> > think
> > of is to split the tests in 3 parts:
> >
> > 1) view reporting with various inclusions using safe_psql()
>
> You mean in the case where the HBA and indent files can be loaded,
> so as it is possible to peek at the system views without the
> EXEC_BACKEND problem, right?
Yes, testing the general behavior when there are no errors in the auth files.
> > 2) log error reporting
>
> This one should be reliable and stable enough by parsing the logs of
> the backend, thanks to connect_ok() and connect_fails().
I meant testing the postgres logs reporting, something like generating entirely
bogus files, restarting, checking that the start failed and verify that the
logs are expected. With a variation that if only the pg_ident.conf is bogus,
the server will (and should) still start, so both needs to be tested
separately.
> > 3) view reporting with various inclusions errors, using safe_psql()
> >
> > And when testing 3), detect first if we can still connect after introducing
> > errors. If not, assume this is Windows / EXEC_BACKEND and give up here
> > without
> > reporting an error. Otherwise continue, and fail the test if we later can't
> > connect anymore. As discussed previously, detecting that the build is using
> > the fork emulation code path doesn't seem worthwhile so guessing it from the
> > psql error may be a better approach.
>
> Yeah, we could do that. Now we may also fail on other patterns, so we
> would need to make sure that a set of expected error outputs are the
> ones generated? I'd be fine to give up testing the error output
> generated in the system views at the end. Without a persistent
> connection state with the same kind of APIs as any of the drivers able
> to do so, that's going to be a PITA to maintain.
Yes, or just checking that the error is expected on the psql side as the server
will report it.
I've been working on all of that and came up with the attached v8.
- 0001: I modified things as discussed previously to report the real auth file
names rather than the hardcoded "pg_ident.conf" and "pg_hba.conf" in the
various log messages
- 0002 and 0003 are minor fixes to error logs more consistent
- 0004: the rule_number / mapping_number addition in the views in a separate
commit
- 0005: the main file inclusion patch. Only a few minor bugfix since
previous version discovered thanks to the tests (a bit more about it after),
and documentation tweaks based on previous discussions
- 0006: the pg_hba_matches() POC, no changes
About the regression tests:
I added a new 003_file_inclusion.pl in src/test/authentication TAP tests to do
things as previously described, ie test both extisting behavior and new
features added, for working and non working scenarios. The view error
reporting are bypassed for Windows / EXEC_BACKEND build by detecting a
connection failure with the expected error reported by the server. If the
error doesn't match, the rest of the tests are still skipped but overall test
will fail due to the mismatch in the reported error.
It's a bit troublesome to write such tests, as you need to keep in sync the
various rule and line number, format things differently depending on whether
you want to check the logs or the view with each new tests added and so on.
To make the tests easier to write (and to maintain) I added some wrapper
functions to add lines in the wanted files that allow to automatically
generates the wanted regex (for the logs) or output (for the view). There are
still things to be careful about and a bit of duplication, but this way I can
easily modify any test, or add new ones, without the need to modify everything
around. As written, it also uses the same included files for the log error
reporting and the view error reporting, so there's no need to define everything
twice.
The tests work as expected locally on a normal build and an EXEC_BACKEND build,
and also on the CI.
>From 7232b708099eec0fd27a43ed722388f4988655da Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Fri, 3 Jun 2022 15:56:23 +0800
Subject: [PATCH v8 1/6] Use real file names rather than
pg_hba.conf/pg_ident.conf in messages.
---
src/backend/postmaster/postmaster.c | 7 ---
src/backend/utils/init/postinit.c | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index e541b16bdb..8bcfedef64 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1419,7 +1419,8 @@ PostmasterMain(int argc, char *argv[])
* since there is no way to connect to the database in this
case.
*/
ereport(FATAL,
- (errmsg("could not load pg_hba.conf")));
+
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Jul 26, 2022 at 01:04:02PM +0800, Julien Rouhaud wrote: > It doesn't have much impact most of the time. The filename is reported if > there's an IO error while reading the already opened correct file. The real > problem is if the hba_file and ident_file are stored in different directory, > any secondary file (@filename) in the pg_ident.conf would be searched in the > wrong directory. With the pending file inclusion patchset, the problem is > immediately visible as the view is reporting the wrong file name. Oops, obviously. I'll go and fix that as that's on me. > Simple fix attached. I'll add a v15 open item shortly. Thanks. Better not to lose track of it. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Mon, Mar 28, 2022 at 04:22:32PM +0900, Michael Paquier wrote:
> On Mon, Mar 28, 2022 at 04:20:07PM +0900, Michael Paquier wrote:
> > See the attached, for reference, but it would fail with EXEC_BACKEND
> > on WIN32.
>
> Ditto.
While working on the full regression test coverage for the file inclusion
thing, I discovered an embarrassing typo in the pg_ident_file_mapping
infrastructure, which was using the hba file name rather than the ident file
name in one of the calls.
It doesn't have much impact most of the time. The filename is reported if
there's an IO error while reading the already opened correct file. The real
problem is if the hba_file and ident_file are stored in different directory,
any secondary file (@filename) in the pg_ident.conf would be searched in the
wrong directory. With the pending file inclusion patchset, the problem is
immediately visible as the view is reporting the wrong file name.
Simple fix attached. I'll add a v15 open item shortly.
>From d36010e7fec78b3ea25255767b8a2478f85fd325 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Tue, 26 Jul 2022 12:52:35 +0800
Subject: [PATCH v1] Fix fill_ident_view incorrect usage of HbaFileName
Thinko introduced in a2c84990bea.
Patchpatch to 15, as the original commit.
Author: Julien Rouhaud
---
src/backend/utils/adt/hbafuncs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index 598259718c..9e5794071c 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -512,7 +512,7 @@ fill_ident_view(Tuplestorestate *tuple_store, TupleDesc
tupdesc)
errmsg("could not open usermap file \"%s\":
%m",
IdentFileName)));
- linecxt = tokenize_auth_file(HbaFileName, file, _lines, DEBUG3);
+ linecxt = tokenize_auth_file(IdentFileName, file, _lines, DEBUG3);
FreeFile(file);
/* Now parse all the lines */
--
2.37.0
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Jul 18, 2022 at 03:11:51PM +0800, Julien Rouhaud wrote: > So first, even if we can test 99% of the features with just testing the views > output, I think it's should use the TAP framework since the tests will have to > mess with the pg_ident/pg_hba files. It's way easier to modify the auth > files, > and it uses a dedicated instance so we don't have to worry about breaking > other > test that would run concurrently. Agreed. > That may still be workable by splitting the output per newline (and possibly > removing the first prompt before sending the query text), and remove the first > and last entry (assuming you want to test somewhat sane data, and not e.g. run > the regression test on a database containing a newline), but then you have to > also account for possible escape sequences, for instance if you use > enable-bracketed-paste. In that case, the output becomes something like > > dbname=# SELECT 1; > [?2004l > 1 > [?2004hpostgres=# > > It could probably be handled with some regexp to remove escape sequences and > remove empty lines, but it seems like really fragile, and thus a very bad > idea. Hmm. Indeed, that sounds fragile. And I don't really think that we should make more testing infrastructure a requirement for this patch as being able to maintain a connection while running the tests is something we've bumped on for a bit of time. > I'm not really sure what should be done here. The best compromise I can think > of is to split the tests in 3 parts: > > 1) view reporting with various inclusions using safe_psql() You mean in the case where the HBA and indent files can be loaded, so as it is possible to peek at the system views without the EXEC_BACKEND problem, right? > 2) log error reporting This one should be reliable and stable enough by parsing the logs of the backend, thanks to connect_ok() and connect_fails(). > 3) view reporting with various inclusions errors, using safe_psql() > > And when testing 3), detect first if we can still connect after introducing > errors. If not, assume this is Windows / EXEC_BACKEND and give up here > without > reporting an error. Otherwise continue, and fail the test if we later can't > connect anymore. As discussed previously, detecting that the build is using > the fork emulation code path doesn't seem worthwhile so guessing it from the > psql error may be a better approach. Yeah, we could do that. Now we may also fail on other patterns, so we would need to make sure that a set of expected error outputs are the ones generated? I'd be fine to give up testing the error output generated in the system views at the end. Without a persistent connection state with the same kind of APIs as any of the drivers able to do so, that's going to be a PITA to maintain. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Mon, Jul 11, 2022 at 10:16:44AM +0900, Michael Paquier wrote: > On Fri, Jul 08, 2022 at 02:57:21PM +0800, Julien Rouhaud wrote: > > My apologies for the late reply. > > > I don't have an extensive knowledge of all the user facing error messages, > > but > > after a quick grep I see multiple usage of OID, PID, GIN and other defined > > acronyms. I also see multiple occurrences of "only heap AM is supported", > > while AM isn't even a defined acronym. > > A lot depends on the context of the code, it seems. > > > It doesn't seem that my proposal would be inconsistent with existing > > messages > > and will help to reduce the message length, but if you prefer to keep the > > full > > name I'm fine with it. Those should be very rare and specialized errors > > anyway. > > So you mean to use "HBA file" instead of pg_hba.conf and > "authentication file" when it can be either one of an HBA file or a > mapping file? That would be okay by me. Yes, it seems to me like a good compromise for not being overly verbose and still being understandable. > We would have a full cycle > to tune them depending on the feedback we'd get afterwards. Agreed. > > While on the bikeshedding part, are you ok with the proposed keywords > > (include > > and include_dir), behaving exactly like for postgresql.conf, and to also add > > include_if_exists, so that we have the exact same possibilities with > > postgresql.conf, pg_hba.conf and pg_ident.conf? > > Okay, agreed for consistency. With include_dir being careful about > the ordering of the entries and ignoring anything else than a .conf > file (that's something you mentioned already upthread). Ok! All those that should be covered by new regression test so it should be clear what is being implemented. While on the regression tests topic, I started to implement those and faced some problems quite fast when trying to workaround the problem we previously discussed (1). So first, even if we can test 99% of the features with just testing the views output, I think it's should use the TAP framework since the tests will have to mess with the pg_ident/pg_hba files. It's way easier to modify the auth files, and it uses a dedicated instance so we don't have to worry about breaking other test that would run concurrently. Also, if we want to test the views error reporting we have to use a persistent connection (as in interactive_psql()), otherwise tests will immediately fail on Windows / EXEC_BACKEND builds. Adding the ability to run queries and wait for completion on top of interactive_psql() doesn't seem to cause any problem, but interpreting the results does. Since it's just manipulating the psql's stdin/stdout, we retrieve the prompt and executed query too. So for instance, if you just want to test "SELECT 1", you will have to cleanup something like dbname=# SELECT 1; 1 dbname# That may still be workable by splitting the output per newline (and possibly removing the first prompt before sending the query text), and remove the first and last entry (assuming you want to test somewhat sane data, and not e.g. run the regression test on a database containing a newline), but then you have to also account for possible escape sequences, for instance if you use enable-bracketed-paste. In that case, the output becomes something like dbname=# SELECT 1; [?2004l 1 [?2004hpostgres=# It could probably be handled with some regexp to remove escape sequences and remove empty lines, but it seems like really fragile, and thus a very bad idea. I'm not really sure what should be done here. The best compromise I can think of is to split the tests in 3 parts: 1) view reporting with various inclusions using safe_psql() 2) log error reporting 3) view reporting with various inclusions errors, using safe_psql() And when testing 3), detect first if we can still connect after introducing errors. If not, assume this is Windows / EXEC_BACKEND and give up here without reporting an error. Otherwise continue, and fail the test if we later can't connect anymore. As discussed previously, detecting that the build is using the fork emulation code path doesn't seem worthwhile so guessing it from the psql error may be a better approach. Do you have any better idea, or do you have comments on this approach? [1]: https://www.postgresql.org/message-id/ykfhpydhyenno...@paquier.xyz
Re: Allow file inclusion in pg_hba and pg_ident files
On Fri, Jul 08, 2022 at 02:57:21PM +0800, Julien Rouhaud wrote: My apologies for the late reply. > I don't have an extensive knowledge of all the user facing error messages, but > after a quick grep I see multiple usage of OID, PID, GIN and other defined > acronyms. I also see multiple occurrences of "only heap AM is supported", > while AM isn't even a defined acronym. A lot depends on the context of the code, it seems. > It doesn't seem that my proposal would be inconsistent with existing messages > and will help to reduce the message length, but if you prefer to keep the full > name I'm fine with it. Those should be very rare and specialized errors > anyway. So you mean to use "HBA file" instead of pg_hba.conf and "authentication file" when it can be either one of an HBA file or a mapping file? That would be okay by me. We would have a full cycle to tune them depending on the feedback we'd get afterwards. > While on the bikeshedding part, are you ok with the proposed keywords (include > and include_dir), behaving exactly like for postgresql.conf, and to also add > include_if_exists, so that we have the exact same possibilities with > postgresql.conf, pg_hba.conf and pg_ident.conf? Okay, agreed for consistency. With include_dir being careful about the ordering of the entries and ignoring anything else than a .conf file (that's something you mentioned already upthread). -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Thu, Jun 02, 2022 at 10:08:15AM +0900, Michael Paquier wrote: > On Thu, May 26, 2022 at 03:26:57PM +0800, Julien Rouhaud wrote: > > > 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? > > Yes, that would be a set of changes only for HEAD, once 16~ opens for > business. FWIW, the acronym "HBA" is defined as "Host-Based > Authentication", so we could use that as a base for the description of > the file, using simply HBA in the follow-up paragraphs for simplicity, > telling that pg_hba.conf is the default. Ok. > > 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? > > Using "HBA file" in the docs is fine by me, knowing that the acronym > is already defined. The modified parts of the docs should perhaps > mention once something like "Host-Based Authentication file (or HBA > file)" for clarity. For the error message, I think that we tend to > avoid those acronyms, don't we? I don't have an extensive knowledge of all the user facing error messages, but after a quick grep I see multiple usage of OID, PID, GIN and other defined acronyms. I also see multiple occurrences of "only heap AM is supported", while AM isn't even a defined acronym. It doesn't seem that my proposal would be inconsistent with existing messages and will help to reduce the message length, but if you prefer to keep the full name I'm fine with it. Those should be very rare and specialized errors anyway. While on the bikeshedding part, are you ok with the proposed keywords (include and include_dir), behaving exactly like for postgresql.conf, and to also add include_if_exists, so that we have the exact same possibilities with postgresql.conf, pg_hba.conf and pg_ident.conf?
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, May 26, 2022 at 03:26:57PM +0800, Julien Rouhaud wrote: > 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. Yes, consistency would be good here. > 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. Hmm. Yes, using only this term could lead to some confusion. Thinking more about that, perhaps you are right to use fully separated terms for the ident and HBA parts, particularly once the inclusion is in place as the file names could be anything, so we would lose context about where a file for used for what. > 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? Yes, that would be a set of changes only for HEAD, once 16~ opens for business. FWIW, the acronym "HBA" is defined as "Host-Based Authentication", so we could use that as a base for the description of the file, using simply HBA in the follow-up paragraphs for simplicity, telling that pg_hba.conf is the default. > 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? Using "HBA file" in the docs is fine by me, knowing that the acronym is already defined. The modified parts of the docs should perhaps mention once something like "Host-Based Authentication file (or HBA file)" for clarity. For the error message, I think that we tend to avoid those acronyms, don't we? > 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. The rule_number shines with the inclusion logic in place, still splitting that into a separate commit makes the review and the commit history cleaner IMO. Perhaps that's just me being overly-pedantic in keeping a clean commit history :) -- Michael signature.asc Description: PGP signature
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. > >> + > >> + Rule number, in priority order, of this rule if the rule is valid, > >> + otherwise null > >> + > >> 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
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. >> + >> + Rule number, in priority order, of this rule if the rule is valid, >> + otherwise null >> + >> 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.
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Mon, May 23, 2022 at 03:53:06PM +0900, Michael Paquier wrote:
>
> + 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.
I'm not sure it would always be the case. For instance, why wouldn't someone
use something like
peer all @app_users ident
rather than
@include app_users
or a mix of both?
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?
> The "default" clause in the
> switch could just be removed, btw, to generate warnings if a new value
> is added in the kind enum.
Right.
> + /* 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
Agreed. I will try to mimic how it's done for
https://www.postgresql.org/docs/current/config-setting.html#CONFIG-INCLUDES.
> (and test!).
Sure. As I said previously I'm waiting for some agreement on the feature (and
the syntax), before adding tests. After that I will definitely include full
test coverage!
> 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/.
Right, same rules as for postgresql.conf's include_dir.
>
> 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.
Well, even if included they are still part of the configuration and lead to a
failure to load the main 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';
After a restart:
2022-05-23 18:52:08.081 CST [41890] LOG: could not open configuration file
"/tmp/myfile": No such file or directory
2022-05-23 18:52:08.081 CST [41890] FATAL: could not load pg_hba.conf
> How does include_dir work with the ordering of the HBA entries across
> multiple files?
Bugs apart, same as for postgresql.conf's include_dir.
> 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?
> +
> + Rule number, in priority order, of this rule if the rule is valid,
> + otherwise null
> +
> 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
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".
+
+ Rule number, in priority order, of this rule if the rule is valid,
+ otherwise null
+
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("::::::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))
+
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Tue, Mar 29, 2022 at 04:14:54PM +0800, Julien Rouhaud wrote: > > I'm attaching a rebase of the rest of the patchset. Note that I also added > support for an "include_dir" directive, as it's also something that would be > quite helpful (and very welcome for my $work use case). The cfbot reports that the patch doesn't apply anymore, rebased v7 attached. >From 04943f3ff8dfb8efa5e538e0d9524fb041c3b39b Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Mon, 21 Feb 2022 15:45:26 +0800 Subject: [PATCH v7 1/2] Allow file inclusion in pg_hba and pg_ident files. Catversion is bumped. Author: Julien Rouhaud Reviewed-by: FIXME Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud --- doc/src/sgml/catalogs.sgml | 48 +++- doc/src/sgml/client-auth.sgml | 48 +++- src/backend/libpq/hba.c| 343 + src/backend/libpq/pg_hba.conf.sample | 10 +- src/backend/libpq/pg_ident.conf.sample | 12 +- src/backend/utils/adt/hbafuncs.c | 51 +++- src/include/catalog/pg_proc.dat| 11 +- src/include/libpq/hba.h| 2 + src/test/regress/expected/rules.out| 12 +- 9 files changed, 453 insertions(+), 84 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index a533a2153e..b44e62d388 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -10554,12 +10554,31 @@ SCRAM-SHA-256$iteration count: + + + rule_number int4 + + + Rule number, in priority order, of this rule if the rule is valid, + otherwise null + + + + + + file_name text + + + File name of this rule + + + line_number int4 - Line number of this rule in pg_hba.conf + Line number of this rule in the given file_name @@ -10694,6 +10713,33 @@ SCRAM-SHA-256$iteration count: + + + mapping_number int4 + + + Rule number, in priority order, of this mapping if the mapping is valid, + otherwise null + + + + + + file_name text + + + File name of this mapping + + + + + + line_number int4 + + + Line number of this mapping in the given file_name + + line_number int4 diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index b2a459fb0d..f7d871e660 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -89,8 +89,21 @@ - Each record specifies a connection type, a client IP address range - (if relevant for the connection type), a database name, a user name, + Each record can either be an inclusion directive or an authentication rule. + Inclusion records specifies files that can be included, which contains + additional records. The records will be inserted in lieu of the inclusion + records. Those records only contains two fields: the + include or include_dir directive and + the file or directory to be included. The file or directory can be a + relative of absolute path, and can be double quoted if needed. For the + include_dir form, all files not starting with a + . and ending with .conf will be + included. + + + + Each authentication record specifies a connection type, a client IP address + range (if relevant for the connection type), a database name, a user name, and the authentication method to be used for connections matching these parameters. The first record with a matching connection type, client address, requested database, and user name is used to perform @@ -103,6 +116,8 @@ A record can have several formats: +include file +include_dir directory local database user auth-method auth-options host database user address auth-method auth-options hostssl database user address auth-method auth-options @@ -118,6 +133,26 @@ hostnogssenc database user + + include + + + This line will be replaced with the content of the given file. + + + + + + include_dir + + + This line will be replaced with the content of all the files found in + the directory, if they don't start with a . and end + with .conf. + + + + local @@ -835,8 +870,10 @@ local db1,db2,@demodbs all md5 cluster's data directory. (It is possible to place the map file elsewhere, however; see the configuration parameter.) - The ident map file contains lines of the general form: + The ident map file contains lines of two general form: +include file +include_dir directory map-name system-username database-username Comments, whitespace and line continuations are
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Tue, Mar 29, 2022 at 10:21:36AM +0900, Michael Paquier wrote: > On Mon, Mar 28, 2022 at 04:33:30PM +0800, Julien Rouhaud wrote: > > Ok, v5 attached without the TAP tests and updated sysviews tests. > > The update of the query related to pg_hba_file_rules in the regression > tests was independant, so I have split and applied that first, as of > 091a971. Ok. > Now, for the rest, I have found one place in the docs that had an > incorrect link, two incorrect comments (aka > s/pg_hba.conf/pg_ident.conf/), and the indentation was a bit off. > Anyway, all that was cosmetic, so applied after adjusting all those > things. Thanks! I'm attaching a rebase of the rest of the patchset. Note that I also added support for an "include_dir" directive, as it's also something that would be quite helpful (and very welcome for my $work use case). >From e5dc352cd2eb8cff24714930338adb0b2d20ff94 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Mon, 21 Feb 2022 15:45:26 +0800 Subject: [PATCH v6 1/2] Allow file inclusion in pg_hba and pg_ident files. Catversion is bumped. Author: Julien Rouhaud Reviewed-by: FIXME Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud --- doc/src/sgml/catalogs.sgml | 48 +++- doc/src/sgml/client-auth.sgml | 48 +++- src/backend/libpq/hba.c| 343 + src/backend/libpq/pg_hba.conf.sample | 10 +- src/backend/libpq/pg_ident.conf.sample | 12 +- src/backend/utils/adt/hbafuncs.c | 51 +++- src/include/catalog/pg_proc.dat| 11 +- src/include/libpq/hba.h| 2 + src/test/regress/expected/rules.out| 12 +- 9 files changed, 453 insertions(+), 84 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 7f4f79d1b5..2b5b7ef5d6 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -10496,12 +10496,31 @@ SCRAM-SHA-256$iteration count: + + + rule_number int4 + + + Rule number, in priority order, of this rule if the rule is valid, + otherwise null + + + + + + file_name text + + + File name of this rule + + + line_number int4 - Line number of this rule in pg_hba.conf + Line number of this rule in the given file_name @@ -10636,6 +10655,33 @@ SCRAM-SHA-256$iteration count: + + + mapping_number int4 + + + Rule number, in priority order, of this mapping if the mapping is valid, + otherwise null + + + + + + file_name text + + + File name of this mapping + + + + + + line_number int4 + + + Line number of this mapping in the given file_name + + line_number int4 diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 142b0affcb..4e1438476e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -89,8 +89,21 @@ - Each record specifies a connection type, a client IP address range - (if relevant for the connection type), a database name, a user name, + Each record can either be an inclusion directive or an authentication rule. + Inclusion records specifies files that can be included, which contains + additional records. The records will be inserted in lieu of the inclusion + records. Those records only contains two fields: the + include or include_dir directive and + the file or directory to be included. The file or directory can be a + relative of absolute path, and can be double quoted if needed. For the + include_dir form, all files not starting with a + . and ending with .conf will be + included. + + + + Each authentication record specifies a connection type, a client IP address + range (if relevant for the connection type), a database name, a user name, and the authentication method to be used for connections matching these parameters. The first record with a matching connection type, client address, requested database, and user name is used to perform @@ -103,6 +116,8 @@ A record can have several formats: +include file +include_dir directory local database user auth-method auth-options host database user address auth-method auth-options hostssl database user address auth-method auth-options @@ -118,6 +133,26 @@ hostnogssenc database user + + include + + + This line will be replaced with the content of the given file. + + + + + + include_dir + + + This line will be replaced with the content of all the files found in + the directory, if they don't start with a . and end + with .conf. + + + +
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Mar 28, 2022 at 04:33:30PM +0800, Julien Rouhaud wrote: > Ok, v5 attached without the TAP tests and updated sysviews tests. The update of the query related to pg_hba_file_rules in the regression tests was independant, so I have split and applied that first, as of 091a971. Now, for the rest, I have found one place in the docs that had an incorrect link, two incorrect comments (aka s/pg_hba.conf/pg_ident.conf/), and the indentation was a bit off. Anyway, all that was cosmetic, so applied after adjusting all those things. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Mar 28, 2022 at 05:02:14PM +0900, Michael Paquier wrote: > On Mon, Mar 28, 2022 at 03:43:41PM +0800, Julien Rouhaud wrote: > > > > Ok. We could still keep the tests for the valid lines part though? > > With the SQLs modified as below, this part is less interesting. Ok. > > Do you mean something like > > > > SELECT count(*) > 0 AS ok, > >count(*) FILTER (WHERE error IS NOT NULL) = 0 AS has_no_error > > FROM pg_hba_file_rules ; > > > > and similar for pg_ident_rule_mappings? > > Something like that, yes. Ok, v5 attached without the TAP tests and updated sysviews tests. >From 681c16db9fcb4e27ebba28e6a0f2134de071 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud Date: Mon, 21 Feb 2022 17:38:34 +0800 Subject: [PATCH v5 1/3] Add a pg_ident_file_mappings view. This view is similar to pg_hba_file_rules view, and can be also helpful to help diagnosing configuration problems. A following commit will add the possibility to include files in pg_hba and pg_ident configuration files, which will then make this view even more useful. Catversion is bumped. Author: Julien Rouhaud Reviewed-by: FIXME Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud --- doc/src/sgml/catalogs.sgml | 108 doc/src/sgml/client-auth.sgml | 10 ++ doc/src/sgml/func.sgml | 5 +- src/backend/catalog/system_views.sql | 6 ++ src/backend/libpq/hba.c| 31 +++--- src/backend/utils/adt/hbafuncs.c | 136 + src/include/catalog/pg_proc.dat| 7 ++ src/include/libpq/hba.h| 1 + src/test/regress/expected/rules.out| 6 ++ src/test/regress/expected/sysviews.out | 16 ++- src/test/regress/sql/sysviews.sql | 6 +- 11 files changed, 311 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 94f01e4099..75fedfa07e 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -9591,6 +9591,11 @@ SCRAM-SHA-256$iteration count: summary of client authentication configuration file contents + + pg_ident_file_mappings + summary of client user name mapping configuration file contents + + pg_indexes indexes @@ -10589,6 +10594,109 @@ SCRAM-SHA-256$iteration count: + + pg_ident_file_mappings + + + pg_ident_file_mappings + + + + The view pg_ident_file_mappings provides a summary + of the contents of the client user name mapping configuration file, + pg_ident.conf. + A row appears in this view for each + non-empty, non-comment line in the file, with annotations indicating + whether the rule could be applied successfully. + + + + This view can be helpful for checking whether planned changes in the + authentication configuration file will work, or for diagnosing a previous + failure. Note that this view reports on the current + contents of the file, not on what was last loaded by the server. + + + + By default, the pg_ident_file_mappings view can be + read only by superusers. + + + + pg_ident_file_mappings Columns + + + + Column Type + + + Description + + + + + + + + line_number int4 + + + Line number of this rule in pg_ident.conf + + + + + + map_name text + + + Name of the map + + + + + + sys_name text + + + Detected user name of the client + + + + + + pg_username text + + + Requested PostgreSQL user name + + + + + + error text + + + If not null, an error message indicating why this line could not be + processed + + + + + + + + Usually, a row reflecting an incorrect entry will have values for only + the line_number and error fields. + + + + See for more information about + client authentication configuration. + + + pg_indexes diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 02f0489112..142b0affcb 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -896,6 +896,16 @@ mymap /^(.*)@otherdomain\.com$ guest -HUP) to make it re-read the file. + + The system view + pg_ident_file_mappings + can be helpful for pre-testing changes to the + pg_ident.conf file, or for diagnosing problems if + loading of the file did not have the desired effects. Rows in the view with + non-null error fields indicate problems in the + corresponding lines of the file. + + A pg_ident.conf file that could be used in conjunction with the pg_hba.conf file in SIGHUP signal to the postmaster process, which in turn sends SIGHUP to each of its children.) You can use the -
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Mar 28, 2022 at 03:43:41PM +0800, Julien Rouhaud wrote: > On Mon, Mar 28, 2022 at 04:20:07PM +0900, Michael Paquier wrote: >> We could use a failure path for each psql command rather than a SKIP >> block, as you told me, if the psql fails and check that we get some >> error strings related to the loading of auth files. However, I am >> scared of this design in the long-term as it could cause the tests to >> pass with a failure triggered on platforms and/or configurations where >> we should have a success. So, I am tempted to drop the ball for now >> with the TAP part. > > Ok. We could still keep the tests for the valid lines part though? With the SQLs modified as below, this part is less interesting. >> The patch still has value for the end-user. I have checked the >> backend part, and I did not notice any obvious issue. There is one >> thing that I am wondering though: should we change the two queries in >> sysviews.sql so as we check that there are zero errors in the two >> views when the files are parsed? This simple change would avoid >> mistakes for users running installcheck on a production installation. > > Do you mean something like > > SELECT count(*) > 0 AS ok, >count(*) FILTER (WHERE error IS NOT NULL) = 0 AS has_no_error > FROM pg_hba_file_rules ; > > and similar for pg_ident_rule_mappings? Something like that, yes. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Mon, Mar 28, 2022 at 04:20:07PM +0900, Michael Paquier wrote:
>
> > Note that those tests fail on Windows (and I'm assuming on EXEC_BACKEND
> > builds), as they're testing invalid files which by definition prevent any
> > further connection attempt. I'm not sure what would be best to do here,
> > apart
> > from bypassing the invalid config tests on such platforms. I don't think
> > that
> > validating that trying to connect on such platforms when an invalid
> > pg_hba/pg_ident file brings anything.
>
> Hmm, indeed. I have been looking at that and that's annoying. As you
> mentioned off-list, in order to know if a build has an emulation of
> fork() that would avoid failures with new connection attempts after
> including some dummy entries in pg_hba.conf or pg_ident.conf, we'd
> need to look at EXEC_BACKEND (well, mostly), as of:
> - CFLAGS in pg_config, or a query on pg_config() (does not work with
> MSVC as CFLAGS is not set there).
> - A potential check on pg_config{_manual}.h.
> - Perhaps an extra dependency on $windows_os, discarding incorrectly
> cygwin that should be able to work.
Yeah, detecting !cygwin-Windows or EXEC_BACKEND in the TAP tests is quite hard.
> We could use a failure path for each psql command rather than a SKIP
> block, as you told me, if the psql fails and check that we get some
> error strings related to the loading of auth files. However, I am
> scared of this design in the long-term as it could cause the tests to
> pass with a failure triggered on platforms and/or configurations where
> we should have a success. So, I am tempted to drop the ball for now
> with the TAP part.
Ok. We could still keep the tests for the valid lines part though?
> The patch still has value for the end-user. I have checked the
> backend part, and I did not notice any obvious issue. There is one
> thing that I am wondering though: should we change the two queries in
> sysviews.sql so as we check that there are zero errors in the two
> views when the files are parsed? This simple change would avoid
> mistakes for users running installcheck on a production installation.
Do you mean something like
SELECT count(*) > 0 AS ok,
count(*) FILTER (WHERE error IS NOT NULL) = 0 AS has_no_error
FROM pg_hba_file_rules ;
and similar for pg_ident_rule_mappings?
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Mar 28, 2022 at 04:20:07PM +0900, Michael Paquier wrote:
> See the attached, for reference, but it would fail with EXEC_BACKEND
> on WIN32.
Ditto.
--
Michael
From 69e02734fd0199ba02cc34bc468b04584bdf0efd Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Mon, 28 Mar 2022 16:20:40 +0900
Subject: [PATCH v5] Add a pg_ident_file_mappings view.
This view is similar to pg_hba_file_rules view, and can be also helpful to help
diagnosing configuration problems.
A following commit will add the possibility to include files in pg_hba and
pg_ident configuration files, which will then make this view even more useful.
Catversion is bumped.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/include/catalog/pg_proc.dat | 6 +
src/include/libpq/hba.h | 1 +
src/backend/catalog/system_views.sql| 6 +
src/backend/libpq/hba.c | 31 +++--
src/backend/utils/adt/hbafuncs.c| 136
src/test/authentication/t/003_auth_views.pl | 108
src/test/regress/expected/rules.out | 6 +
src/test/regress/expected/sysviews.out | 6 +
src/test/regress/sql/sysviews.sql | 2 +
doc/src/sgml/catalogs.sgml | 108
doc/src/sgml/client-auth.sgml | 10 ++
doc/src/sgml/func.sgml | 5 +-
12 files changed, 409 insertions(+), 16 deletions(-)
create mode 100644 src/test/authentication/t/003_auth_views.pl
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5e612a6b67..915bc19176 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6115,6 +6115,12 @@
proargmodes => '{o,o,o,o,o,o,o,o,o}',
proargnames => '{line_number,type,database,user_name,address,netmask,auth_method,options,error}',
prosrc => 'pg_hba_file_rules' },
+{ oid => '9556', descr => 'show pg_ident.conf mappings',
+ proname => 'pg_ident_file_mappings', prorows => '1000', proretset => 't',
+ provolatile => 'v', prorettype => 'record', proargtypes => '',
+ proallargtypes => '{int4,text,text,text,text}', proargmodes => '{o,o,o,o,o}',
+ proargnames => '{line_number,map_name,sys_name,pg_username,error}',
+ prosrc => 'pg_ident_file_mappings' },
{ oid => '1371', descr => 'view system lock information',
proname => 'pg_lock_status', prorows => '1000', proretset => 't',
provolatile => 'v', prorettype => 'record', proargtypes => '',
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 13ecb329f8..90036f7bcd 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -171,6 +171,7 @@ extern int check_usermap(const char *usermap_name,
const char *pg_role, const char *auth_user,
bool case_sensitive);
extern HbaLine *parse_hba_line(TokenizedAuthLine *tok_line, int elevel);
+extern IdentLine *parse_ident_line(TokenizedAuthLine *tok_line, int elevel);
extern bool pg_isblank(const char c);
extern MemoryContext tokenize_auth_file(const char *filename, FILE *file,
List **tok_lines, int elevel);
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 9570a53e7b..9eaa51df29 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -617,6 +617,12 @@ CREATE VIEW pg_hba_file_rules AS
REVOKE ALL ON pg_hba_file_rules FROM PUBLIC;
REVOKE EXECUTE ON FUNCTION pg_hba_file_rules() FROM PUBLIC;
+CREATE VIEW pg_ident_file_mappings AS
+ SELECT * FROM pg_ident_file_mappings() AS A;
+
+REVOKE ALL ON pg_ident_file_mappings FROM PUBLIC;
+REVOKE EXECUTE ON FUNCTION pg_ident_file_mappings() FROM PUBLIC;
+
CREATE VIEW pg_timezone_abbrevs AS
SELECT * FROM pg_timezone_abbrevs();
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 673135144d..f8393ca8ed 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -887,25 +887,22 @@ do { \
} while (0)
/*
- * Macros for handling pg_ident problems.
- * Much as above, but currently the message level is hardwired as LOG
- * and there is no provision for an err_msg string.
+ * Macros for handling pg_ident problems, similar as above.
*
* IDENT_FIELD_ABSENT:
- * Log a message and exit the function if the given ident field ListCell is
- * not populated.
+ * Reports when the given ident field ListCell is not populated.
*
* IDENT_MULTI_VALUE:
- * Log a message and exit the function if the given ident token List has more
- * than one element.
+ * Reports when the given ident token List has more than one element.
*/
#define IDENT_FIELD_ABSENT(field) \
do { \
if (!field) { \
- ereport(LOG, \
+ ereport(elevel, \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("missing entry in file \"%s\" at end of line %d", \
IdentFileName, line_num))); \
+ *err_msg = psprintf("missing entry at end of line"); \
return NULL; \
Re: Allow file inclusion in pg_hba and pg_ident files
On Sun, Mar 27, 2022 at 05:52:22PM +0800, Julien Rouhaud wrote:
> I didn't like the various suggestions, as it would mean to scatter the tests
> all over the place. The whole point of those views is indeed to check the
> current content of a file without applying the configuration change (not on
> Windows or EXEC_BACKEND, but there's nothing we can do there), so let's use
> this way. I added a naive src/test/authentication/003_hba_ident_views.pl test
> that validates that specific new valid and invalid lines in both files are
> correctly reported. Note that I didn't update those tests for the file
> inclusion.
I see. The tests ought to be more extensive though, as hba.c checks
for multiple or missing fields for a varying number of expecting
parameters. Here are the patterns that would cover most of the
failures of the backend. For hba.conf:
+host
+host incorrect_db
+host incorrect_db incorrect_user
+host incorrect_db incorrect_user incorrect_host
For pg_ident.conf:
+# Error with incomplete lines.
+incorrect_map
+incorrect_map os_user
+# Errors with lines that have multiple values, for each field.
+incorrect_map_1,incorrect_map_2
+incorrect_map os_user_1,os_user_2
+incorrect_map os_user pg_role_1,pg_role_2
Then I was thinking about doing full scan of each view and could the
expected errors with some GROUP BY magic. See the attached, for
reference, but it would fail with EXEC_BACKEND on WIN32.
> Note that those tests fail on Windows (and I'm assuming on EXEC_BACKEND
> builds), as they're testing invalid files which by definition prevent any
> further connection attempt. I'm not sure what would be best to do here, apart
> from bypassing the invalid config tests on such platforms. I don't think that
> validating that trying to connect on such platforms when an invalid
> pg_hba/pg_ident file brings anything.
Hmm, indeed. I have been looking at that and that's annoying. As you
mentioned off-list, in order to know if a build has an emulation of
fork() that would avoid failures with new connection attempts after
including some dummy entries in pg_hba.conf or pg_ident.conf, we'd
need to look at EXEC_BACKEND (well, mostly), as of:
- CFLAGS in pg_config, or a query on pg_config() (does not work with
MSVC as CFLAGS is not set there).
- A potential check on pg_config{_manual}.h.
- Perhaps an extra dependency on $windows_os, discarding incorrectly
cygwin that should be able to work.
We could use a failure path for each psql command rather than a SKIP
block, as you told me, if the psql fails and check that we get some
error strings related to the loading of auth files. However, I am
scared of this design in the long-term as it could cause the tests to
pass with a failure triggered on platforms and/or configurations where
we should have a success. So, I am tempted to drop the ball for now
with the TAP part.
The patch still has value for the end-user. I have checked the
backend part, and I did not notice any obvious issue. There is one
thing that I am wondering though: should we change the two queries in
sysviews.sql so as we check that there are zero errors in the two
views when the files are parsed? This simple change would avoid
mistakes for users running installcheck on a production installation.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
On Fri, Mar 25, 2022 at 08:18:31PM +0900, Michael Paquier wrote:
>
> Now looking at 0002. The changes in hba.c are straight-forward,
> that's a nice read.
Thanks!
> if (!field) { \
> - ereport(LOG, \
> + ereport(elevel, \
> (errcode(ERRCODE_CONFIG_FILE_ERROR), \
>errmsg("missing entry in file \"%s\" at end of
> line %d", \
> IdentFileName, line_num))); \
> + *err_msg = psprintf("missing entry at end of line"); \
> return NULL; \
> } \
> I think that we'd better add to err_msg the line number and the file
> name. This would become particularly important once the facility to
> include files gets added. We won't use IdentFileName for this
> purpose, but at least we would know which areas to change. Also, even
> if the the view proposes line_number, there is an argument in favor of
> consistency here.
I don't really like it. The file inclusion patch adds a file_name column in
both views so that you have a direct access to the information, whether the
line is in error or not. Having the file name and line number in error message
doesn't add any value as it would be redundant, and just make the view output
bigger (on top of making testing more difficult). I kept the err_msg as-is
(and fixed the ereport filename in the file inclusion patch that I indeed
missed).
>
> +select count(*) >= 0 as ok from pg_ident_file_mappings;
>
> I'd really like to see more tests for this stuff
I didn't like the various suggestions, as it would mean to scatter the tests
all over the place. The whole point of those views is indeed to check the
current content of a file without applying the configuration change (not on
Windows or EXEC_BACKEND, but there's nothing we can do there), so let's use
this way. I added a naive src/test/authentication/003_hba_ident_views.pl test
that validates that specific new valid and invalid lines in both files are
correctly reported. Note that I didn't update those tests for the file
inclusion.
Note that those tests fail on Windows (and I'm assuming on EXEC_BACKEND
builds), as they're testing invalid files which by definition prevent any
further connection attempt. I'm not sure what would be best to do here, apart
from bypassing the invalid config tests on such platforms. I don't think that
validating that trying to connect on such platforms when an invalid
pg_hba/pg_ident file brings anything.
> +a.pg_usernamee,
> [...]
> +
> + pg_username text
>
> Perhaps that's just a typo in the function output and you
> intended to use pg_username?
Yes that was a typo :) It's correctly documented in catalogs.sgml, so I just
fixed pg_proc.dat and rules.out.
> + /* Free parse_hba_line memory */
> + MemoryContextSwitchTo(oldcxt);
> + MemoryContextDelete(identcxt);
> Incorrect comment, this should be parse_ident_line.
Indeed. I actually fixed it before but lost the change when rebasing after the
2nd hbafuncs.c refactoring. I also fixed an incorrect comment about
pg_hba_file_mappings.
>From ab685e1db37239df06ccdd7be8cfc14789e73e8c Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 21 Feb 2022 17:38:34 +0800
Subject: [PATCH v4 1/3] Add a pg_ident_file_mappings view.
This view is similar to pg_hba_file_rules view, and can be also helpful to help
diagnosing configuration problems.
A following commit will add the possibility to include files in pg_hba and
pg_ident configuration files, which will then make this view even more useful.
Catversion is bumped.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
doc/src/sgml/catalogs.sgml| 108 ++
doc/src/sgml/client-auth.sgml | 10 ++
doc/src/sgml/func.sgml| 5 +-
src/backend/catalog/system_views.sql | 6 +
src/backend/libpq/hba.c | 31 ++--
src/backend/utils/adt/hbafuncs.c | 136 ++
src/include/catalog/pg_proc.dat | 7 +
src/include/libpq/hba.h | 1 +
.../authentication/t/003_hba_ident_views.pl | 80 +++
src/test/regress/expected/rules.out | 6 +
src/test/regress/expected/sysviews.out| 6 +
src/test/regress/sql/sysviews.sql | 2 +
12 files changed, 382 insertions(+), 16 deletions(-)
create mode 100644 src/test/authentication/t/003_hba_ident_views.pl
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 94f01e4099..75fedfa07e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9591,6 +9591,11 @@ SCRAM-SHA-256$iteration
count:
summary of client authentication configuration file
contents
+
+ pg_ident_file_mappings
+ summary of client user name mapping configuration file
contents
+
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Mar 24, 2022 at 04:08:38PM +0800, Julien Rouhaud wrote:
> On Thu, Mar 24, 2022 at 04:50:31PM +0900, Michael Paquier wrote:
>> And so, this first part has been applied as of d4781d8. I'll try to
>> look at 0002 shortly.
>
> Thanks!
Now looking at 0002. The changes in hba.c are straight-forward,
that's a nice read.
if (!field) { \
- ereport(LOG, \
+ ereport(elevel, \
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("missing entry in file \"%s\" at end of
line %d", \
IdentFileName, line_num))); \
+ *err_msg = psprintf("missing entry at end of line"); \
return NULL; \
} \
I think that we'd better add to err_msg the line number and the file
name. This would become particularly important once the facility to
include files gets added. We won't use IdentFileName for this
purpose, but at least we would know which areas to change. Also, even
if the the view proposes line_number, there is an argument in favor of
consistency here.
+select count(*) >= 0 as ok from pg_ident_file_mappings;
I'd really like to see more tests for this stuff (pg_hba_file_rules is
not a great example in this area), where we could rely on something
for *nix platforms. Windows would provide some valid coverage as we
enable it by default for SSPI in pg_regress but that's not great for
the average Joe. One thing I thought about first is that
src/test/authentication/ does not test peer authentication at all,
which would map nicely with pg_ident.conf and some user mappings. So
we could have a new test there, that depends on how the backend reacts
when calling getpeereid(), making the results conditional. This would
be nice in the long-term.
As of a set of tests, I think that for now I would add two things, for
a total of four tests:
- Stick some queries on pg_ident_file_mappings only for Windows in
some of the tests of src/test/authentication/, say 001_password.pl.
One test should test for some valid fields. Another idea I have here
is to add some junk to pg_ident.conf, reload and check that an error
is generated (the missing field case on a given line).
- Do the same for src/test/kerberos/, with one positive and one
negative test with some junk in the ident conf file.
A last idea is to abuse of the fact that pg_ident is loaded even if we
don't use it: aka we could add some right and junky contents in
pg_ident.conf, then check its validity. Using one of the existing
tests may not be right, particularly if we finish by extending it, so
I would move that to a new fresh test script.
+a.pg_usernamee,
[...]
+
+ pg_username text
pg_usernamee sounds a bit weird as attribute name for the view. We
could stick to a simple postgres_user_name, or postgres_name, for
example. Perhaps that's just a typo in the function output and you
intended to use pg_username?
+ /* Free parse_hba_line memory */
+ MemoryContextSwitchTo(oldcxt);
+ MemoryContextDelete(identcxt);
Incorrect comment, this should be parse_ident_line.
--
Michael
signature.asc
Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Thu, Mar 24, 2022 at 04:50:31PM +0900, Michael Paquier wrote: > On Wed, Mar 23, 2022 at 01:14:02PM +0900, Michael Paquier wrote: > > On Wed, Mar 23, 2022 at 10:16:34AM +0800, Julien Rouhaud wrote: > >> Yeah, I thought about it but didn't rename it given your concerns about git > >> history. I'm fine either way. > > > > Oh, OK. The amount of diffs created by 0001 is still fine to grab > > even with the struct rename, so let's stick with it. > > And so, this first part has been applied as of d4781d8. I'll try to > look at 0002 shortly. Thanks!
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Mar 23, 2022 at 01:14:02PM +0900, Michael Paquier wrote: > On Wed, Mar 23, 2022 at 10:16:34AM +0800, Julien Rouhaud wrote: >> Yeah, I thought about it but didn't rename it given your concerns about git >> history. I'm fine either way. > > Oh, OK. The amount of diffs created by 0001 is still fine to grab > even with the struct rename, so let's stick with it. And so, this first part has been applied as of d4781d8. I'll try to look at 0002 shortly. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Mar 23, 2022 at 10:16:34AM +0800, Julien Rouhaud wrote: > Yeah, I thought about it but didn't rename it given your concerns about git > history. I'm fine either way. Oh, OK. The amount of diffs created by 0001 is still fine to grab even with the struct rename, so let's stick with it. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Wed, Mar 23, 2022 at 11:03:46AM +0900, Michael Paquier wrote: > > Pushing forward with 0001 by the end of the CF is the part that has no > controversy IMO, and I have no objections to it. Now, after looking > at this part, I found a few things, as of: > - HbaToken, the set of elements in the lists of TokenizedAuthLine, is > a weird to use as this layer gets used by both pg_hba.conf and > pg_indent.conf before transforming them into each HbaLine and > IdentLine. While making this part of the internals exposed, I think > that we'd better rename that to AuthToken at least. This impacts the > names of some routines internal to hba.c to copy and create > AuthTokens. Yeah, I thought about it but didn't rename it given your concerns about git history. I'm fine either way. > - s/gethba_options/get_hba_options/, to be consistent with > fill_hba_view() and other things. > - The comment at the top of tokenize_auth_file() needed a refresh. > > That's mostly cosmetic, and the rest of the code moved is identical. > So at the end this part looks rather commitable to me. Looks good to me, thanks. > I have not been able to test 0002 in details, but it looks rather > rather sane to me at quick glance, and it is simple. The argument > about more TAP tests applies to it, though, even if there is one SQL > test to check the function execution. It is probably better to not > consider 0003 and 0004 for this CF. No objection to moving 0003 and 0004 to the next commitfest.
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Mar 22, 2022 at 09:38:00PM +0800, Julien Rouhaud wrote:
> On Tue, Mar 22, 2022 at 03:21:20PM +0300, Aleksander Alekseev wrote:
>> Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also
>> increase CATALOG_VERSION_NO? Not sure if we generally do this in the
>> patches or expect the committer to make the change manually.
>
> It's better to no include any catversion bump, otherwise the patches will
> rot very fast. Author can mention the need for a catversion bump in the
> commit message, and I usually do so but I apparently forgot.
Yeah, committers take care of that. You would just expose yourself to
more noise in the CF bot for no gain, as a catversion bump is useful
after a patch has been merged so as as users are able to know when a
cluster needs to be pg_upgrade'd or initdb'd because the catalog
created and run are incompatible.
>> All in all, the patchset seems to be in good shape and I don't have
>> anything but some little nitpicks. It passes `make installcheck` and I
>> verified manually that the file inclusion 1) works 2) write proper error
>> messages to the logfile when the included file doesn't exist or has wrong
>> permissions.
>
> Thanks!
Pushing forward with 0001 by the end of the CF is the part that has no
controversy IMO, and I have no objections to it. Now, after looking
at this part, I found a few things, as of:
- HbaToken, the set of elements in the lists of TokenizedAuthLine, is
a weird to use as this layer gets used by both pg_hba.conf and
pg_indent.conf before transforming them into each HbaLine and
IdentLine. While making this part of the internals exposed, I think
that we'd better rename that to AuthToken at least. This impacts the
names of some routines internal to hba.c to copy and create
AuthTokens.
- s/gethba_options/get_hba_options/, to be consistent with
fill_hba_view() and other things.
- The comment at the top of tokenize_auth_file() needed a refresh.
That's mostly cosmetic, and the rest of the code moved is identical.
So at the end this part looks rather commitable to me.
I have not been able to test 0002 in details, but it looks rather
rather sane to me at quick glance, and it is simple. The argument
about more TAP tests applies to it, though, even if there is one SQL
test to check the function execution. It is probably better to not
consider 0003 and 0004 for this CF.
--
Michael
From dbee2a5b673b3bac2e4df0ec966912c29416fc2c Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Wed, 23 Mar 2022 10:56:15 +0900
Subject: [PATCH v4] Extract view processing code from hba.c
This file is already quite big and a following commit will add yet an
additional view, so let's move all the view related code in hba.c into a new
adt/hbafuncs.c.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/include/libpq/hba.h | 31 ++
src/backend/libpq/hba.c | 517 +++
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/hbafuncs.c | 428 +
src/tools/pgindent/typedefs.list | 4 +-
5 files changed, 511 insertions(+), 470 deletions(-)
create mode 100644 src/backend/utils/adt/hbafuncs.c
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 8d9f3821b1..13ecb329f8 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -132,6 +132,34 @@ typedef struct IdentLine
regex_t re;
} IdentLine;
+/*
+ * A single string token lexed from an authentication configuration file
+ * (pg_ident.conf or pg_hba.conf), together with whether the token has
+ * been quoted.
+ */
+typedef struct AuthToken
+{
+ char *string;
+ bool quoted;
+} AuthToken;
+
+/*
+ * TokenizedAuthLine represents one line lexed from an authentication
+ * configuration file. Each item in the "fields" list is a sub-list of
+ * AuthTokens. We don't emit a TokenizedAuthLine for empty or all-comment
+ * lines, so "fields" is never NIL (nor are any of its sub-lists).
+ *
+ * Exception: if an error occurs during tokenization, we might have
+ * fields == NIL, in which case err_msg != NULL.
+ */
+typedef struct TokenizedAuthLine
+{
+ List *fields; /* List of lists of AuthTokens */
+ int line_num; /* Line number */
+ char *raw_line; /* Raw line text */
+ char *err_msg; /* Error message if any */
+} TokenizedAuthLine;
+
/* kluge to avoid including libpq/libpq-be.h here */
typedef struct Port hbaPort;
@@ -142,6 +170,9 @@ extern void hba_getauthmethod(hbaPort *port);
extern int check_usermap(const char *usermap_name,
const char *pg_role, const char *auth_user,
bool case_sensitive);
+extern HbaLine *parse_hba_line(TokenizedAuthLine *tok_line, int elevel);
extern bool pg_isblank(const char c);
+extern MemoryContext tokenize_auth_file(const char *filename, FILE *file,
+ List **tok_lines, int elevel);
#endif /* HBA_H */
diff --git a/src/backend/libpq/hba.c
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Tue, Mar 22, 2022 at 03:21:20PM +0300, Aleksander Alekseev wrote: > > The v3-0001 patch LGTM. > > Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also > increase CATALOG_VERSION_NO? Not sure if we generally do this in the > patches or expect the committer to make the change manually. It's better to no include any catversion bump, otherwise the patches will rot very fast. Author can mention the need for a catversion bump in the commit message, and I usually do so but I apparently forgot. > Same question regarding v3-0003. Other than that the patch looks OK, but > doesn't seem to add any tests for the new functionality. Do you think it > would be possible to test-cover the file inclusion? Personally I don't > think it's that critical to have these particular tests, but if they can be > added, I think we should do so. Yes, as I mentioned in the first email I'm willing to add test coverage. But this will require TAP tests, and it's likely going to be a bit annoying to make sure it has decent coverage and works on all platforms, so I'd rather do it once I know there's at least some basic agreement on the feature and/or the approach. Unfortunately, until now there was no feedback on that part despite the activity on the thread. In my experience it's a good sign that this will get rejected soon, so I still didn't write tests. > I didn't review v3-0004 since it's marked as PoC and seems to be a separate > feature that is not targeting PG15. I suggest excluding it from the > patchset in order to keep the focus. Consider creating a new thread and a > new CF entry after we deal with v3-0001...v3-0003. This feature was discussed in some old thread when the file inclusion was first discussed almost a decade ago, and in my understanding it was part of the "must have" (along with 0002) in order to accept the feature. That's why I added it, since I'm also willing to work of that if needed, whether before or after the file inclusion thing. > All in all, the patchset seems to be in good shape and I don't have > anything but some little nitpicks. It passes `make installcheck` and I > verified manually that the file inclusion 1) works 2) write proper error > messages to the logfile when the included file doesn't exist or has wrong > permissions. Thanks!
Re: Allow file inclusion in pg_hba and pg_ident files
Hi hackers, > It passes `make installcheck` ... `make installcheck-world`, of course. Sorry for the confusion. -- Best regards, Aleksander Alekseev
Re: Allow file inclusion in pg_hba and pg_ident files
Hi hackers, > The cfbot says that the patch doesn't apply anymore, so here's a v3 with the > changes mentioned below. I came across this thread while looking for the patches that need review. The v3-0001 patch LGTM. Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also increase CATALOG_VERSION_NO? Not sure if we generally do this in the patches or expect the committer to make the change manually. Same question regarding v3-0003. Other than that the patch looks OK, but doesn't seem to add any tests for the new functionality. Do you think it would be possible to test-cover the file inclusion? Personally I don't think it's that critical to have these particular tests, but if they can be added, I think we should do so. I didn't review v3-0004 since it's marked as PoC and seems to be a separate feature that is not targeting PG15. I suggest excluding it from the patchset in order to keep the focus. Consider creating a new thread and a new CF entry after we deal with v3-0001...v3-0003. All in all, the patchset seems to be in good shape and I don't have anything but some little nitpicks. It passes `make installcheck` and I verified manually that the file inclusion 1) works 2) write proper error messages to the logfile when the included file doesn't exist or has wrong permissions. -- Best regards, Aleksander Alekseev
Re: Allow file inclusion in pg_hba and pg_ident files
Hi,
The cfbot says that the patch doesn't apply anymore, so here's a v3 with the
changes mentioned below.
On Tue, Mar 01, 2022 at 05:19:50PM +0800, Julien Rouhaud wrote:
>
> If you prefer to interleave static and non static function I can change it.
Change the split to not reorder functions.
> > +#include "utils/guc.h"
> > +//#include "utils/tuplestore.h"
>
> Yes I noticed this one this morning. I didn't want to send a new patch
> version
> just for that, but I already fixed it locally.
Included.
> Yes I'm aware of that thread. I will be happy to change the patch to use
> MakeFuncResultTuplestore() as soon as it lands. Thanks for the notice though.
Done, with the new SetSingleFuncCall().
> > It could be possible to do installcheck on an instance that has user
> > mappings meaning that this had better be ">= 0", no?
>
> I thought about it, and supposed it would bring a bit more value with the test
> like that. I can change it if you prefer.
Change this way.
>From 865b181ea989bb5c52fad2c3c9e20a38aa173cf3 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Tue, 1 Mar 2022 21:45:42 +0800
Subject: [PATCH v3 1/4] Extract view processing code from hba.c
This file is already quite big and a following commit will add yet an
additional view, so let's move all the view related code in hba.c into a new
adt/hbafuncs.c.
Author: Julien Rouhaud
Reviewed-by: FIXME
Discussion: https://postgr.es/m/20220223045959.35ipdsvbxcstrhya%40jrouhaud
---
src/backend/libpq/hba.c | 462 ++-
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/hbafuncs.c | 423
src/include/libpq/hba.h | 29 ++
src/tools/pgindent/typedefs.list | 2 +-
5 files changed, 475 insertions(+), 442 deletions(-)
create mode 100644 src/backend/utils/adt/hbafuncs.c
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 90953c38f3..f9843a0b30 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -68,32 +68,6 @@ typedef struct check_network_data
#define token_is_keyword(t, k) (!t->quoted && strcmp(t->string, k) == 0)
#define token_matches(t, k) (strcmp(t->string, k) == 0)
-/*
- * A single string token lexed from a config file, together with whether
- * the token had been quoted.
- */
-typedef struct HbaToken
-{
- char *string;
- boolquoted;
-} HbaToken;
-
-/*
- * TokenizedLine represents one line lexed from a config file.
- * Each item in the "fields" list is a sub-list of HbaTokens.
- * We don't emit a TokenizedLine for empty or all-comment lines,
- * so "fields" is never NIL (nor are any of its sub-lists).
- * Exception: if an error occurs during tokenization, we might
- * have fields == NIL, in which case err_msg != NULL.
- */
-typedef struct TokenizedLine
-{
- List *fields; /* List of lists of HbaTokens */
- int line_num; /* Line number */
- char *raw_line; /* Raw line text */
- char *err_msg;/* Error message if any */
-} TokenizedLine;
-
/*
* pre-parsed content of HBA config file: list of HbaLine structs.
* parsed_hba_context is the memory context where it lives.
@@ -138,16 +112,10 @@ static const char *const UserAuthName[] =
};
-static MemoryContext tokenize_file(const char *filename, FILE *file,
- List
**tok_lines, int elevel);
static List *tokenize_inc_file(List *tokens, const char *outer_filename,
const char
*inc_filename, int elevel, char **err_msg);
static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
int elevel, char
**err_msg);
-static ArrayType *gethba_options(HbaLine *hba);
-static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc,
- int lineno, HbaLine *hba,
const char *err_msg);
-static void fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc);
/*
@@ -419,7 +387,7 @@ tokenize_inc_file(List *tokens,
}
/* There is possible recursion here if the file contains @ */
- linecxt = tokenize_file(inc_fullname, inc_file, _lines, elevel);
+ linecxt = tokenize_auth_file(inc_fullname, inc_file, _lines,
elevel);
FreeFile(inc_file);
pfree(inc_fullname);
@@ -427,7 +395,7 @@ tokenize_inc_file(List *tokens,
/* Copy all tokens found in the file and append to the tokens list */
foreach(inc_line, inc_lines)
{
- TokenizedLine *tok_line = (TokenizedLine *) lfirst(inc_line);
+ TokenizedAuthLine *tok_line = (TokenizedAuthLine *)
lfirst(inc_line);
ListCell *inc_field;
/* If any line has an error, propagate that up to caller */
@@ -458,7 +426,8 @@
Re: Allow file inclusion in pg_hba and pg_ident files
On Tue, Mar 01, 2022 at 05:19:50PM +0800, Julien Rouhaud wrote: > On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote: >> Hmm. The diffs of 0001 are really hard to read. Do you know why this >> is happening? Is that because some code has been moved around? > > Yes, I followed the file convention to put the static functions first and then > the exposed functions, and git-diff makes a terrible mess out of it :( I'd like to think that not doing such a thing would be more helpful in this case. As the diffs show, anyone is going to have a hard time to figure out if there are any differences in any of those routines, and if these are the origin of a different problem. A second thing is that this is going to make back-patching unnecessarily harder. > There's no functional change apart from exposing some functions and moving > some > in another file, so I though it's still ok to keep some consistency. There > isn't much changes backpatched in that file, so it shouldn't create more > maintenance burden than simply splitting the file. A lot of files do that already. History clarity matters most IMO. > As I mentioned in my initial email, I intentionally didn't add any test in the > patchset yet, except the exact same coverage for the new view as there's for > pg_hba_file_rules. Ideally I'd like to add tests only once, to cover both 002 > and 0003. But I don't want to waste time for that right now, especially since > no one seems to be interested in 0003. But that would be helpful for 0002. I think that we should have a bit more coverage in this area. pg_hba_file_rules could gain in coverage, additionally, but this is unrelated to what you are proposing here.. >> Does this pass >> on Windows where pg_regress sets some mappings for SSPI when creating >> one or more roles? > > According to CI and cfbot yes. E.g. > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558. > Note that the failed runs are the warning I mentioned for mingw32 and the POC > 0004, which is now fixed. Interesting, I would not have expected that. I may poke at that more seriously. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote: > > Hmm. The diffs of 0001 are really hard to read. Do you know why this > is happening? Is that because some code has been moved around? Yes, I followed the file convention to put the static functions first and then the exposed functions, and git-diff makes a terrible mess out of it :( > I > have been doing a comparison of all the routines showing up in the > diffs, to note that the contents of load_hba(), load_ident(), > hba_getauthmethod() & friends are actually the same, but this makes > the change history harder to follow. Moving around fill_hba_line() > and fill_hba_view() should be enough, indeed. There's no functional change apart from exposing some functions and moving some in another file, so I though it's still ok to keep some consistency. There isn't much changes backpatched in that file, so it shouldn't create more maintenance burden than simply splitting the file. If you prefer to interleave static and non static function I can change it. > +#include "utils/guc.h" > +//#include "utils/tuplestore.h" Yes I noticed this one this morning. I didn't want to send a new patch version just for that, but I already fixed it locally. > + /* Build a tuple descriptor for our result type */ > + if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE) > + elog(ERROR, "return type must be a row type"); > > Worth noting that I was planning to apply a patch from Melanie > Plageman to simplify the creation of tupledesc and tuplestores for > set-returning functions like this one, so this would cut a bit of code > here. This is not directly related to your patch, though, that's my > business :) Yes I'm aware of that thread. I will be happy to change the patch to use MakeFuncResultTuplestore() as soon as it lands. Thanks for the notice though. > Well, as of 0002, one thing that makes things harder to follow is that > parse_ident_line() is moved at a different place in hba.c, one slight > difference being err_msg to store the error message in the token > line.. But shouldn't the extension of parse_ident_line() with its > elevel be included in 0001? No, because 0001 is unrelated. The changes you mentioned (exposing the function and adding the error reporting) are only needed for the new view introduced in 0002, thus not part of 0001. > Or, well, it could just be done in its own patch to make for a cleaner > history, so as 0002 could be shaped as two commits itself. It seems strange to me to add a commit (or include in a commit) just to make a single function exposed while nothing needs it, but I can do it this way if you prefer. > Also, it seems to me that we'd better have some TAP tests for that to > make sure of its contents? As I mentioned in my initial email, I intentionally didn't add any test in the patchset yet, except the exact same coverage for the new view as there's for pg_hba_file_rules. Ideally I'd like to add tests only once, to cover both 002 and 0003. But I don't want to waste time for that right now, especially since no one seems to be interested in 0003. > +-- We expect no user mapping in this test > +select count(*) = 0 as ok from pg_ident_file_mappings; > > It could be possible to do installcheck on an instance that has user > mappings meaning that this had better be ">= 0", no? I thought about it, and supposed it would bring a bit more value with the test like that. I can change it if you prefer. > Does this pass > on Windows where pg_regress sets some mappings for SSPI when creating > one or more roles? According to CI and cfbot yes. E.g. https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558. Note that the failed runs are the warning I mentioned for mingw32 and the POC 0004, which is now fixed.
Re: Allow file inclusion in pg_hba and pg_ident files
On Mon, Feb 28, 2022 at 07:42:17PM +0800, Julien Rouhaud wrote: > Done in attached v2. I did the split in a separate commit, as the diff is > otherwise unreadable. While at it I also fixed a few minor issues (I missed a > MemoryContextDelete, and now avoid relying on inet_net_pton which apparently > doesn't exist in cygwin). Hmm. The diffs of 0001 are really hard to read. Do you know why this is happening? Is that because some code has been moved around? I have been doing a comparison of all the routines showing up in the diffs, to note that the contents of load_hba(), load_ident(), hba_getauthmethod() & friends are actually the same, but this makes the change history harder to follow. Moving around fill_hba_line() and fill_hba_view() should be enough, indeed. +#include "utils/guc.h" +//#include "utils/tuplestore.h" Ditto. + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); Worth noting that I was planning to apply a patch from Melanie Plageman to simplify the creation of tupledesc and tuplestores for set-returning functions like this one, so this would cut a bit of code here. This is not directly related to your patch, though, that's my business :) Well, as of 0002, one thing that makes things harder to follow is that parse_ident_line() is moved at a different place in hba.c, one slight difference being err_msg to store the error message in the token line.. But shouldn't the extension of parse_ident_line() with its elevel be included in 0001? Or, well, it could just be done in its own patch to make for a cleaner history, so as 0002 could be shaped as two commits itself. Also, it seems to me that we'd better have some TAP tests for that to make sure of its contents? One place would be src/test/auth/. Another place where we make use of user mapping is the krb5 tests but these are run in a limited fashion in the buildfarm. We also set some mappings for SSPI on Windows all the time, so we'd better be careful about that and paint some $windows_os in the tests when looking at the output of the view. +-- We expect no user mapping in this test +select count(*) = 0 as ok from pg_ident_file_mappings; It could be possible to do installcheck on an instance that has user mappings meaning that this had better be ">= 0", no? Does this pass on Windows where pg_regress sets some mappings for SSPI when creating one or more roles? -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote: > > > 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: > > I think another use-case for this is testing updates to your configuration > files. For example, I could ensure that hba_forbid_non_ssl.conf wasn't > accidentally reverted as part of an unrelated change. Indeed, that function could really be helpful in many scenario. Note that this isn't my idea but Magnus idea, which he mentioned quite a long time ago.
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Feb 26, 2022 at 02:50:33PM +0800, Julien Rouhaud wrote: > Of course. I was thinking using "auth" for something that's common to pg_hba > and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current > hba/ident prefix. Okay, thanks. > Unless someone object or suggest better naming in the next few days I will > take > care of that. I don't have an opinion to share about 0002 and 0003 yet, but 0001 seems like a good idea on its own. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Feb 26, 2022 at 03:36:19PM +0900, Michael Paquier wrote: > On Sat, Feb 26, 2022 at 02:27:15PM +0800, Julien Rouhaud wrote: > > > Note that in order to do so we would need to expose quite a lot more about > > hba > > internals, like tokenize_file() and parse_hba_line(), along with structs > > HbaToken and TokenizedLine. > > I'd be rather fine with exposing all that in the shape of a clean > split, renaming some of those structures and/or function with an > Hba-like prefix, for consistency. Of course. I was thinking using "auth" for something that's common to pg_hba and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current hba/ident prefix. Unless someone object or suggest better naming in the next few days I will take care of that. I would also welcome some opinion on the points I mentioned about 0002.
Re: Allow file inclusion in pg_hba and pg_ident files
On Sat, Feb 26, 2022 at 02:27:15PM +0800, Julien Rouhaud wrote: > I'm fine with it. Assuming that you meant to move also the underlying > functions that goes with it (fill_hba_line and such), that would end up > removing about 15% of hba.c (after applying 0001, 0002 and 0003). Cool. Thanks for the number. > Note that in order to do so we would need to expose quite a lot more about hba > internals, like tokenize_file() and parse_hba_line(), along with structs > HbaToken and TokenizedLine. I'd be rather fine with exposing all that in the shape of a clean split, renaming some of those structures and/or function with an Hba-like prefix, for consistency. -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
Hi, On Sat, Feb 26, 2022 at 03:04:43PM +0900, Michael Paquier wrote: > On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote: > > On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote: > >> 0001 adds a new pg_ident_file_mappings view, which is basically the same as > >> pg_hba_file_rules view but for mappings. It's probably already useful, for > >> instance if you need to tweak some regexp. > > > > This seems reasonable. > > Interesting. One can note that hba.c is already large, and this makes > the file larger. I'd like to think that it would be better to move > all the code related to the SQL functions for pg_hba.conf and such to > a new hbafuncs.c under adt/. Would that make sense? I'm fine with it. Assuming that you meant to move also the underlying functions that goes with it (fill_hba_line and such), that would end up removing about 15% of hba.c (after applying 0001, 0002 and 0003). Note that in order to do so we would need to expose quite a lot more about hba internals, like tokenize_file() and parse_hba_line(), along with structs HbaToken and TokenizedLine.
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote: > On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote: >> 0001 adds a new pg_ident_file_mappings view, which is basically the same as >> pg_hba_file_rules view but for mappings. It's probably already useful, for >> instance if you need to tweak some regexp. > > This seems reasonable. Interesting. One can note that hba.c is already large, and this makes the file larger. I'd like to think that it would be better to move all the code related to the SQL functions for pg_hba.conf and such to a new hbafuncs.c under adt/. Would that make sense? -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote: > To address that, I'd like to propose the possibility to include files in hba > and ident configuration files. This was already discussed in the past, and in > my understanding this is mostly wanted, while some people expressed concerned > on a use case that wouldn't rely on thousands of entries. +1, I think this would be very useful. > 0001 adds a new pg_ident_file_mappings view, which is basically the same as > pg_hba_file_rules view but for mappings. It's probably already useful, for > instance if you need to tweak some regexp. This seems reasonable. > 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: I think another use-case for this is testing updates to your configuration files. For example, I could ensure that hba_forbid_non_ssl.conf wasn't accidentally reverted as part of an unrelated change. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
