Обсуждение: usermap regexp support
The attached patch tries to implement regexp support in the usermaps
(pg_ident.conf).
The use-case will be to do things like realm-based matches in
kerberos/GSSAPI authentication (will require some further hacks on that
one to expose the realm name). For example you could have:
krb /^(.*)@myrealm.com$ \1
krb /^(.*)@otherrealm.com guest
and things like that.
It will also likely be quite useful once we get SSL certificate based
authentication.
Reviews particularly appreciated - I really can't claim to *know* our
regexp code :-O
Obviously docs need to be updated as well.
//Magnus
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 27,32 ****
--- 27,33 ----
#include "libpq/ip.h"
#include "libpq/libpq.h"
+ #include "regex/regex.h"
#include "storage/fd.h"
#include "utils/flatfiles.h"
#include "utils/guc.h"
***************
*** 1325,1344 **** parse_ident_usermap(List *line, int line_number, const char *usermap_name,
token = lfirst(line_item);
file_pgrole = token;
/* Match? */
! if (case_insensitive)
{
! if (strcmp(file_map, usermap_name) == 0 &&
! pg_strcasecmp(file_pgrole, pg_role) == 0 &&
! pg_strcasecmp(file_ident_user, ident_user) == 0)
! *found_p = true;
}
else
{
! if (strcmp(file_map, usermap_name) == 0 &&
! strcmp(file_pgrole, pg_role) == 0 &&
! strcmp(file_ident_user, ident_user) == 0)
! *found_p = true;
}
return;
--- 1326,1453 ----
token = lfirst(line_item);
file_pgrole = token;
+ if (strcmp(file_map, usermap_name) != 0)
+ /* Line does not match the map name we're looking for, so just abort */
+ return;
+
/* Match? */
! if (file_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;
! 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));
!
! /*
! * XXX: Major room for optimization: regexps could be compiled when the file is loaded
! * and then re-used in every connection.
! */
! r = pg_regcomp(&re, wstr, wlen, REG_ADVANCED);
! if (r)
! {
! char errstr[100];
!
! pg_regerror(r, &re, errstr, sizeof(errstr));
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("invalid regular expression '%s': %s", file_ident_user+1, errstr)));
!
! pfree(wstr);
! *error_p = true;
! return;
! }
! pfree(wstr);
!
! 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);
! if (r)
! {
! char errstr[100];
!
! if (r != REG_NOMATCH)
! {
! /* REG_NOMATCH is not an error, everything else is */
! pg_regerror(r, &re, errstr, sizeof(errstr));
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("regular expression match for '%s' failed: %s", file_ident_user+1, errstr)));
! *error_p = true;
! }
!
! pfree(wstr);
! pg_regfree(&re);
! return;
! }
! pfree(wstr);
!
! if ((ofs = strstr(file_pgrole, "\\1")) != NULL)
! {
! /* substitution of the first argument requested */
! if (matches[1].rm_so < 0)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("regular expression '%s' has no subexpressions as requested by backreference in
'%s'",
! file_ident_user+1, file_pgrole)));
! /* length: original length minus length of \1 plus length of match */
! regexp_pgrole = palloc0(strlen(file_pgrole)-2+(matches[1].rm_so-matches[1].rm_so));
! strncpy(regexp_pgrole, file_pgrole, (ofs-file_pgrole));
! memcpy(regexp_pgrole+strlen(regexp_pgrole),
! ident_user+matches[1].rm_so,
! matches[1].rm_eo-matches[1].rm_so);
! strcat(regexp_pgrole, ofs+2);
! }
! else
! {
! /* no substitution, so copy the match */
! regexp_pgrole = pstrdup(file_pgrole);
! }
!
! pg_regfree(&re);
!
! /* now check if the username actually matched what the user is trying to connect as */
! if (case_insensitive)
! {
! if (pg_strcasecmp(regexp_pgrole, pg_role) == 0)
! *found_p = true;
! }
! else
! {
! if (strcmp(regexp_pgrole, pg_role) == 0)
! *found_p = true;
! }
! pfree(regexp_pgrole);
!
! return;
}
else
{
! /* 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)
! *found_p = true;
! }
! else
! {
! if (strcmp(file_pgrole, pg_role) == 0 &&
! strcmp(file_ident_user, ident_user) == 0)
! *found_p = true;
! }
}
return;
On Tue, Oct 28, 2008 at 08:59:53PM +0100, Magnus Hagander wrote:
> The attached patch tries to implement regexp support in the usermaps
> (pg_ident.conf).
Hi Magnus,
I am currently reviewing your patch.
I found out that the execution of
pfree(regexp_pgrole);
(there's only one such line in your patch) might raise on the current
HEAD the following message
WARNING: detected write past chunk end in Postmaster 0x9b13650
in the case of a regexp expansion.
Let me describe the conditions under which I obtain that warning.
To test it without having to create a new user I used the "identical"
substitution
review /(.*)nni \1nni
in my pg_ident.conf so that I can trigger regexps while connecting as
"gianni".
To avoid removing all the "standard" lines from pg_hba.conf, I edited
postgresql.conf in order to make it listen from a different IP address
(actually the other IP address of my machine 192.168...). Then I added
to pg_hba.conf one line enabling ident connections with map=review
from that IP address.
Finally I started the server, and as I connected with psql -h
192.168... the above warning showed.
Best regards,
Dr. Gianni Ciolli - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gianni.ciolli@2ndquadrant.it | www.2ndquadrant.it
Gianni Ciolli wrote:
> On Tue, Oct 28, 2008 at 08:59:53PM +0100, Magnus Hagander wrote:
>> The attached patch tries to implement regexp support in the usermaps
>> (pg_ident.conf).
>
> Hi Magnus,
>
> I am currently reviewing your patch.
Hi!
Thanks for the review!
> I found out that the execution of
>
> pfree(regexp_pgrole);
>
> (there's only one such line in your patch) might raise on the current
> HEAD the following message
>
> WARNING: detected write past chunk end in Postmaster 0x9b13650
Yes, that's a stupid bug:
- /* length: original length minus length of \1
plus length of match */
- regexp_pgrole =
palloc0(strlen(file_pgrole)-2+(matches[1].rm_so-matches[1].rm_so));
+ /* 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);
Two bugs: the length calculation used the start of the string vs the
start of the string, instead of end vs start. And there was no room for
the NULL terminator.
Attached is an updated version of the patch that fixes this.
//Magnus
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
***************
*** 27,32 ****
--- 27,33 ----
#include "libpq/ip.h"
#include "libpq/libpq.h"
+ #include "regex/regex.h"
#include "storage/fd.h"
#include "utils/flatfiles.h"
#include "utils/guc.h"
***************
*** 1325,1344 **** parse_ident_usermap(List *line, int line_number, const char *usermap_name,
token = lfirst(line_item);
file_pgrole = token;
/* Match? */
! if (case_insensitive)
{
! if (strcmp(file_map, usermap_name) == 0 &&
! pg_strcasecmp(file_pgrole, pg_role) == 0 &&
! pg_strcasecmp(file_ident_user, ident_user) == 0)
! *found_p = true;
}
else
{
! if (strcmp(file_map, usermap_name) == 0 &&
! strcmp(file_pgrole, pg_role) == 0 &&
! strcmp(file_ident_user, ident_user) == 0)
! *found_p = true;
}
return;
--- 1326,1453 ----
token = lfirst(line_item);
file_pgrole = token;
+ if (strcmp(file_map, usermap_name) != 0)
+ /* Line does not match the map name we're looking for, so just abort */
+ return;
+
/* Match? */
! if (file_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;
! 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));
!
! /*
! * XXX: Major room for optimization: regexps could be compiled when the file is loaded
! * and then re-used in every connection.
! */
! r = pg_regcomp(&re, wstr, wlen, REG_ADVANCED);
! if (r)
! {
! char errstr[100];
!
! pg_regerror(r, &re, errstr, sizeof(errstr));
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("invalid regular expression '%s': %s", file_ident_user+1, errstr)));
!
! pfree(wstr);
! *error_p = true;
! return;
! }
! pfree(wstr);
!
! 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);
! if (r)
! {
! char errstr[100];
!
! if (r != REG_NOMATCH)
! {
! /* REG_NOMATCH is not an error, everything else is */
! pg_regerror(r, &re, errstr, sizeof(errstr));
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("regular expression match for '%s' failed: %s", file_ident_user+1, errstr)));
! *error_p = true;
! }
!
! pfree(wstr);
! pg_regfree(&re);
! return;
! }
! pfree(wstr);
!
! if ((ofs = strstr(file_pgrole, "\\1")) != NULL)
! {
! /* substitution of the first argument requested */
! if (matches[1].rm_so < 0)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
! errmsg("regular expression '%s' has no subexpressions as requested by backreference in
'%s'",
! file_ident_user+1, file_pgrole)));
! /* 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));
! memcpy(regexp_pgrole+strlen(regexp_pgrole),
! ident_user+matches[1].rm_so,
! matches[1].rm_eo-matches[1].rm_so);
! strcat(regexp_pgrole, ofs+2);
! }
! else
! {
! /* no substitution, so copy the match */
! regexp_pgrole = pstrdup(file_pgrole);
! }
!
! pg_regfree(&re);
!
! /* now check if the username actually matched what the user is trying to connect as */
! if (case_insensitive)
! {
! if (pg_strcasecmp(regexp_pgrole, pg_role) == 0)
! *found_p = true;
! }
! else
! {
! if (strcmp(regexp_pgrole, pg_role) == 0)
! *found_p = true;
! }
! pfree(regexp_pgrole);
!
! return;
}
else
{
! /* 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)
! *found_p = true;
! }
! else
! {
! if (strcmp(file_pgrole, pg_role) == 0 &&
! strcmp(file_ident_user, ident_user) == 0)
! *found_p = true;
! }
}
return;
On Tue, Nov 11, 2008 at 02:59:01PM +0100, Magnus Hagander wrote:
> Gianni Ciolli wrote:
> > WARNING: detected write past chunk end in Postmaster 0x9b13650
>
> Yes, that's a stupid bug:
(...)
> Attached is an updated version of the patch that fixes this.
Hi Magnus,
please find below the review of the second version of your patch,
which I think is "Ready for Committer" now.
Best regards,
Dr. Gianni Ciolli - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gianni.ciolli@2ndquadrant.it | www.2ndquadrant.it
---8<------8<------8<------8<------8<------8<------8<------8<------8<---
= Introduction =
The patch adds regexp support for username maps (see chapter "Client
Authentication", section "Username maps").
Basically, it allows a "parametric" approach to username maps, which
can be useful whenever the database user name can be computed
dynamically from the system user name.
For example, this can simplify user provisioning operations; the
pg_ident.conf file does no longer need to be updated each time an user
is added/removed to a particular system.
If the parameter SYSTEM-USERNAME begins with slash, then the remaining
string is interpreted as a regexp, and the parameter DATABASE-USERNAME
is then computed from the regexp match.
Example:
---8<------8<------8<------8<------8<------8<------8<------8<------8<---
mymap /(.*)@realm1.com$ \1
mymap /(.*)@realm2.com$ guest
---8<------8<------8<------8<------8<------8<------8<------8<------8<---
means that johnsmith@realm1.com will have PostgreSQL username
"johnsmith", while johnsmith@realm2.com will connect as "guest", and
that similar rules will exist for any other user of these two realms.
= En-passant remarks =
* I found out that the comments in the pg_hba.conf file installed by default are obsolete; we should either update
themor replace them with a reference to the Manual, chapter "Client Authentication", section "the pg_hba.conf file".
= Review =
(Note. I followed the guidelines found on the Wiki page.)
Submission review
* Is the patch in the standard form?
Yes, it is.
* Does it apply cleanly to the current CVS HEAD?
>> patching file src/backend/libpq/hba.c >> Hunk #2 succeeded at 1404 (offset 78 lines).
(CVS HEAD as of 2008-11-25 22:10+00)
* Does it include reasonable tests, docs, etc?
The patch modifies a single source file.
No docs are enclosed; the submission e-mail ends with "Obviously docs need to be updated as well."
No tests are enclosed.
Usability review
Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
It seems so.
* Do we want that?
Yes, may be useful for several reasons.
* Do we already have it?
No.
* Does it follow SQL spec, or the community-agreed behavior?
It is a configuration option, having nothing to do with SQL spec. About the latter, I didn't find anything in
themailing lists archive.
* Are there dangers? It seems not at a first glance.
* Have all the bases been covered?
Yes, it seems complete.
Feature test
Apply the patch, compile it and test:
* Does the feature work as advertised?
Yes. For the second revision of the patch I repeated exactly the same test procedure that I used for the first
revision,and described in the mail reported in the Appendix.
* Are there corner cases the author has failed to consider?
I don't know if there are systems where the username can begin with "/"; however, on such systems it would be
enoughto add another slash to the beginning and to escape any regexp-like character, so that the remaining part
willbe interpreted as it really is.
Performance review
* Does the patch slow down simple tests?
The patched code is triggered by a connection attempt. So in principle it could slow down, but in practice
networklag time should be fairly larger than the time consumed by this code.
* If it claims to improve performance, does it?
-
* Does it slow down other things?
Should not, and seems not to.
Coding review
Read the changes to the code in detail and consider:
* Does it follow the project coding guidelines?
Yes.
* Are there portability issues?
No, it is all about string manipulation and regexp matching.
* Will it work on Windows/BSD etc?
I don't see any reason to believe the contrary.
* Are the comments sufficient and accurate?
Yes. There is a minor change: in the commented phrase "This is replaced for $1 in the database username
string",the dollar character "$" should be replaced by the backslash "\".
* Does it do what it says, correctly?
To check it I had to: * compile and install patched HEAD * edit postgresql.conf in order to make it listen
froma different IP address * add to pg_hba.conf one line enabling ident connection mode for
connectionsfrom localhost, with map=review * add to pg_ident.conf some lines for the "review" map
* Does it produce compiler warnings?
No.
* Can you make it crash?
On the first version of the patch I found a statement causing a problem. The execution of
pfree(regexp_pgrole);raised the message: "WARNING: detected write past chunk end in Postmaster 0x9b13650".
On the second version of the patch the error seems to be disappeared.
Architecture review
Consider the changes to the code in the context of the project as a whole:
* Is everything done in a way that fits together coherently with other features/modules?
Yes, because it seems fairly independent on the rest.
* Are there interdependencies than can cause None that I can see.
Review review
* Did the reviewer cover all the things that kind of reviewer is supposed to do?
I followed the guidelines. As for the rest, maybe I shouldn't be the one who reviews my own review... :-)
= Appendix. =
On Wed, Nov 05, 2008 at 02:10:58PM +0100, Gianni Ciolli wrote:
> On Tue, Oct 28, 2008 at 08:59:53PM +0100, Magnus Hagander wrote:
> > The attached patch tries to implement regexp support in the usermaps
> > (pg_ident.conf).
>
> Hi Magnus,
>
> I am currently reviewing your patch.
>
> I found out that the execution of
>
> pfree(regexp_pgrole);
>
> (there's only one such line in your patch) might raise on the current
> HEAD the following message
>
> WARNING: detected write past chunk end in Postmaster 0x9b13650
>
> in the case of a regexp expansion.
>
> Let me describe the conditions under which I obtain that warning.
>
> To test it without having to create a new user I used the "identical"
> substitution
>
> review /(.*)nni \1nni
>
> in my pg_ident.conf so that I can trigger regexps while connecting as
> "gianni".
>
> To avoid removing all the "standard" lines from pg_hba.conf, I edited
> postgresql.conf in order to make it listen from a different IP address
> (actually the other IP address of my machine 192.168...). Then I added
> to pg_hba.conf one line enabling ident connections with map=review
> from that IP address.
>
> Finally I started the server, and as I connected with psql -h
> 192.168... the above warning showed.
>
> Best regards,
> Dr. Gianni Ciolli - 2ndQuadrant Italia
> PostgreSQL Training, Services and Support
> gianni.ciolli@2ndquadrant.it | www.2ndquadrant.it
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
Gianni Ciolli wrote: > On Tue, Nov 11, 2008 at 02:59:01PM +0100, Magnus Hagander wrote: >> Gianni Ciolli wrote: >>> WARNING: detected write past chunk end in Postmaster 0x9b13650 >> Yes, that's a stupid bug: > (...) >> Attached is an updated version of the patch that fixes this. > > Hi Magnus, > > please find below the review of the second version of your patch, > which I think is "Ready for Committer" now. Thanks for a very thorough review! I'll incorporate your suggested changes and do a commit soon. > = En-passant remarks = > > * I found out that the comments in the pg_hba.conf file installed by > default are obsolete; we should either update them or replace them > with a reference to the Manual, chapter "Client Authentication", > section "the pg_hba.conf file". I'll update the hba file for now - moving it to the manual and just reference that is outside the scope of this patch. //Magnus