[WIP] Patch : Change pg_ident.conf parsing to be the same as pg_hba.conf

Started by Amit Kapilaover 13 years ago3 messages
#1Amit Kapila
amit.kapila@huawei.com
1 attachment(s)

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,
#2Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Amit Kapila (#1)
Re: [WIP] Patch : Change pg_ident.conf parsing to be the same as pg_hba.conf

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

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

#3Amit kapila
amit.kapila@huawei.com
In reply to: Heikki Linnakangas (#2)
Re: [WIP] Patch : Change pg_ident.conf parsing to be the same as pg_hba.conf

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

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