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

[Maxim Dounin]

Hello!

On Tue, Aug 02, 2016 at 03:24:55PM -0700, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1470107238 25200
> #      Mon Aug 01 20:07:18 2016 -0700
> # Node ID cd72e0a1164abd70aafdb391b3470869508532e5
> # Parent  d43ee392e825186545d81e683b88cc58ef8479bc
> SSL: fix order of checks during SSL certificate verification.
> 
> SSL_get_verify_result() should be called only if certificate was presented
> by the peer, otherwise returned value is the default one, which happens to
> be X509_V_OK, but it doesn't indicate success and it's considered a bug:
> https://www.openssl.org/docs/manmaster/ssl/SSL_get_verify_result.html

This behaviour is explicitly documented for years.  The BUGS 
section outlines that the API is not intuitive and requires use of 
SSL_get_peer_certificate() in addition ot SSL_get_verify_result().  
And this is what nginx does.  I don't see compelling reasons to 
change the order of the calls here.

> While there, move common verification logic to ngx_ssl_verify_client() and
> ngx_ssl_check_host() in order to make the callers crypto-library-agnostic.

This looks like a separate patch, or two patches.  Though I'm 
somewhat sceptical about the use of "upstream" and "client" in 
error messages introduced, this looks like a wrong approach for a 
generic SSL code.  As well as magic values in the "verify" argument, 
and the change of the ngx_ssl_check_host() semantics.

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