Reporting hba lines

Started by Magnus Haganderover 13 years ago12 messages
#1Magnus Hagander
magnus@hagander.net
1 attachment(s)

When debugging strange and complex pg_hba lines, it can often be quite
useful to know which line is matching a particular connection that
failed for some reason. Because more often than not, it's actually not
using the line in pg_hba.conf that's expected.

The easiest way to do this is to emit an errdetail for the login
failure, per this patch.

Question is - is that leaking information to the client that we
shouldn't be leaking?

And if it is, what would be the preferred way to deal with it? We
could put that as a detail to basically every single error message
coming out of the auth system, but that seems like a bad idea. Or we
could make a separate ereport(LOG) before send it to the client,
perhaps?

Thoughts?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Attachments:

hba_line.patchapplication/octet-stream; name=hba_line.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 9cdee2b..2d1020a 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -299,7 +299,8 @@ auth_failed(Port *port, int status)
 
 	ereport(FATAL,
 			(errcode(errcode_return),
-			 errmsg(errstr, port->user_name)));
+			 errmsg(errstr, port->user_name),
+			 errdetail("matched pg_hba.conf line %d", port->hba->linenumber)));
 	/* doesn't return */
 }
 
#2Cédric Villemain
cedric@2ndquadrant.com
In reply to: Magnus Hagander (#1)
Re: Reporting hba lines

Le mercredi 27 juin 2012 14:54:15, Magnus Hagander a écrit :

When debugging strange and complex pg_hba lines, it can often be quite
useful to know which line is matching a particular connection that
failed for some reason. Because more often than not, it's actually not
using the line in pg_hba.conf that's expected.

The easiest way to do this is to emit an errdetail for the login
failure, per this patch.

Question is - is that leaking information to the client that we
shouldn't be leaking?

I fear it is exactly the problem. Too bad because the feature is interesting.

And if it is, what would be the preferred way to deal with it? We
could put that as a detail to basically every single error message
coming out of the auth system, but that seems like a bad idea. Or we
could make a separate ereport(LOG) before send it to the client,
perhaps?

The problem is also that 1/ you're not sure how you did connect exactly 2/
you're not connecting like you wanted to do. (my case with ipv4 vs ipv6 just
below). Providing more information on what we receive from client and tell it
to the client sounds good, no ?

Thoughts?

I just consume some moment before fixing an ipv6 vs ipv4 login failure. The
matched line from pg_hba .conf would have been very useful.

Maybe it is enough to report what happens on the connection: from which host,
TCP/socket (and which one, given that we should have multiple option here
soon), dbname, user, .... inspecting pg_hba.conf will remain difficult if map is
used or some other +group option.

--
Cédric Villemain +33 (0)6 20 30 22 52
http://2ndQuadrant.fr/
PostgreSQL: Support 24x7 - Développement, Expertise et Formation

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#1)
Re: Reporting hba lines

Magnus Hagander <magnus@hagander.net> writes:

When debugging strange and complex pg_hba lines, it can often be quite
useful to know which line is matching a particular connection that
failed for some reason. Because more often than not, it's actually not
using the line in pg_hba.conf that's expected.

The easiest way to do this is to emit an errdetail for the login
failure, per this patch.

Question is - is that leaking information to the client that we
shouldn't be leaking?

Yes.

And if it is, what would be the preferred way to deal with it?

Report to the postmaster log only. errdetail_log should do.

BTW, are you sure that auth_failed is only called in cases where
an hba line has already been identified? Even if true today,
it seems fairly risky to assume that.

regards, tom lane

#4Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#3)
1 attachment(s)
Re: Reporting hba lines

On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

When debugging strange and complex pg_hba lines, it can often be quite
useful to know which line is matching a particular connection that
failed for some reason. Because more often than not, it's actually not
using the line in pg_hba.conf that's expected.

The easiest way to do this is to emit an errdetail for the login
failure, per this patch.

Question is - is that leaking information to the client that we
shouldn't be leaking?

Yes.

And if it is, what would be the preferred way to deal with it?

Report to the postmaster log only.  errdetail_log should do.

Oh, I wasn't aware we had that :) You learn something new every day.

BTW, are you sure that auth_failed is only called in cases where
an hba line has already been identified?  Even if true today,
it seems fairly risky to assume that.

It is true today, but yes, it might be safe to guard against it with
something like this?

I also fixed the error message to follow the guidelines better - I think :)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Attachments:

hba_line.patchapplication/octet-stream; name=hba_line.patchDownload
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 9cdee2b..7975d2d 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -297,9 +297,16 @@ auth_failed(Port *port, int status)
 			break;
 	}
 
-	ereport(FATAL,
-			(errcode(errcode_return),
-			 errmsg(errstr, port->user_name)));
+	if (port->hba)
+		ereport(FATAL,
+				(errcode(errcode_return),
+				 errmsg(errstr, port->user_name),
+				 errdetail_log("Connection matched pg_hba.conf line %d", port->hba->linenumber)));
+	else
+		ereport(FATAL,
+				(errcode(errcode_return),
+				 errmsg(errstr, port->user_name)));
+
 	/* doesn't return */
 }
 
#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#4)
Re: Reporting hba lines

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, are you sure that auth_failed is only called in cases where
an hba line has already been identified? Even if true today,
it seems fairly risky to assume that.

It is true today, but yes, it might be safe to guard against it with
something like this?

FWIW, the usual approach for conditionally emitting bits of an ereport
is more like

ereport(FATAL,
(errcode(errcode_return),
errmsg(errstr, port->user_name),
port->hba ? errdetail_log("Connection matched pg_hba.conf line %d", port->hba->linenumber) : 0));

but that's just a nitpick. A bigger issue is that I'm not convinced
that a line number will be tremendously helpful: it's easy to miscount
lines, and a line number will certainly not be helpful in the frequent
cases where people are modifying the wrong hba file. Can we show
the source text of the hba line?

regards, tom lane

#6Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#5)
Re: Reporting hba lines

On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jun 27, 2012 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, are you sure that auth_failed is only called in cases where
an hba line has already been identified?  Even if true today,
it seems fairly risky to assume that.

It is true today, but yes, it might be safe to guard against it with
something like this?

FWIW, the usual approach for conditionally emitting bits of an ereport
is more like

       ereport(FATAL,
               (errcode(errcode_return),
                errmsg(errstr, port->user_name),
                port->hba ? errdetail_log("Connection matched pg_hba.conf line %d", port->hba->linenumber) : 0));

Hmm. Ok. So it treats a 0/NULL there as a way to ignore it. I tried
something with the NULL inside the errdetail, which obviously failed.

but that's just a nitpick.  A bigger issue is that I'm not convinced
that a line number will be tremendously helpful: it's easy to miscount
lines, and a line number will certainly not be helpful in the frequent

Editors will help you count the lines, no? :-)

cases where people are modifying the wrong hba file.  Can we show
the source text of the hba line?

We don't currently keep the full source text around - but we certainly
could do that if we wanted to.

I'm not sure how much it helps - usually, you're going to end up on a
line that's completely irrelevant if you get the wrong hba file (e.g.
a comment or a line that's not even in the file at all due to size).
Maybe we should just include the *name* of the HBA file in the error
message?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#6)
Re: Reporting hba lines

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

cases where people are modifying the wrong hba file. �Can we show
the source text of the hba line?

We don't currently keep the full source text around - but we certainly
could do that if we wanted to.

If we're concerned about providing a message like this, I think it'd be
well worthwhile. We presumably would only store the active lines not
comment lines, so the extra memory space would be negligible in just
about any real use-case.

I'm not sure how much it helps - usually, you're going to end up on a
line that's completely irrelevant if you get the wrong hba file (e.g.
a comment or a line that's not even in the file at all due to size).

Only if you count accurately, and aren't fooled into miscounting by the
expectation that you must arrive on a non-comment line. I don't buy the
"the editor can do it for you" argument either, at least not since
noticing that recent versions of emacs don't count lines the way I do.

regards, tom lane

#8Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#7)
1 attachment(s)
Re: Reporting hba lines

On Wed, Jun 27, 2012 at 4:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

On Wed, Jun 27, 2012 at 4:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

cases where people are modifying the wrong hba file.  Can we show
the source text of the hba line?

We don't currently keep the full source text around - but we certainly
could do that if we wanted to.

If we're concerned about providing a message like this, I think it'd be
well worthwhile.  We presumably would only store the active lines not
comment lines, so the extra memory space would be negligible in just
about any real use-case.

Turned out to be a bit more work than I thought, since the current
parser reads pg_hba byte by byte, and not line by line. So I had to
change that. See attached, seems reasonable?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Attachments:

hba_line2.patchapplication/octet-stream; name=hba_line2.patchDownload
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 297,305 **** auth_failed(Port *port, int status)
  			break;
  	}
  
! 	ereport(FATAL,
! 			(errcode(errcode_return),
! 			 errmsg(errstr, port->user_name)));
  	/* doesn't return */
  }
  
--- 297,312 ----
  			break;
  	}
  
! 	if (port->hba)
! 		ereport(FATAL,
! 				(errcode(errcode_return),
! 				 errmsg(errstr, port->user_name),
! 				 errdetail_log("Connection matched pg_hba.conf line %d: \"%s\"", port->hba->linenumber, port->hba->rawline)));
! 	else
! 		ereport(FATAL,
! 				(errcode(errcode_return),
! 				 errmsg(errstr, port->user_name)));
! 
  	/* doesn't return */
  }
  
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 42,47 ****
--- 42,48 ----
  #define atoxid(x)  ((TransactionId) strtoul((x), NULL, 10))
  
  #define MAX_TOKEN	256
+ #define MAX_LINE	8192
  
  /* callback data for check_network_callback */
  typedef struct check_network_data
***************
*** 88,94 **** static MemoryContext ident_context = NULL;
  
  
  static MemoryContext tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums);
  static List *tokenize_inc_file(List *tokens, const char *outer_filename,
  				  const char *inc_filename);
  static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
--- 89,95 ----
  
  
  static MemoryContext tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums, List **raw_lines);
  static List *tokenize_inc_file(List *tokens, const char *outer_filename,
  				  const char *inc_filename);
  static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
***************
*** 106,112 **** pg_isblank(const char c)
  
  
  /*
!  * Grab one token out of fp. Tokens are strings of non-blank
   * characters bounded by blank characters, commas, beginning of line, and
   * end of line. Blank means space or tab. Tokens can be delimited by
   * double quotes (this allows the inclusion of blanks, but not newlines).
--- 107,114 ----
  
  
  /*
!  * Grab one token out of the string pointed to by lineptr.
!  * Tokens are strings of non-blank
   * characters bounded by blank characters, commas, beginning of line, and
   * end of line. Blank means space or tab. Tokens can be delimited by
   * double quotes (this allows the inclusion of blanks, but not newlines).
***************
*** 129,135 **** pg_isblank(const char c)
   * Handle comments.
   */
  static bool
! next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  		   bool *terminating_comma)
  {
  	int			c;
--- 131,137 ----
   * Handle comments.
   */
  static bool
! next_token(char **lineptr, char *buf, int bufsz, bool *initial_quote,
  		   bool *terminating_comma)
  {
  	int			c;
***************
*** 146,155 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  	*terminating_comma = false;
  
  	/* Move over initial whitespace and commas */
! 	while ((c = getc(fp)) != EOF && (pg_isblank(c) || c == ','))
  		;
  
! 	if (c == EOF || c == '\n')
  	{
  		*buf = '\0';
  		return false;
--- 148,157 ----
  	*terminating_comma = false;
  
  	/* Move over initial whitespace and commas */
! 	while ((c = (*(*lineptr)++)) != '\0' && (pg_isblank(c) || c == ','))
  		;
  
! 	if (c == '\0' || c == '\n')
  	{
  		*buf = '\0';
  		return false;
***************
*** 159,175 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  	 * Build a token in buf of next characters up to EOF, EOL, unquoted comma,
  	 * or unquoted whitespace.
  	 */
! 	while (c != EOF && c != '\n' &&
  		   (!pg_isblank(c) || in_quote))
  	{
  		/* skip comments to EOL */
  		if (c == '#' && !in_quote)
  		{
! 			while ((c = getc(fp)) != EOF && c != '\n')
  				;
  			/* If only comment, consume EOL too; return EOL */
! 			if (c != EOF && buf == start_buf)
! 				c = getc(fp);
  			break;
  		}
  
--- 161,177 ----
  	 * Build a token in buf of next characters up to EOF, EOL, unquoted comma,
  	 * or unquoted whitespace.
  	 */
! 	while (c != '\0' && c != '\n' &&
  		   (!pg_isblank(c) || in_quote))
  	{
  		/* skip comments to EOL */
  		if (c == '#' && !in_quote)
  		{
! 			while ((c = (*(*lineptr)++)) && c != '\n')
  				;
  			/* If only comment, consume EOL too; return EOL */
! 			if (c != '\0' && buf == start_buf)
! 				(*lineptr)++;
  			break;
  		}
  
***************
*** 181,188 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  			   errmsg("authentication file token too long, skipping: \"%s\"",
  					  start_buf)));
  			/* Discard remainder of line */
! 			while ((c = getc(fp)) != EOF && c != '\n')
! 				;
  			break;
  		}
  
--- 183,190 ----
  			   errmsg("authentication file token too long, skipping: \"%s\"",
  					  start_buf)));
  			/* Discard remainder of line */
! 			while ((c = (*(*lineptr)++)) && c != '\n')
! 				(*lineptr)++;
  			break;
  		}
  
***************
*** 210,224 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  				*initial_quote = true;
  		}
  
! 		c = getc(fp);
  	}
  
  	/*
  	 * Put back the char right after the token (critical in case it is EOL,
  	 * since we need to detect end-of-line at next call).
  	 */
! 	if (c != EOF)
! 		ungetc(c, fp);
  
  	*buf = '\0';
  
--- 212,227 ----
  				*initial_quote = true;
  		}
  
! 		c = (**lineptr);
! 		(*lineptr)++;
  	}
  
  	/*
  	 * Put back the char right after the token (critical in case it is EOL,
  	 * since we need to detect end-of-line at next call).
  	 */
! 	if (c != '\0')
! 		(*lineptr)--;
  
  	*buf = '\0';
  
***************
*** 253,265 **** copy_hba_token(HbaToken *in)
  
  
  /*
!  * Tokenize one HBA field from a file, handling file inclusion and comma lists.
   *
   * The result is a List of HbaToken structs for each individual token,
   * or NIL if we reached EOL.
   */
  static List *
! next_field_expand(const char *filename, FILE *file)
  {
  	char		buf[MAX_TOKEN];
  	bool		trailing_comma;
--- 256,268 ----
  
  
  /*
!  * Tokenize one HBA field from a line, handling file inclusion and comma lists.
   *
   * The result is a List of HbaToken structs for each individual token,
   * or NIL if we reached EOL.
   */
  static List *
! next_field_expand(const char *filename, char **lineptr)
  {
  	char		buf[MAX_TOKEN];
  	bool		trailing_comma;
***************
*** 268,274 **** next_field_expand(const char *filename, FILE *file)
  
  	do
  	{
! 		if (!next_token(file, buf, sizeof(buf), &initial_quote, &trailing_comma))
  			break;
  
  		/* Is this referencing a file? */
--- 271,277 ----
  
  	do
  	{
! 		if (!next_token(lineptr, buf, sizeof(buf), &initial_quote, &trailing_comma))
  			break;
  
  		/* Is this referencing a file? */
***************
*** 330,336 **** tokenize_inc_file(List *tokens,
  	}
  
  	/* There is possible recursion here if the file contains @ */
! 	linecxt = tokenize_file(inc_fullname, inc_file, &inc_lines, &inc_line_nums);
  
  	FreeFile(inc_file);
  	pfree(inc_fullname);
--- 333,339 ----
  	}
  
  	/* There is possible recursion here if the file contains @ */
! 	linecxt = tokenize_file(inc_fullname, inc_file, &inc_lines, &inc_line_nums, NULL);
  
  	FreeFile(inc_file);
  	pfree(inc_fullname);
***************
*** 372,378 **** tokenize_inc_file(List *tokens,
   */
  static MemoryContext
  tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums)
  {
  	List	   *current_line = NIL;
  	List	   *current_field = NIL;
--- 375,381 ----
   */
  static MemoryContext
  tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums, List **raw_lines)
  {
  	List	   *current_line = NIL;
  	List	   *current_field = NIL;
***************
*** 391,420 **** tokenize_file(const char *filename, FILE *file,
  
  	while (!feof(file) && !ferror(file))
  	{
! 		current_field = next_field_expand(filename, file);
  
! 		/* add tokens to list, unless we are at EOL or comment start */
! 		if (list_length(current_field) > 0)
  		{
! 			if (current_line == NIL)
! 			{
! 				/* make a new line List, record its line number */
! 				current_line = lappend(current_line, current_field);
! 				*lines = lappend(*lines, current_line);
! 				*line_nums = lappend_int(*line_nums, line_number);
! 			}
! 			else
  			{
! 				/* append tokens to current line's list */
! 				current_line = lappend(current_line, current_field);
  			}
  		}
! 		else
! 		{
! 			/* we are at real or logical EOL, so force a new line List */
! 			current_line = NIL;
! 			line_number++;
! 		}
  	}
  
  	MemoryContextSwitchTo(oldcxt);
--- 394,444 ----
  
  	while (!feof(file) && !ferror(file))
  	{
! 		char rawline[MAX_LINE];
! 		char *lineptr;
! 
! 		if (!fgets(rawline, sizeof(rawline), file))
! 			break;
! 		if (strlen(rawline) == MAX_LINE-1)
! 			/* Line too long! */
! 			ereport(ERROR,
! 					(errcode(ERRCODE_CONFIG_FILE_ERROR),
! 					 errmsg("authentication file line too long"),
! 					 errcontext("line %d of configuration file \"%s\"",
! 								line_number, filename)));
  
! 		lineptr = rawline;
! 		while (strlen(lineptr) > 0)
  		{
! 			current_field = next_field_expand(filename, &lineptr);
! 
! 			/* add tokens to list, unless we are at EOL or comment start */
! 			if (list_length(current_field) > 0)
  			{
! 				if (current_line == NIL)
! 				{
! 					/* make a new line List, record its line number */
! 					current_line = lappend(current_line, current_field);
! 					*lines = lappend(*lines, current_line);
! 					*line_nums = lappend_int(*line_nums, line_number);
! 					if (raw_lines)
! 					{
! 						/* We don't store the trailing newline */
! 						if (rawline[strlen(rawline)-1] == '\n')
! 							rawline[strlen(rawline)-1] = '\0';
! 						*raw_lines = lappend(*raw_lines, rawline);
! 					}
! 				}
! 				else
! 				{
! 					/* append tokens to current line's list */
! 					current_line = lappend(current_line, current_field);
! 				}
  			}
  		}
! 		/* we are at real or logical EOL, so force a new line List */
! 		current_line = NIL;
! 		line_number++;
  	}
  
  	MemoryContextSwitchTo(oldcxt);
***************
*** 812,818 **** check_same_host_or_net(SockAddr *raddr, IPCompareMethod method)
   * NULL.
   */
  static HbaLine *
! parse_hba_line(List *line, int line_num)
  {
  	char	   *str;
  	struct addrinfo *gai_result;
--- 836,842 ----
   * NULL.
   */
  static HbaLine *
! parse_hba_line(List *line, int line_num, char *raw_line)
  {
  	char	   *str;
  	struct addrinfo *gai_result;
***************
*** 828,833 **** parse_hba_line(List *line, int line_num)
--- 852,858 ----
  
  	parsedline = palloc0(sizeof(HbaLine));
  	parsedline->linenumber = line_num;
+ 	parsedline->rawline = pstrdup(raw_line);
  
  	/* Check the record type. */
  	field = list_head(line);
***************
*** 1705,1712 **** load_hba(void)
  	FILE	   *file;
  	List	   *hba_lines = NIL;
  	List	   *hba_line_nums = NIL;
  	ListCell   *line,
! 			   *line_num;
  	List	   *new_parsed_lines = NIL;
  	bool		ok = true;
  	MemoryContext linecxt;
--- 1730,1739 ----
  	FILE	   *file;
  	List	   *hba_lines = NIL;
  	List	   *hba_line_nums = NIL;
+ 	List	   *hba_raw_lines = NIL;
  	ListCell   *line,
! 			   *line_num,
! 			   *raw_line;
  	List	   *new_parsed_lines = NIL;
  	bool		ok = true;
  	MemoryContext linecxt;
***************
*** 1723,1729 **** load_hba(void)
  		return false;
  	}
  
! 	linecxt = tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums);
  	FreeFile(file);
  
  	/* Now parse all the lines */
--- 1750,1756 ----
  		return false;
  	}
  
! 	linecxt = tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums, &hba_raw_lines);
  	FreeFile(file);
  
  	/* Now parse all the lines */
***************
*** 1733,1743 **** load_hba(void)
  								   ALLOCSET_DEFAULT_MINSIZE,
  								   ALLOCSET_DEFAULT_MAXSIZE);
  	oldcxt = MemoryContextSwitchTo(hbacxt);
! 	forboth(line, hba_lines, line_num, hba_line_nums)
  	{
  		HbaLine    *newline;
  
! 		if ((newline = parse_hba_line(lfirst(line), lfirst_int(line_num))) == NULL)
  		{
  			/*
  			 * Parse error in the file, so indicate there's a problem.  NB: a
--- 1760,1770 ----
  								   ALLOCSET_DEFAULT_MINSIZE,
  								   ALLOCSET_DEFAULT_MAXSIZE);
  	oldcxt = MemoryContextSwitchTo(hbacxt);
! 	forthree(line, hba_lines, line_num, hba_line_nums, raw_line, hba_raw_lines)
  	{
  		HbaLine    *newline;
  
! 		if ((newline = parse_hba_line(lfirst(line), lfirst_int(line_num), lfirst(raw_line))) == NULL)
  		{
  			/*
  			 * Parse error in the file, so indicate there's a problem.  NB: a
***************
*** 2072,2078 **** load_ident(void)
  	else
  	{
  		ident_context = tokenize_file(IdentFileName, file, &ident_lines,
! 									  &ident_line_nums);
  		FreeFile(file);
  	}
  }
--- 2099,2105 ----
  	else
  	{
  		ident_context = tokenize_file(IdentFileName, file, &ident_lines,
! 									  &ident_line_nums, NULL);
  		FreeFile(file);
  	}
  }
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
***************
*** 52,57 **** typedef enum ConnType
--- 52,58 ----
  typedef struct HbaLine
  {
  	int			linenumber;
+ 	char	   *rawline;
  	ConnType	conntype;
  	List	   *databases;
  	List	   *roles;
#9Tom Lane
tgl@sss.pgh.pa.us
In reply to: Magnus Hagander (#8)
Re: Reporting hba lines

Magnus Hagander <magnus@hagander.net> writes:

Turned out to be a bit more work than I thought, since the current
parser reads pg_hba byte by byte, and not line by line. So I had to
change that. See attached, seems reasonable?

A couple of comments:

* In some places you have "if ((c = *(*lineptr)++) != '\0')" and in other
places just "if ((c = *(*lineptr)++))". This should be consistent (and
personally I prefer the first way).

* I'm not sure that this conversion is right:

! if (c != EOF)
! ungetc(c, fp);
---
! if (c != '\0')
! (*lineptr)--;

In the file case, it's impossible to push back EOF, and unnecessary
since another getc will produce EOF again anyway. In the string case,
though, I think you might need to decrement the lineptr unconditionally,
else next call will run off the end of the string no?

* This bit seems a bit ugly, and not Windows-aware either:

! /* We don't store the trailing newline */
! if (rawline[strlen(rawline)-1] == '\n')
! rawline[strlen(rawline)-1] = '\0';
!

It might be better to strip trailing \n and \r from the line immediately
upon read, rather than here.

regards, tom lane

#10Magnus Hagander
magnus@hagander.net
In reply to: Tom Lane (#9)
1 attachment(s)
Re: Reporting hba lines

On Fri, Jun 29, 2012 at 4:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Turned out to be a bit more work than I thought, since the current
parser reads pg_hba byte by byte, and not line by line. So I had to
change that. See attached, seems reasonable?

A couple of comments:

* In some places you have "if ((c = *(*lineptr)++) != '\0')" and in other
places just "if ((c = *(*lineptr)++))". This should be consistent (and
personally I prefer the first way).

* I'm not sure that this conversion is right:

! if (c != EOF)
! ungetc(c, fp);
---
! if (c != '\0')
! (*lineptr)--;

In the file case, it's impossible to push back EOF, and unnecessary
since another getc will produce EOF again anyway. In the string case,
though, I think you might need to decrement the lineptr unconditionally,
else next call will run off the end of the string no?

* This bit seems a bit ugly, and not Windows-aware either:

! /* We don't store the trailing newline */
! if (rawline[strlen(rawline)-1] == '\n')
! rawline[strlen(rawline)-1] = '\0';
!

It might be better to strip trailing \n and \r from the line immediately
upon read, rather than here.

I re-found this branch that I had completely forgotten about, when
cleaning up my reposiroty. oops.

Attached is an updated version of the patch, per the comments from Tom
and rebased on top of the current master. Since it's been a long time
ago, and some code churn in the area, another round of review is
probably a good thing...

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Attachments:

hba_line3.patchapplication/octet-stream; name=hba_line3.patchDownload
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 297,305 **** auth_failed(Port *port, int status)
  			break;
  	}
  
! 	ereport(FATAL,
! 			(errcode(errcode_return),
! 			 errmsg(errstr, port->user_name)));
  	/* doesn't return */
  }
  
--- 297,312 ----
  			break;
  	}
  
! 	if (port->hba)
! 		ereport(FATAL,
! 				(errcode(errcode_return),
! 				 errmsg(errstr, port->user_name),
! 				 errdetail_log("Connection matched pg_hba.conf line %d: \"%s\"", port->hba->linenumber, port->hba->rawline)));
! 	else
! 		ereport(FATAL,
! 				(errcode(errcode_return),
! 				 errmsg(errstr, port->user_name)));
! 
  	/* doesn't return */
  }
  
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 50,55 ****
--- 50,56 ----
  #define atoxid(x)  ((TransactionId) strtoul((x), NULL, 10))
  
  #define MAX_TOKEN	256
+ #define MAX_LINE	8192
  
  /* callback data for check_network_callback */
  typedef struct check_network_data
***************
*** 93,99 **** static MemoryContext parsed_ident_context = NULL;
  
  
  static MemoryContext tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums);
  static List *tokenize_inc_file(List *tokens, const char *outer_filename,
  				  const char *inc_filename);
  static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
--- 94,100 ----
  
  
  static MemoryContext tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums, List **raw_lines);
  static List *tokenize_inc_file(List *tokens, const char *outer_filename,
  				  const char *inc_filename);
  static bool parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
***************
*** 111,117 **** pg_isblank(const char c)
  
  
  /*
!  * Grab one token out of fp. Tokens are strings of non-blank
   * characters bounded by blank characters, commas, beginning of line, and
   * end of line. Blank means space or tab. Tokens can be delimited by
   * double quotes (this allows the inclusion of blanks, but not newlines).
--- 112,119 ----
  
  
  /*
!  * Grab one token out of the string pointed to by lineptr.
!  * Tokens are strings of non-blank
   * characters bounded by blank characters, commas, beginning of line, and
   * end of line. Blank means space or tab. Tokens can be delimited by
   * double quotes (this allows the inclusion of blanks, but not newlines).
***************
*** 134,140 **** pg_isblank(const char c)
   * Handle comments.
   */
  static bool
! next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  		   bool *terminating_comma)
  {
  	int			c;
--- 136,142 ----
   * Handle comments.
   */
  static bool
! next_token(char **lineptr, char *buf, int bufsz, bool *initial_quote,
  		   bool *terminating_comma)
  {
  	int			c;
***************
*** 151,160 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  	*terminating_comma = false;
  
  	/* Move over initial whitespace and commas */
! 	while ((c = getc(fp)) != EOF && (pg_isblank(c) || c == ','))
  		;
  
! 	if (c == EOF || c == '\n')
  	{
  		*buf = '\0';
  		return false;
--- 153,162 ----
  	*terminating_comma = false;
  
  	/* Move over initial whitespace and commas */
! 	while ((c = (*(*lineptr)++)) != '\0' && (pg_isblank(c) || c == ','))
  		;
  
! 	if (c == '\0' || c == '\n')
  	{
  		*buf = '\0';
  		return false;
***************
*** 164,180 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  	 * Build a token in buf of next characters up to EOF, EOL, unquoted comma,
  	 * or unquoted whitespace.
  	 */
! 	while (c != EOF && c != '\n' &&
  		   (!pg_isblank(c) || in_quote))
  	{
  		/* skip comments to EOL */
  		if (c == '#' && !in_quote)
  		{
! 			while ((c = getc(fp)) != EOF && c != '\n')
  				;
  			/* If only comment, consume EOL too; return EOL */
! 			if (c != EOF && buf == start_buf)
! 				c = getc(fp);
  			break;
  		}
  
--- 166,182 ----
  	 * Build a token in buf of next characters up to EOF, EOL, unquoted comma,
  	 * or unquoted whitespace.
  	 */
! 	while (c != '\0' && c != '\n' &&
  		   (!pg_isblank(c) || in_quote))
  	{
  		/* skip comments to EOL */
  		if (c == '#' && !in_quote)
  		{
! 			while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
  				;
  			/* If only comment, consume EOL too; return EOL */
! 			if (c != '\0' && buf == start_buf)
! 				(*lineptr)++;
  			break;
  		}
  
***************
*** 186,193 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  			   errmsg("authentication file token too long, skipping: \"%s\"",
  					  start_buf)));
  			/* Discard remainder of line */
! 			while ((c = getc(fp)) != EOF && c != '\n')
! 				;
  			break;
  		}
  
--- 188,195 ----
  			   errmsg("authentication file token too long, skipping: \"%s\"",
  					  start_buf)));
  			/* Discard remainder of line */
! 			while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
! 				(*lineptr)++;
  			break;
  		}
  
***************
*** 215,229 **** next_token(FILE *fp, char *buf, int bufsz, bool *initial_quote,
  				*initial_quote = true;
  		}
  
! 		c = getc(fp);
  	}
  
  	/*
  	 * Put back the char right after the token (critical in case it is EOL,
  	 * since we need to detect end-of-line at next call).
  	 */
! 	if (c != EOF)
! 		ungetc(c, fp);
  
  	*buf = '\0';
  
--- 217,231 ----
  				*initial_quote = true;
  		}
  
! 		c = (**lineptr);
! 		(*lineptr)++;
  	}
  
  	/*
  	 * Put back the char right after the token (critical in case it is EOL,
  	 * since we need to detect end-of-line at next call).
  	 */
! 	(*lineptr)--;
  
  	*buf = '\0';
  
***************
*** 258,270 **** copy_hba_token(HbaToken *in)
  
  
  /*
!  * Tokenize one HBA field from a file, handling file inclusion and comma lists.
   *
   * The result is a List of HbaToken structs for each individual token,
   * or NIL if we reached EOL.
   */
  static List *
! next_field_expand(const char *filename, FILE *file)
  {
  	char		buf[MAX_TOKEN];
  	bool		trailing_comma;
--- 260,272 ----
  
  
  /*
!  * Tokenize one HBA field from a line, handling file inclusion and comma lists.
   *
   * The result is a List of HbaToken structs for each individual token,
   * or NIL if we reached EOL.
   */
  static List *
! next_field_expand(const char *filename, char **lineptr)
  {
  	char		buf[MAX_TOKEN];
  	bool		trailing_comma;
***************
*** 273,279 **** next_field_expand(const char *filename, FILE *file)
  
  	do
  	{
! 		if (!next_token(file, buf, sizeof(buf), &initial_quote, &trailing_comma))
  			break;
  
  		/* Is this referencing a file? */
--- 275,281 ----
  
  	do
  	{
! 		if (!next_token(lineptr, buf, sizeof(buf), &initial_quote, &trailing_comma))
  			break;
  
  		/* Is this referencing a file? */
***************
*** 335,341 **** tokenize_inc_file(List *tokens,
  	}
  
  	/* There is possible recursion here if the file contains @ */
! 	linecxt = tokenize_file(inc_fullname, inc_file, &inc_lines, &inc_line_nums);
  
  	FreeFile(inc_file);
  	pfree(inc_fullname);
--- 337,343 ----
  	}
  
  	/* There is possible recursion here if the file contains @ */
! 	linecxt = tokenize_file(inc_fullname, inc_file, &inc_lines, &inc_line_nums, NULL);
  
  	FreeFile(inc_file);
  	pfree(inc_fullname);
***************
*** 377,383 **** tokenize_inc_file(List *tokens,
   */
  static MemoryContext
  tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums)
  {
  	List	   *current_line = NIL;
  	List	   *current_field = NIL;
--- 379,385 ----
   */
  static MemoryContext
  tokenize_file(const char *filename, FILE *file,
! 			  List **lines, List **line_nums, List **raw_lines)
  {
  	List	   *current_line = NIL;
  	List	   *current_field = NIL;
***************
*** 396,425 **** tokenize_file(const char *filename, FILE *file,
  
  	while (!feof(file) && !ferror(file))
  	{
! 		current_field = next_field_expand(filename, file);
  
! 		/* add tokens to list, unless we are at EOL or comment start */
! 		if (list_length(current_field) > 0)
  		{
! 			if (current_line == NIL)
! 			{
! 				/* make a new line List, record its line number */
! 				current_line = lappend(current_line, current_field);
! 				*lines = lappend(*lines, current_line);
! 				*line_nums = lappend_int(*line_nums, line_number);
! 			}
! 			else
  			{
! 				/* append tokens to current line's list */
! 				current_line = lappend(current_line, current_field);
  			}
  		}
! 		else
! 		{
! 			/* we are at real or logical EOL, so force a new line List */
! 			current_line = NIL;
! 			line_number++;
! 		}
  	}
  
  	MemoryContextSwitchTo(oldcxt);
--- 398,448 ----
  
  	while (!feof(file) && !ferror(file))
  	{
! 		char rawline[MAX_LINE];
! 		char *lineptr;
! 
! 		if (!fgets(rawline, sizeof(rawline), file))
! 			break;
! 		if (strlen(rawline) == MAX_LINE-1)
! 			/* Line too long! */
! 			ereport(ERROR,
! 					(errcode(ERRCODE_CONFIG_FILE_ERROR),
! 					 errmsg("authentication file line too long"),
! 					 errcontext("line %d of configuration file \"%s\"",
! 								line_number, filename)));
  
! 		/* Strip trailing linebreak from rawline */
! 		while (rawline[strlen(rawline)-1] == '\n' ||
! 			   rawline[strlen(rawline)-1] == '\r')
! 			rawline[strlen(rawline)-1] = '\0';
! 
! 		lineptr = rawline;
! 		while (strlen(lineptr) > 0)
  		{
! 			current_field = next_field_expand(filename, &lineptr);
! 
! 			/* add tokens to list, unless we are at EOL or comment start */
! 			if (list_length(current_field) > 0)
  			{
! 				if (current_line == NIL)
! 				{
! 					/* make a new line List, record its line number */
! 					current_line = lappend(current_line, current_field);
! 					*lines = lappend(*lines, current_line);
! 					*line_nums = lappend_int(*line_nums, line_number);
! 					if (raw_lines)
! 						*raw_lines = lappend(*raw_lines, pstrdup(rawline));
! 				}
! 				else
! 				{
! 					/* append tokens to current line's list */
! 					current_line = lappend(current_line, current_field);
! 				}
  			}
  		}
! 		/* we are at real or logical EOL, so force a new line List */
! 		current_line = NIL;
! 		line_number++;
  	}
  
  	MemoryContextSwitchTo(oldcxt);
***************
*** 815,821 **** check_same_host_or_net(SockAddr *raddr, IPCompareMethod method)
   * NULL.
   */
  static HbaLine *
! parse_hba_line(List *line, int line_num)
  {
  	char	   *str;
  	struct addrinfo *gai_result;
--- 838,844 ----
   * NULL.
   */
  static HbaLine *
! parse_hba_line(List *line, int line_num, char *raw_line)
  {
  	char	   *str;
  	struct addrinfo *gai_result;
***************
*** 831,836 **** parse_hba_line(List *line, int line_num)
--- 854,860 ----
  
  	parsedline = palloc0(sizeof(HbaLine));
  	parsedline->linenumber = line_num;
+ 	parsedline->rawline = pstrdup(raw_line);
  
  	/* Check the record type. */
  	field = list_head(line);
***************
*** 1761,1768 **** load_hba(void)
  	FILE	   *file;
  	List	   *hba_lines = NIL;
  	List	   *hba_line_nums = NIL;
  	ListCell   *line,
! 			   *line_num;
  	List	   *new_parsed_lines = NIL;
  	bool		ok = true;
  	MemoryContext linecxt;
--- 1785,1794 ----
  	FILE	   *file;
  	List	   *hba_lines = NIL;
  	List	   *hba_line_nums = NIL;
+ 	List	   *hba_raw_lines = NIL;
  	ListCell   *line,
! 			   *line_num,
! 			   *raw_line;
  	List	   *new_parsed_lines = NIL;
  	bool		ok = true;
  	MemoryContext linecxt;
***************
*** 1779,1785 **** load_hba(void)
  		return false;
  	}
  
! 	linecxt = tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums);
  	FreeFile(file);
  
  	/* Now parse all the lines */
--- 1805,1811 ----
  		return false;
  	}
  
! 	linecxt = tokenize_file(HbaFileName, file, &hba_lines, &hba_line_nums, &hba_raw_lines);
  	FreeFile(file);
  
  	/* Now parse all the lines */
***************
*** 1789,1799 **** load_hba(void)
  								   ALLOCSET_DEFAULT_MINSIZE,
  								   ALLOCSET_DEFAULT_MAXSIZE);
  	oldcxt = MemoryContextSwitchTo(hbacxt);
! 	forboth(line, hba_lines, line_num, hba_line_nums)
  	{
  		HbaLine    *newline;
  
! 		if ((newline = parse_hba_line(lfirst(line), lfirst_int(line_num))) == NULL)
  		{
  			/*
  			 * Parse error in the file, so indicate there's a problem.  NB: a
--- 1815,1825 ----
  								   ALLOCSET_DEFAULT_MINSIZE,
  								   ALLOCSET_DEFAULT_MAXSIZE);
  	oldcxt = MemoryContextSwitchTo(hbacxt);
! 	forthree(line, hba_lines, line_num, hba_line_nums, raw_line, hba_raw_lines)
  	{
  		HbaLine    *newline;
  
! 		if ((newline = parse_hba_line(lfirst(line), lfirst_int(line_num), lfirst(raw_line))) == NULL)
  		{
  			/*
  			 * Parse error in the file, so indicate there's a problem.  NB: a
***************
*** 2153,2159 **** load_ident(void)
  		return false;
  	}
  
! 	linecxt = tokenize_file(IdentFileName, file, &ident_lines, &ident_line_nums);
  	FreeFile(file);
  
  	/* Now parse all the lines */
--- 2179,2185 ----
  		return false;
  	}
  
! 	linecxt = tokenize_file(IdentFileName, file, &ident_lines, &ident_line_nums, NULL);
  	FreeFile(file);
  
  	/* Now parse all the lines */
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
***************
*** 53,58 **** typedef enum ConnType
--- 53,59 ----
  typedef struct HbaLine
  {
  	int			linenumber;
+ 	char	   *rawline;
  	ConnType	conntype;
  	List	   *databases;
  	List	   *roles;
#11Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Magnus Hagander (#10)
Re: Reporting hba lines

On 5 January 2013 16:58, Magnus Hagander <magnus@hagander.net> wrote:

Attached is an updated version of the patch, per the comments from Tom
and rebased on top of the current master. Since it's been a long time
ago, and some code churn in the area, another round of review is
probably a good thing...

I took a look at this patch, and it seems to be in pretty good shape.
It applies cleanly to head, and seems to work as advertised/discussed.
I have a couple of comments on the code...

In next_token(), in the case of an overlong token, this change looks wrong:

/* Discard remainder of line */
! while ((c = getc(fp)) != EOF && c != '\n')
! ;
break;
}

--- 188,195 ----
  			   errmsg("authentication file token too long, skipping: \"%s\"",
  					  start_buf)));
  			/* Discard remainder of line */
! 			while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
! 				(*lineptr)++;
  			break;

It appears to be incrementing lineptr twice per loop iteration, so it
risks missing the EOL/EOF and running off the end of the buffer.

Nitpicking, at the end of the loop you have:

! c = (**lineptr);
! (*lineptr)++;

perhaps for consistency with the preceding code, that should be "c =
(*(*lineptr)++)". Personally, I'd also get rid of the outer sets of
brackets in each of these expressions and just write "c =
*(*lineptr)++", since I don't think they add anything.

Finally, the comment block for tokenize_file() needs updating, now
that it returns three lists.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Magnus Hagander
magnus@hagander.net
In reply to: Dean Rasheed (#11)
Re: Reporting hba lines

On Sun, Jan 20, 2013 at 5:56 PM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 5 January 2013 16:58, Magnus Hagander <magnus@hagander.net> wrote:

Attached is an updated version of the patch, per the comments from Tom
and rebased on top of the current master. Since it's been a long time
ago, and some code churn in the area, another round of review is
probably a good thing...

I took a look at this patch, and it seems to be in pretty good shape.
It applies cleanly to head, and seems to work as advertised/discussed.
I have a couple of comments on the code...

In next_token(), in the case of an overlong token, this change looks wrong:

/* Discard remainder of line */
! while ((c = getc(fp)) != EOF && c != '\n')
! ;
break;
}

--- 188,195 ----
errmsg("authentication file token too long, skipping: \"%s\"",
start_buf)));
/* Discard remainder of line */
!                       while ((c = (*(*lineptr)++)) != '\0' && c != '\n')
!                               (*lineptr)++;
break;

It appears to be incrementing lineptr twice per loop iteration, so it
risks missing the EOL/EOF and running off the end of the buffer.

Nitpicking, at the end of the loop you have:

! c = (**lineptr);
! (*lineptr)++;

perhaps for consistency with the preceding code, that should be "c =
(*(*lineptr)++)". Personally, I'd also get rid of the outer sets of
brackets in each of these expressions and just write "c =
*(*lineptr)++", since I don't think they add anything.

Finally, the comment block for tokenize_file() needs updating, now
that it returns three lists.

Thanks for the review - I've updated the patch per your comments
(minus the changing of the outer set of brackets - kept that the way
it was for consistency, but that can always be changed later if people
prefer that way), and will push it as it now stands.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers