Hello!

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

> # HG changeset patch
> # User Piotr Sikora <piotrsikora@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/

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel