httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Reid <da...@jetnet.co.uk>
Subject Re: [PATCH] ssl_ext_lookup #2
Date Wed, 14 Sep 2005 22:11:44 GMT
Joe Orton wrote:
> On Mon, Sep 12, 2005 at 04:02:02PM +0100, David Reid wrote:
> 
>>Following the comments from Joe, here is a revised patch that should
>>work better :-) I've tried to add a sensible comment about why we have
>>both functions listed.
> 
> 
> "OpenSSL... isn't up to much" isn't really very helpful (or sensible).  
> If the problem is that X509_ext_print will only handle particular types 
> of extension and that you can fall back on ASN1_print for extensions 
> which are simple e.g. string types then say that and cut out the waffle.

OK. Guess spending too long figuring out what did didn't work led me to
a dark place :-)

>>It removes the nastiness of the len pointer and also converts the
>>extlist fucntion to simply call into ssl_ext_lookup.
> 
> 
> That's pretty nasty, going through all the setup overhead and iterating 
> through the extension list again for each call.  But you miss my point: 
> the overlap in functionality is a bad thing not an missed opportunity 
> for refactoring.

OK, then what about the below.

> 
>>I've changed the log level down to INFO.

> 
> 
> DEBUG is the maximum acceptable IMO.

OK. OK. OK. I give in. DEBUG.

david


Index: modules/ssl/ssl_private.h
===================================================================
--- modules/ssl/ssl_private.h   (revision 279892)
+++ modules/ssl/ssl_private.h   (working copy)
@@ -646,10 +646,8 @@
 /**  Variables  */
 void         ssl_var_register(void);
 char        *ssl_var_lookup(apr_pool_t *, server_rec *, conn_rec *,
request_rec *, char *);
-const char  *ssl_ext_lookup(apr_pool_t *p, conn_rec *c, int peer, const
char *oid);
+apr_array_header_t *ssl_ext_list(apr_pool_t *p, conn_rec *c, int peer,
const char *extension);

-extern apr_array_header_t *ssl_extlist_by_oid(request_rec *r, const
char *oidstr);
-
 void         ssl_var_log_config_register(apr_pool_t *p);

 #define APR_SHM_MAXSIZE (64 * 1024 * 1024)
Index: modules/ssl/ssl_expr_eval.c
===================================================================
--- modules/ssl/ssl_expr_eval.c (revision 279892)
+++ modules/ssl/ssl_expr_eval.c (working copy)
@@ -198,63 +198,6 @@
     }
 }

-#define NUM_OID_ELTS 8 /* start with 8 oid slots, resize when needed */
-
-apr_array_header_t *ssl_extlist_by_oid(request_rec *r, const char *oidstr)
-{
-    int count = 0, j;
-    X509 *xs = NULL;
-    ASN1_OBJECT *oid;
-    apr_array_header_t *val_array;
-    SSLConnRec *sslconn = myConnConfig(r->connection);
-
-    /* trivia */
-    if (oidstr == NULL || sslconn == NULL || sslconn->ssl == NULL)
-        return NULL;
-
-    /* Determine the oid we are looking for */
-    if ((oid = OBJ_txt2obj(oidstr, 1)) == NULL) {
-        ERR_clear_error();
-        return NULL;
-    }
-
-    /* are there any extensions in the cert? */
-    if ((xs = SSL_get_peer_certificate(sslconn->ssl)) == NULL ||
-        (count = X509_get_ext_count(xs)) == 0) {
-        return NULL;
-    }
-
-    val_array = apr_array_make(r->pool, NUM_OID_ELTS, sizeof(char *));
-
-    /* Loop over all extensions, extract the desired oids */
-    for (j = 0; j < count; j++) {
-        X509_EXTENSION *ext = X509_get_ext(xs, j);
-
-        if (OBJ_cmp(ext->object, oid) == 0) {
-            BIO *bio = BIO_new(BIO_s_mem());
-
-            if (X509V3_EXT_print(bio, ext, 0, 0) == 1) {
-                BUF_MEM *buf;
-                char **new = apr_array_push(val_array);
-
-                BIO_get_mem_ptr(bio, &buf);
-
-                *new = apr_pstrdup(r->pool, buf->data);
-            }
-
-            BIO_vfree(bio);
-        }
-    }
-
-    X509_free(xs);
-    ERR_clear_error();
-
-    if (val_array->nelts == 0)
-        return NULL;
-    else
-        return val_array;
-}
-
 static BOOL ssl_expr_eval_oid(request_rec *r, const char *word, const
char *oidstr)
 {
     int j;
@@ -262,7 +205,7 @@
     apr_array_header_t *oid_array;
     char **oid_value;

-    if (NULL == (oid_array = ssl_extlist_by_oid(r, oidstr))) {
+    if (NULL == (oid_array = ssl_ext_list(r->pool, r->connection, 1,
oidstr))) {
         return FALSE;
     }

Index: modules/ssl/ssl_engine_vars.c
===================================================================
--- modules/ssl/ssl_engine_vars.c       (revision 279892)
+++ modules/ssl/ssl_engine_vars.c       (working copy)
@@ -62,7 +62,7 @@
 {
     APR_REGISTER_OPTIONAL_FN(ssl_is_https);
     APR_REGISTER_OPTIONAL_FN(ssl_var_lookup);
-    APR_REGISTER_OPTIONAL_FN(ssl_ext_lookup);
+    APR_REGISTER_OPTIONAL_FN(ssl_ext_list);
     return;
 }

@@ -660,23 +660,30 @@
     return result;
 }

-const char *ssl_ext_lookup(apr_pool_t *p, conn_rec *c, int peer,
-                           const char *oidnum)
+apr_array_header_t *ssl_ext_list(apr_pool_t *p, conn_rec *c, int peer,
+                                 const char *extension)
 {
     SSLConnRec *sslconn = myConnConfig(c);
-    SSL *ssl;
+    SSL *ssl = NULL;
+    apr_array_header_t *array = NULL;
     X509 *xs = NULL;
-    ASN1_OBJECT *oid;
+    ASN1_OBJECT *oid = NULL;
     int count = 0, j;
-    char *result = NULL;
-
-    if (!sslconn || !sslconn->ssl) {
+
+    if (!sslconn || !sslconn->ssl || !extension) {
         return NULL;
     }
     ssl = sslconn->ssl;

-    oid = OBJ_txt2obj(oidnum, 1);
+    /* We accept the "extension" string to be converted as
+     * a long name (nsComment), short name (DN) or
+     * numeric OID (1.2.3.4).
+     */
+    oid = OBJ_txt2obj(extension, 0);
     if (!oid) {
+        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
+                      "Failed to create an object for extension '%s'",
+                      extension);
         ERR_clear_error();
         return NULL;
     }
@@ -685,25 +692,42 @@
     if (xs == NULL) {
         return NULL;
     }
-
+
     count = X509_get_ext_count(xs);
+    /* Create an array large enough to accomodate every extension. This is
+     * likely overkill, but safe.
+     */
+    array = apr_array_make(p, count, sizeof(char *));
+    if (array) {
+        for (j = 0; j < count; j++) {
+            X509_EXTENSION *ext = X509_get_ext(xs, j);

-    for (j = 0; j < count; j++) {
-        X509_EXTENSION *ext = X509_get_ext(xs, j);
+            if (OBJ_cmp(ext->object, oid) == 0) {
+                BIO *bio = BIO_new(BIO_s_mem());

-        if (OBJ_cmp(ext->object, oid) == 0) {
-            BIO *bio = BIO_new(BIO_s_mem());
-
-            if (X509V3_EXT_print(bio, ext, 0, 0) == 1) {
-                BUF_MEM *buf;
-
-                BIO_get_mem_ptr(bio, &buf);
-                result = apr_pstrmemdup(p, buf->data, buf->length);
+                /* We want to obtain a string representation of the
extensions
+                 * value and add it to the array we're building.
+                 * X509V3_EXT_print() doesn't know about all the possible
+                 * data types, but the value is stored as an
ASN1_OCTET_STRING
+                 * allowing us a fallback in case of X509V3_EXT_print
+                 * not knowing how to handle the data.
+                 */
+                if (X509V3_EXT_print(bio, ext, 0, 0) == 1 ||
+                    ASN1_STRING_print(bio, ext->value) == 1) {
+                    BUF_MEM *buf;
+                    char **ptr = apr_array_push(array);
+                    BIO_get_mem_ptr(bio, &buf);
+                    *ptr = apr_pstrmemdup(p, buf->data, buf->length);
+                } else {
+                    ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
+                                  "Found an extension '%s', but failed to "
+                                  "create a string from it", extension);
+                }
+                BIO_vfree(bio);
             }
-
-            BIO_vfree(bio);
-            break;
         }
+        if (array->nelts == 0)
+            array = NULL;
     }

     if (peer) {
@@ -712,7 +736,7 @@
     }

     ERR_clear_error();
-    return result;
+    return array;
 }

 static char *ssl_var_lookup_ssl_compress_meth(SSL *ssl)
Index: modules/ssl/mod_ssl.c
===================================================================
--- modules/ssl/mod_ssl.c       (revision 279892)
+++ modules/ssl/mod_ssl.c       (working copy)
@@ -504,8 +504,6 @@

     APR_REGISTER_OPTIONAL_FN(ssl_proxy_enable);
     APR_REGISTER_OPTIONAL_FN(ssl_engine_disable);
-
-    APR_REGISTER_OPTIONAL_FN(ssl_extlist_by_oid);
 }

 module AP_MODULE_DECLARE_DATA ssl_module = {
Index: modules/ssl/mod_ssl.h
===================================================================
--- modules/ssl/mod_ssl.h       (revision 279892)
+++ modules/ssl/mod_ssl.h       (working copy)
@@ -36,15 +36,20 @@
                          conn_rec *, request_rec *,
                          char *));

-/** The ssl_ext_lookup() optional function retrieves the value of a SSL
- * certificate X.509 extension.  The client certificate is used if
- * peer is non-zero; the server certificate is used otherwise.  The
- * oidnum parameter specifies the numeric OID (e.g. "1.2.3.4") of the
- * desired extension.  The string value of the extension is returned,
- * or NULL on error. */
-APR_DECLARE_OPTIONAL_FN(const char *, ssl_ext_lookup,
+/** The ssl_ext_list() optional function attempts to build an array
+ * of all the values contained in the named X.509 extension. The
+ * returned array will be created in the supplied pool.
+ * The client certificate is used if peer is non-zero; the server
+ * certificate is used otherwise.
+ * Extension specifies the extensions to use as a string. This can be
+ * one of the "known" long or short names, or a numeric OID,
+ * e.g. "1.2.3.4", 'nsComment' and 'DN' are all valid.
+ * A pointer to an apr_array_header_t structure is returned if at
+ * least one matching extension is found, NULL otherwise.
+ */
+APR_DECLARE_OPTIONAL_FN(apr_array_header_t *, ssl_ext_list,
                         (apr_pool_t *p, conn_rec *c, int peer,
-                         const char *oidnum));
+                         const char *extension));

 /** An optional function which returns non-zero if the given connection
  * is using SSL/TLS. */
@@ -58,7 +63,5 @@

 APR_DECLARE_OPTIONAL_FN(int, ssl_engine_disable, (conn_rec *));

-APR_DECLARE_OPTIONAL_FN(apr_array_header_t *, ssl_extlist_by_oid,
(request_rec *r, const char *oidstr));
-
 #endif /* __MOD_SSL_H__ */
 /** @} */

Mime
View raw message