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
David Reid wrote: Joe Orton wrote: On Fri, Aug 05, 2005 at 08:00:01PM +0200, Martin Kraemer wrote: On Tue, Aug 02, 2005 at 07:14:10PM +0200, Martin Kraemer wrote: I wanted something like SSLRequire committers in SSLPeerExtList(1.3.6.1.4.1.18060.1); to mean at least one extension with an OID of 1.3.6.1.4.1.18060.1 with a value of 'committers' exists in the client cert. I'll be on vacation next week, and will send in another patch after that. OK, hope you had a good holiday. I wasn't trying to argue about the semantics just to nitpick the naming. Having SSL in the SSLRequire function is redundant, but not in the context of mod_setenvif. So, my preference is: SSLRequire committers in PeerExtList(1.3.6.1.4.1.18060.1); SetEnvIf SSL_PeerExtList(etc) ... +1 on the revised naming. Patch looks OK as well. Late to the party here, but giving the same function two different names sucks! Or am I misunderstanding? Cheers, Ben. -- http://www.apache-ssl.org/ben.html http://www.thebunker.net/ There is no limit to what a man can do or how far he can go if he doesn't mind who gets the credit. - Robert Woodruff
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
Joe Orton wrote: On Fri, Aug 05, 2005 at 08:00:01PM +0200, Martin Kraemer wrote: On Tue, Aug 02, 2005 at 07:14:10PM +0200, Martin Kraemer wrote: I wanted something like SSLRequire committers in SSLPeerExtList(1.3.6.1.4.1.18060.1); to mean at least one extension with an OID of 1.3.6.1.4.1.18060.1 with a value of 'committers' exists in the client cert. I'll be on vacation next week, and will send in another patch after that. OK, hope you had a good holiday. I wasn't trying to argue about the semantics just to nitpick the naming. Having SSL in the SSLRequire function is redundant, but not in the context of mod_setenvif. So, my preference is: SSLRequire committers in PeerExtList(1.3.6.1.4.1.18060.1); SetEnvIf SSL_PeerExtList(etc) ... +1 on the revised naming. Patch looks OK as well. david
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
On Mon, Aug 15, 2005 at 02:36:18PM +0100, Joe Orton wrote: OK, hope you had a good holiday. I wasn't trying to argue about the semantics just to nitpick the naming. Having SSL in the SSLRequire function is redundant, but not in the context of mod_setenvif. So, my preference is: SSLRequire committers in PeerExtList(1.3.6.1.4.1.18060.1); SetEnvIf SSL_PeerExtList(etc) ... +1 on concept /peanutgallery vh Mads Toftum -- `Darn it, who spiked my coffee with water?!' - lwall
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
On Mon, Aug 15, 2005 at 02:36:18PM +0100, Joe Orton wrote: I just went to write a test case for the SetEnvIf function, and there seems to be a rather annoying fundamental problem: the match_headers hooks runs too early to be useful for this when doing per-dir client cert negotiation. I can't see any simple way round this, and I don't think this feature should go in 2.2 unless this can be solved. Any ideas? joe
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
Joe Orton wrote: On Mon, Aug 15, 2005 at 02:36:18PM +0100, Joe Orton wrote: I just went to write a test case for the SetEnvIf function, and there seems to be a rather annoying fundamental problem: the match_headers hooks runs too early to be useful for this when doing per-dir client cert negotiation. I can't see any simple way round this, and I don't think this feature should go in 2.2 unless this can be solved. Any ideas? I've not looked at it in detail, so would have to dig through the code. Care to post your test case? david
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
On Tue, Aug 16, 2005 at 04:45:41PM +0100, David Reid wrote: Joe Orton wrote: On Mon, Aug 15, 2005 at 02:36:18PM +0100, Joe Orton wrote: I just went to write a test case for the SetEnvIf function, and there seems to be a rather annoying fundamental problem: the match_headers hooks runs too early to be useful for this when doing per-dir client cert negotiation. I can't see any simple way round this, and I don't think this feature should go in 2.2 unless this can be solved. Any ideas? I've not looked at it in detail, so would have to dig through the code. Care to post your test case? Well, try testing it in any configuration with SSLVerifyClient require in Directory or Location context rather than in the vhost context. httpd-test test case: mkdir t/htdocs/modules/setenvif/ssl + applying Index: t/conf/extra.conf.in === --- t/conf/extra.conf.in(revision 231019) +++ t/conf/extra.conf.in(working copy) @@ -358,6 +358,11 @@ Options +Includes AllowOverride All /Directory + +Directory @SERVERROOT@/htdocs/modules/setenvif/ssl +Options +Includes +AllowOverride All +/Directory /IfModule ## Index: t/htdocs/modules/setenvif/ssl/.htaccess === --- t/htdocs/modules/setenvif/ssl/.htaccess (revision 0) +++ t/htdocs/modules/setenvif/ssl/.htaccess (revision 0) @@ -0,0 +1 @@ +SetEnvIf SSL_PeerExtList(2.16.840.1.113730.1.13) (.*) NETSCAPE_COMMENT=$1 Property changes on: t/htdocs/modules/setenvif/ssl/.htaccess ___ Name: svn:eol-style + native Index: t/htdocs/modules/setenvif/ssl/peerextlist.shtml === --- t/htdocs/modules/setenvif/ssl/peerextlist.shtml (revision 0) +++ t/htdocs/modules/setenvif/ssl/peerextlist.shtml (revision 0) @@ -0,0 +1 @@ +0:!--#echo var=NETSCAPE_COMMENT-- Property changes on: t/htdocs/modules/setenvif/ssl/peerextlist.shtml ___ Name: svn:eol-style + native Index: t/ssl/setenvif.t === --- t/ssl/setenvif.t(revision 0) +++ t/ssl/setenvif.t(revision 0) @@ -0,0 +1,21 @@ +use strict; +use warnings FATAL = 'all'; + +use Apache::Test; +use Apache::TestRequest; +use Apache::TestUtil; + +plan tests = 2, need 'setenvif', need_min_apache_version(2.1.6); + +Apache::TestRequest::scheme(https); + +my $r = GET(/require/asf/modules/setenvif/ssl/peerextlist.shtml, cert = 'client_ok'); + +ok t_cmp($r-code, 200, fetched page works); + +my $c = $r-content; + +chomp $c; + +ok t_cmp($c, 0:This Is A Comment, Retrieve nsComment extension); +
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
On Fri, Aug 05, 2005 at 08:00:01PM +0200, Martin Kraemer wrote:
On Tue, Aug 02, 2005 at 07:14:10PM +0200, Martin Kraemer wrote:
I wanted something like
SSLRequire committers in SSLPeerExtList(1.3.6.1.4.1.18060.1);
to mean at least one extension with an OID of
1.3.6.1.4.1.18060.1 with a value of 'committers' exists in the
client cert.
I'll be on vacation next week, and will send in another patch after
that.
OK, hope you had a good holiday. I wasn't trying to argue about the
semantics just to nitpick the naming. Having SSL in the SSLRequire
function is redundant, but not in the context of mod_setenvif. So, my
preference is:
SSLRequire committers in PeerExtList(1.3.6.1.4.1.18060.1);
SetEnvIf SSL_PeerExtList(etc) ...
I just went to write a test case for the SetEnvIf function, and there
seems to be a rather annoying fundamental problem: the match_headers
hooks runs too early to be useful for this when doing per-dir client
cert negotiation.
Attached the patch I have for mod_setenvif to clean it up and adopt the
naming above; untested so far as I'm blocked by the fact that it doesn't
work for per-dir negotiation.
joe
Index: modules/metadata/mod_setenvif.c
===
--- modules/metadata/mod_setenvif.c (revision 232271)
+++ modules/metadata/mod_setenvif.c (working copy)
@@ -104,7 +104,7 @@
SPECIAL_REQUEST_METHOD,
SPECIAL_REQUEST_PROTOCOL,
SPECIAL_SERVER_ADDR,
-SPECIAL_OID_VALUE
+SPECIAL_SSL_PEEREXTLIST
};
typedef struct {
char *name; /* header name */
@@ -349,30 +349,30 @@
else if (!strcasecmp(fname, server_addr)) {
new-special_type = SPECIAL_SERVER_ADDR;
}
-else if (!strncasecmp(fname, oid(,4)) {
-ap_regmatch_t match[AP_MAX_REG_MATCH];
+else if (!strncasecmp(fname, ssl_peerextlist(,
+ sizeof(ssl_peerextlist() - 1)) {
+const char *oid, *oidend;
-new-special_type = SPECIAL_OID_VALUE;
+new-special_type = SPECIAL_SSL_PEEREXTLIST;
-/* Syntax check and extraction of the OID as a regex: */
-new-pnamereg = ap_pregcomp(cmd-pool,
-^oid\\(\?([0-9.]+)\?\\)$,
-(AP_REG_EXTENDED // | AP_REG_NOSUB
- | AP_REG_ICASE));
-/* this can never happen, as long as pcre works:
- if (new-pnamereg == NULL)
-return apr_pstrcat(cmd-pool, cmd-cmd-name,
- OID regex could not be compiled.,
NULL);
- */
-if (ap_regexec(new-pnamereg, fname, AP_MAX_REG_MATCH, match, 0)
== AP_REG_NOMATCH) {
+oid = fname + strlen(ssl_peerextlist();
+oidend = ap_strchr_c(oid, ')');
+
+/* skip over quotes if present */
+if (oid[0] == '') {
+oid++;
+}
+if (oidend oidend[-1] == '') {
+oidend--;
+}
+
+if (!oidend || oidend = oid) {
return apr_pstrcat(cmd-pool, cmd-cmd-name,
- OID syntax is: oid(\1.2.3.4.5\);
error in: ,
- fname, NULL);
+ invalid ssl_peerextlist() syntax in: ,
+ fname, NULL);
}
-new-pnamereg = NULL;
-/* The name field is used for the stripped oid string */
-new-name = fname = apr_pstrdup(cmd-pool, fname+match[1].rm_so);
-fname[match[1].rm_eo - match[1].rm_so] = '\0';
+
+new-name = apr_pstrmemdup(cmd-pool, oid, oidend - oid);
}
else {
new-special_type = SPECIAL_NOT;
@@ -504,8 +504,10 @@
* same header. Remember we don't need to strcmp the two header
* names because we made sure the pointers were equal during
* configuration.
- * In the case of SPECIAL_OID_VALUE values, each oid string is
- * dynamically allocated, thus there are no duplicates.
+ *
+ * In the case of SPECIAL_SSL_PEEREXTLIST values, each
+ * extension list is dynamically allocated, thus there are no
+ * duplicates.
*/
if (b-name != last_name) {
last_name = b-name;
@@ -529,32 +531,19 @@
case SPECIAL_REQUEST_PROTOCOL:
val = r-protocol;
break;
-case SPECIAL_OID_VALUE:
+case SPECIAL_SSL_PEEREXTLIST:
/* If mod_ssl is not loaded, the accessor function is NULL */
-if (ssl_extlist_by_oid_func != NULL)
-{
-apr_array_header_t *oid_array;
-char **oid_value;
-int j, len = 0;
-char
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
On Tue, Aug 02, 2005 at 07:14:10PM +0200, Martin Kraemer wrote: I wanted something like SSLRequire committers in SSLPeerExtList(1.3.6.1.4.1.18060.1); to mean at least one extension with an OID of 1.3.6.1.4.1.18060.1 with a value of 'committers' exists in the client cert. I'll be on vacation next week, and will send in another patch after that. L8er, Martin -- [EMAIL PROTECTED] | Fujitsu Siemens Fon: +49-89-636-46021, FAX: +49-89-636-48332 | 81730 Munich, Germany
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
On Fri, Jul 22, 2005 at 02:24:50PM +0200, Sander Striker wrote: Joe Orton wrote: On Fri, Jul 22, 2005 at 12:11:56PM -, Martin Kraemer wrote: Author: martin Date: Fri Jul 22 05:11:55 2005 New Revision: 220307 URL: http://svn.apache.org/viewcvs?rev=220307view=rev Log: Allow extraction of the values of SSL certificate extensions into environment variables, so that their value can be used by any module that is aware of environment variables, as in: So what is the point in posting patches for review if you don't actually wait for anyone to review them before committing? That would be my fault. We're here at ApacheCon and when Martin said he posted to the list first I asked him why he didn't commit to trunk directly, since that is C-T-R. It's a reasonable smallish patch, with little impact on existing functionality; hence the nudge. C-T-R is a good way to accumulate a codebase full of unfinished changes if the R bit is ignored by the committer. Ping Martin. http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL PROTECTED] http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL PROTECTED] http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL PROTECTED] joe
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
On Tue, Aug 02, 2005 at 12:00:24PM +0100, Joe Orton wrote:
On Fri, Jul 22, 2005 at 02:24:50PM +0200, Sander Striker wrote:
Joe Orton wrote:
On Fri, Jul 22, 2005 at 12:11:56PM -, Martin Kraemer wrote:
Author: martin
Date: Fri Jul 22 05:11:55 2005
New Revision: 220307
URL: http://svn.apache.org/viewcvs?rev=220307view=rev
Log:
Allow extraction of the values of SSL certificate extensions into
environment variables, so that their value can be used by any
module that is aware of environment variables, as in:
So what is the point in posting patches for review if you don't actually
wait for anyone to review them before committing?
That would be my fault. We're here at ApacheCon and when Martin said
he posted to the list first I asked him why he didn't commit to trunk
directly, since that is C-T-R. It's a reasonable smallish patch, with
little impact on existing functionality; hence the nudge.
C-T-R is a good way to accumulate a codebase full of unfinished changes
if the R bit is ignored by the committer. Ping Martin.
http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL
PROTECTED]
http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL
PROTECTED]
http://mail-archives.apache.org/mod_mbox/httpd-dev/200507.mbox/[EMAIL
PROTECTED]
Oops, sorry. Thanks for pinging.
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?
2) you must always check in the regenerated generated scanner source
along with changes to the lex file.
My bad, sorry for missing that. Committed right now.
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.
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.
In ssl_peerext.patch there is a patch which changes OID()
to SSLPeerExt() for mod_ssl.
Martin
--
[EMAIL PROTECTED] | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-48332 | 81730 Munich, Germany
Index: ssl_expr.h
===
--- ssl_expr.h (Revision 226989)
+++ ssl_expr.h (Arbeitskopie)
@@ -61,7 +61,7 @@
#endif
typedef enum {
-op_NOP, op_ListElement, op_OidListElement,
+op_NOP, op_ListElement, op_PeerExtElement,
op_True, op_False, op_Not, op_Or, op_And, op_Comp,
op_EQ, op_NE, op_LT, op_LE, op_GT, op_GE, op_IN, op_REG, op_NRE,
op_Digit, op_String, op_Regex, op_Var, op_Func
Index: ssl_expr_eval.c
===
--- ssl_expr_eval.c (Revision 226989)
+++ ssl_expr_eval.c (Arbeitskopie)
@@ -118,7 +118,7 @@
e3 = (ssl_expr *)e2-node_arg1;
e2 = (ssl_expr *)e2-node_arg2;
-if (op == op_OidListElement) {
+if (op == op_PeerExtElement) {
char *w3 = ssl_expr_eval_word(r, e3);
found = ssl_expr_eval_oid(r, w1, w3);
@@ -204,7 +204,7 @@
{
int count = 0, j;
X509 *xs = NULL;
-ASN1_OBJECT *oid;
+ASN1_OBJECT *peerext;
apr_array_header_t *val_array;
SSLConnRec *sslconn = myConnConfig(r-connection);
@@ -212,8 +212,8 @@
if (oidstr == NULL || sslconn == NULL || sslconn-ssl == NULL)
return NULL;
-/* Determine the oid we are looking for */
-if ((oid =
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
On Fri, Jul 22, 2005 at 12:11:56PM -, Martin Kraemer wrote: Author: martin Date: Fri Jul 22 05:11:55 2005 New Revision: 220307 URL: http://svn.apache.org/viewcvs?rev=220307view=rev Log: Allow extraction of the values of SSL certificate extensions into environment variables, so that their value can be used by any module that is aware of environment variables, as in: So what is the point in posting patches for review if you don't actually wait for anyone to review them before committing? 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. mod_setenvif.c: module AP_MODULE_DECLARE_DATA setenvif_module; +#if (MODULE_MAGIC_NUMBER_MAJOR 20020903) +#include mod_ssl.h unnecessary for trunk code to care about the MMN, it can always rely on mod_ssl.h being available. mod_ssl.h: +extern apr_array_header_t *ssl_extlist_by_oid(request_rec *r, const char *oidstr); and don't export the function as well as the optional function.
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
Joe Orton wrote: On Fri, Jul 22, 2005 at 12:11:56PM -, Martin Kraemer wrote: Author: martin Date: Fri Jul 22 05:11:55 2005 New Revision: 220307 URL: http://svn.apache.org/viewcvs?rev=220307view=rev Log: Allow extraction of the values of SSL certificate extensions into environment variables, so that their value can be used by any module that is aware of environment variables, as in: So what is the point in posting patches for review if you don't actually wait for anyone to review them before committing? That would be my fault. We're here at ApacheCon and when Martin said he posted to the list first I asked him why he didn't commit to trunk directly, since that is C-T-R. It's a reasonable smallish patch, with little impact on existing functionality; hence the nudge. Sander
