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 <[email protected]> # 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 <[email protected]> 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 <[email protected]> >> # 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 > [email protected] > http://mailman.nginx.org/mailman/listinfo/nginx-devel -- Aviram Cohen, R&D Adallom, 1 Ha'Barzel st., Tel-Aviv, Israel Mobile: +972 (54) 5833508 [email protected], www.adallom.com _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
