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

[Piotr Sikora]

Hey Maxim,

> 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().

Exactly.

> And this is what nginx does.  I don't see compelling reasons to
> change the order of the calls here.

No, it doesn't, which is the reason for this patch in the first
place... NGINX uses the result of SSL_get_verify_result() without
calling SSL_get_peer_certificate(), which is what the BUGS section
requires.

Also, this breaks implementations (i.e. BoringSSL) that are a bit more
strict and don't use X509_V_OK as the initial value.

> This looks like a separate patch, or two patches.

Fair enough.

> 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.

What you would prefer, then?

1. Returning error string back the the caller?
2. Returning "rc" back to the caller and adding another function to
abstract X509_verify_cert_error_string()?
3. Adding the caller string ("client" or "upstream") as the parameter
and writing error from within the function?

> As well as magic values in the "verify" argument,

Good call.

> and the change of the ngx_ssl_check_host() semantics.

You mean the introduction of error logs? If so, then it seems that
returning "rc" and adding another function to retrieve the error
string would be the best way to keep the behavior as close to the
existing one as possible.

Best regards,
Piotr Sikora