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 _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
