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

[Maxim Dounin]

Hello!

On Thu, Sep 01, 2016 at 02:16:37PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > 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.
> 
> Per BUGS section:
> 
>     SSL_get_verify_result() is only useful in connection with
> SSL_get_peer_certificate.
> 
> But the code you pasted, i.e.:
> 
>     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);
> 
> uses result of SSL_get_verify_result() without ever calling
> SSL_get_peer_ceritficate(), which is what the BUGS section warns
> against.

You are misreading the BUGS section.  It doesn't say that 
SSL_get_peer_certificate() must be always called when 
SSL_get_verify_result() is called.  It says that SSL_get_verify_result() is 
only useful in connection with SSL_get_peer_certificate().  More 
specifically, this is needed to distinguish two of the different 
meanings of X509_V_OK.  And this is what nginx does.

> > 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.
> 
> I was referring to:
> 
>     Alternatively,
>     we can consider abstracting printing of verification results
>     errors with something similar to ngx_ssl_error().
> 
> which is basically (2), unless I've misunderstood you.

The difference between ngx_ssl_error() and what you've suggested 
is that ngx_ssl_error() doesn't try to cast errors to an nginx rc 
value.  Instead, it uses the error stack saved in the relevant 
connection object.

> But that's not really important... what's important is which approach
> would be acceptable for your? Because the only reason for the change
> in previous patch was the fact that you didn't like my original
> version, which printed "client" and "upstream" in ngx_ssl_openssl.c.

As previously suggested, it might be a good solution to use "peer", as 
already used in serveral error messages in ngx_event_openssl.c

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