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

[Maxim Dounin]

Hello!

On Fri, Sep 02, 2016 at 04:18:53PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > 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().
> 
> Those 2 sentences are mutually exclusive, if result of
> SSL_get_verify_result() is useless without SSL_get_peer_certificate(),
> then those two should be called together,

No, your are incorrect here.  "In connection with" means that 
SSL_get_peer_certificate() should be used, but doesn't require it 
to be used always, in all cases.  In particular, 
SSL_get_peer_certificate() is useless when SSL_get_verify_result() 
returns anything but X509_V_OK.

> or more precisely,
> SSL_get_peer_certificate() should be called before
> SSL_get_verify_result().

This is simply not true, sorry.

[...]

> > 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.
> 
> Except that SSL_get_verify_result() doesn't save its result on the
> error stack, so what I suggested is as close to ngx_ssl_error() as
> possible.

What your patch does is what you initially suggested in (2) 
and I objected against.

Obviously enough, SSL_get_verify_result() doesn't use error stack 
in OpenSSL, and implementing something like ngx_ssl_error() (or 
extending ngx_ssl_error() itself) would require additional work to 
save the verify result.

> > As previously suggested, it might be a good solution to use "peer", as
> > already used in serveral error messages in ngx_event_openssl.c
> 
> Again, could you elaborate why the use of "client" in
> ngx_ssl_verify_client() and "upstream" in ngx_ssl_verify_host() is
> wrong?

Because ngx_ssl_verify_host() is expected to be a generic 
function, and it can be used in situations different from talking 
to upstream servers.

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