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
Attachments:
ident_regexp.difftext/x-diff; name=ident_regexp.diffDownload
*** 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 messageWARNING: 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
Attachments:
ident_regexp.patchtext/x-diff; name=ident_regexp.patchDownload
*** 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 them or 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 the mailing
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 enough to add
another slash to the beginning and to escape any regexp-like
character, so that the remaining part will be 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 network lag 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 from a
different IP address
* add to pg_hba.conf one line enabling ident connection mode
for connections from 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. =
Show quoted text
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 messageWARNING: 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"
substitutionreview /(.*)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