[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