Hello!

On Tue, Aug 27, 2013 at 11:47:38AM +0300, Aviram Cohen wrote:

> Added a new version, with all the required fixes.

This looks better, modulo various style problems. It also looks
like verification code isn't complete.

See below for comments.

>
> diff -Nrpu nginx-1.4.1/src/http/modules/ngx_http_proxy_module.c
> nginx-1.4.1-proxy-ssl-verification/src/http/modules/ngx_http_proxy_module.c
> --- nginx-1.4.1/src/http/modules/ngx_http_proxy_module.c 2013-05-06
> 13:26:50.000000000 +0300
> +++ nginx-1.4.1-proxy-ssl-verification/src/http/modules/ngx_http_proxy_module.c
> 2013-08-26 10:43:15.639557701 +0300
> @@ -511,6 +511,27 @@ static ngx_command_t ngx_http_proxy_com
> offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_session_reuse),
> NULL },
>
> +

Style: extra empty line.

> + { 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_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, upstream.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, upstream.ssl_certificate),
> + NULL },
> #endif

Style: missing empty line before "#endif".

It also might make sense to name the field ssl_trusted_certificate
to match directive name.

It probably also a good idea to move both certificate and
verify_depth to ngx_http_proxy_loc_conf_t as they aren't needed in
upstream configuration. (At upstream level, only upstream.verify
is needed/used.)

>
> ngx_null_command
> @@ -2419,8 +2440,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->upstream.ssl_verify_depth = NGX_CONF_UNSET_UINT;
> #endif
>
> /* "proxy_cyclic_temp_file" is disabled */

Style: please add conf->upstream.ssl_certificate (or whatever) to
a comment "set by ngx_pcalloc()".

> @@ -2697,6 +2721,30 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t
> #if (NGX_HTTP_SSL)
> ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
> prev->upstream.ssl_session_reuse, 1);

Style: if you add empty line before "#endif", please add another
one after "#if".

> + ngx_conf_merge_value(conf->upstream.ssl_verify,
> + prev->upstream.ssl_verify, 0);
> + ngx_conf_merge_uint_value(conf->upstream.ssl_verify_depth,
> + prev->upstream.ssl_verify_depth, 1);
> + ngx_conf_merge_str_value(conf->upstream.ssl_certificate,
> + prev->upstream.ssl_certificate, "");
> +
> + if (conf->upstream.ssl_verify) {
> + if (conf->upstream.ssl_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");

Style: no lines longer than 80 chars, please.

It might also be a good idea to only complain if there is
conf->upstream.ssl.

> +
> + return NGX_CONF_ERROR;
> + }
> + }
> +
> + if (conf->upstream.ssl &&
> + ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
> + &conf->upstream.ssl_certificate, conf->upstream.ssl_verify_depth) != NGX_OK)
> + {

Style:

- no lines longer than 80 chars, please.
- if you wrap long conditions, please put operators a the start of
a continuation line.

That is,

if (conf->upstream.ssl
&& ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
&conf->upstream.ssl_certificate
conf->upstream.ssl_verify_depth)
!= NGX_OK)
{
...
}

Additional question is what happens in a configuration like

location / {
proxy_pass https://example.com;
proxy_ssl_verify on;
proxy_ssl_trusted_ceritifcate example.crt;

if ($foo) {
# do nothing
}
}

or the same with a nested location instead of "if". Quick look
suggest it will result in trusted certs loaded twice (and stale
alerts later due to how OpenSSL handles this).

> + return NGX_CONF_ERROR;
> + }
> +
> #endif
>
> ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
> diff -Nrpu nginx-1.4.1/src/http/ngx_http_upstream.c
> nginx-1.4.1-proxy-ssl-verification/src/http/ngx_http_upstream.c
> --- nginx-1.4.1/src/http/ngx_http_upstream.c 2013-05-06 13:26:50.000000000 +0300
> +++ nginx-1.4.1-proxy-ssl-verification/src/http/ngx_http_upstream.c
> 2013-08-26 10:44:35.323558884 +0300
> @@ -1324,7 +1324,13 @@ ngx_http_upstream_ssl_handshake(ngx_conn
> u = r->upstream;
>
> if (c->ssl->handshaked) {
> -
> + if (u->conf->ssl_verify && SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
> + ngx_log_error(NGX_LOG_ERR, c->log, 0, "upstream ssl certificate validation failed");
> + c = r->connection;
> + ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
> + goto fail;
> + }
> +

Style: 80 chars.

Please also note that SSL_get_verify_result() will return
X509_V_OK if there is no certificate at all, quote from
SSL_get_verify_result manpage:

If no peer certificate was presented, the returned result code is
X509_V_OK. This is because no verification error occurred, it does
however not indicate success. SSL_get_verify_result() is only useful in
connection with SSL_get_peer_certificate(3).

Please take a look at relevant code at ngx_http_process_request().
It also seems to have better error reporting.

> if (u->conf->ssl_session_reuse) {
> u->peer.save_session(&u->peer, u->peer.data);
> }
> @@ -1332,13 +1338,21 @@ ngx_http_upstream_ssl_handshake(ngx_conn
> c->write->handler = ngx_http_upstream_handler;
> c->read->handler = ngx_http_upstream_handler;
>
> + c = r->connection;
> +
> ngx_http_upstream_send_request(r, u);
>
> +fail:
> + ngx_http_run_posted_requests(c);
> +
> return;
> }

Probably just adding

ngx_http_run_posted_requests(c);
return;

instead of "goto" above would be more readable...

>
> + c = r->connection;
> +
> ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
>
> + ngx_http_run_posted_requests(c);
> }

.... or, alternatively, "fail:" may be moved before these lines
(and "c = ..." and "ngx_http_upstream_next(...)" lines before
"goto" removed), resulting in less code duplication.

[...]

> On Thu, Aug 22, 2013 at 5:00 PM, Aviram Cohen <aviram@adallom.com> wrote:
> > Hello!
> >
> > I have a couple of questions regarding the two last comments:

[...]

Sorry, I somehow missed this message. Glad to see you've
succcessfully found answers.

--
Maxim Dounin
http://nginx.org/en/donation.html

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel