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@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@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
