[REVIEW] (was Re: usermap regexp support) - Mailing list pgsql-hackers

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