[PATCH] (re-post) Add "optional_no_ca" option to ssl_verify_client to enable app-only CA chain validation
Eric O'Connor
eoconnor at coincident.com
Thu Sep 27 16:30:29 UTC 2012
Here is a modified patch addressing issues that Maxim brought up earlier:
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))
+
+
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->client_certificate.len == 0 && conf->verify != 3) {
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 (rc != X509_V_OK
+ && (sscf->verify != 3 ||
!ngx_ssl_verify_error_is_optional(rc)))
+ {
ngx_log_error(NGX_LOG_INFO, c->log, 0,
"client SSL certificate verify error: (%l:%s)",
rc, X509_verify_cert_error_string(rc));
On Thu, Sep 27, 2012 at 12:10 PM, Eric O'Connor <eoconnor at coincident.com> wrote:
> It appears to be technically correct, but not semantically correct. I
> would describe it as a nasty kludge.
>
> I'm not sure why that would be preferable to a small ~3 line + a macro
> patch that solves the issue and provides a nice interface. Error page
> handling should be used for handling errors, not supporting
> workarounds for a new set of technologies.
>
> On Thu, Sep 27, 2012 at 10:05 AM, Mike Kazantsev <mk.fraggod at gmail.com> wrote:
>> On Tue, 25 Sep 2012 08:39:28 +0400
>> Maxim Dounin <mdounin at mdounin.ru> wrote:
>>
>>> 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.
>>>
>>
>> Ok.
>>
>> Don't think finding any cert to put there should be a problem anyway.
>>
>>
>>> > - 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:
>>> >
>>
>> ...
>>
>>> >
>>> > 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
>>>
>>
>> With a bit more universal catch-all config like this:
>>
>> error_page 495 = @cert_err;
>> location @cert_err { ...
>>
>> It seem to work pretty much as desired, was some misconfiguration on my
>> part before, I guess.
>>
>> Since it's possible to use and rewrite $uri in named locations, I don't
>> see any obvious flaws in this approach, so I think "error_page" might
>> make this patch redundant indeed.
>>
>> I'll try it out (and ping other users) and re-raise the issue with
>> modified patch if there'll be any less obvious issues with it.
>>
>> Eric, does it work for your use-case as well?
>>
>>
>>> > - "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.
>>> >
>>> >
>>
>> ...
>>
>>> See below for some comments.
>>
>> I'll look into addressing these, thanks, though as noted, the patch
>> itself doesn't seem to be necessary, since "error_page 495 ..." works.
>>
>>
>> --
>> Mike Kazantsev // fraggod.net
More information about the nginx-devel
mailing list