[PATCH] (re-post) Add "optional_no_ca" option to ssl_verify_client to enable app-only CA chain validation

[Maxim Dounin]

Hello!

On Sat, Sep 22, 2012 at 05:09:53PM +0600, Mike Kazantsev wrote:

[...]

> > In particular, I would like someone to actually test if the 
> > error_page 495 aproach works instead as suggested here:
> > 
> > http://mailman.nginx.org/pipermail/nginx-devel/2012-August/002650.html
> > 
> 
> It doesn't seem to be of much use in current state, problems:
> 
>  - Requires "ssl_client_certificate" to be set to some valid
>    certificate, which then shouldn't actually never be used in this
>    case.

This actually counts as a separate problem.  I think it should be 
possible to specify empty certificate authorities list to be sent 
to clients even if nginx does actual verification.  Part of the 
problem is addressed by the ssl_trusted_certificate directive which is 
going to be introduced along with OCSP stapling patches.

>  - In case of "ssl_verify_client" set to "on" or "optional", setting
>    "error_page 400 495 496 =200 /altpath;" doesn't seem to stop nginx
>    from returning "HTTP/1.1 400 Bad Request" response with "400 The SSL
>    certificate error" in html body.
> 
>    This code in ngx_http_request.c is probable cause of that:
> 
>         if (sscf->verify) {
>             rc = SSL_get_verify_result(c->ssl->connection);
> 
>             if (rc != X509_V_OK) {
>                 ngx_log_error(NGX_LOG_INFO, c->log, 0,
>                               "client SSL certificate verify error: (%l:%s)",
>                               rc, X509_verify_cert_error_string(rc));
> 
>                 ngx_ssl_remove_cached_session(sscf->ssl.ctx,
>                                        (SSL_get0_session(c->ssl->connection)));
> 
>                 ngx_http_finalize_request(r, NGX_HTTPS_CERT_ERROR);
>                 return;
>             }
> 
>    I'm still unsure if it's a general bug in ssl module or some error
>    in my configuration above, because docs clearly state that error
>    should be handled by error_page and it doesn't seem to be.

Works fine here:

    ssl_verify_client on;
    ssl_client_certificate ca.crt;
 
    error_page 495 = /foo;

    location = /foo {
        return 200 "dn: $ssl_client_s_dn, verify: $ssl_client_verify\n";
    }

$ openssl s_client -connect ... -cert ... -key ...
...
GET / HTTP/1.0

HTTP/1.1 200 OK
Server: nginx/1.3.6
Date: Tue, 25 Sep 2012 02:37:45 GMT
Content-Type: text/plain
Content-Length: 35
Connection: close

dn: /CN=mdounin.ru, verify: FAILED

>  - "ssl_verify_client off;" isn't much useful, because it doesn't return
>    clent certificate and doesn't check it in any way.
> 
>  - "ssl_verify_client on;", still gives all-or-nothing check, though
>    I see that it's what might indeed be desirable, as Eric indicated.
> 
>  - I think it's really non-obvious way to do it.
> 
> 
> > And a quick comment for your patch: I tend to think that 
> > introduction of ngx_http_ssl_variable_get_client_verify() is 
> > misleading.  We shouldn't try to claim the certificate was 
> > verified unless it actually was.
> 
> It might be misleading, indeed, I see your point.
> Attached patch doesn't try to alter ssl_client_verify result.
> 
> On a completely unrelated note - nginx fails to build from svn here due
> to 'Exception in thread "main" java.lang.NoClassDefFoundError:
> com/pault/StyleSheet' (so I just disabled building changelog for tests).

Just checkout docs/xslt/changes.xslt again (or touch it).  (It's 
dependencies was changed without any changes to commit in the 
resulting file, hence make tries to rebuild it but fails as you 
have no com.pault.StyleSheet java class needed to convert 
XSLScript to XSLT.)

On the other hand, building changelog is completely separate 
process which is needed to build release tarball, but not to build 
nginx itself.

> 
> URL for the revised patch: https://gist.github.com/3319062
> 
> 
> Inline patch follows.

See below for some comments.

> 
> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
> index cd6d885..97da051 100644
> --- a/src/event/ngx_event_openssl.h
> +++ b/src/event/ngx_event_openssl.h
> @@ -141,6 +141,14 @@ ngx_int_t ngx_ssl_get_client_verify(ngx_connection_t *c, ngx_pool_t *pool,
>      ngx_str_t *s);
>  
>  
> +#define ngx_ssl_verify_error_is_optional(errnum) \
> +   ((errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) \
> +    || (errnum == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN) \
> +    || (errnum == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY) \
> +    || (errnum == X509_V_ERR_CERT_UNTRUSTED) \
> +    || (errnum == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE))

This should be adapted to match nginx style.

> +
> +
>  ngx_int_t ngx_ssl_handshake(ngx_connection_t *c);
>  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);
> diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
> index d759489..ab91670 100644
> --- a/src/http/modules/ngx_http_ssl_module.c
> +++ b/src/http/modules/ngx_http_ssl_module.c
> @@ -48,6 +48,7 @@ static ngx_conf_enum_t  ngx_http_ssl_verify[] = {
>      { ngx_string("off"), 0 },
>      { ngx_string("on"), 1 },
>      { ngx_string("optional"), 2 },
> +    { ngx_string("optional_no_ca"), 3 },
>      { ngx_null_string, 0 }
>  };
>  
> @@ -466,7 +467,7 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
>  
>      if (conf->verify) {
>  
> -        if (conf->client_certificate.len == 0) {
> +        if (conf->verify != 3 && conf->client_certificate.len == 0) {

>From readability point of view I would like to see this reversed, 
i.e.

           if (conf->client_certificate.len == 0 && conf->verify != 3) {

This way it would be more obvious that we check client certificate 
to be present (but allow it to be empty in the optional_no_ca 
case).

>              ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>                            "no ssl_client_certificate for ssl_client_verify");
>              return NGX_CONF_ERROR;
> diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> index cb970c5..96cec55 100644
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -1642,7 +1642,9 @@ ngx_http_process_request(ngx_http_request_t *r)
>          if (sscf->verify) {
>              rc = SSL_get_verify_result(c->ssl->connection);
>  
> -            if (rc != X509_V_OK) {
> +            if ((sscf->verify != 3 && rc != X509_V_OK)
> +                || !(sscf->verify == 3 && ngx_ssl_verify_error_is_optional(rc)))
> +            {

What happens here if sscf->verify == 3 and rc == X509_V_OK?  Is 
this intentional?

It probably should be

    if (rc != X509_V_OK
        && (sscf->verify != 3 || !ngx_ssl_verify_error_is_optional(rc)))
    {

instead.

>                  ngx_log_error(NGX_LOG_INFO, c->log, 0,
>                                "client SSL certificate verify error: (%l:%s)",
>                                rc, X509_verify_cert_error_string(rc));
> -- 
> 1.7.12

Maxim Dounin