Re: Keywords in pg_hba.conf should be field-specific

On 16 June 2011 00:22, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I try to apply your patch, but it is finished with some failed hinks.

Please, can you refresh your patch

Hi Pavel,

Thanks for taking a look.  I have attached v2 of the patch, as against
current HEAD.  I've also added the new patch to the CF app.  I look
forward to your comments.

Cheers,
BJ

Excerpts from Brendan Jurd's message of vie jun 17 19:31:41 -0400 2011:

On 16 June 2011 00:22, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I try to apply your patch, but it is finished with some failed hinks.

Please, can you refresh your patch

Hi Pavel,

Thanks for taking a look.  I have attached v2 of the patch, as against
current HEAD.  I've also added the new patch to the CF app.  I look
forward to your comments.

Is this really a WIP patch? I'm playing a bit with it currently, seems
fairly sane.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 18 June 2011 13:43, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Is this really a WIP patch?  I'm playing a bit with it currently, seems
fairly sane.

In this case, the WIP designation is meant to convey "warning: only
casual testing has beeen done". I tried it out with various
permutations of pg_hba.conf, and it worked as advertised in those
tests, but I have not made any attempt to formulate a more rigorous
testing regimen.

In particular I haven't tested that the more exotic authentication
methods still work properly, and I can't recall whether I tested
recursive file inclusion and group membership.

Is that a wrongful use of the WIP designation?

Cheers,
BJ

Hello Brendan,

I checked your patch, it is applied cleanly and I don't see any mayor
problem. This patch does all what is expected.

I have two minor comments

a) you don't use macro "token_matches" consistently

func: parse_hba_line

<------>if (strcmp(token->string, "local") == 0)

should be
if (token_is_keyword(token, "local"))
...

I don't see any sense when somebody use a quotes there.

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Regards

Pavel Stehule

2011/6/18 Brendan Jurd <direvus@gmail.com>:

sorry

a) you don't use macro "token_matches" consistently

should be

a) you don't use macro "token_is_keyword" consistently

it should be used for all keywords

Regards

Pavel Stehule

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though. I'll send an updated version shortly.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though. I'll send an updated version shortly.

Here it is, please let me know what you think. I took the liberty of
cleaning up some things that were clearly historical leftovers.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 21 June 2011 06:06, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though.  I'll send an updated version shortly.

Here it is, please let me know what you think.  I took the liberty of
cleaning up some things that were clearly historical leftovers.

Okay, yeah, the MemoryContext approach is far more elegant than what I
had. Boy was I ever barking up the wrong tree.

Cheers,
BJ

Excerpts from Brendan Jurd's message of lun jun 20 20:06:39 -0400 2011:

On 21 June 2011 06:06, Alvaro Herrera <alvherre@commandprompt.com> wrote:

Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though.  I'll send an updated version shortly.

Here it is, please let me know what you think.  I took the liberty of
cleaning up some things that were clearly historical leftovers.

Okay, yeah, the MemoryContext approach is far more elegant than what I
had. Boy was I ever barking up the wrong tree.

Eh, whoever wrote the original code was barking up the same tree, so I
don't blame you for following the well-trodden path.

I realize I took out most of the fun of this patch from you, but -- are
you still planning to do some more exhaustive testing of it? I checked
some funny scenarios (including include files and groups) but it's not
all that unlikely that I missed some cases. I also didn't check any
auth method other than ident, md5 and trust, 'cause I don't have what's
required for anything else. (It's a pity that the regression tests
don't exercise anything else.)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 21 June 2011 11:11, Alvaro Herrera <alvherre@commandprompt.com> wrote:

I realize I took out most of the fun of this patch from you, but -- are
you still planning to do some more exhaustive testing of it?  I checked
some funny scenarios (including include files and groups) but it's not
all that unlikely that I missed some cases.  I also didn't check any
auth method other than ident, md5 and trust, 'cause I don't have what's
required for anything else.  (It's a pity that the regression tests
don't exercise anything else.)

I've pretty much tested all the things I can think to test, and I'm in
the same boat as you re auth methods. If there are bugs, I think they
will be of the obscure kind, that only reveal themselves under
peculiar circumstances.

Cheers,
BJ

2011/6/20 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though.  I'll send an updated version shortly.

Here it is, please let me know what you think.  I took the liberty of
cleaning up some things that were clearly historical leftovers.

I have one question. I can't find any rules for work with tokens, etc,
where is quotes allowed and disallowed?

I don't see any other issues.

Regards

Pavel Stehule

On 21 June 2011 13:51, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I have one question. I can't find any rules for work with tokens, etc,
where is quotes allowed and disallowed?

I don't see any other issues.

I'm not sure I understand your question, but quotes are allowed
anywhere and they always act to remove any special meaning the token
might otherwise have had. It's much like quoting a column name in
SQL.

I didn't change any of the hba parsing rules, so escaping and whatnot
should all work the way it did before. The only difference should be
that after parsing, keywords will only be evaluated for fields where
they matter.

Cheers,
BJ

2011/6/21 Brendan Jurd <direvus@gmail.com>:

On 21 June 2011 13:51, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I have one question. I can't find any rules for work with tokens, etc,
where is quotes allowed and disallowed?

I don't see any other issues.

I'm not sure I understand your question, but quotes are allowed
anywhere and they always act to remove any special meaning the token
might otherwise have had.  It's much like quoting a column name in
SQL.

I don't understand to using a macro

#define token_is_keyword(t, k) (!t->quoted && strcmp(t->string, k) == 0)

because you disallowed a quoting?

Regards

Pavel

On 21 June 2011 14:34, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I don't understand to using a macro

#define token_is_keyword(t, k)  (!t->quoted && strcmp(t->string, k) == 0)

because you disallowed a quoting?

Well, a token can only be treated as a special keyword if it is unquoted.

As an example, in the 'database' field, the bare token 'replication'
is a keyword meaning the pseudo-database for streaming rep. Whereas
the quoted token "replication" would mean a real database which is
called 'replication'.

Likewise, the bare token 'all' in the username field is a keyword --
it matches any username. Whereas the quoted token "all" would only
match a user named 'all'.

Therefore, token_is_keyword only returns true where the token matches
the given string as is also unquoted.

Does that make sense?

Cheers,
BJ

2011/6/21 Brendan Jurd <direvus@gmail.com>:

On 21 June 2011 14:34, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I don't understand to using a macro

#define token_is_keyword(t, k)  (!t->quoted && strcmp(t->string, k) == 0)

because you disallowed a quoting?

Well, a token can only be treated as a special keyword if it is unquoted.

As an example, in the 'database' field, the bare token 'replication'
is a keyword meaning the pseudo-database for streaming rep.  Whereas
the quoted token "replication" would mean a real database which is
called 'replication'.

Likewise, the bare token 'all' in the username field is a keyword --
it matches any username.  Whereas the quoted token "all" would only
match a user named 'all'.

Therefore, token_is_keyword only returns true where the token matches
the given string as is also unquoted.

Does that make sense?

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

else if (strcmp(token, "pamservice") == 0)
- {
- REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
- parsedline->pamservice = pstrdup(c);
- }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

??

Regards

Pavel

Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

else if (strcmp(token, "pamservice") == 0)
- {
- REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
- parsedline->pamservice = pstrdup(c);
- }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?). I opted for
leaving it alone, but maybe this needs to be fixed. (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/21 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

        else if (strcmp(token, "pamservice") == 0)
-             {
-                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
-                 parsedline->pamservice = pstrdup(c);
-             }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?).  I opted for
leaving it alone, but maybe this needs to be fixed.  (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).

It's question about compatibility - sure. But a description inside
pg_hba.conf speaks cleanly - quoting means a lost of original
semantic.

And if we allow a quoting somewhere, then I can't imagine a
description - "somewhere quoting means a string literal, somewhere
have not impact?"

Regards

Pavel Stehule

Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:

2011/6/21 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

        else if (strcmp(token, "pamservice") == 0)
-             {
-                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
-                 parsedline->pamservice = pstrdup(c);
-             }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?).  I opted for
leaving it alone, but maybe this needs to be fixed.  (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).

I tested it and it works: This line

"local" @dbs +b "trust"

is accepted and it works in the unpatched code. I don't think we want
to break people's existing pg_hba.conf files for no reason. I doubt
that many people are using pg_hba.conf tokens with quotes, mind you, but
there might be some ...

In any case, if people here thinks we should tighten this, it's easy to
do on top of this patch by changing the strcmp() calls to
token_is_keyword, as you say. Let's not burden this patch with the
responsibility of doing so, because that's likely to get it punted.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/21 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:

2011/6/21 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

        else if (strcmp(token, "pamservice") == 0)
-             {
-                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
-                 parsedline->pamservice = pstrdup(c);
-             }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?).  I opted for
leaving it alone, but maybe this needs to be fixed.  (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).

I tested it and it works: This line

"local" @dbs +b "trust"

is accepted and it works in the unpatched code.  I don't think we want
to break people's existing pg_hba.conf files for no reason.  I doubt
that many people are using pg_hba.conf tokens with quotes, mind you, but
there might be some ...

In any case, if people here thinks we should tighten this, it's easy to
do on top of this patch by changing the strcmp() calls to
token_is_keyword, as you say.  Let's not burden this patch with the
responsibility of doing so, because that's likely to get it punted.

It is time to discuss about it.

I thinking so current behave is strange and should be fixed - it
doesn't respect a description stored in pg_hba.conf. I agree, so this
will have impact on compatibility, but pg_hba is config file so this
impact is not too hard. The cleaning now can carry a benefit in
future, when pg_hba can be more complex.

Regards

Pavel