tls: support BoringSSL private key async functionality

[ipuustin]

This PR adds BoringSSL private key API abstraction, as discussed in #6248. All comments and discussion is welcomed to get the API sufficient for most private key API tasks.

The PR contains the proposed API and the way how it can be used from ssl_socket.h. Also there is some code showing how the PrivateKeyMethodProvider is coming from TLS certificate config. Two example private key method providers are included in the tests.

Description: tls: support BoringSSL private key async functionality
Risk Level: medium
Testing: two basic private key provider implementation
Docs Changes: TLS arch doc, cert.proto doc

[rojkov]

I guess this function should be incapsulated somewhere inside source/extensions/transport_socket_tls/private_key to avoid messing with OpenSSL's API if we add conditional OpenSSL builds.

[ipuustin]

You're right -- the setup could also be in a PrivateKeyImpl class or similar. On the other hand the whole concept of BoringSSL private key calls doesn't make sense if OpenSSL is used.

[ipuustin]

@htuch @PiotrSikora I added a second patch which adds support for registering private key provider factories. Any comments whether this is going the right way would be helpful. I was considering between this approach and extending Connection API to have network read filters to be able to access SSL internal structures; WDYT?

[htuch]

Starting from first principles on this review (just noticed it when you tagged me). How come we need to use ConfigSource here? Wouldn't the correct thing be a typed config, i.e. an Any, similar to how we configure other extensions? I think the best example for what you're trying to integrate here is something like the gRPC credential extension, see https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/google_grpc_creds_impl.h

[ipuustin]

That makes sense. I added a patch which does just that. I was bit unsure if the plan is to use the combined Struct/Any approach for new extension types, or just go with Any.

I'm currently thinking that the configuration conversion would be done in the private key provider extensions something like this:

Ssl::PrivateKeyOperationsProviderSharedPtr TestPrivateKeyOperationsFactory::createPrivateKeyOperationsProviderInstance(
    const envoy::api::v2::auth::PrivateKeyOperations& message,
    Server::Configuration::TransportSocketFactoryContext& private_key_provider_context) {
  ProtobufTypes::MessagePtr proto_config = std::make_unique<test::TestPrivateKeyOperationsProto>();

  Config::Utility::translateOpaqueConfig(message.typed_config(), ProtobufWkt::Struct(), *proto_config);
  const test::TestPrivateKeyOperationsProto config = MessageUtil::downcastAndValidate<const test::TestPrivateKeyOperationsProto&>(*proto_config);

  return std::make_shared<TestPrivateKeyOperationsProvider>(config);
}

[htuch]

Yeah, you can factor some of this out into an extension factory base, you can see what was done for L7 filters at https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/common/factory_base.h.

[htuch]

Some answers to questions above:

Validate the thread model. Now there is an unique PrivateKeyOperations instance for each connection, but the PrivateKeyOperationsProvider is shared. This makes the dispatcher model easier and the PrivateKeyOperations lifecycle can be tied with the caller.

Yep, this is how to do things in Envoy. It's fine to share the provider, assuming it's essentially a stateless (or immutable) factory, and all the private key operation state lives thread-local.

How to get the private key to the provider? Just let the provider use SSL_get_privatekey()?

Isn't this highly dependent on the extension? Won't some have their private key material living in an HSM?

Does SDS require any special handling?

SDS is orthogonal. It's inherently about fetching keys from a remote server and then using them for crypto, rather than delegating to dedicated crypto hardware. Maybe in theory you could imagine SDS being implemented behind this new interface, but I think what we have today works well in parallel to offload.

Should we expose BoringSSL primitives (such as SSL *ssl) in the API?

Ideally what is being built works for both OpenSSL and BoringSSL, or some abstraction exists that can support this. I have no idea of the varying offload implementations, but just throwing this out there.

[htuch]

Thanks, I've taken a further look and have a few more suggestions and questions.

[htuch]

@lizan should we continue to require Struct untyped option, or is it fine to have new future looking extensions go Any-only at this point. My intuition on the protobuf hashing issue is that this is still problematic, but maybe we have confluence between that fix and when a feature like this goes prod ready?

[htuch]

I think the answer to this thread is we should support both, since Any is not production ready yet and Struct has been undeprecated.

[htuch]

Do you have any thoughts on how this can work for OpenSSL implementations?

[ipuustin]

It doesn't - private key operations are a BoringSSL concept. OpenSSL uses a bit similar mechanism called asynchronous mode, which makes almost all operations potentially asynchronous. It's up to the backend (an OpenSSL engine) to implement the operations in an asynchronous way. OpenSSL provides a file descriptor to tell when the operation is ready.

The benefit of OpenSSL model is that many more operations can be performed asynchronously and also the possibility to have dynamic shared objects (engines) with a specified ABI to run the asynchronous operations. Also, if an asynchronous engine isn't used, the library falls back to synchronous operation. The drawback is that the OpenSSL library needs to be dynamically linked to Envoy in order to load external engines. Also, the ASYNC API is not present in BoringSSL.

I wrote a patch on top of @bdecoste's OpenSSL Envoy patches some time back to enable the OpenSSL asynchronous mode:
ipuustin@41bc5be

[htuch]

Is there any hope of unifying these behind a common Envoy abstraction? That would be ideal, but obviously not if it needs to be a mega-project to make happen. Thoughts?

[ipuustin]

It's my understanding that the BoringSSL and OpenSLL support will be implemented as separate TLS extensions. This means that the way the library is used will stay "hidden" in the implementations. I think the abstraction could be done in two different possible scopes:

1. Write support to loading OpenSSL Engines to OpenSSL TLS extension. Abstract the configuration and callback interface.
2. Find functions corresponding to the BoringSSL private key ops from OpenSSL ASYNC API, and provide a way to do "OpenSSL mini-engines" which would have the same functionality as the BoringSSL private key operations.

The problem with the first item is that it's already pretty well abstracted: there is a bunch of OpenSSL engines out there, and the benefit of Envoy knowing about them would be mainly to pass them configuration values. Envoy can use the dynamic engines quite easily (the only requirement is linking the OpenSSL library dynamically into Envoy), and there is Dispatcher access from the Connection interface to the TLS extension to facilitate the async event channel. The OpenSSL engines can also be configured from outside Envoy, by using OpenSSL configuration files (see https://www.openssl.org/docs/man1.1.1/man5/config.html). Writing the OpenSSL TLS extension to use asynchronous mode from day one would be a good idea.

The second item would mean in a sense that there would be feature parity between OpenSSL and BoringSSL implementations regarding private key operations. The OpenSSL would be pretty handicapped in this case, since the async engine support is more complete than BoringSSL private key operations. Using the QAT HW acceleration as an example, OpenSSL QAT engine (https://github.com/intel/QAT_Engine) can accelerate also EDCHE and some crypto operations, while the BoringSSL private key support is limited to RSA decryption and RSA+ECDSA signing. I think people would gravitate using the full OpenSSL engines rather than writing the Envoy-specific OpenSSL "mini-engines".

TL;DR: it feels to me that having a common abstraction for OpenSSL engines and BoringSSL private key operations might not have enough benefits compared to letting the TLS extensions set up the asynchronous operations as they please. However, I've had only limited exposure to OpenSSL engines, so it could be there are other considerations that I'm not aware of. Maybe there would be a special Envoy configuration Protobuf type for the TLS extensions, so that they could be configured separately? This could mean OpenSSL protobuf configuration message having a submessage for setting up the requested OpenSSL engine(s).

[htuch]

@ipuustin thanks for the analysis. My thinking was that if X wanted to write a new private key offload extension Y for X's HSM, then it would be nice to have Y work no matter whether you go with BoringSSL or OpenSSL TLS. If there are already a number of OpenSSL engines out there that only require a light wrapping for Envoy, and OpenSSL is strictly more expressive than BoringSSL, I think there's not much hope or point of reuse.

So, I think this is resolved, we go with your current approach. Thanks again for taking the time to provide such a detailed discussion.

[htuch]

What kind of threading guarantees does BoringSSL or OpenSSL provide? Do we know that the thread that private key operations are run on is the same as the invoking thread? Should complete post to the dispatcher to continue, or is that the job of the extension? This is one place that I'm not 100% sure of the thread correctness.

[ipuustin]

The BoringSSL operations are driven by the caller: BoringSSL's private key complete function will be called when the SSL_do_handshake() is called the second time. However, the complete() function (indicating that the asynchronous operation is ready and ssl_socket.cc needs to call the handshake again) might in principle be called from whatever thread the private key operations extension happens to use. I believe the correct semantics is to define this to be an error, and expect the extension to call the complete() function from the worker thread context.

Should we add ASSERT(isThreadSafe()); to SslSocket::complete() the way it is done in dispatcher_impl.cc?

I just wrote a (rough) implementation of a QAT private key extension to see if it fits the private key API. There I'm using a pipe and dispatcher to make sure that the callback thread is correct, because QAT device polling is done from a different thread. See the code here: https://github.com/ipuustin/envoy/blob/qat-provider/source/extensions/private_key_operations_providers/qat/qat_private_key_provider.cc#L20

[htuch]

Yeah, I think it should be an error to SslSocket::complete() on a different thread. The Envoy pattern for handling this is to do a post to the original thread's dispatcher with the completion as the body. The only cross-thread safe dispatcher operation is post().

[htuch]

Are these going to be BoringSSL specific or an opaque placeholder for both (incompatible) types of private key operations (OpenSSL engines and BoringSSL)?

[PiotrSikora]

OpenSSL engines are much more than just private key operations, and I don't think treating them as alternatives is going to result in a clean API.

[htuch]

Typically we are making these names reverse DNS, e.g. com.acme.foo.bar FWIW. Inbuilt extensions can live in the envoy.foo.bar namespace.

[htuch]

This seems effectively a oneof with TlsCertificate.private_key. We can't promote it to a oneof without breaking the API, but should this field live next to that in this file and have a comment indicating the exclusive nature of these fields?

[PiotrSikora]

Yes, this should definitely be a part of TlsCertificate, so that it could be configured on a per-certificate basis.

Ideally, this would be a oneof between private_key and private_key_method, which works well for a lot of use cases (remote oracle, etc.) and is more generic, but it makes this use case (crypto in separate thread) a bit awkward, since then we would need to pass private_key to private_key_method via different means.

Thoughts?

(Using private_key_method instead of private_key_operations to align with the BoringSSL name of this feature, though I don't feel too strongly about it).

[htuch]

We can't do oneof for wire compat reasons anyway, so you have flexibility here.

[PiotrSikora]

I was under the impression that you can add oneof to non-repeatable fields and stay wire-compatible?

[htuch]

Wire compatible yes, but it breaks Go language bindings. We've had a request from the gRPC team to avoid doing this gratuitously, this seems a reasonable accommodation to make. See #6271.

[PiotrSikora]

Ah, I wasn't aware of that, thanks! Yes, it makes sense to accommodate them (though, it's unfortunate that language bindings are source of this restriction).

[ipuustin]

Ok, I'll move the private key operations provider to TlsCertificate. I'll have to set up the SSL socket bit differentlythen (because I'll need to initialize the private key methods for each SSL_CTX).

[htuch]

Is this to break an include dependency loop?

[ipuustin]

Yes, that's why this is needed -- Bazel will complain if this is done with a real dependency.

[htuch]

Arguably we could do some header refactoring to fix this, but I think this is OK if this is a one off.

[ipuustin]

Ok. Just for the record, the dep loop would be this:

    //include/envoy/ssl:context_manager_interface
    //include/envoy/ssl:context_config_interface
.-> //include/envoy/ssl:tls_certificate_config_interface
|   //include/envoy/ssl/private_key:private_key_interface
|   //include/envoy/server:transport_socket_config_interface
|   //include/envoy/secret:secret_manager_interface
|   //include/envoy/secret:secret_provider_interface
`-- //include/envoy/ssl:tls_certificate_config_interface

[htuch]

Nit: handshake

[htuch]

@PiotrSikora do you want to take a pass and do some design level feedback? I'm good with the high-level now, would be good to allow @ipuustin to commit on it for implementation.

[PiotrSikora]

OpenSSL engines are much more than just private key operations, and I don't think treating them as alternatives is going to result in a clean API.

[PiotrSikora]

Yes, this should definitely be a part of TlsCertificate, so that it could be configured on a per-certificate basis.

Ideally, this would be a oneof between private_key and private_key_method, which works well for a lot of use cases (remote oracle, etc.) and is more generic, but it makes this use case (crypto in separate thread) a bit awkward, since then we would need to pass private_key to private_key_method via different means.

Thoughts?

(Using private_key_method instead of private_key_operations to align with the BoringSSL name of this feature, though I don't feel too strongly about it).

[PiotrSikora]

This should probably be fatal, not ignored (same for private_key_methods below).

[ipuustin]

I changed the logic a bit. Originally the idea was that there might be a fallback to software processing, but now with the potential replacing of private_key with private_key_method this should be fatal.

[PiotrSikora]

Can this method be ever called with handshake_complete_ == true? Perhaps this should be an ASSERT() instead?

[ipuustin]

Ok, changed to ASSERT().

[ipuustin]

I mean, changed async_handshake_in_progress_ check to ASSERT(). I think it's possible that (depending on how the private key method provider is implemented) there's a race between the async callback and a doHandshake() call from doWrite() or doRead(). In that case it could be that the handshake is already completed and an ASSERT for !handshake_complete_ would be wrong.

[PiotrSikora]

Do we need to store this?

[ipuustin]

The provider is no longer needed. The ops_ (or actually a vector now) is meant for connecting the operations lifecycle with the SslSocket lifecycle.

[PiotrSikora]

I'm a bit worried about the case where the connection (and SslSocket) goes away while waiting for the async handshake to complete, and then the complete() callback is called, resulting in crash.

Do we have any protections against that?

[ipuustin]

Originally the idea was that the PrivateKeyOperations lifecycle would be tied to the SslSocket lifecycle. I'll have to do this bit differently now when the operations are tied to certificates (which may change on-the-fly if multiple certificate support is used).

[ipuustin]

I moved the private key method support into the TlsCertificate message (from TlsContext). Some things are not yet very well tested, such as if the multiple certificate support works as expected. I changed the naming conventions a bit, but I'll need to think about it still some more.

[htuch]

The move to TlsCertificate LGTM, will wait for the other feedback and more progress to review again.
/wait

[htuch]

Why isn't this part of PrivateKeyOperationsCallbacks? I.e. someone implementing the lifecycle callbacks around an async method should also be able to take an initial SSL binding call as well.

[ipuustin]

I removed the associateWithSsl() function because the SSL object pointer was already available in the provider call. I also did a bunch of renaming effort which should hopefully move the naming conventions closer to the BoringSSL.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #6326 was synchronize by ipuustin.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #6326 was synchronize by ipuustin.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #6326 was synchronize by ipuustin.

see: more, trace.

[ipuustin]

@rojkov @lizan : I replaced the bools with an enum, please take a look. I also merged the master to get the tests working again.

@PiotrSikora : ping!

[rojkov]

Thanks @ipuustin ! I'll update openssl tls accordingly.

[lizan]

LGTM, Appreciate the patience. Let's ship this and see how it goes? @PiotrSikora @htuch

[htuch]

@lizan yeah, I think we can ship as soon as we have sign-off from SSL owners on this, which is @PiotrSikora (and optionally @agl / @davidben).

[PiotrSikora]

Great work, thanks! Sorry about the delay.

[PiotrSikora]

Nit: message -> config (here and in other places, but feel free to ignore it)

[PiotrSikora]

Nit: HandShakeComplete -> HandshakeComplete (here and in other places)

[PiotrSikora]

Nit: private_key_method_provider_context -> transport_socket_factory_context or simply factory_context? (here and in other places)

[PiotrSikora]

Nit: you can pull this into previous line.

[PiotrSikora]

Nit: ...connections using asynchronous private key operations.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #6326 was synchronize by ipuustin.

see: more, trace.

[PiotrSikora]

Thanks!

[lizan]

Needs @htuch's approval (for requested changes and api)

[htuch]

API LGTM, I will approve/merge based on my previous reviews and @lizan + @PiotrSikora recent reviews with approval. Thanks a ton @ipuustin, this is a very valuable contribution to Envoy.

[ipuustin]

A big thanks to all reviewers!

[rgs1]

Hmm any chance this (accidentally) changed the build system so that TLS isn't included in upstream transport sockets anymore?

After this change we get:

 transport_sockets.upstream: envoy.transport_sockets.alts,envoy.transport_sockets.tap,raw_buffer

Note no TLS...

ccc: @ipuustin @htuch

[rgs1]

cc: @fishcakez

[rgs1]

Oops, i think it's just our internal build that accidentally broke. Sorry for the false ping!