[WIP] Patch : Change pg_ident.conf parsing to be the same as pg_hba.conf
Attached is a Patch to change the parsing of pg_ident.conf to make it
similar to pg_hba.conf.
This is based on Todo Item:
http://archives.postgresql.org/pgsql-hackers/2011-06/msg02204.php
Purpose - This will allow to catch syntax errors in pg_ident at the startup
or reload time.
Changes are described as follows:
a. Make the load_ident() functionality same as load_hba, such that it
cleans the previous context, after successful parsing.
b. Change the load_ident(), so that parsing can be done during load
time and the parsed lines are saved.
c. Change the functionality of parse_ident_usermap() so that parsing is
not done during authentication.
d. If load_ident() fails for parsing, it returns false and error is
issued.
This point I am not sure, as for pg_hba failure it issues FATAL at
startup. Currently I have kept error handling for load of pg_ident same as
pg_hba
I have done the basic testing and test is still in progress.
Suggestions?
With Regards,
Amit Kapila.
Attachments:
pg_ident_parsing.patchapplication/octet-stream; name=pg_ident_parsing.patchDownload
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 828f6dc..653e641 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -82,9 +82,18 @@ static MemoryContext parsed_hba_context = NULL;
* line number for each line represented in ident_lines. ident_context is
* the memory context holding all this.
*/
-static List *ident_lines = NIL;
-static List *ident_line_nums = NIL;
-static MemoryContext ident_context = NULL;
+typedef struct IdentLine
+{
+ int linenumber;
+
+ char *usermap;
+ char *ident_user;
+ char *pg_role;
+ regex_t re;
+} IdentLine;
+
+static List *parsed_ident_lines = NIL;
+static MemoryContext parsed_ident_context = NULL;
static MemoryContext tokenize_file(const char *filename, FILE *file,
@@ -782,8 +791,7 @@ check_same_host_or_net(SockAddr *raddr, IPCompareMethod method)
(errcode(ERRCODE_CONFIG_FILE_ERROR), \
errmsg("missing entry in file \"%s\" at end of line %d", \
IdentFileName, line_number))); \
- *error_p = true; \
- return; \
+ return NULL; \
} \
} while (0);
@@ -794,8 +802,7 @@ check_same_host_or_net(SockAddr *raddr, IPCompareMethod method)
errmsg("multiple values in ident field"), \
errcontext("line %d of configuration file \"%s\"", \
line_number, IdentFileName))); \
- *error_p = true; \
- return; \
+ return NULL; \
} \
} while (0);
@@ -1794,34 +1801,36 @@ load_hba(void)
}
/*
- * Process one line from the ident config file.
+ * Parse one tokenised line from the ident config file and store the result in a
+ * IdentLine structure, or NULL if parsing fails.
+ *
+ * The tokenised ident_line is a triple-nested List of lines, fields
+ * and tokens.
*
- * Take the line and compare it to the needed map, pg_role and ident_user.
- * *found_p and *error_p are set according to our results.
+ * If the ident_user name contains the regular expression then compiled
+ * regular expression is stored in IdentLine structure and
+ * re-used in every connection
*/
-static void
-parse_ident_usermap(List *line, int line_number, const char *usermap_name,
- const char *pg_role, const char *ident_user,
- bool case_insensitive, bool *found_p, bool *error_p)
+
+static IdentLine *
+parse_ident_line(List *line, int line_number)
{
ListCell *field;
List *tokens;
HbaToken *token;
- char *file_map;
- char *file_pgrole;
- char *file_ident_user;
-
- *found_p = false;
- *error_p = false;
+ IdentLine *parsedline;
Assert(line != NIL);
field = list_head(line);
+ parsedline = palloc0(sizeof(IdentLine));
+ parsedline->linenumber = line_number;
+
/* Get the map token (must exist) */
tokens = lfirst(field);
IDENT_MULTI_VALUE(tokens);
token = linitial(tokens);
- file_map = token->string;
+ parsedline->usermap = pstrdup(token->string);
/* Get the ident user token */
field = lnext(field);
@@ -1829,7 +1838,7 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
tokens = lfirst(field);
IDENT_MULTI_VALUE(tokens);
token = linitial(tokens);
- file_ident_user = token->string;
+ parsedline->ident_user = pstrdup(token->string);
/* Get the PG rolename token */
field = lnext(field);
@@ -1837,14 +1846,10 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
tokens = lfirst(field);
IDENT_MULTI_VALUE(tokens);
token = linitial(tokens);
- file_pgrole = token->string;
-
- if (strcmp(file_map, usermap_name) != 0)
- /* Line does not match the map name we're looking for, so just abort */
- return;
+ parsedline->pg_role = pstrdup(token->string);
/* Match? */
- if (file_ident_user[0] == '/')
+ if (parsedline->ident_user[0] == '/')
{
/*
* When system username starts with a slash, treat it as a regular
@@ -1853,41 +1858,77 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
* for \1 in the database username string, if present.
*/
int r;
- regex_t re;
- regmatch_t matches[2];
pg_wchar *wstr;
int wlen;
- char *ofs;
- char *regexp_pgrole;
- wstr = palloc((strlen(file_ident_user + 1) + 1) * sizeof(pg_wchar));
- wlen = pg_mb2wchar_with_len(file_ident_user + 1, wstr, strlen(file_ident_user + 1));
+ wstr = palloc((strlen(parsedline->ident_user + 1) + 1) * sizeof(pg_wchar));
+ wlen = pg_mb2wchar_with_len(parsedline->ident_user + 1,
+ wstr, strlen(parsedline->ident_user + 1));
/*
- * XXX: Major room for optimization: regexps could be compiled when
- * the file is loaded and then re-used in every connection.
+ * Regular expression are compiled when the file is loaded
+ * and then re-used in every connection.
*/
- r = pg_regcomp(&re, wstr, wlen, REG_ADVANCED, C_COLLATION_OID);
+ r = pg_regcomp(&parsedline->re, wstr, wlen, REG_ADVANCED, C_COLLATION_OID);
if (r)
{
char errstr[100];
- pg_regerror(r, &re, errstr, sizeof(errstr));
+ pg_regerror(r, &parsedline->re, errstr, sizeof(errstr));
ereport(LOG,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
errmsg("invalid regular expression \"%s\": %s",
- file_ident_user + 1, errstr)));
+ parsedline->ident_user + 1, errstr)));
pfree(wstr);
- *error_p = true;
- return;
+ return NULL;
}
pfree(wstr);
+ }
+
+ return parsedline;
+}
+
+/*
+ * Process one line from the parsed ident config lines.
+ *
+ * Compare input parsed ident line to the needed map, pg_role and ident_user.
+ * *found_p and *error_p are set according to our results.
+ */
+static void
+check_ident_usermap(IdentLine *identLine, const char *usermap_name,
+ const char *pg_role, const char *ident_user,
+ bool case_insensitive, bool *found_p, bool *error_p)
+{
+
+ *found_p = false;
+ *error_p = false;
+
+ if (strcmp(identLine->usermap, usermap_name) != 0)
+ /* Line does not match the map name we're looking for, so just abort */
+ return;
+
+ /* Match? */
+ if (identLine->ident_user[0] == '/')
+ {
+ /*
+ * When system username starts with a slash, treat it as a regular
+ * expression. In this case, we process the system username as a
+ * regular expression that returns exactly one match. This is replaced
+ * for \1 in the database username string, if present.
+ */
+ int r;
+ regmatch_t matches[2];
+ pg_wchar *wstr;
+ int wlen;
+ char *ofs;
+ char *regexp_pgrole;
+
wstr = palloc((strlen(ident_user) + 1) * sizeof(pg_wchar));
wlen = pg_mb2wchar_with_len(ident_user, wstr, strlen(ident_user));
- r = pg_regexec(&re, wstr, wlen, 0, NULL, 2, matches, 0);
+ r = pg_regexec(&identLine->re, wstr, wlen, 0, NULL, 2, matches, 0);
if (r)
{
char errstr[100];
@@ -1895,21 +1936,20 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
if (r != REG_NOMATCH)
{
/* REG_NOMATCH is not an error, everything else is */
- pg_regerror(r, &re, errstr, sizeof(errstr));
+ pg_regerror(r, &identLine->re, errstr, sizeof(errstr));
ereport(LOG,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
errmsg("regular expression match for \"%s\" failed: %s",
- file_ident_user + 1, errstr)));
+ identLine->ident_user + 1, errstr)));
*error_p = true;
}
pfree(wstr);
- pg_regfree(&re);
return;
}
pfree(wstr);
- if ((ofs = strstr(file_pgrole, "\\1")) != NULL)
+ if ((ofs = strstr(identLine->pg_role, "\\1")) != NULL)
{
/* substitution of the first argument requested */
if (matches[1].rm_so < 0)
@@ -1917,8 +1957,7 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
ereport(LOG,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
errmsg("regular expression \"%s\" has no subexpressions as requested by backreference in \"%s\"",
- file_ident_user + 1, file_pgrole)));
- pg_regfree(&re);
+ identLine->ident_user + 1, identLine->pg_role)));
*error_p = true;
return;
}
@@ -1927,8 +1966,8 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
* length: original length minus length of \1 plus length of match
* plus null terminator
*/
- regexp_pgrole = palloc0(strlen(file_pgrole) - 2 + (matches[1].rm_eo - matches[1].rm_so) + 1);
- strncpy(regexp_pgrole, file_pgrole, (ofs - file_pgrole));
+ regexp_pgrole = palloc0(strlen(identLine->pg_role) - 2 + (matches[1].rm_eo - matches[1].rm_so) + 1);
+ strncpy(regexp_pgrole, identLine->pg_role, (ofs - identLine->pg_role));
memcpy(regexp_pgrole + strlen(regexp_pgrole),
ident_user + matches[1].rm_so,
matches[1].rm_eo - matches[1].rm_so);
@@ -1937,11 +1976,9 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
else
{
/* no substitution, so copy the match */
- regexp_pgrole = pstrdup(file_pgrole);
+ regexp_pgrole = pstrdup(identLine->pg_role);
}
- pg_regfree(&re);
-
/*
* now check if the username actually matched what the user is trying
* to connect as
@@ -1965,14 +2002,14 @@ parse_ident_usermap(List *line, int line_number, const char *usermap_name,
/* Not regular expression, so make complete match */
if (case_insensitive)
{
- if (pg_strcasecmp(file_pgrole, pg_role) == 0 &&
- pg_strcasecmp(file_ident_user, ident_user) == 0)
+ if (pg_strcasecmp(identLine->pg_role, pg_role) == 0 &&
+ pg_strcasecmp(identLine->ident_user, ident_user) == 0)
*found_p = true;
}
else
{
- if (strcmp(file_pgrole, pg_role) == 0 &&
- strcmp(file_ident_user, ident_user) == 0)
+ if (strcmp(identLine->pg_role, pg_role) == 0 &&
+ strcmp(identLine->ident_user, ident_user) == 0)
*found_p = true;
}
}
@@ -2021,13 +2058,12 @@ check_usermap(const char *usermap_name,
}
else
{
- ListCell *line_cell,
- *num_cell;
+ ListCell *line_cell;
- forboth(line_cell, ident_lines, num_cell, ident_line_nums)
+ foreach(line_cell, parsed_ident_lines)
{
- parse_ident_usermap(lfirst(line_cell), lfirst_int(num_cell),
- usermap_name, pg_role, auth_user, case_insensitive,
+ check_ident_usermap(lfirst(line_cell), usermap_name,
+ pg_role, auth_user, case_insensitive,
&found_entry, &error);
if (found_entry || error)
break;
@@ -2044,21 +2080,25 @@ check_usermap(const char *usermap_name,
/*
- * Read the ident config file and populate ident_lines and ident_line_nums.
+ * Read the ident config file and populate parsed_ident_lines.
*
- * Like parsed_hba_lines, ident_lines is a triple-nested List of lines, fields
- * and tokens.
+ * Like parsed_hba_lines, parsed_ident_lines is a List of lines IdentLine
*/
-void
+bool
load_ident(void)
{
FILE *file;
-
- if (ident_context != NULL)
- {
- MemoryContextDelete(ident_context);
- ident_context = NULL;
- }
+ List *ident_lines = NIL;
+ List *ident_line_nums = NIL;
+ ListCell *line_cell,
+ *num_cell,
+ *parsed_line_cell;
+ List *new_parsed_lines = NIL;
+ bool ok = true;
+ MemoryContext linecxt;
+ MemoryContext oldcxt;
+ MemoryContext ident_context;
+ IdentLine *newline;
file = AllocateFile(IdentFileName, "r");
if (file == NULL)
@@ -2068,13 +2108,89 @@ load_ident(void)
(errcode_for_file_access(),
errmsg("could not open usermap file \"%s\": %m",
IdentFileName)));
+
+ /*
+ * It is ok to continue if we fail to load the IDENT file.
+ */
+ return true;
}
- else
+
+ linecxt = tokenize_file(IdentFileName, file, &ident_lines, &ident_line_nums);
+ FreeFile(file);
+
+ /* Now parse all the lines */
+ ident_context = AllocSetContextCreate(TopMemoryContext,
+ "ident parser context",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ oldcxt = MemoryContextSwitchTo(ident_context);
+ forboth(line_cell, ident_lines, num_cell, ident_line_nums)
+ {
+ if ((newline = parse_ident_line(lfirst(line_cell), lfirst_int(num_cell))) == NULL)
+ {
+ /*
+ * Parse error in the file, so indicate there's a problem. NB: a
+ * problem in a line will free the memory for all previous lines
+ * as well!
+ */
+ foreach(parsed_line_cell, new_parsed_lines)
+ {
+ newline = (IdentLine *) lfirst(parsed_line_cell);
+ if (newline->ident_user[0] == '/')
+ pg_regfree(&newline->re);
+ }
+
+ MemoryContextReset(ident_context);
+ new_parsed_lines = NIL;
+ ok = false;
+
+ /*
+ * Keep parsing the rest of the file so we can report errors on
+ * more than the first row. Error has already been reported in the
+ * parsing function, so no need to log it here.
+ */
+ continue;
+ }
+
+ new_parsed_lines = lappend(new_parsed_lines, newline);
+ }
+
+ /* Free tokenizer memory */
+ MemoryContextDelete(linecxt);
+ MemoryContextSwitchTo(oldcxt);
+
+ if (!ok)
{
- ident_context = tokenize_file(IdentFileName, file, &ident_lines,
- &ident_line_nums);
- FreeFile(file);
+ /* File contained one or more errors, so bail out */
+ foreach(parsed_line_cell, new_parsed_lines)
+ {
+ newline = (IdentLine *) lfirst(parsed_line_cell);
+ if (newline->ident_user[0] == '/')
+ pg_regfree(&newline->re);
+ }
+ MemoryContextDelete(ident_context);
+ return false;
}
+
+ if (parsed_ident_lines != NULL)
+ {
+ foreach(parsed_line_cell, parsed_ident_lines)
+ {
+ newline = (IdentLine *) lfirst(parsed_line_cell);
+ if (newline->ident_user[0] == '/')
+ pg_regfree(&newline->re);
+ }
+ }
+
+ /* Loaded new file successfully, replace the one we use */
+ if (parsed_ident_context != NULL)
+ MemoryContextDelete(parsed_ident_context);
+
+ parsed_ident_context = ident_context;
+ parsed_ident_lines = new_parsed_lines;
+
+ return true;
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 913734f..1d1de8d 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1096,7 +1096,12 @@ PostmasterMain(int argc, char *argv[])
ereport(FATAL,
(errmsg("could not load pg_hba.conf")));
}
- load_ident();
+ if (!load_ident())
+ {
+ ereport(FATAL,
+ (errmsg("could not load pg_ident.conf")));
+ }
+
/*
* Remove old temporary files. At this point there can be no other
@@ -2087,7 +2092,9 @@ SIGHUP_handler(SIGNAL_ARGS)
ereport(WARNING,
(errmsg("pg_hba.conf not reloaded")));
- load_ident();
+ if (!load_ident())
+ ereport(WARNING,
+ (errmsg("pg_ident.conf not reloaded")));
#ifdef EXEC_BACKEND
/* Update the starting-point file for future children */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 4d4a895..727e16d 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -197,7 +197,15 @@ PerformAuthentication(Port *port)
ereport(FATAL,
(errmsg("could not load pg_hba.conf")));
}
- load_ident();
+
+ if (!load_ident())
+ {
+ /*
+ * It is ok to continue if we fail to load the IDENT file.
+ */
+ ereport(FATAL,
+ (errmsg("could not load pg_ident.conf")));
+ }
#endif
/*
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index f3b8be6..2830a59 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -82,11 +82,12 @@ typedef struct HbaLine
int radiusport;
} HbaLine;
+
/* kluge to avoid including libpq/libpq-be.h here */
typedef struct Port hbaPort;
extern bool load_hba(void);
-extern void load_ident(void);
+extern bool load_ident(void);
extern void hba_getauthmethod(hbaPort *port);
extern int check_usermap(const char *usermap_name,
const char *pg_role, const char *auth_user,
On 02.07.2012 15:08, Amit Kapila wrote:
Attached is a Patch to change the parsing of pg_ident.conf to make it
similar to pg_hba.conf.
This is based on Todo Item:
http://archives.postgresql.org/pgsql-hackers/2011-06/msg02204.phpPurpose - This will allow to catch syntax errors in pg_ident at the startup
or reload time.Changes are described as follows:
a. Make the load_ident() functionality same as load_hba, such that it
cleans the previous context, after successful parsing.
b. Change the load_ident(), so that parsing can be done during load
time and the parsed lines are saved.
c. Change the functionality of parse_ident_usermap() so that parsing is
not done during authentication.
d. If load_ident() fails for parsing, it returns false and error is
issued.
Looks good to me, committed with some small cleanup.
This point I am not sure, as for pg_hba failure it issues FATAL at
startup. Currently I have kept error handling for load of pg_ident same as
pg_hba
I think we should be more lenient with pg_ident.conf, and behave as if
the file was empty. That is the old behavior, and it seems sensible. You
can still connect using an authentication method that doesn't use
pg_ident.conf, but if pg_hba.conf is missing, you cannot log in at all.
Thanks!
- Heikki
On Friday, September 21, 2012 8:28 PM Heikki Linnakangas wrote:
On 02.07.2012 15:08, Amit Kapila wrote:
Attached is a Patch to change the parsing of pg_ident.conf to make it
similar to pg_hba.conf.
This is based on Todo Item:
http://archives.postgresql.org/pgsql-hackers/2011-06/msg02204.phpPurpose - This will allow to catch syntax errors in pg_ident at the startup
or reload time.
Looks good to me, committed with some small cleanup.
Thank you.
With Regards,
Amit Kapila.