[PATCH] Proxy remote server SSL certificate verification

Aviram Cohen aviram at adallom.com
Wed Oct 16 12:28:09 UTC 2013


Hello!

I've fixed the comments and made a new version (for a newer Nginx version).
I haven't removed the name verification however. OpenSSL's X509_check_host will
be available in v 1.1.0, and it doesn't seem to be available soon, and name
verification is significant when verifying SSL certificates.

The new version is here
https://gist.github.com/aviramc/7006607

and here:

# HG changeset patch
# User Aviram Cohen <aviram at adallom.com>
# Date 1381924204 -7200
# Node ID eb4a27153a24e4477d9074bd51ba56ce58be4177
# Parent  70c5cd3a61cb476c2afb3a61826e59c7cda0b7a7
Added remote end SSL certificate verification in the proxy module.

This patch adds the following directives to the proxy module:
 - proxy_ssl_verify - whether or not to verify the remote end's
   certificate. Default is off.
 - proxy_ssl_verify_name - the remote end's name to verify. If no
   value is given, name verification is turned off.
 - proxy_ssl_verify_depth - how deep the ssl verification should be
   done. Default is 1.
 - proxy_ssl_trusted_certificate - the path of the certificate file
   that is used for verification. This must be provided when
   proxy_ssl_verify is on.
 - proxy_ssl_crl - the path a file that contains the CRLs of the hosts
   to which we proxy. Default is empty, and CRL verification is not done.

diff -r 70c5cd3a61cb -r eb4a27153a24 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Tue Oct 01 17:44:51 2013 +0400
+++ b/src/event/ngx_event_openssl.c Wed Oct 16 13:50:04 2013 +0200
@@ -38,6 +38,11 @@ static void ngx_ssl_expire_sessions(ngx_
 static void ngx_ssl_session_rbtree_insert_value(ngx_rbtree_node_t *temp,
     ngx_rbtree_node_t *node, ngx_rbtree_node_t *sentinel);

+static int ngx_ssl_host_wildcard_match(ASN1_STRING *pattern,
+    ngx_str_t *hostname);
+static int ngx_ssl_host_exact_match(ASN1_STRING *pattern,
+    ngx_str_t *hostname);
+
 static void *ngx_openssl_create_conf(ngx_cycle_t *cycle);
 static char *ngx_openssl_engine(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
 static void ngx_openssl_exit(ngx_cycle_t *cycle);
@@ -2541,6 +2546,159 @@ ngx_ssl_get_client_verify(ngx_connection
 }


+static int
+ngx_ssl_host_wildcard_match(ASN1_STRING *pattern,
+    ngx_str_t *hostname)
+{
+    int        n;
+    u_char    *p;
+    u_char    *wp;
+
+    /* sanity check */
+    if (!pattern
+        || pattern->length <= 0
+        || !hostname
+        || !hostname->len)
+    {
+        return 0;
+    }
+
+    /* trivial case */
+    if (ngx_strncasecmp((u_char *) pattern->data,
+                        hostname->data,
+                        hostname->len)
+        == 0)
+    {
+        return 1;
+    }
+
+    /* simple wildcard matching - only in the beginning of the string. */
+    if (pattern->length > 2
+        && pattern->data[0] == '*'
+        && pattern->data[1] == '.')
+    {
+
+        wp = (u_char *) (pattern->data + 1);
+
+        p = ngx_strlchr(hostname->data,
+                        hostname->data + hostname->len,
+                        '.');
+
+        /*
+         *  If the pattern begings with "*." and the hostname consists of
+         *  a top level domain, compare the pattern to the top level domain.
+         */
+        if (p != NULL) {
+            n = hostname->len - (int) (p - hostname->data);
+
+            if (n == pattern->length - 1
+                && ngx_strncasecmp(wp,
+                                   p,
+                                   pattern->length - 1)
+                   == 0)
+            {
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+
+static int
+ngx_ssl_host_exact_match(ASN1_STRING *pattern,
+    ngx_str_t *hostname)
+{
+    /* sanity check */
+    if (!pattern
+        || pattern->length <= 0
+        || !hostname
+        || !hostname->len
+        || pattern->length != (int) hostname->len)
+    {
+        return 0;
+    }
+
+    if (ngx_strncmp((u_char *) pattern->data,
+                    hostname->data,
+                    hostname->len)
+        == 0)
+    {
+        return 1;
+    }
+
+    return 0;
+}
+
+
+ngx_int_t
+ngx_ssl_verify_name(X509 *cert, ngx_str_t *name)
+{
+    X509_NAME_ENTRY *ne;
+    GENERAL_NAMES   *gens;
+    GENERAL_NAME    *gen;
+    ASN1_STRING     *cstr;
+    X509_NAME       *sn;
+    int              rc;
+    int              i;
+
+    /* based on OpenSSL's do_x509_check */
+
+    gens = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
+
+    if (gens) {
+
+        rc = 0;
+
+        for (i = 0; i < sk_GENERAL_NAME_num(gens); i++) {
+
+            gen = sk_GENERAL_NAME_value(gens, i);
+
+            /* we only check for name */
+            switch (gen->type) {
+
+            case GEN_DNS:
+                cstr = gen->d.dNSName;
+                rc = ngx_ssl_host_wildcard_match(cstr,
+                                                 name);
+                break;
+
+            default:
+                cstr = NULL;
+                rc = 0;
+            }
+
+            if (rc) {
+                break;
+            }
+
+        }
+
+        GENERAL_NAMES_free(gens);
+
+        if (rc) {
+            return NGX_OK;
+        }
+    }
+
+    sn = X509_get_subject_name(cert);
+    i = X509_NAME_get_index_by_NID(sn, NID_commonName, -1);
+    while (i >= 0) {
+        ne = X509_NAME_get_entry(sn, i);
+        cstr = X509_NAME_ENTRY_get_data(ne);
+
+        if (ngx_ssl_host_exact_match(cstr, name)) {
+            return NGX_OK;
+        }
+
+        i = X509_NAME_get_index_by_NID(sn, NID_commonName, i);
+    }
+
+    return NGX_ERROR;
+}
+
+
 static void *
 ngx_openssl_create_conf(ngx_cycle_t *cycle)
 {
diff -r 70c5cd3a61cb -r eb4a27153a24 src/event/ngx_event_openssl.h
--- a/src/event/ngx_event_openssl.h Tue Oct 01 17:44:51 2013 +0400
+++ b/src/event/ngx_event_openssl.h Wed Oct 16 13:50:04 2013 +0200
@@ -158,6 +158,7 @@ ngx_int_t ngx_ssl_get_client_verify(ngx_


 ngx_int_t ngx_ssl_handshake(ngx_connection_t *c);
+ngx_int_t ngx_ssl_verify_name(X509 *cert, ngx_str_t *name);
 ssize_t ngx_ssl_recv(ngx_connection_t *c, u_char *buf, size_t size);
 ssize_t ngx_ssl_write(ngx_connection_t *c, u_char *data, size_t size);
 ssize_t ngx_ssl_recv_chain(ngx_connection_t *c, ngx_chain_t *cl);
diff -r 70c5cd3a61cb -r eb4a27153a24 src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c Tue Oct 01 17:44:51 2013 +0400
+++ b/src/http/modules/ngx_http_proxy_module.c Wed Oct 16 13:50:04 2013 +0200
@@ -81,6 +81,10 @@ typedef struct {
     ngx_uint_t                     ssl;
     ngx_uint_t                     ssl_protocols;
     ngx_str_t                      ssl_ciphers;
+    ngx_uint_t                     ssl_verify_depth;
+    ngx_str_t                      ssl_trusted_certificate;
+    ngx_str_t                      ssl_crl;
+    ngx_http_complex_value_t       ssl_verify_name;
 #endif
 } ngx_http_proxy_loc_conf_t;

@@ -168,6 +172,8 @@ static ngx_int_t ngx_http_proxy_rewrite_
 #if (NGX_HTTP_SSL)
 static ngx_int_t ngx_http_proxy_set_ssl(ngx_conf_t *cf,
     ngx_http_proxy_loc_conf_t *plcf);
+static char * ngx_http_proxy_verify_name(ngx_conf_t *cf, ngx_command_t *cmd,
+    void *conf);
 #endif
 static void ngx_http_proxy_set_vars(ngx_url_t *u, ngx_http_proxy_vars_t *v);

@@ -546,6 +552,41 @@ static ngx_command_t  ngx_http_proxy_com
       offsetof(ngx_http_proxy_loc_conf_t, ssl_ciphers),
       NULL },

+    { ngx_string("proxy_ssl_verify"),
+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+      ngx_conf_set_flag_slot,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_verify),
+      NULL },
+
+    { ngx_string("proxy_ssl_verify_name"),
+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+      ngx_http_proxy_verify_name,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      0,
+      NULL },
+
+    { ngx_string("proxy_ssl_verify_depth"),
+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+      ngx_conf_set_num_slot,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      offsetof(ngx_http_proxy_loc_conf_t, ssl_verify_depth),
+      NULL },
+
+    { ngx_string("proxy_ssl_trusted_certificate"),
+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+      ngx_conf_set_str_slot,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      offsetof(ngx_http_proxy_loc_conf_t, ssl_trusted_certificate),
+      NULL },
+
+    { ngx_string("proxy_ssl_crl"),
+      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+      ngx_conf_set_str_slot,
+      NGX_HTTP_LOC_CONF_OFFSET,
+      offsetof(ngx_http_proxy_loc_conf_t, ssl_crl),
+      NULL },
+
 #endif

       ngx_null_command
@@ -698,6 +739,20 @@ ngx_http_proxy_handler(ngx_http_request_
         }
     }

+#if (NGX_HTTP_SSL)
+
+    if (plcf->ssl
+        && plcf->ssl_verify_name.value.data
+        && ngx_http_complex_value(r,
+                                  &plcf->ssl_verify_name,
+                                  &u->ssl_verify_name)
+           != NGX_OK)
+    {
+        return NGX_ERROR;
+    }
+
+#endif
+
     u->output.tag = (ngx_buf_tag_t) &ngx_http_proxy_module;

     u->conf = &plcf->upstream;
@@ -2460,8 +2515,11 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
     conf->upstream.pass_headers = NGX_CONF_UNSET_PTR;

     conf->upstream.intercept_errors = NGX_CONF_UNSET;
+
 #if (NGX_HTTP_SSL)
     conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
+    conf->upstream.ssl_verify = NGX_CONF_UNSET;
+    conf->ssl_verify_depth = NGX_CONF_UNSET_UINT;
 #endif

     /* "proxy_cyclic_temp_file" is disabled */
@@ -2740,10 +2798,45 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t

     ngx_conf_merge_str_value(conf->ssl_ciphers, prev->ssl_ciphers,
                              "DEFAULT");
+    ngx_conf_merge_value(conf->upstream.ssl_verify,
+                              prev->upstream.ssl_verify, 0);
+    ngx_conf_merge_uint_value(conf->ssl_verify_depth,
+                              prev->ssl_verify_depth, 1);
+    ngx_conf_merge_str_value(conf->ssl_trusted_certificate,
+                             prev->ssl_trusted_certificate, "");
+    ngx_conf_merge_str_value(conf->ssl_crl,
+                             prev->ssl_crl, "");

     if (conf->ssl && ngx_http_proxy_set_ssl(cf, conf) != NGX_OK) {
         return NGX_CONF_ERROR;
     }
+
+    if (conf->ssl_verify_name.value.data == NULL) {
+        conf->ssl_verify_name = prev->ssl_verify_name;
+    }
+
+    if (conf->ssl && conf->upstream.ssl && conf->upstream.ssl_verify) {
+        if (conf->ssl_trusted_certificate.len == 0) {
+            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+                               "no \"proxy_ssl_trusted_certificate\" is "
+                               " defined for the \"proxy_ssl_verify\" "
+                               "directive");
+
+            return NGX_CONF_ERROR;
+        }
+
+        if (ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
+                                        &conf->ssl_trusted_certificate,
+                                        conf->ssl_verify_depth)
+            != NGX_OK)
+        {
+            return NGX_CONF_ERROR;
+        }
+
+        if (ngx_ssl_crl(cf, conf->upstream.ssl, &conf->ssl_crl) != NGX_OK) {
+            return NGX_CONF_ERROR;
+        }
+    }
 #endif

     ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
@@ -3811,6 +3904,35 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, n
     return NGX_OK;
 }

+
+char *
+ngx_http_proxy_verify_name(ngx_conf_t *cf, ngx_command_t *cmd,
+    void *conf)
+{
+    ngx_http_proxy_loc_conf_t *plcf = conf;
+
+    ngx_str_t                          *value;
+    ngx_http_compile_complex_value_t    ccv;
+
+    value = cf->args->elts;
+
+    if (plcf->ssl_verify_name.value.data) {
+        return "is duplicate";
+    }
+
+    ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
+
+    ccv.cf = cf;
+    ccv.value = &value[1];
+    ccv.complex_value = &plcf->ssl_verify_name;
+
+    if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
+        return NGX_CONF_ERROR;
+    }
+
+    return NGX_CONF_OK;
+}
+
 #endif


diff -r 70c5cd3a61cb -r eb4a27153a24 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c Tue Oct 01 17:44:51 2013 +0400
+++ b/src/http/ngx_http_upstream.c Wed Oct 16 13:50:04 2013 +0200
@@ -1372,6 +1372,8 @@ ngx_http_upstream_ssl_init_connection(ng
 static void
 ngx_http_upstream_ssl_handshake(ngx_connection_t *c)
 {
+    X509                 *cert;
+    long                  rc;
     ngx_http_request_t   *r;
     ngx_http_upstream_t  *u;

@@ -1379,7 +1381,37 @@ ngx_http_upstream_ssl_handshake(ngx_conn
     u = r->upstream;

     if (c->ssl->handshaked) {
-
+        if (u->conf->ssl_verify) {
+
+            rc = SSL_get_verify_result(c->ssl->connection);
+
+            if (rc != X509_V_OK) {
+                ngx_log_error(NGX_LOG_ERR, c->log, 0,
+                              "upstream SSL certificate verify error: (%l:%s)",
+                              rc, X509_verify_cert_error_string(rc));
+                goto fail;
+            }
+
+            cert = SSL_get_peer_certificate(c->ssl->connection);
+
+            if (cert == NULL) {
+                ngx_log_error(NGX_LOG_ERR, c->log, 0,
+                              "upstream sent no SSL certificate");
+                goto fail;
+            }
+
+            if (u->ssl_verify_name.data
+                && ngx_ssl_verify_name(cert, &u->ssl_verify_name) != NGX_OK)
+            {
+                X509_free(cert);
+                ngx_log_error(NGX_LOG_ERR, c->log, 0,
+                              "upstream SSL certificate name
validation error");
+                goto fail;
+            }
+
+            X509_free(cert);
+        }
+
         if (u->conf->ssl_session_reuse) {
             u->peer.save_session(&u->peer, u->peer.data);
         }
@@ -1395,6 +1427,8 @@ ngx_http_upstream_ssl_handshake(ngx_conn
         return;
     }

+fail:
+
     c = r->connection;

     ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
diff -r 70c5cd3a61cb -r eb4a27153a24 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h Tue Oct 01 17:44:51 2013 +0400
+++ b/src/http/ngx_http_upstream.h Wed Oct 16 13:50:04 2013 +0200
@@ -193,6 +193,7 @@ typedef struct {
 #if (NGX_HTTP_SSL)
     ngx_ssl_t                       *ssl;
     ngx_flag_t                       ssl_session_reuse;
+    ngx_flag_t                       ssl_verify;
 #endif

     ngx_str_t                        module;
@@ -299,6 +300,10 @@ struct ngx_http_upstream_s {
     ngx_int_t                      (*input_filter)(void *data, ssize_t bytes);
     void                            *input_filter_ctx;

+#if (NGX_HTTP_SSL)
+    ngx_str_t                        ssl_verify_name;
+#endif
+
 #if (NGX_HTTP_CACHE)
     ngx_int_t                      (*create_key)(ngx_http_request_t *r);
 #endif






On Fri, Oct 11, 2013 at 3:33 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
>
> On Wed, Oct 09, 2013 at 07:32:52PM +0300, Aviram Cohen wrote:
>
>> Hello!,
>>
>> I've made the necessary fixes. A few comments about those:
>>  - Name validation
>>       - Unlike Apache, in this patch, the configuration must contain the name
>>         to verify. In most cases, this should be set to the $host variable (and
>>         this is the default value). I've encountered certificates that validate
>>         names differently (some of Microsoft's certificates), and this is useful
>>         for such cases.
>
> Use of $host is certainly wrong.  By default nginx uses
> $proxy_host as a name of the upstream server, and having different
> default for the verification is bad idea.
>
>>       - Many certificates use wildcards in the names on which they sign (i.e.
>>         "*.google.com"). Though wildcards can appear anywhere according to the
>>         standard, I've added support only for a wildcard in the beginning of the
>>         name. This is the normal case, and it is easier to implement. This is
>>         is how Apache implements the wildcard name validation as well. Note also
>>          that the function X509_check_host will be introduced in future versions
>>           of OpenSSL that will provide the entire feature.
>
> It may be a good idea to actually use X509_check_host() if it's
> available.  And may be even refuse to do a validation if it's not,
> instead of reimplementing the wheel.
>
>>  - CRL verification - I've added CRL validation, but note that OpenSSL doesn't
>>    download CRL files from the servers, so the CRL file that is used should
>>    contain the revocation lists of all the proxied hosts. This also
>> means that it
>>    is out of Nginx's scope to update this file. Apache does the same thing.
>
> This is in line with how ssl_crl works, everything else probably
> doesn't matter.
>
>>  - The patch was made for v1.4.1.
>
> This is a bit strange - there is no chance the patch will be
> committed over 1.4.1, and there are conflicting changes in recent
> versions.
>
> [...]
>
>> # HG changeset patch
>> # User Aviram Cohen <aviram at adallom.com>
>> # Date 1381334949 -7200
>> # Branch stable-1.4
>> # Node ID 9a6e20bf72f8cf4d17653e4fdfcbac48c4de03aa
>> # Parent  0702de638a4c51123d7b97801d393e8e25eb48de
>> Added remote end SSL certificate verification in the proxy module.
>>
>> This patch adds the following directives to the proxy module:
>>  - proxy_ssl_verify - whether or not to verify the remote end's
>> certificate. Default is off.
>>  - proxy_ssl_verify_name - the remote end's name to verify. Default is
>> $host, can be set to "" in order to avoid name verification.
>
> Not sure if "" to avoid name verification is a good value.
>
>>  - proxy_ssl_verify_depth - how deep the ssl verification should be
>> done. Default is 1.
>>  - proxy_ssl_trusted_certificate - the path of the certificate file
>> that is used for verification. This must be provided when
>> proxy_ssl_verify is on.
>>  - proxy_ssl_crl - the path a file that contains the CRLs of the hosts
>> to which we proxy. Default is empty, and CRL verification is not done.
>
> Given the number of changes, it probably should be more than one
> patch.
>
>>
>> diff -r 0702de638a4c -r 9a6e20bf72f8 src/event/ngx_event_openssl.c
>> --- a/src/event/ngx_event_openssl.c Mon May 06 14:20:27 2013 +0400
>> +++ b/src/event/ngx_event_openssl.c Wed Oct 09 18:09:09 2013 +0200
>> @@ -42,6 +42,11 @@
>>  static char *ngx_openssl_engine(ngx_conf_t *cf, ngx_command_t *cmd,
>> void *conf);
>>  static void ngx_openssl_exit(ngx_cycle_t *cycle);
>>
>> +static int ngx_openssl_host_wildcard_match(ASN1_STRING *match_pattern,
>> +    ngx_str_t *hostname);
>> +static int ngx_openssl_host_exact_match(ASN1_STRING *match_pattern,
>> +    ngx_str_t *hostname);
>> +
>>
>>  static ngx_command_t  ngx_openssl_commands[] = {
>>
>> @@ -2562,3 +2567,163 @@
>
> Please add
>
> [diff]
> showfunc=1
>
> to your ~/.hgrc.
>
>>      EVP_cleanup();
>>      ENGINE_cleanup();
>>  }
>> +
>> +
>> +static int
>> +ngx_openssl_host_wildcard_match(ASN1_STRING *match_pattern,
>> +    ngx_str_t *hostname)
>> +{
>
> Nipicking: functions seems to be badly placed.  Configuration
> parsing and init/exit handlers are intentionally at the end.
>
> They are also badly named, as they aren't OpenSSL-specific, and
> should be ngx_ssl_* instead.
>
>> +    int        host_top_domain_length;
>> +    u_char    *host_top_domain;
>> +    u_char    *wildcard_pattern;
>> +
>> +    /* sanity check */
>> +    if (!match_pattern
>> +        || match_pattern->length <= 0
>> +        || !hostname
>> +        || !hostname->len)
>> +    {
>> +        return 0;
>> +    }
>> +
>> +    /* trivial case */
>> +    if (ngx_strncasecmp((u_char *) match_pattern->data,
>> +                        hostname->data,
>> +                        hostname->len)
>> +        == 0)
>> +    {
>> +        return 1;
>> +    }
>> +
>> +    /* simple wildcard matching - only in the beginning of the string. */
>> +    if (match_pattern->length > 2
>> +        && match_pattern->data[0] == '*'
>> +        && match_pattern->data[1] == '.')
>> +    {
>> +
>> +        wildcard_pattern = (u_char *) (match_pattern->data + 1);
>> +
>> +        host_top_domain = ngx_strlchr(hostname->data,
>> +                                      hostname->data + hostname->len,
>> +                                      '.');
>> +
>
> Long variable names used seems to result in hardly readable code.
>
> [...]
>
>> +int
>> +ngx_openssl_verify_name(X509 *cert, ngx_str_t *expected_name)
>> +{
>> +    GENERAL_NAMES   *gens = NULL;
>> +    GENERAL_NAME    *gen;
>> +    X509_NAME       *name = NULL;
>> +    ASN1_STRING     *cstr = NULL;
>> +    X509_NAME_ENTRY *ne;
>> +    int              i;
>> +    int              rc = 0;
>
> See elsewhere about variables sorting.  And please don't use
> local variables initialization mixed with declaration.  Well,
> initialization seems to be unneeded here at all.
>
>> +
>> +    /* based on OpenSSL's do_x509_check */
>> +
>> +    gens = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
>> +
>> +    if (gens) {
>> +
>> +        rc = 0;
>> +
>> +        for (i = 0; i < sk_GENERAL_NAME_num(gens); i++) {
>> +
>> +            gen = sk_GENERAL_NAME_value(gens, i);
>> +
>> +            /* we only check for either name or IP */
>> +            switch (gen->type) {
>> +
>> +            case GEN_DNS:
>> +                cstr = gen->d.dNSName;
>> +                rc = ngx_openssl_host_wildcard_match(cstr,
>> +                                                     expected_name);
>> +                break;
>> +
>> +            case GEN_IPADD:
>> +                cstr = gen->d.iPAddress;
>> +                rc = ngx_openssl_host_exact_match(cstr,
>> +                                                  expected_name);
>
> Why IP address matching?  It doesn't looks like something present
> in other implementations, nor something used in real certificates.
>
> It also doesn't looks like something correctly implemented, as
> d.iPAddress is expected to contain a binary address.
>
> [...]
>
>> --- a/src/http/modules/ngx_http_proxy_module.c Mon May 06 14:20:27 2013 +0400
>> +++ b/src/http/modules/ngx_http_proxy_module.c Wed Oct 09 18:09:09 2013 +0200
>> @@ -74,6 +74,15 @@
>>
>>      ngx_uint_t                     http_version;
>>
>> +#if (NGX_HTTP_SSL)
>> +    ngx_uint_t                     ssl_verify_depth;
>> +    ngx_str_t                      ssl_trusted_certificate;
>> +    ngx_str_t                      ssl_crl;
>> +    ngx_str_t                      ssl_verify_name_source;
>> +    ngx_array_t                   *ssl_verify_name_lengths;
>> +    ngx_array_t                   *ssl_verify_name_values;
>
> Using a complex value should be simplier.
>
> [...]
>
>> +        n = ngx_http_script_variables_count(&conf->ssl_verify_name_source);
>> +
>> +        if (n) {
>> +            ngx_memzero(&sc, sizeof(ngx_http_script_compile_t));
>> +
>> +            sc.cf = cf;
>> +            sc.source = &conf->ssl_verify_name_source;
>> +            sc.variables = n;
>> +            sc.lengths = &conf->ssl_verify_name_lengths;
>> +            sc.values = &conf->ssl_verify_name_values;
>> +            sc.complete_lengths = 1;
>> +            sc.complete_values = 1;
>> +
>> +            if (ngx_http_script_compile(&sc) != NGX_OK) {
>> +                return NGX_CONF_ERROR;
>> +            }
>> +
>
> Doing a compilation on every merge isn't a good idea.
>
>> +        }
>> +    }
>> +
>>  #endif
>>
>>      ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
>> diff -r 0702de638a4c -r 9a6e20bf72f8 src/http/ngx_http_upstream.c
>> --- a/src/http/ngx_http_upstream.c Mon May 06 14:20:27 2013 +0400
>> +++ b/src/http/ngx_http_upstream.c Wed Oct 09 18:09:09 2013 +0200
>> @@ -1319,12 +1319,43 @@
>>  {
>>      ngx_http_request_t   *r;
>>      ngx_http_upstream_t  *u;
>> -
>> +    X509                 *cert;
>> +    long                  rc;
>> +
>
> Style nitpicking:
>
> 1. Empty lines with "-" and "+" suggests there is something wrong
> with newlines.
>
> 2. Variables order is wrong, shortest types first.
>
>>      r = c->data;
>>      u = r->upstream;
>>
>>      if (c->ssl->handshaked) {
>> -
>> +        if (u->conf->ssl_verify) {
>> +            rc = SSL_get_verify_result(c->ssl->connection);
>> +            if (rc != X509_V_OK) {
>
> Style nitpicking: I would suggest to preserve empty line after "if
> (c->ssl->handshaked)", and add one between SSL_get_verify_result()
> and "if (rc != X509_V_OK)".
>
>> +                ngx_log_error(NGX_LOG_ERR, c->log, 0,
>> +                              "upstream SSL certificate verify error: (%l:%s)",
>> +                              rc, X509_verify_cert_error_string(rc));
>> +                goto fail;
>> +            }
>> +
>> +            cert = SSL_get_peer_certificate(c->ssl->connection);
>> +
>> +            if (cert == NULL) {
>> +                ngx_log_error(NGX_LOG_ERR, c->log, 0,
>> +                              "upstream sent no SSL certificate");
>> +                goto fail;
>> +            }
>> +
>> +            if (u->ssl_verify_name.len
>> +                && ngx_openssl_verify_name(cert, &u->ssl_verify_name)
>> +                == 0)
>
> Style nitpicking: "==" should be aligned with a function which
> return value it checks,
>
>             if (u->ssl_verify_name.len
>                 && ngx_openssl_verify_name(cert, &u->ssl_verify_name)
>                    == 0)
>             {
>
> And the ngx_openssl_verify_name() interface probably needs to be
> changed to be more like other interfaces in nginx - i.e., return
> NGX_OK on success.  With already suggested rename, and a 80 chars
> limit which isn't actually reached even with long name, this gives us
> something like this:
>
>             if (u->ssl_verify_name.len
>                 && ngx_ssl_verify_name(cert, &u->ssl_verify_name) != NGX_OK)
>             {
>
> [...]
>
> --
> Maxim Dounin
> http://nginx.org/en/donation.html
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



-- 
Aviram Cohen, R&D
Adallom, 1 Ha'Barzel st., Tel-Aviv, Israel
Mobile: +972 (54) 5833508
aviram at adallom.com, www.adallom.com



More information about the nginx-devel mailing list