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 <mich...@paquier.xyz>
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 token_matches(t, k) (strcmp(t->string, k) == 0)
+/*
+ * 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;
+
/*
* pre-parsed content of HBA config file: list of HbaLine structs.
* parsed_hba_context is the memory context where it lives.
@@ -121,9 +128,9 @@ static const char *const UserAuthName[] =
};
-static List *tokenize_inc_file(List *tokens, const char *outer_filename,
- const char *inc_filename, int elevel,
- int depth, char **err_msg);
+static List *tokenize_expand_file(List *tokens, const char *outer_filename,
+ 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,
@@ -437,17 +444,27 @@ 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, depth + 1, err_msg);
+ tokens = tokenize_expand_file(tokens, filename, buf + 1,
+ elevel, depth + 1, err_msg);
else
+ {
+ MemoryContext oldcxt;
+
+ /*
+ * lappend() may do its own allocations, so move to the context
+ * for the list of tokens.
+ */
+ oldcxt = MemoryContextSwitchTo(tokenize_context);
tokens = lappend(tokens, make_auth_token(buf, initial_quote));
+ MemoryContextSwitchTo(oldcxt);
+ }
} while (trailing_comma && (*err_msg == NULL));
return tokens;
}
/*
- * tokenize_inc_file
+ * tokenize_expand_file
* Expand a file included from another file into an hba "field"
*
* Opens and tokenises a file included from another HBA config file with @,
@@ -462,18 +479,17 @@ next_field_expand(const char *filename, char **lineptr,
* there was an error.
*/
static List *
-tokenize_inc_file(List *tokens,
- const char *outer_filename,
- const char *inc_filename,
- int elevel,
- int depth,
- char **err_msg)
+tokenize_expand_file(List *tokens,
+ const char *outer_filename,
+ const char *inc_filename,
+ int elevel,
+ int depth,
+ char **err_msg)
{
char *inc_fullname;
FILE *inc_file;
List *inc_lines;
ListCell *inc_line;
- MemoryContext linecxt;
inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename);
inc_file = open_auth_file(inc_fullname, elevel, depth, err_msg);
@@ -486,13 +502,15 @@ tokenize_inc_file(List *tokens,
}
/* There is possible recursion here if the file contains @ */
- linecxt = tokenize_auth_file(inc_fullname, inc_file, &inc_lines, elevel,
- depth);
+ tokenize_auth_file(inc_fullname, inc_file, &inc_lines, elevel,
+ depth);
- FreeFile(inc_file);
pfree(inc_fullname);
- /* Copy all tokens found in the file and append to the tokens list */
+ /*
+ * Move all the tokens found in the file to the tokens list. These are
+ * already saved in tokenize_context.
+ */
foreach(inc_line, inc_lines)
{
TokenizedAuthLine *tok_line = (TokenizedAuthLine *) lfirst(inc_line);
@@ -513,16 +531,40 @@ tokenize_inc_file(List *tokens,
foreach(inc_token, inc_tokens)
{
AuthToken *token = lfirst(inc_token);
+ MemoryContext oldcxt;
- tokens = lappend(tokens, copy_auth_token(token));
+ /*
+ * lappend() may do its own allocations, so move to the
+ * context for the list of tokens.
+ */
+ oldcxt = MemoryContextSwitchTo(tokenize_context);
+ tokens = lappend(tokens, token);
+ MemoryContextSwitchTo(oldcxt);
}
}
}
- MemoryContextDelete(linecxt);
+ free_auth_file(inc_file, depth);
return tokens;
}
+/*
+ * free_auth_file
+ * Free a file opened by open_auth_file().
+ */
+void
+free_auth_file(FILE *file, int depth)
+{
+ FreeFile(file);
+
+ /* If this is the last cleanup, remove the tokenization context */
+ if (depth == 0)
+ {
+ MemoryContextDelete(tokenize_context);
+ tokenize_context = NULL;
+ }
+}
+
/*
* open_auth_file
* Open the given file.
@@ -558,6 +600,22 @@ open_auth_file(const char *filename, int elevel, int depth,
return NULL;
}
+ /*
+ * When opening the top-level file, create the memory context used for the
+ * tokenization. This will be closed with this file when coming back to
+ * this level of cleanup.
+ */
+ if (depth == 0)
+ {
+ /*
+ * A context may be present, but assume that it has been eliminated
+ * already.
+ */
+ tokenize_context = AllocSetContextCreate(CurrentMemoryContext,
+ "tokenize_context",
+ ALLOCSET_START_SMALL_SIZES);
+ }
+
file = AllocateFile(filename, "r");
if (file == NULL)
{
@@ -570,6 +628,8 @@ open_auth_file(const char *filename, int elevel, int depth,
if (err_msg)
*err_msg = psprintf("could not open file \"%s\": %s",
filename, strerror(save_errno));
+ /* the caller may care about some specific errno */
+ errno = save_errno;
return NULL;
}
@@ -593,32 +653,34 @@ tokenize_error_callback(void *arg)
* Tokenize the given file.
*
* The output is a list of TokenizedAuthLine structs; see the struct definition
- * in libpq/hba.h.
+ * in libpq/hba.h. This is the central pieces in charge of parsing the
+ * authentication files. All the operations of this function happen in its own
+ * local memory context, easing the cleanup of anything allocated locally.
*
* filename: the absolute path to the target file
* file: the already-opened target file
- * tok_lines: receives output list
+ * tok_lines: receives output list, saved into tokenize_context
* elevel: message logging level
* depth: level of recursion when tokenizing the target file
*
* Errors are reported by logging messages at ereport level elevel and by
* adding TokenizedAuthLine structs containing non-null err_msg fields to the
* output list.
- *
- * Return value is a memory context which contains all memory allocated by
- * this function (it's a child of caller's context).
*/
-MemoryContext
+void
tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
int elevel, int depth)
{
- int line_number = 1;
StringInfoData buf;
+ int line_number = 1;
MemoryContext linecxt;
+ MemoryContext funccxt;
MemoryContext oldcxt;
ErrorContextCallback tokenerrcontext;
tokenize_error_callback_arg callback_arg;
+ Assert(tokenize_context);
+
callback_arg.filename = filename;
callback_arg.linenum = line_number;
@@ -627,14 +689,19 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
tokenerrcontext.previous = error_context_stack;
error_context_stack = &tokenerrcontext;
+ /*
+ * Do all the local tokenization in its own context, to ease the cleanup
+ * of any memory allocated while tokenizing.
+ */
linecxt = AllocSetContextCreate(CurrentMemoryContext,
"tokenize_auth_file",
ALLOCSET_SMALL_SIZES);
- oldcxt = MemoryContextSwitchTo(linecxt);
+ funccxt = MemoryContextSwitchTo(linecxt);
initStringInfo(&buf);
- *tok_lines = NIL;
+ if (depth == 0)
+ *tok_lines = NIL;
while (!feof(file) && !ferror(file))
{
@@ -694,7 +761,15 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
elevel, depth, &err_msg);
/* add field to line, unless we are at EOL or comment start */
if (current_field != NIL)
+ {
+ /*
+ * lappend() may do its own allocations, so move to the
+ * context for the list of tokens.
+ */
+ oldcxt = MemoryContextSwitchTo(tokenize_context);
current_line = lappend(current_line, current_field);
+ MemoryContextSwitchTo(oldcxt);
+ }
}
/*
@@ -704,24 +779,25 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines,
{
TokenizedAuthLine *tok_line;
+ oldcxt = MemoryContextSwitchTo(tokenize_context);
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;
+ tok_line->err_msg = err_msg ? pstrdup(err_msg) : NULL;
*tok_lines = lappend(*tok_lines, tok_line);
+ MemoryContextSwitchTo(oldcxt);
}
line_number += continuations + 1;
callback_arg.linenum = line_number;
}
- MemoryContextSwitchTo(oldcxt);
+ MemoryContextSwitchTo(funccxt);
+ MemoryContextDelete(linecxt);
error_context_stack = tokenerrcontext.previous;
-
- return linecxt;
}
@@ -2409,7 +2485,6 @@ load_hba(void)
ListCell *line;
List *new_parsed_lines = NIL;
bool ok = true;
- MemoryContext linecxt;
MemoryContext oldcxt;
MemoryContext hbacxt;
@@ -2420,8 +2495,7 @@ load_hba(void)
return false;
}
- linecxt = tokenize_auth_file(HbaFileName, file, &hba_lines, LOG, 0);
- FreeFile(file);
+ tokenize_auth_file(HbaFileName, file, &hba_lines, LOG, 0);
/* Now parse all the lines */
Assert(PostmasterContext);
@@ -2472,7 +2546,7 @@ load_hba(void)
}
/* Free tokenizer memory */
- MemoryContextDelete(linecxt);
+ free_auth_file(file, 0);
MemoryContextSwitchTo(oldcxt);
if (!ok)
@@ -2776,7 +2850,6 @@ load_ident(void)
*parsed_line_cell;
List *new_parsed_lines = NIL;
bool ok = true;
- MemoryContext linecxt;
MemoryContext oldcxt;
MemoryContext ident_context;
IdentLine *newline;
@@ -2789,8 +2862,7 @@ load_ident(void)
return false;
}
- linecxt = tokenize_auth_file(IdentFileName, file, &ident_lines, LOG, 0);
- FreeFile(file);
+ tokenize_auth_file(IdentFileName, file, &ident_lines, LOG, 0);
/* Now parse all the lines */
Assert(PostmasterContext);
@@ -2826,7 +2898,7 @@ load_ident(void)
}
/* Free tokenizer memory */
- MemoryContextDelete(linecxt);
+ free_auth_file(file, 0);
MemoryContextSwitchTo(oldcxt);
if (!ok)
diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c
index b662e7b55f..87996da487 100644
--- a/src/backend/utils/adt/hbafuncs.c
+++ b/src/backend/utils/adt/hbafuncs.c
@@ -370,7 +370,6 @@ fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
List *hba_lines = NIL;
ListCell *line;
int rule_number = 0;
- MemoryContext linecxt;
MemoryContext hbacxt;
MemoryContext oldcxt;
@@ -382,8 +381,7 @@ fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
*/
file = open_auth_file(HbaFileName, ERROR, 0, NULL);
- linecxt = tokenize_auth_file(HbaFileName, file, &hba_lines, DEBUG3, 0);
- FreeFile(file);
+ tokenize_auth_file(HbaFileName, file, &hba_lines, DEBUG3, 0);
/* Now parse all the lines */
hbacxt = AllocSetContextCreate(CurrentMemoryContext,
@@ -408,7 +406,7 @@ fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
}
/* Free tokenizer memory */
- MemoryContextDelete(linecxt);
+ free_auth_file(file, 0);
/* Free parse_hba_line memory */
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(hbacxt);
@@ -514,7 +512,6 @@ fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
List *ident_lines = NIL;
ListCell *line;
int map_number = 0;
- MemoryContext linecxt;
MemoryContext identcxt;
MemoryContext oldcxt;
@@ -526,8 +523,7 @@ fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
*/
file = open_auth_file(IdentFileName, ERROR, 0, NULL);
- linecxt = tokenize_auth_file(IdentFileName, file, &ident_lines, DEBUG3, 0);
- FreeFile(file);
+ tokenize_auth_file(IdentFileName, file, &ident_lines, DEBUG3, 0);
/* Now parse all the lines */
identcxt = AllocSetContextCreate(CurrentMemoryContext,
@@ -553,7 +549,7 @@ fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc)
}
/* Free tokenizer memory */
- MemoryContextDelete(linecxt);
+ free_auth_file(file, 0);
/* Free parse_ident_line memory */
MemoryContextSwitchTo(oldcxt);
MemoryContextDelete(identcxt);
--
2.38.1
From 1173e0badf1572816b30a251106a6d4feb8c070c Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Wed, 23 Nov 2022 14:47:17 +0900 Subject: [PATCH v21 2/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 | 182 ++++- src/backend/libpq/pg_hba.conf.sample | 27 + src/backend/libpq/pg_ident.conf.sample | 34 + src/backend/utils/adt/hbafuncs.c | 39 +- src/test/authentication/meson.build | 1 + .../authentication/t/004_file_inclusion.pl | 721 ++++++++++++++++++ src/test/regress/expected/rules.out | 6 +- doc/src/sgml/client-auth.sgml | 85 ++- doc/src/sgml/system-views.sgml | 22 +- 10 files changed, 1075 insertions(+), 54 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}', + proallargtypes => '{int4,text,int4,text,_text,_text,text,text,text,_text,text}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}', + proargnames => '{rule_number,file_name,line_number,type,database,user_name,address,netmask,auth_method,options,error}', prosrc => 'pg_hba_file_rules' }, { oid => '6250', descr => 'show pg_ident.conf mappings', proname => 'pg_ident_file_mappings', prorows => '1000', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{int4,int4,text,text,text,text}', - proargmodes => '{o,o,o,o,o,o}', - proargnames => '{map_number,line_number,map_name,sys_name,pg_username,error}', + proallargtypes => '{int4,text,int4,text,text,text,text}', + proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{map_number,file_name,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', diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index 9064ad6714..3f94359c4f 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -463,6 +463,64 @@ next_field_expand(const char *filename, char **lineptr, return tokens; } +/* + * tokenize_inclusion_file + * Include a file from another file into an hba "field". + * + * Opens and tokenises a file included from another HBA config file with + * one of the inclusion records ("include", "include_if_exists" or + * "include_dir"), and assign all values found to an existing list of + * list of AuthTokens. + * + * All new tokens are allocated in the memory context dedicated to the + * tokenization. + * + * If missing_ok is true, ignore the missing files. + * + * In event of an error, log a message at ereport level elevel, and also + * set *err_msg to a string describing the error. Note that the result + * may be non-NIL anyway, so *err_msg must be tested to determine whether + * there was an error. + */ +static void +tokenize_inclusion_file(const char *outer_filename, + const char *inc_filename, + List **tok_lines, + int elevel, + int depth, + bool missing_ok, + char **err_msg) +{ + char *inc_fullname; + FILE *inc_file; + + inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename); + inc_file = open_auth_file(inc_fullname, elevel, depth, err_msg); + + if (!inc_file) + { + if (errno == ENOENT && missing_ok) + { + ereport(elevel, + (errmsg("skipping missing authentication file \"%s\"", + inc_fullname))); + *err_msg = NULL; + pfree(inc_fullname); + return; + } + + /* error in err_msg, so leave and report */ + pfree(inc_fullname); + Assert(err_msg); + return; + } + + tokenize_auth_file(inc_fullname, inc_file, tok_lines, elevel, + depth); + free_auth_file(inc_file, depth); + pfree(inc_fullname); +} + /* * tokenize_expand_file * Expand a file included from another file into an hba "field" @@ -471,7 +529,8 @@ next_field_expand(const char *filename, char **lineptr, * and returns all values found therein as a flat list of AuthTokens. If a * @-token is found, recursively expand it. The newly read tokens are * appended to "tokens" (so that foo,bar,@baz does what you expect). - * All new tokens are allocated in caller's memory context. + * All new tokens are allocated in the memory context dedicated to the + * tokenization. * * In event of an error, log a message at ereport level elevel, and also * set *err_msg to a string describing the error. Note that the result @@ -488,7 +547,7 @@ tokenize_expand_file(List *tokens, { char *inc_fullname; FILE *inc_file; - List *inc_lines; + List *inc_lines = NIL; ListCell *inc_line; inc_fullname = AbsoluteConfigLocation(inc_filename, outer_filename); @@ -705,6 +764,7 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines, while (!feof(file) && !ferror(file)) { + TokenizedAuthLine *tok_line; char *lineptr; List *current_line = NIL; char *err_msg = NULL; @@ -773,23 +833,115 @@ tokenize_auth_file(const char *filename, FILE *file, List **tok_lines, } /* - * Reached EOL; emit line to TokenizedAuthLine list unless it's boring + * Reached EOL; no need to emit line to TokenizedAuthLine list if it's + * boring. */ - if (current_line != NIL || err_msg != NULL) - { - TokenizedAuthLine *tok_line; + if (current_line == NIL && err_msg == NULL) + goto next_line; - oldcxt = MemoryContextSwitchTo(tokenize_context); - 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 ? pstrdup(err_msg) : NULL; - *tok_lines = lappend(*tok_lines, tok_line); - MemoryContextSwitchTo(oldcxt); + /* If the line is valid, check if that's an include directive */ + if (err_msg == NULL && list_length(current_line) == 2) + { + AuthToken *first, + *second; + + first = linitial(linitial_node(List, current_line)); + second = linitial(lsecond_node(List, current_line)); + + if (strcmp(first->string, "include") == 0) + { + tokenize_inclusion_file(filename, second->string, tok_lines, + elevel, depth + 1, false, &err_msg); + + if (err_msg) + goto process_line; + + /* + * tokenize_auth_file() has taken care of creating the + * TokenizedAuthLines. + */ + goto next_line; + } + else if (strcmp(first->string, "include_dir") == 0) + { + char **filenames; + char *dir_name = second->string; + int num_filenames; + StringInfoData err_buf; + + filenames = GetConfFilesInDir(dir_name, filename, elevel, + &num_filenames, &err_msg); + + if (!filenames) + { + /* the error is in err_msg, so create an entry */ + goto process_line; + } + + initStringInfo(&err_buf); + for (int i = 0; i < num_filenames; i++) + { + tokenize_inclusion_file(filename, filenames[i], tok_lines, + elevel, depth + 1, false, &err_msg); + /* cumulate errors if any */ + if (err_msg) + { + if (err_buf.len > 0) + appendStringInfoChar(&err_buf, '\n'); + appendStringInfoString(&err_buf, err_msg); + } + } + + /* clean up things */ + for (int i = 0; i < num_filenames; i++) + pfree(filenames[i]); + pfree(filenames); + + /* + * If there were no errors, the line is fully processed, + * bypass the general TokenizedAuthLine processing. + */ + if (err_buf.len == 0) + goto next_line; + + /* Otherwise, process the cumulated errors, if any. */ + err_msg = err_buf.data; + goto process_line; + } + else if (strcmp(first->string, "include_if_exists") == 0) + { + + tokenize_inclusion_file(filename, second->string, tok_lines, + elevel, depth + 1, true, &err_msg); + if (err_msg) + goto process_line; + + /* + * tokenize_auth_file() has taken care of creating the + * TokenizedAuthLines. + */ + goto next_line; + } } +process_line: + + /* + * General processing: report the error if any and emit line to the + * TokenizedAuthLine. This is saved in the memory context dedicated + * to this list. + */ + oldcxt = MemoryContextSwitchTo(tokenize_context); + tok_line = (TokenizedAuthLine *) palloc0(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 ? pstrdup(err_msg) : NULL; + *tok_lines = lappend(*tok_lines, tok_line); + MemoryContextSwitchTo(oldcxt); + +next_line: line_number += continuations + 1; callback_arg.linenum = line_number; } diff --git a/src/backend/libpq/pg_hba.conf.sample b/src/backend/libpq/pg_hba.conf.sample index 5f3f63eb0c..f72f471d23 100644 --- a/src/backend/libpq/pg_hba.conf.sample +++ b/src/backend/libpq/pg_hba.conf.sample @@ -5,6 +5,10 @@ # documentation for a complete description of this file. A short # synopsis follows. # +# ------------------------------- +# Authentication records +# ------------------------------- +# # This file controls: which hosts are allowed to connect, how clients # are authenticated, which PostgreSQL user names they can use, which # databases they can access. Records take one of these forms: @@ -64,11 +68,34 @@ # its special character, and just match a database or username with # that name. # +# -------------------------------- +# Inclusion records +# --------------------------------- +# +# This files allow the inclusion of external files or directories holding +# extra authentication records, using the following keywords: +# +# include FILE +# include_if_exists FILE +# include_dir DIRECTORY +# +# FILE is the file name to include, and DIR is the directory name containing +# the file(s) to include. Any file in a directory will be loaded if suffixed +# with ".conf". The files of a directory are ordered by name. +# include_if_exists ignored missing files. FILE and DIRECTORY can be +# specified as a relative or absolute path, and can be double-quoted if they +# contain spaces. +# +# ------------------------------- +# Miscellaneous +# ------------------------------- +# # This file is read on server startup and when the server receives a # SIGHUP signal. If you edit the file on a running system, you have to # SIGHUP the server for the changes to take effect, run "pg_ctl reload", # or execute "SELECT pg_reload_conf()". # +# --------------------------------- # Put your actual configuration here # ---------------------------------- # diff --git a/src/backend/libpq/pg_ident.conf.sample b/src/backend/libpq/pg_ident.conf.sample index a5870e6448..59092bacf2 100644 --- a/src/backend/libpq/pg_ident.conf.sample +++ b/src/backend/libpq/pg_ident.conf.sample @@ -1,6 +1,10 @@ # PostgreSQL User Name Maps # ========================= # +# ------------------------------ +# Ident Records +# ------------------------------ +# # Refer to the PostgreSQL documentation, chapter "Client # Authentication" for a complete description. A short synopsis # follows. @@ -13,6 +17,14 @@ # # (The uppercase quantities must be replaced by actual values.) # +# If the first field is "include", "include_if_exists" or "include_dir", it's +# not a mapping record but a directive to include records from respectively +# another file, another file if it exists or all the files in the given +# directory ending in '.conf'. FILE is the file name to include, and +# DIR is the directory name containing the file(s) to include. FILE and +# DIRECTORY can be specified with a relative or absolute path, and can be +# double quoted if they contains spaces. +# # MAPNAME is the (otherwise freely chosen) map name that was used in # pg_hba.conf. SYSTEM-USERNAME is the detected user name of the # client. PG-USERNAME is the requested PostgreSQL user name. The @@ -31,6 +43,28 @@ # system user names and PostgreSQL user names are the same, you don't # need anything in this file. # +# ------------------------------ +# Inclusion records +# ------------------------------ +# +# This files allow the inclusion of external files or directories holding +# extra records, using the following keywords: +# +# include FILE +# include_if_exists FILE +# include_dir DIRECTORY +# +# FILE is the file name to include, and DIR is the directory name containing +# the file(s) to include. Any file in a directory will be loaded if suffixed +# with ".conf". The files of a directory are ordered by name. +# include_if_exists ignored missing files. FILE and DIRECTORY can be +# specified as a relative or absolute path, and can be double-quoted if they +# contain spaces. +# +# ------------------------------- +# Miscellaneous +# ------------------------------- +# # This file is read on server startup and when the postmaster receives # a SIGHUP signal. If you edit the file on a running system, you have # to SIGHUP the postmaster for the changes to take effect. You can diff --git a/src/backend/utils/adt/hbafuncs.c b/src/backend/utils/adt/hbafuncs.c index 87996da487..633eda30d3 100644 --- a/src/backend/utils/adt/hbafuncs.c +++ b/src/backend/utils/adt/hbafuncs.c @@ -26,12 +26,12 @@ static ArrayType *get_hba_options(HbaLine *hba); static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, - int rule_number, int lineno, HbaLine *hba, - const char *err_msg); + int rule_number, char *filename, 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 map_number, int lineno, IdentLine *ident, - const char *err_msg); + int map_number, char *filename, int lineno, + IdentLine *ident, const char *err_msg); static void fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc); @@ -159,7 +159,7 @@ get_hba_options(HbaLine *hba) } /* Number of columns in pg_hba_file_rules view */ -#define NUM_PG_HBA_FILE_RULES_ATTS 10 +#define NUM_PG_HBA_FILE_RULES_ATTS 11 /* * fill_hba_line @@ -168,7 +168,8 @@ get_hba_options(HbaLine *hba) * tuple_store: where to store data * tupdesc: tuple descriptor for the view * rule_number: unique identifier among all valid rules - * lineno: pg_hba.conf line number (must always be valid) + * filename: configuration file name (must always be valid) + * lineno: line number of configuration file (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) * @@ -177,7 +178,7 @@ get_hba_options(HbaLine *hba) */ static void fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, - int rule_number, int lineno, HbaLine *hba, + int rule_number, char *filename, int lineno, HbaLine *hba, const char *err_msg) { Datum values[NUM_PG_HBA_FILE_RULES_ATTS]; @@ -203,6 +204,9 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, else values[index++] = Int32GetDatum(rule_number); + /* file_name */ + values[index++] = CStringGetTextDatum(filename); + /* line_number */ values[index++] = Int32GetDatum(lineno); @@ -346,7 +350,7 @@ fill_hba_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, else { /* no parsing result, so set relevant fields to nulls */ - memset(&nulls[2], true, (NUM_PG_HBA_FILE_RULES_ATTS - 3) * sizeof(bool)); + memset(&nulls[3], true, (NUM_PG_HBA_FILE_RULES_ATTS - 4) * sizeof(bool)); } /* error */ @@ -402,7 +406,8 @@ fill_hba_view(Tuplestorestate *tuple_store, TupleDesc tupdesc) rule_number++; fill_hba_line(tuple_store, tupdesc, rule_number, - tok_line->line_num, hbaline, tok_line->err_msg); + tok_line->file_name, tok_line->line_num, hbaline, + tok_line->err_msg); } /* Free tokenizer memory */ @@ -439,7 +444,7 @@ pg_hba_file_rules(PG_FUNCTION_ARGS) } /* Number of columns in pg_ident_file_mappings view */ -#define NUM_PG_IDENT_FILE_MAPPINGS_ATTS 6 +#define NUM_PG_IDENT_FILE_MAPPINGS_ATTS 7 /* * fill_ident_line: build one row of pg_ident_file_mappings view, add it to @@ -448,7 +453,8 @@ pg_hba_file_rules(PG_FUNCTION_ARGS) * tuple_store: where to store data * tupdesc: tuple descriptor for the view * map_number: unique identifier among all valid maps - * lineno: pg_ident.conf line number (must always be valid) + * filename: configuration file name (must always be valid) + * lineno: line number of configuration file (must always be valid) * ident: parsed line data (can be NULL, in which case err_msg should be set) * err_msg: error message (NULL if none) * @@ -457,7 +463,7 @@ pg_hba_file_rules(PG_FUNCTION_ARGS) */ static void fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, - int map_number, int lineno, IdentLine *ident, + int map_number, char *filename, int lineno, IdentLine *ident, const char *err_msg) { Datum values[NUM_PG_IDENT_FILE_MAPPINGS_ATTS]; @@ -477,6 +483,9 @@ fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, else values[index++] = Int32GetDatum(map_number); + /* file_name */ + values[index++] = CStringGetTextDatum(filename); + /* line_number */ values[index++] = Int32GetDatum(lineno); @@ -489,7 +498,7 @@ fill_ident_line(Tuplestorestate *tuple_store, TupleDesc tupdesc, else { /* no parsing result, so set relevant fields to nulls */ - memset(&nulls[2], true, (NUM_PG_IDENT_FILE_MAPPINGS_ATTS - 3) * sizeof(bool)); + memset(&nulls[3], true, (NUM_PG_IDENT_FILE_MAPPINGS_ATTS - 4) * sizeof(bool)); } /* error */ @@ -544,8 +553,8 @@ fill_ident_view(Tuplestorestate *tuple_store, TupleDesc tupdesc) map_number++; fill_ident_line(tuple_store, tupdesc, map_number, - tok_line->line_num, identline, - tok_line->err_msg); + tok_line->file_name, tok_line->line_num, + identline, tok_line->err_msg); } /* Free tokenizer memory */ 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 0000000000..dbcdb3320f --- /dev/null +++ b/src/test/authentication/t/004_file_inclusion.pl @@ -0,0 +1,721 @@ + +# 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::Run qw(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 / + + +# 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) + { + return [ $filename, $fileline, $err_str ]; + } + else + { + return $fileline; + } + } + + # Get (and increment) the global rule number + $globline = $cur_line{'hba_rule'}++; + + # If caller provided an err_str, just returns the needed metadata + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + + # Otherwise, generate the expected pg_hba_file_rules line + @tokens = split(/ /, $payload); + $tokens[1] = '{' . $tokens[1] . '}'; # database + $tokens[2] = '{' . $tokens[2] . '}'; # user_name + + # add empty address and netmask betweed user_name and auth_method + splice @tokens, 3, 0, ''; + splice @tokens, 3, 0, ''; + + # append empty options and error + push @tokens, ''; + push @tokens, ''; + + # generate the expected final line + $line = ""; + $line .= "\n" if ($globline > 1); + $line .= "$globline|$data_dir/$filename|$fileline|"; + $line .= join('|', @tokens); + + return $line; +} + +# Add the given payload to the given relative ident file of the given node. +# Same as add_hba_line but for pg_ident files +sub add_ident_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) + { + return [ $filename, $fileline, $err_str ]; + } + else + { + return $fileline; + } + } + + # Get (and increment) the global rule number + $globline = $cur_line{'ident_rule'}++; + + # If caller provided an err_str, just returns the needed metadata + if (defined $err_str) + { + return [ $filename, $fileline, $err_str ]; + } + + # Otherwise, generate the expected pg_ident_file_mappings line + @tokens = split(/ /, $payload); + + # append empty error + push @tokens, ''; + + # generate the expected final line + $line = ""; + $line .= "\n" if ($globline > 1); + $line .= "$globline|$data_dir/$filename|$fileline|"; + $line .= join('|', @tokens); + + return $line; +} + +# Delete pg_hba.conf from the given node, add various entries to test the +# include infrastructure and then execute a reload to refresh it. +sub generate_valid_auth_files +{ + my $node = shift; + my $hba_expected = ''; + my $ident_expected = ''; + + # customise main auth file names + $node->safe_psql('postgres', + "ALTER SYSTEM SET hba_file = '$data_dir/$hba_file'"); + $node->safe_psql('postgres', + "ALTER SYSTEM SET ident_file = '$data_dir/$ident_file'"); + + # and make original ones invalid to be sure they're not used anywhere + $node->append_conf('pg_hba.conf', "some invalid line"); + $node->append_conf('pg_ident.conf', "some invalid line"); + + # pg_hba stuff + mkdir("$data_dir/subdir1"); + mkdir("$data_dir/hba_inc"); + mkdir("$data_dir/hba_inc_if"); + mkdir("$data_dir/hba_pos"); + + # Make sure we will still be able to connect + $hba_expected .= add_hba_line($node, "$hba_file", 'local all all trust'); + + # Add include data + add_hba_line($node, "$hba_file", "include ../pg_hba_pre.conf"); + $hba_expected .= + add_hba_line($node, 'pg_hba_pre.conf', "local pre all reject"); + + $hba_expected .= add_hba_line($node, "$hba_file", "local all all reject"); + + add_hba_line($node, "$hba_file", "include ../hba_pos/pg_hba_pos.conf"); + $hba_expected .= + add_hba_line($node, 'hba_pos/pg_hba_pos.conf', "local pos all reject"); + # include is relative to current path + add_hba_line($node, 'hba_pos/pg_hba_pos.conf', + "include pg_hba_pos2.conf"); + $hba_expected .= add_hba_line($node, 'hba_pos/pg_hba_pos2.conf', + "local pos2 all reject"); + + # include_if_exists data + add_hba_line($node, "$hba_file", "include_if_exists ../hba_inc_if/none"); + add_hba_line($node, "$hba_file", "include_if_exists ../hba_inc_if/some"); + $hba_expected .= + add_hba_line($node, 'hba_inc_if/some', "local if_some all reject"); + + # include_dir data + add_hba_line($node, "$hba_file", "include_dir ../hba_inc"); + add_hba_line($node, 'hba_inc/garbageconf', + "ignore - should not be included"); + $hba_expected .= + add_hba_line($node, 'hba_inc/01_z.conf', "local dir_z all reject"); + $hba_expected .= + add_hba_line($node, 'hba_inc/02_a.conf', "local dir_a all reject"); + + # secondary auth file + add_hba_line($node, $hba_file, 'local @../dbnames.conf all reject'); + $node->append_conf('dbnames.conf', "db1"); + $node->append_conf('dbnames.conf', "db3"); + $hba_expected .= "\n" + . ($cur_line{'hba_rule'} - 1) + . "|$data_dir/$hba_file|" + . ($cur_line{$hba_file} - 1) + . '|local|{db1,db3}|{all}|||reject||'; + + # pg_ident stuff + mkdir("$data_dir/subdir2"); + mkdir("$data_dir/ident_inc"); + mkdir("$data_dir/ident_inc_if"); + mkdir("$data_dir/ident_pos"); + + # Add include data + add_ident_line($node, "$ident_file", "include ../pg_ident_pre.conf"); + $ident_expected .= + add_ident_line($node, 'pg_ident_pre.conf', "pre foo bar"); + + $ident_expected .= add_ident_line($node, "$ident_file", "test a b"); + + add_ident_line($node, "$ident_file", + "include ../ident_pos/pg_ident_pos.conf"); + $ident_expected .= + add_ident_line($node, 'ident_pos/pg_ident_pos.conf', "pos foo bar"); + # include is relative to current path + add_ident_line($node, 'ident_pos/pg_ident_pos.conf', + "include pg_ident_pos2.conf"); + $ident_expected .= + add_ident_line($node, 'ident_pos/pg_ident_pos2.conf', "pos2 foo bar"); + + # include_if_exists data + add_ident_line($node, "$ident_file", + "include_if_exists ../ident_inc_if/none"); + add_ident_line($node, "$ident_file", + "include_if_exists ../ident_inc_if/some"); + $ident_expected .= + add_ident_line($node, 'ident_inc_if/some', "if_some foo bar"); + + # include_dir data + add_ident_line($node, "$ident_file", "include_dir ../ident_inc"); + add_ident_line($node, 'ident_inc/garbageconf', + "ignore - should not be included"); + $ident_expected .= + add_ident_line($node, 'ident_inc/01_z.conf', "dir_z foo bar"); + $ident_expected .= + add_ident_line($node, 'ident_inc/02_a.conf', "dir_a foo bar"); + + $node->restart; + $node->connect_ok('dbname=postgres', + 'Connection ok after generating valid auth files'); + + return ($hba_expected, $ident_expected); +} + +# Delete pg_hba.conf and pg_ident.conf from the given node and add minimal +# entries to allow authentication. +sub reset_auth_files +{ + my $node = shift; + + unlink("$data_dir/$hba_file"); + unlink("$data_dir/$ident_file"); + + %cur_line = ('hba_rule' => 1, 'ident_rule' => 1); + + return add_hba_line($node, "$hba_file", 'local all all trust'); +} + +# Generate a list of expected error regex for the given array of error +# conditions, as generated by add_hba_line/add_ident_line with an err_str. +# +# 2 regex are generated per array entry: one for the given err_str, and one for +# the expected line in the specific file. Since all lines are independant, +# there's no guarantee that a specific failure regex and the per-line regex +# will match the same error. Calling code should add at least one test with a +# single error to make sure that the line number / file name is correct. +# +# On top of that, an extra line is generated for the general failure to process +# the main auth file. +sub generate_log_err_patterns +{ + my $node = shift; + my $raw_errors = shift; + my $is_hba_err = shift; + my @errors; + + foreach my $arr (@{$raw_errors}) + { + my $filename = @{$arr}[0]; + my $fileline = @{$arr}[1]; + my $err_str = @{$arr}[2]; + + push @errors, qr/$err_str/; + + # Context messages with the file / line location aren't always emitted + if ( $err_str !~ /maximum nesting depth exceeded/ + and $err_str !~ /could not open file/) + { + push @errors, + qr/line $fileline of configuration file "$data_dir\/$filename"/; + } + } + + push @errors, qr/could not load $data_dir\/$hba_file/ if ($is_hba_err); + + return \@errors; +} + +# 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 +{ + my $node = shift; + my $raw_errors = shift; + my $exp_rows = ''; + + foreach my $arr (@{$raw_errors}) + { + my $filename = @{$arr}[0]; + my $fileline = @{$arr}[1]; + my $err_str = @{$arr}[2]; + + $exp_rows .= "\n" if ($exp_rows ne ""); + + # Unescape regex patterns if any + $err_str =~ s/\\([\(\)])/$1/g; + $exp_rows .= "|$data_dir\/$filename|$fileline|$err_str"; + } + + return $exp_rows; +} + +# Reset the main auth files, append the given payload to the given config file, +# and check that the instance cannot start, raising the expected error line(s). +sub start_errors_like +{ + my $node = shift; + my $file = shift; + my $payload = shift; + my $pattern = shift; + my $should_fail = shift; + + reset_auth_files($node); + $node->append_conf($file, $payload); + + unlink($node->logfile); + my $ret = + PostgreSQL::Test::Utils::system_log('pg_ctl', '-D', $data_dir, + '-l', $node->logfile, 'start'); + + if ($should_fail) + { + ok($ret != 0, "Cannot start postgres with faulty $file"); + } + else + { + ok($ret == 0, "postgres can start with faulty $file"); + } + + my $log_contents = slurp_file($node->logfile); + + foreach (@{$pattern}) + { + like($log_contents, $_, "Expected failure found in the logs"); + } + + if (not $should_fail) + { + # We can't simply call $node->stop here as the call is optimized out + # when the server isn't started with $node->start. + my $ret = + PostgreSQL::Test::Utils::system_log('pg_ctl', '-D', + $data_dir, 'stop', '-m', 'fast'); + ok($ret == 0, "Could stop postgres"); + } +} + +# We should be able to connect, and see an empty pg_ident.conf +is($node->psql('postgres', 'SELECT count(*) FROM pg_ident_file_mappings'), + qq(0), 'pg_ident.conf is empty'); + +############################################ +# part 1, test view reporting for valid data +############################################ +my ($exp_hba, $exp_ident) = generate_valid_auth_files($node); + +$node->connect_ok('dbname=postgres', 'Connection still ok'); + +is($node->safe_psql('postgres', 'SELECT * FROM pg_hba_file_rules'), + qq($exp_hba), 'pg_hba_file_rules content is expected'); + +is($node->safe_psql('postgres', 'SELECT * FROM pg_ident_file_mappings'), + qq($exp_ident), 'pg_ident_file_mappings content is expected'); + +############################################# +# part 2, test log reporting for invalid data +############################################# +reset_auth_files($node); +$node->restart('fast'); +$node->connect_ok('dbname=postgres', + 'Connection ok after resetting auth files'); + +$node->stop('fast'); + +start_errors_like( + $node, + $hba_file, + "include ../not_a_file", + [ + qr/could not open file "$data_dir\/not_a_file": No such file or directory/, + qr/could not load $data_dir\/$hba_file/ + ], + 1); + +# include_dir, single included file +mkdir("$data_dir/hba_inc_fail"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "local all all reject"); +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "not_a_token"); +start_errors_like( + $node, + $hba_file, + "include_dir ../hba_inc_fail", + [ + qr/invalid connection type "not_a_token"/, + qr/line 4 of configuration file "$data_dir\/hba_inc_fail\/inc_dir\.conf"/, + qr/could not load $data_dir\/$hba_file/ + ], + 1); + +# include_dir, single included file with nested inclusion +unlink("$data_dir/hba_inc_fail/inc_dir.conf"); +my @hba_raw_errors_step1; + +add_hba_line($node, "hba_inc_fail/inc_dir.conf", "include file1"); + +add_hba_line($node, "hba_inc_fail/file1", "include file2"); +add_hba_line($node, "hba_inc_fail/file2", "local all all reject"); +add_hba_line($node, "hba_inc_fail/file2", "include file3"); + +add_hba_line($node, "hba_inc_fail/file3", "local all all reject"); +add_hba_line($node, "hba_inc_fail/file3", "local all all reject"); +push @hba_raw_errors_step1, + add_hba_line( + $node, "hba_inc_fail/file3", + "local all all zuul", + 'invalid authentication method "zuul"'); + +start_errors_like( + $node, $hba_file, + "include_dir ../hba_inc_fail", + generate_log_err_patterns($node, \@hba_raw_errors_step1, 1), 1); + +# start_errors_like will reset the main auth files, so the previous error won't +# occur again. We keep it around as we will put back both bogus inclusions for +# the tests at step 3. +my @hba_raw_errors_step2; + +# include_if_exists, with various problems +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local", + "end-of-line before database specification"); +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local,host", + "multiple values specified for connection type"); +push @hba_raw_errors_step2, + add_hba_line($node, "hba_if_exists.conf", "local all", + "end-of-line before role specification"); +push @hba_raw_errors_step2, + add_hba_line( + $node, "hba_if_exists.conf", + "local all all", + "end-of-line before authentication method"); +push @hba_raw_errors_step2, + add_hba_line( + $node, "hba_if_exists.conf", + "host all all test/42", + 'specifying both host name and CIDR mask is invalid: "test/42"'); +push @hba_raw_errors_step2, + add_hba_line( + $node, + "hba_if_exists.conf", + 'local @dbnames_fails.conf all reject', + "could not open file \"$data_dir/dbnames_fails.conf\": No such file or directory" + ); + +add_hba_line($node, "hba_if_exists.conf", "include recurse.conf"); +push @hba_raw_errors_step2, + add_hba_line( + $node, + "recurse.conf", + "include recurse.conf", + "could not open file \"$data_dir/recurse.conf\": maximum nesting depth exceeded" + ); + +# Generate the regex for the expected errors in the logs. There's no guarantee +# that the generated "line X of file..." will be emitted for the expected line, +# but previous tests already ensured that the correct line number / file name +# was emitted, so ensuring that there's an error in all expected lines is +# enough here. +my $expected_errors = + generate_log_err_patterns($node, \@hba_raw_errors_step2, 1); + +# Not an error, but it should raise a message in the logs. Manually add an +# extra log message to detect +add_hba_line($node, "hba_if_exists.conf", "include_if_exists if_exists_none"); +push @{$expected_errors}, + qr/skipping missing authentication file "$data_dir\/if_exists_none"/; + +start_errors_like($node, $hba_file, "include_if_exists ../hba_if_exists.conf", + $expected_errors, 1); + +# Mostly the same, but for ident files +reset_auth_files($node); + +my @ident_raw_errors_step1; + +# include_dir, single included file with nested inclusion +mkdir("$data_dir/ident_inc_fail"); +add_ident_line($node, "ident_inc_fail/inc_dir.conf", "include file1"); + +add_ident_line($node, "ident_inc_fail/file1", "include file2"); +add_ident_line($node, "ident_inc_fail/file2", "ok ok ok"); +add_ident_line($node, "ident_inc_fail/file2", "include file3"); + +add_ident_line($node, "ident_inc_fail/file3", "ok ok ok"); +add_ident_line($node, "ident_inc_fail/file3", "ok ok ok"); +push @ident_raw_errors_step1, + add_ident_line( + $node, "ident_inc_fail/file3", + "failmap /(fail postgres", + 'invalid regular expression "\(fail": parentheses \(\) not balanced'); + +start_errors_like( + $node, $ident_file, + "include_dir ../ident_inc_fail", + generate_log_err_patterns($node, \@ident_raw_errors_step1, 0), 0); + +# start_errors_like will reset the main auth files, so the previous error won't +# occur again. We keep it around as we will put back both bogus inclusions for +# the tests at step 3. +my @ident_raw_errors_step2; + +# include_if_exists, with various problems +push @ident_raw_errors_step2, + add_ident_line($node, "ident_if_exists.conf", "map", + "missing entry at end of line"); +push @ident_raw_errors_step2, + add_ident_line($node, "ident_if_exists.conf", "map1,map2", + "multiple values in ident field"); +push @ident_raw_errors_step2, + add_ident_line( + $node, + "ident_if_exists.conf", + 'map @osnames_fails.conf postgres', + "could not open file \"$data_dir/osnames_fails.conf\": No such file or directory" + ); + +add_ident_line($node, "ident_if_exists.conf", "include ident_recurse.conf"); +push @ident_raw_errors_step2, + add_ident_line( + $node, + "ident_recurse.conf", + "include ident_recurse.conf", + "could not open file \"$data_dir/ident_recurse.conf\": maximum nesting depth exceeded" + ); + +start_errors_like( + $node, $ident_file, "include_if_exists ../ident_if_exists.conf", + # There's no guarantee that the generated "line X of file..." will be + # emitted for the expected line, but previous tests already ensured that + # the correct line number / file name was emitted, so ensuring that there's + # an error in all expected lines is enough here. + generate_log_err_patterns($node, \@ident_raw_errors_step2, 0), + 0); + +##################################################### +# part 3, test reporting of various error scenario +# NOTE: this will be bypassed -DEXEC_BACKEND or win32 +##################################################### +reset_auth_files($node); + +$node->start; +$node->connect_ok('dbname=postgres', 'Can connect after an auth file reset'); + +is( $node->safe_psql( + 'postgres', + 'SELECT count(*) FROM pg_hba_file_rules WHERE error IS NOT NULL'), + qq(0), + 'No error expected in pg_hba_file_rules'); + +add_ident_line($node, $ident_file, ''); +is( $node->safe_psql( + 'postgres', + 'SELECT count(*) FROM pg_ident_file_mappings WHERE error IS NOT NULL' + ), + qq(0), + 'No error expected in pg_ident_file_mappings'); + +# The instance could be restarted and no error is detected. Now check if the +# build is compatible with the view error reporting (EXEC_BACKEND / win32 will +# fail when trying to connect as they always rely on the current auth files +# content) +my @hba_raw_errors; + +push @hba_raw_errors, + add_hba_line($node, $hba_file, "include ../not_a_file", + "could not open file \"$data_dir/not_a_file\": No such file or directory" + ); + +my ($stdout, $stderr); +my $cmdret = $node->psql( + 'postgres', 'SELECT 1', + stdout => \$stdout, + stderr => \$stderr); + +if ($cmdret != 0) +{ + # Connection failed. Bail out, but make sure to raise a failure if it + # didn't fail for the expected hba file modification. + like( + $stderr, + qr/connection to server.* failed: FATAL: could not load $data_dir\/$hba_file/, + "Connection failed due to loading an invalid hba file"); + + done_testing(); + diag( + "Build not compatible with auth file view error reporting, bail out.\n" + ); + exit; +} + +# Combine errors generated at step 2, in the same order. +$node->append_conf($hba_file, "include_dir ../hba_inc_fail"); +push @hba_raw_errors, @hba_raw_errors_step1; + +$node->append_conf($hba_file, "include_if_exists ../hba_if_exists.conf"); +push @hba_raw_errors, @hba_raw_errors_step2; + +my $hba_expected = generate_log_err_rows($node, \@hba_raw_errors); +is( $node->safe_psql( + 'postgres', + 'SELECT rule_number, file_name, line_number, error FROM pg_hba_file_rules' + . ' WHERE error IS NOT NULL ORDER BY rule_number'), + qq($hba_expected), + 'Detected all error in hba file'); + +# and do the same for pg_ident +my @ident_raw_errors; + +push @ident_raw_errors, + add_ident_line( + $node, + $ident_file, + "include ../not_a_file", + "could not open file \"$data_dir/not_a_file\": No such file or directory" + ); + +$node->append_conf($ident_file, "include_dir ../ident_inc_fail"); +push @ident_raw_errors, @ident_raw_errors_step1; + +$node->append_conf($ident_file, "include_if_exists ../ident_if_exists.conf"); +push @ident_raw_errors, @ident_raw_errors_step2; + +my $ident_expected = generate_log_err_rows($node, \@ident_raw_errors); +is( $node->safe_psql( + 'postgres', + 'SELECT map_number, file_name, line_number, error FROM pg_ident_file_mappings' + . ' WHERE error IS NOT NULL ORDER BY map_number'), + qq($ident_expected), + 'Detected all error in ident file'); + +done_testing(); diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 7c7adbc004..37c1c86473 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1339,6 +1339,7 @@ pg_group| SELECT pg_authid.rolname AS groname, FROM pg_authid WHERE (NOT pg_authid.rolcanlogin); pg_hba_file_rules| SELECT a.rule_number, + a.file_name, a.line_number, a.type, a.database, @@ -1348,14 +1349,15 @@ pg_hba_file_rules| SELECT a.rule_number, a.auth_method, a.options, a.error - FROM pg_hba_file_rules() a(rule_number, line_number, type, database, user_name, address, netmask, auth_method, options, error); + FROM pg_hba_file_rules() a(rule_number, file_name, line_number, type, database, user_name, address, netmask, auth_method, options, error); pg_ident_file_mappings| SELECT a.map_number, + a.file_name, a.line_number, a.map_name, a.sys_name, a.pg_username, a.error - FROM pg_ident_file_mappings() a(map_number, line_number, map_name, sys_name, pg_username, error); + FROM pg_ident_file_mappings() a(map_number, file_name, line_number, map_name, sys_name, pg_username, error); pg_indexes| SELECT n.nspname AS schemaname, c.relname AS tablename, i.relname AS indexname, diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 32d5d45863..e5815a5390 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -89,8 +89,8 @@ </para> <para> - Each record specifies a connection type, a client IP address range - (if relevant for the connection type), a database name, a user name, + 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 @@ -100,20 +100,38 @@ access is denied. </para> + <para> + Each record can be an inclusion directive or an authentication record. + Inclusion directives specify files that can be included, that contain + additional records. The records will be inserted in place of the + inclusion records. Those records only contains two fields: + <literal>include</literal>, <literal>include_if_exists</literal> or + <literal>include_dir</literal> 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. For the <literal>include_dir</literal> form, all files + not starting with a <literal>.</literal> and ending with + <literal>.conf</literal> will be included. Multiple files within an include + directory are processed in file name order (according to C locale rules, + i.e., numbers before letters, and uppercase letters before lowercase ones). + </para> + <para> A record can have several formats: <synopsis> -local <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -host <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostssl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostnossl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostgssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -host <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostssl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostnossl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostgssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> -hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +local <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +host <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostssl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostnossl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostgssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>address</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +host <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostssl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostnossl <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostgssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +hostnogssenc <replaceable>database</replaceable> <replaceable>user</replaceable> <replaceable>IP-address</replaceable> <replaceable>IP-mask</replaceable> <replaceable>auth-method</replaceable> <optional><replaceable>auth-options</replaceable></optional> +include <replaceable>file</replaceable> +include_if_exists <replaceable>file</replaceable> +include_dir <replaceable>directory</replaceable> </synopsis> The meaning of the fields is as follows: @@ -655,6 +673,39 @@ openssl x509 -in myclient.crt -noout --subject -nameopt RFC2253 | sed "s/^subjec </para> </listitem> </varlistentry> + + <varlistentry> + <term><literal>include</literal></term> + <listitem> + <para> + This line will be replaced by the contents of the given file. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>include_if_exists</literal></term> + <listitem> + <para> + This line will be replaced with the content of the given file if the + file exists and can be read. Otherwise, a message will be logged to + indicate that the file is skipped. + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><literal>include_dir</literal></term> + <listitem> + <para> + This line will be replaced with the content of all the files found in + the directory, if they don't start with a <literal>.</literal> and end + with <literal>.conf</literal>, processed in file name order (according + to C locale rules, i.e., numbers before letters, and uppercase letters + before lowercase ones). + </para> + </listitem> + </varlistentry> </variablelist> </para> @@ -863,9 +914,11 @@ local db1,db2,@demodbs all md5 cluster's data directory. (It is possible to place the map file elsewhere, however; see the <xref linkend="guc-ident-file"/> configuration parameter.) - The ident map file contains lines of the general form: + The ident map file contains lines of two general form: <synopsis> <replaceable>map-name</replaceable> <replaceable>system-username</replaceable> <replaceable>database-username</replaceable> +<replaceable>include</replaceable> <replaceable>file</replaceable> +<replaceable>include_dir</replaceable> <replaceable>directory</replaceable> </synopsis> Comments, whitespace and line continuations are handled in the same way as in <filename>pg_hba.conf</filename>. The @@ -875,6 +928,10 @@ local db1,db2,@demodbs all md5 database user name. The same <replaceable>map-name</replaceable> can be used repeatedly to specify multiple user-mappings within a single map. </para> + <para> + As for <filename>pg_hba.conf</filename>, the lines in this file can + be inclusion directives, following the same rules. + </para> <para> There is no restriction regarding how many database users a given operating system user can correspond to, nor vice versa. Thus, entries diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml index 7c716fe327..d38b42c5cd 100644 --- a/doc/src/sgml/system-views.sgml +++ b/doc/src/sgml/system-views.sgml @@ -1002,12 +1002,21 @@ </para></entry> </row> + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>file_name</structfield> <type>text</type> + </para> + <para> + Name of the file containing this rule + </para></entry> + </row> + <row> <entry role="catalog_table_entry"><para role="column_definition"> <structfield>line_number</structfield> <type>int4</type> </para> <para> - Line number of this rule in <filename>pg_hba.conf</filename> + Line number of this rule in <literal>file_name</literal> </para></entry> </row> @@ -1152,12 +1161,21 @@ </para></entry> </row> + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>file_name</structfield> <type>text</type> + </para> + <para> + Name of the file containing this map + </para></entry> + </row> + <row> <entry role="catalog_table_entry"><para role="column_definition"> <structfield>line_number</structfield> <type>int4</type> </para> <para> - Line number of this map in <filename>pg_ident.conf</filename> + Line number of this map in <literal>file_name</literal> </para></entry> </row> -- 2.38.1
signature.asc
Description: PGP signature
