Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
2011/6/15 Pavel Stehule pavel.steh...@gmail.com: Hello I try to apply your patch, but it is finished with some failed hinks. Please, can you refresh your patch Regards Pavel [pavel@nemesis postgresql]$ patch -p1 pghba.patch patching file src/backend/libpq/hba.c Hunk #1 succeeded at 45 (offset 2 lines). Hunk #2 succeeded at 56 (offset 2 lines). Hunk #3 succeeded at 120 (offset 2 lines). Hunk #4 succeeded at 212 (offset 2 lines). Hunk #5 succeeded at 244 (offset 2 lines). Hunk #6 succeeded at 286 (offset 2 lines). Hunk #7 succeeded at 327 (offset 2 lines). Hunk #8 succeeded at 363 (offset 2 lines). Hunk #9 succeeded at 372 (offset 2 lines). Hunk #10 succeeded at 411 (offset 2 lines). Hunk #11 FAILED at 470. Hunk #12 succeeded at 783 (offset 4 lines). Hunk #13 FAILED at 795. Hunk #14 succeeded at 866 (offset 25 lines). Hunk #15 succeeded at 882 (offset 25 lines). Hunk #16 succeeded at 899 (offset 25 lines). Hunk #17 succeeded at 918 (offset 25 lines). Hunk #18 succeeded at 939 (offset 25 lines). Hunk #19 succeeded at 972 (offset 25 lines). Hunk #20 succeeded at 989 (offset 25 lines). Hunk #21 FAILED at 995. Hunk #22 succeeded at 1034 (offset 25 lines). Hunk #23 succeeded at 1102 (offset 25 lines). Hunk #24 succeeded at 1112 (offset 25 lines). Hunk #25 succeeded at 1167 (offset 25 lines). Hunk #26 FAILED at 1178. Hunk #27 succeeded at 1255 (offset 25 lines). Hunk #28 succeeded at 1267 (offset 25 lines). Hunk #29 succeeded at 1360 (offset 25 lines). Hunk #30 succeeded at 1657 (offset 25 lines). Hunk #31 succeeded at 1686 (offset 25 lines). Hunk #32 succeeded at 1834 (offset 25 lines). Hunk #33 succeeded at 1845 (offset 25 lines). Hunk #34 succeeded at 2006 (offset 25 lines). Hunk #35 succeeded at 2074 (offset 25 lines). 4 out of 35 hunks FAILED -- saving rejects to file src/backend/libpq/hba.c.rej patching file src/include/libpq/hba.h -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 29 October 2010 09:59, Brendan Jurd dire...@gmail.com wrote: On 18 October 2010 01:19, Tom Lane t...@sss.pgh.pa.us wrote: Brendan Jurd dire...@gmail.com writes: On 17 October 2010 09:59, Tom Lane t...@sss.pgh.pa.us wrote: Good point. Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. I still think that the current method is impossibly ugly ... I'm happy to revise the patch on that basis. Any suggestions about how to communicate the 'quotedness' of each token? We could make each token a struct consisting of the token itself, plus a boolean flag to indicate whether it had been quoted. Does that work for you? Seems reasonable. I had the idea of a parallel list of booleans in the back of my mind, but a list of structs is probably easier to understand, and to extend further if necessary. Okay, I've taken the red pill and I'm finding out how deep the rabbit hole goes ... The logical structure of pg_hba.conf is a set of lines, each line containing a set of fields, each field containing a set of tokens. The way the existing implementation handles this is to create a list of lines containing sublists of fields, containing comma-separated strings for the set of tokens, with newlines embedded next to tokens which might be keywords. The tokeniser breaks apart the comma-separated tokens ... and then reassembles them into a comma-separated string. Which the db/role matching functions then have to break apart *again*. In order to keep track of whether each individual token was quoted, I first need to impose some sanity here. Rather than using a magical string for each field, I intend to use a List of HbaToken structs which explicitly note whether quoting was used. Introducing an extra List level does mean a bit more work copying and freeing, and it makes the patch really quite intrusive. I have to touch a lot of lines in hba.c, but I think the additional clarity is worth it. If nobody dissuades me from this approach I hope to post a patch in a couple of days. Hi folks, I've finally managed to get a patch together for the above. It ended up being a much more difficult project than I anticipated. Just to recap, the HEAD code in hba.c parses pg_hba.conf (and ident.conf) into a List of lines, each line being a List of strings. These strings have various magical properties, including commas to separate individual tokens and newlines to mark up keywords. I have replaced this behaviour with something which more closely represents the true logical structure of the file, and is (hopefully) much more intelligible. The patch introduces the HbaToken struct, which consists of a string and a boolean to indicate whether the string was parsed inside quotes. The parser now produces a triple-nested list -- a List of lines, each line being a List of fields, each field being a List of tokens. It sounds straightforward enough, but to make this happen I had to change a very substantial fraction of pg_hba.c. I have tested a few basic pg_hba.conf setups and everything seems to be working as intended. There is much more testing to be done, it probably leaks memory and there is a lot more room for cleanup in hba.c, but I would like to open the patch up for review and comment as a WIP. Posting it to the 2011-Next CF. Cheers, BJ hba-keywords.diff.bz2 Description: BZip2 compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Brendan Jurd wrote: On 26 February 2011 18:06, Bruce Momjian br...@momjian.us wrote: Any progress on this? I ended up doing most of the work, but never got around to finishing it off. Thanks for the reminder, though. I'll get that one ready and drop it onto the next CF. Added to TODO: Have pg_hba.conf consider replication special only in the database field * http://archives.postgresql.org/pgsql-hackers/2010-10/msg00632.php -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 26 February 2011 18:06, Bruce Momjian br...@momjian.us wrote: Any progress on this? I ended up doing most of the work, but never got around to finishing it off. Thanks for the reminder, though. I'll get that one ready and drop it onto the next CF. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On Sat, Feb 26, 2011 at 10:57, Brendan Jurd dire...@gmail.com wrote: On 26 February 2011 18:06, Bruce Momjian br...@momjian.us wrote: Any progress on this? I ended up doing most of the work, but never got around to finishing it off. Thanks for the reminder, though. I'll get that one ready and drop it onto the next CF. Just as a point, which i proably moot if you've done most of the work already :) But at some point it might be worthwhile to see if our life would be mad easier by using flex (with or without bison) to parse pg_hba.conf as wel, since we do it for postgresql.conf and other things. If it makes maintenance easier, it's probably worth it. -- 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
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Any progress on this? --- Brendan Jurd wrote: On 18 October 2010 01:19, Tom Lane t...@sss.pgh.pa.us wrote: Brendan Jurd dire...@gmail.com writes: On 17 October 2010 09:59, Tom Lane t...@sss.pgh.pa.us wrote: Good point. ?Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. ?I still think that the current method is impossibly ugly ... I'm happy to revise the patch on that basis. ?Any suggestions about how to communicate the 'quotedness' of each token? ?We could make each token a struct consisting of the token itself, plus a boolean flag to indicate whether it had been quoted. ?Does that work for you? Seems reasonable. ?I had the idea of a parallel list of booleans in the back of my mind, but a list of structs is probably easier to understand, and to extend further if necessary. Okay, I've taken the red pill and I'm finding out how deep the rabbit hole goes ... The logical structure of pg_hba.conf is a set of lines, each line containing a set of fields, each field containing a set of tokens. The way the existing implementation handles this is to create a list of lines containing sublists of fields, containing comma-separated strings for the set of tokens, with newlines embedded next to tokens which might be keywords. The tokeniser breaks apart the comma-separated tokens ... and then reassembles them into a comma-separated string. Which the db/role matching functions then have to break apart *again*. In order to keep track of whether each individual token was quoted, I first need to impose some sanity here. Rather than using a magical string for each field, I intend to use a List of HbaToken structs which explicitly note whether quoting was used. Introducing an extra List level does mean a bit more work copying and freeing, and it makes the patch really quite intrusive. I have to touch a lot of lines in hba.c, but I think the additional clarity is worth it. If nobody dissuades me from this approach I hope to post a patch in a couple of days. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 18 October 2010 01:19, Tom Lane t...@sss.pgh.pa.us wrote: Brendan Jurd dire...@gmail.com writes: On 17 October 2010 09:59, Tom Lane t...@sss.pgh.pa.us wrote: Good point. Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. I still think that the current method is impossibly ugly ... I'm happy to revise the patch on that basis. Any suggestions about how to communicate the 'quotedness' of each token? We could make each token a struct consisting of the token itself, plus a boolean flag to indicate whether it had been quoted. Does that work for you? Seems reasonable. I had the idea of a parallel list of booleans in the back of my mind, but a list of structs is probably easier to understand, and to extend further if necessary. Okay, I've taken the red pill and I'm finding out how deep the rabbit hole goes ... The logical structure of pg_hba.conf is a set of lines, each line containing a set of fields, each field containing a set of tokens. The way the existing implementation handles this is to create a list of lines containing sublists of fields, containing comma-separated strings for the set of tokens, with newlines embedded next to tokens which might be keywords. The tokeniser breaks apart the comma-separated tokens ... and then reassembles them into a comma-separated string. Which the db/role matching functions then have to break apart *again*. In order to keep track of whether each individual token was quoted, I first need to impose some sanity here. Rather than using a magical string for each field, I intend to use a List of HbaToken structs which explicitly note whether quoting was used. Introducing an extra List level does mean a bit more work copying and freeing, and it makes the patch really quite intrusive. I have to touch a lot of lines in hba.c, but I think the additional clarity is worth it. If nobody dissuades me from this approach I hope to post a patch in a couple of days. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Brendan Jurd dire...@gmail.com writes: On 17 October 2010 09:59, Tom Lane t...@sss.pgh.pa.us wrote: Good point. Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. I still think that the current method is impossibly ugly ... I'm happy to revise the patch on that basis. Any suggestions about how to communicate the 'quotedness' of each token? We could make each token a struct consisting of the token itself, plus a boolean flag to indicate whether it had been quoted. Does that work for you? Seems reasonable. I had the idea of a parallel list of booleans in the back of my mind, but a list of structs is probably easier to understand, and to extend further if necessary. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 13 October 2010 00:28, Fujii Masao masao.fu...@gmail.com wrote: On Sat, Oct 9, 2010 at 11:13 AM, Brendan Jurd dire...@gmail.com wrote: I understand that 'replication' is a keyword as far as the database name is concerned, but I was surprised to find that it was treated as a keyword in the username field also. I had a look in src/backend/libpq/hba.c, and next_token() appears to be completely naive about this. 'replication' (along with 'all', 'sameuser', 'samegroup' and 'samerole') is treated as a keyword wherever it appears, not just in the field where it is relevant. next_token() appends a newline to the end of the 'replication' username token, and that's why the entry doesn't match my connection attempt. I suspect this is going to trip a lot of people up. We could just document it and tell people that if they want to use 'replication' as a username, they'd better quote it in pg_hba.conf. But I'd prefer to actually solve the problem. Agreed. We should address that. Hi folks, Per the above discussion, I've prepared a patch to make keywords in pg_hba.conf field-specific. This patch takes a least-resistance approach to solving the problem. next_token() continues to blithely append a newline character to any token which *might* be a keyword, but the patch teaches hba_parse_line() to be a bit more savvy about whether to take next_token's word for it. In the Database field, hba_parse_line() acknowledges 'all', 'sameuser', 'samegroup', 'samerole' and 'replication' as legitimate keywords. Anything else is considered not a keyword, and if there is a trailing newline it is stripped from the token (by just reassigning it to NUL). Likewise, in the Role field, only 'all' is considered legitimate. In the Host field, we allow 'samehost' and 'samenet'. The bottom line is that this makes 'replication' safe to use as a username, no quoting required. It seemed a little bit inelegant to allow next_token() to mark up a keyword, only to ignore that marking later on, but as the number of fields in each line is not decided until we parse it, there was no sensible way to teach next_token() which field it was evaluating. Added to the November CF. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 17 October 2010 01:52, Brendan Jurd dire...@gmail.com wrote:
Per the above discussion, I've prepared a patch to make keywords in
pg_hba.conf field-specific.
Try New and Improved This Message (tm), now with attachment!
Cheers,
BJ
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***
*** 855,861 parse_hba_line(List *line, int line_num, HbaLine *parsedline)
line_num,
HbaFileName)));
return false;
}
! parsedline-database = pstrdup(lfirst(line_item));
/* Get the role. */
line_item = lnext(line_item);
--- 855,873
line_num,
HbaFileName)));
return false;
}
! token = pstrdup(lfirst(line_item));
!
! /* Filter out inapplicable keywords for the database field. */
! if (token[strlen(token) - 1] == '\n'
!strcmp(token, all\n) != 0
!strcmp(token, sameuser\n) != 0
!strcmp(token, samerole\n) != 0
!strcmp(token, samegroup\n) != 0
!strcmp(token, replication\n) != 0)
! {
! token[strlen(token) - 1] = '\0';
! }
! parsedline-database = token;
/* Get the role. */
line_item = lnext(line_item);
***
*** 868,874 parse_hba_line(List *line, int line_num, HbaLine *parsedline)
line_num,
HbaFileName)));
return false;
}
! parsedline-role = pstrdup(lfirst(line_item));
if (parsedline-conntype != ctLocal)
{
--- 880,894
line_num,
HbaFileName)));
return false;
}
! token = pstrdup(lfirst(line_item));
!
! /* Filter out inapplicable keywords for the role field. */
! if (token[strlen(token) - 1] == '\n'
!strcmp(token, all\n) != 0)
! {
! token[strlen(token) - 1] = '\0';
! }
! parsedline-role = token;
if (parsedline-conntype != ctLocal)
{
***
*** 904,909 parse_hba_line(List *line, int line_num, HbaLine *parsedline)
--- 924,937
/* need a modifiable copy of token */
token = pstrdup(token);
+ /*
+* Filter out any remaining keywords, as the only valid
keywords
+* for this context ('samehost' and 'samenet') have
already been
+* handled above.
+*/
+ if (token[strlen(token) - 1] = '\n')
+ token[strlen(token) - 1] = '\0';
+
/* Check if it has a CIDR suffix and if so isolate it */
cidr_slash = strchr(token, '/');
if (cidr_slash)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Excerpts from Brendan Jurd's message of sáb oct 16 11:53:31 -0300 2010: On 17 October 2010 01:52, Brendan Jurd dire...@gmail.com wrote: Per the above discussion, I've prepared a patch to make keywords in pg_hba.conf field-specific. Try New and Improved This Message (tm), now with attachment! Hmm. Would it be possible to list keywords _applicable_ to each field, and have these passed down to next_token by the caller instead? This seems backwards, but I'm not sure if the other way is really workable. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 17 October 2010 02:27, Alvaro Herrera alvhe...@commandprompt.com wrote: Hmm. Would it be possible to list keywords _applicable_ to each field, and have these passed down to next_token by the caller instead? This seems backwards, but I'm not sure if the other way is really workable. Short answer: I don't think it is workable, or I would have done it that way in the first place. Full answer: The problem is that pg_hba.conf doesn't have a fixed structure. Each line can be 4, 5 or 6 fields (not including the final 'options' field) long, and which of these structures apply to any given line isn't decided until parse_hba_line goes to work on it. At the time that next_token gets called, we have no way of knowing which field is currently being tokenised, at least not without doing some serious rearrangement of hba.c, so that it tokenises and then parses one token at a time, instead of tokenising the whole file and then parsing all the lines. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Alvaro Herrera alvhe...@commandprompt.com writes: Excerpts from Brendan Jurd's message of sáb oct 16 11:53:31 -0300 2010: Try New and Improved This Message (tm), now with attachment! Hmm. Would it be possible to list keywords _applicable_ to each field, and have these passed down to next_token by the caller instead? +1. That newline thing is a crude hack and always has been. If people are dissatisfied with it, it's time to start over rather than piling additional crockery on top. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
Brendan Jurd dire...@gmail.com writes: Full answer: The problem is that pg_hba.conf doesn't have a fixed structure. Each line can be 4, 5 or 6 fields (not including the final 'options' field) long, and which of these structures apply to any given line isn't decided until parse_hba_line goes to work on it. At the time that next_token gets called, we have no way of knowing which field is currently being tokenised, at least not without doing some serious rearrangement of hba.c, so that it tokenises and then parses one token at a time, instead of tokenising the whole file and then parsing all the lines. Good point. Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. I still think that the current method is impossibly ugly ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On 17 October 2010 09:59, Tom Lane t...@sss.pgh.pa.us wrote: Brendan Jurd dire...@gmail.com writes: At the time that next_token gets called, we have no way of knowing which field is currently being tokenised, at least not without doing some serious rearrangement of hba.c, so that it tokenises and then parses one token at a time, instead of tokenising the whole file and then parsing all the lines. Good point. Maybe the correct fix is to remember whether each token was quoted or not, so that keyword detection can be done safely after the initial lexing. I still think that the current method is impossibly ugly ... I'm happy to revise the patch on that basis. Any suggestions about how to communicate the 'quotedness' of each token? We could make each token a struct consisting of the token itself, plus a boolean flag to indicate whether it had been quoted. Does that work for you? Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On Sat, Oct 9, 2010 at 11:13 AM, Brendan Jurd dire...@gmail.com wrote: I understand that 'replication' is a keyword as far as the database name is concerned, but I was surprised to find that it was treated as a keyword in the username field also. I had a look in src/backend/libpq/hba.c, and next_token() appears to be completely naive about this. 'replication' (along with 'all', 'sameuser', 'samegroup' and 'samerole') is treated as a keyword wherever it appears, not just in the field where it is relevant. next_token() appends a newline to the end of the 'replication' username token, and that's why the entry doesn't match my connection attempt. I suspect this is going to trip a lot of people up. We could just document it and tell people that if they want to use 'replication' as a username, they'd better quote it in pg_hba.conf. But I'd prefer to actually solve the problem. Agreed. We should address that. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Keywords in pg_hba.conf should be field-specific
On Fri, Oct 8, 2010 at 10:13 PM, Brendan Jurd dire...@gmail.com wrote: One of the speedbumps I hit when setting up HS+SR was naming the user the slave would connect as for streaming replication. At first I picked 'replication', which seemed quite natural to me (and I don't doubt will seem natural to others as well). When I started up the slave, I got this error: FATAL: could not connect to the primary server: FATAL: no pg_hba.conf entry for replication connection from host 192.168.21.10, user replication, SSL on Bzzzt. Wrong. There *was* such an entry in pg_hba.conf. I wasted a lot of time checking my conf files for typos before I wondered whether there might be something wrong with using 'replication' as a username. I changed the username to 'streamrep' and it all started working perfectly. I understand that 'replication' is a keyword as far as the database name is concerned, but I was surprised to find that it was treated as a keyword in the username field also. Yikes. That does seems surprising. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
