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

[Piotr Sikora]

Hey Maxim,

> Calling SSL_get_peer_certificate() is only needed if
> SSL_get_verify_result() returns X509_V_OK, to distinguish two of
> its different meanings:
>
>        X509_V_OK
>            The verification succeeded or no peer certificate was presented.
>
> And that's what nginx does.

That's not what the documentation says, though...

At this point, NGINX is just relying on a known bug, nothing else.

> Ok, this looks like the real reason for the patch.  This looks
> like an API change in BoringSSL, and should be threated
> accordingly.

Nope, that's not the real reason... BoringSSL actually added a
workaround for this [1], just to play nicely with NGINX and
netty-tcnative, because virtually everything else is using OpenSSL API
correctly, i.e. checking if peer sent the certificate before checking
if that certificate passed verification.

The real reason for this patch is that existing code is misusing OpenSSL API.

[1] https://boringssl.googlesource.com/boringssl/+/37646838e9bb62a0d9d506b117193611c4c46012

> Given the number of various API changes BoringSSL introduces here
> and there - we probably don't want to follow, at least till some
> version is actually released.

While this might have been true 2 years ago, nowadays BoringSSL tries
to be a drop-in replacement for OpenSSL... and looking at the commit
log for ngx_event_openssl.c, it seems that compatibility with new
OpenSSL requires way more changes to NGINX than compatibility with
BoringSSL.

> I can't say I like either of the variants.  (1) will require
> memory allocations,

Agreed.

> (2) looks hardly portable (what if another
> library will have different rc values? or will have more than one
> error string to print?),

Well, it's portable assuming that the library also converts the "rc"
value back to the error string (like in the patchset I sent
yesterday).

> and (3) looks strange.

Agreed.

> May be something like "peer" in error messages instead would be a
> better solution, assuming context is already known from the
> c->log->action == "SSL handshaking to upstream".

To be honest, I don't see why we need to lose "client" and "upstream"
from the error log if those functions are called only for client and
host verification.

> Alternatively,
> we can consider abstracting printing of verification results
> errors with something similar to ngx_ssl_error().

That's acceptable, but I don't see any advantage over providing an
alias for X509_verify_cert_error_string().

Best regards,
Piotr Sikora