httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r220307 - in /httpd/httpd/trunk/modules: metadata/mod_setenvif.c ssl/mod_ssl.c ssl/mod_ssl.h ssl/ssl_expr_eval.c
Date Tue, 02 Aug 2005 14:03:45 GMT
On Tue, Aug 02, 2005 at 03:23:44PM +0200, Martin Kraemer wrote:
> On Tue, Aug 02, 2005 at 12:00:24PM +0100, Joe Orton wrote:
> >> 1) this is a pretty specific to way to code it.  Is there no way to make
> >> it more general so that OID() is just a function like file() and can be
> >> used e.g. in regex matches too?
> 
> The problem with the OID() "function" is that it where file() (or
> another file() like function) return a single value, what OID()
> stands for is an "array of zero or more values". But there is no
> syntax to deal with arrays in place of expressions. I tried to
> implement it as function first, but noticed that it would break when
> an OID was specified more than once. In the ASF scenario, the
> intention is to add multiple extensions with this OID, each one
> containing as value a group name of which the client is member.
> 
> Because of the pre-existing syntax "<expr> in {value,value}", and
> because "{value,value}" is effectively an array, I chose to implement
> the OID() "function" as a special case of the "<expr> in" operator.
> 
> Do you have a good idea how to use a function-like syntax, and still
> maintain the "is an array" property?

No, OK, that makes sense.  Perhaps a more general implementation would 
be to have a "peerextlist()" which evalutes to a "wordlist" array like 
the current function, along with a "peerext()" which evaluates to a 
"word" (just the first extension with given name).

> >> 3) oid() is a terrible name for this; it's simply the type of the
> >> parameter.  It would be like calling malloc() "size()".  The function
> >> expands (conceptually) to the values of an extension in the peer's
> >> certificate, identified by OID; so call it peerext() or something
> >> meaningful like that.
> 
> Good point - Thanks a lot -- that is a *very* good idea, and (if
> nobody objects) I'd want to follow this suggestion. I had been a
> little unhappy with OID() myself. peerext() is especially good
> because it also documents where the certificate came from.

I'd prefer peerexts() or peerextlist() given the above, to emphasize the 
the fact that it returns a list, and to allow a peerext() which doesn't 
do that, in the future.

> >> >   SetEnvIf OID("2.16.840.1.113730.1.13") "(.*) Generated (Certificate)"
ca=$1
> >> 
> >> -1 on the naming since OID is completely entirely meaningless in this
> >> context.
> 
> In the context of mod_setenvif, I'd even use "SSLPeerExt()" because
> it makes it clear that we are dealing with an SSL-related thing.
>
> Patch <<mod_setenvif.c.patch>> attached.

Likewise on naming; sslpeerexts() or sslpeerextlist() seems better 
although it's perhaps getting a little long. 

Also note that apr_array_pstrcat(pool, array, ',') can be used instead 
of all that code to flatten the array to a string.
 
> In <<ssl_peerext.patch>> there is a patch which changes OID()
> to SSLPeerExt() for mod_ssl.

Other than the choice of name, this looks fine.  Please split out the 
unrelated change to match tokens case-insensitively to a separate 
commit.

Regards,

joe


Mime
View raw message