Dual cert support for BoringSSL

[randall]

BoringSSL removed support for slotted SSL_CTXs. This is key for
supporting multiple certs for a CTX. When BoringSSL is used,
multiple CTXs will be created if more than one cert type is specified.
When a client connection does come in, ATS will interrogate the
connection and present the approriate CTX (preferring ECDSA)

[bneradt]

Looks good. In general the patch looks fine, I just have a few suggestions about using auto& or auto const& if it's possible in some places. It would probably be helpful to have someone more familiar with TLS than I review this as well.

[randall]

Looks good. In general the patch looks fine, I just have a few suggestions about using auto& or auto const& if it's possible in some places. It would probably be helpful to have someone more familiar with TLS than I review this as well.

Thanks for the early feedback I think I hit all those autos (and maybe some more in that file)!

[shinrich]

Maybe you want a different name for the BORINGSSL version of this function, which is really a utility function for the callback instead of the proper callback. Took me a few minutes looking around to resolve in my head that the boring version of the function and openssl version with the same name weren't going to conflict with each other.

[maskit]

I don't think this function should have a different name, although it's a shame that we have two very similar functions.

The lambda expression in _set_handshake_callbacks mimics OpenSSL's behavior, and it enables reusing two other callback functions as well. In that sense I think having the same name make sense.

Having this in a #elif clause may ease the confusion.

#if TS_USE_HELLO_CB
static int
ssl_client_hello_callback(SSL *s, int *al, void *arg)
{
}
#elif defined(OPENSSL_IS_BORINGSSL)
static ssl_select_cert_result_t
ssl_client_hello_callback(const SSL_CLIENT_HELLO *client_hello)
{
}
#endif

And we may also be able to have exactly the same function signature by passing SSL_CLIENT_HELLO to arg.

[randall]

Doing the #elif here makes sense.

The callback return values are different types (and different values for equivalent states):

enum ssl_select_cert_result_t BORINGSSL_ENUM_INT {
  // ssl_select_cert_success indicates that the certificate selection was
  // successful.
  ssl_select_cert_success = 1,
  // ssl_select_cert_retry indicates that the operation could not be
  // immediately completed and must be reattempted at a later point.
  ssl_select_cert_retry = 0,
  // ssl_select_cert_error indicates that a fatal error occured and the
  // handshake should be terminated.
  ssl_select_cert_error = -1,
};

vs openssl:

# define SSL_CLIENT_HELLO_SUCCESS 1
# define SSL_CLIENT_HELLO_ERROR   0
# define SSL_CLIENT_HELLO_RETRY   (-1)

[maskit]

I'm fine with the current code, but I guess we could have aliases for the type and also the values.

trafficserver/iocore/net/P_SSLUtils.h

Lines 45 to 49 in 870bbdb

	#ifndef OPENSSL_IS_BORING
	typedef int ssl_curve_id;
	#else
	typedef uint16_t ssl_curve_id;
	#endif

[shinrich]

What is the difference between pause and retry with boring? Guess I should go back and look more carefully at your docs.

[maskit]

Seems like there is not "pause" in BoringSSL. Maybe they just use different terms.

OpenSSL

The callback can then inspect the passed ssl structure and set or clear any appropriate certificates. If the callback is successful it MUST return 1 even if no certificates have been set. A zero is returned on error which will abort the handshake with a fatal internal error alert. A negative return value will suspend the handshake and the handshake function will return immediately. SSL_get_error(3) will return SSL_ERROR_WANT_X509_LOOKUP to indicate, that the handshake was suspended. The next call to the handshake function will again lead to the call of cert_cb(). It is the job of the cert_cb() to store information about the state of the last call, if required to continue.

BoringSSL

enum ssl_select_cert_result_t BORINGSSL_ENUM_INT {
  // ssl_select_cert_success indicates that the certificate selection was
  // successful.
  ssl_select_cert_success = 1,
  // ssl_select_cert_retry indicates that the operation could not be
  // immediately completed and must be reattempted at a later point.
  ssl_select_cert_retry = 0,
  // ssl_select_cert_error indicates that a fatal error occured and the
  // handshake should be terminated.
  ssl_select_cert_error = -1,
};

[shinrich]

I notice that you enabled one of the tlshook checks with this test (removed the openssl requirement). But I see that the tls_check_dual_cert* checks have no restriction to only openssl. Are they failing when built against boringssl without this PR?

[bryancall]

@shinrich is going to take a look

[randall]

I notice that you enabled one of the tlshook checks with this test (removed the openssl requirement). But I see that the tls_check_dual_cert* checks have no restriction to only openssl. Are they failing when built against boringssl without this PR?

Currently, a large number of tls tests fail when run under BoringSSL. I added #8014 recently which added the IsOpenSSL condition, and marked up a few tests that I knew depended on OpenSSL. I didn't add the dual cert tests because I knew I had this PR and #7298 in the wings.

And even after this PR, there are still a few tls_hooks test that still fail which I probably won't worry about since we don't depend on them. I'll document(by adding the Condition.IsOpenSSL) to them.

[shinrich]

I made on rather nit-picky comment on a Debug message. Once the clang-format is happy, I am willing to approve.

Brian's issues were resolved.