[PATCH] SSL: fix order of checks during SSL certificate verification

[Maxim Dounin]

Hello!

On Wed, Aug 31, 2016 at 03:24:19PM -0700, Piotr Sikora wrote:

> > At this point nginx just uses the interface provided by the
> > OpenSSL library, exactly as documented.  The fact that the
> > interface is flawed (and documented to be flawed to make sure
> > people will use it properly) has nothing to do with nginx use of
> > it.
> 
> And yet, NGINX doesn't use it properly.

Uhm, nginx uses the interface according to the documentation.  
That is, when SSL_get_verify_result() return X509_V_OK, it uses 
SSL_get_peer_certificate() to differentiate two of its possible 
meanings:

    if (SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
        ngx_str_set(s, "FAILED");
        return NGX_OK;
    }

    cert = SSL_get_peer_certificate(c->ssl->connection);

    if (cert) {
        ngx_str_set(s, "SUCCESS");

    } else {
        ngx_str_set(s, "NONE");
    }

This is exactly in line with the SSL_get_verify_result() 
documentation, as previously quoted here: 

       X509_V_OK
           The verification succeeded or no peer certificate was presented.

I don't understand why you think that nginx doesn't use it 
properly.  If you still think nginx "doesn't use it properly", 
please elaborate.

A different order of checks may make nginx more robust against API 
changes similar to one BoringSSL tried to implement.  But it has 
nothing to do with proper use of the existing API.

> > Your patches assume that there are no conflicts between nginx
> > error codes (NGX_OK, NGX_DECLINED) and SSL_get_verify_result()
> > error codes.  While this is currently true, this is not something
> > I would rely on, even assuming OpenSSL only.
> 
> You didn't like the previous approach, when logging was done in
> ngx_event_openssl.c (because of the use of "client" and "upstream"),
> and you don't like this approach, when logging is done outside of
> ngx_event_openssl.c (since the error code must be passed back to the
> caller), which is actually one of the solutions you suggested.

The "one of the solutions you suggested" claim isn't really true.  
I never suggested such a solution.  Quoting myself, 
http://mailman.nginx.org/pipermail/nginx-devel/2016-August/008680.html:

: I can't say I like either of the variants.  (1) will require
: memory allocations, (2) looks hardly portable (what if another
: library will have different rc values? or will have more than one
: error string to print?), and (3) looks strange.

The (2) here corresponds to the variant in question you suggested.

-- 
Maxim Dounin
http://nginx.org/