A107 - TLS Private Key Offloading

[gtcooke94]

gRFC for TLS Private Key Offloading

[salrashid123]

there's an obscure usecase for supporting MessageSigner here for restricted TPM keys

• crypto/tls: support crypto.MessageSigner golang/go#75656

[gtcooke94]

Thanks for the heads up - I took a look at the change and I think someone implementing crypto.MessageSigner, and I think it should "just work" since crypto/tls supports a crypto.MessageSigner as a PrivateKey in the tls.Certificate.

So I think gRPC will pretty blindly pass this through while configuring tls.Config. If the user implements a crypto.MessageSigner with a SignMessage it'll use that

[salrashid123]

yeah, if anything implements messagesigner, it'd get automatically pickedup and used.

as mentioned internally, that the advancedtls pakage which AFAIK, is only in go, you can offload TLS now

• https://github.com/salrashid123/grpc-cert-provider

[sergiitk]

I'd like to restart the conversation we had elsewhere.

Would it make sense to abstract this call away? Make the signer just return the bytes, and the callback will be handled by our custom function? With this approach, we'll avoid the problem where users forgot to call the callback (f.e. by not wrapping their method into try ... finally).

The main point is to abstract away internal implementation details as much as possible.

# Internal wrapper
def _grpc_invoke_custom_private_key_sign(
    private_key_sign_fn: CustomPrivateKeySign,
    unsigned_data: bytes,
    algorithm: SignatureAlgorithm,
    on_done: PrivateKeySignDoneCallback,
) -> None:
    try:
        signed_bytes = private_key_sign_fn(unsigned_data, algorithm)
        if signed_bytes:
            on_done(signed_bytes, True)
        else:
            on_done(None, False)
    except Exception as e:
        logging.warning("Exception during custom private key sign: %s", e)
        on_done(None, False)


# User's implementation example
def example_signer(
    unsigned_data: bytes, algorithm: SignatureAlgorithm
) -> Optional[bytes]:
    # Manually sign the bytes.
    signed_bytes = ...
    return signed_bytes

[sergiitk]

Spoke with @matthewstevenson88 - he explained some of the async patterns our users might apply when CustomPrivateKeySign. One of them is creating a thread that will do the computation, passing on_done callback to the thread, while immediately returning from the CustomPrivateKeySign. This is a good argument for not introducing an internal wrapper.

I need think about this some more, and consider a few nuances:

• Whether a thread will actually be useful for offloading CPU-bound tasks - because if GIL is stuck on this thread waiting for the signing to complete, there won't be any parallelization/concurrency (unless it calls into a Python C extension that disables GIL)
• Whether we still can use the wrapper approach, but with a different interface, like using the Future interface. Future should make it easier for the user to work with various PoolExecutors.
• Whether we should also allow an async method signature (make private_key_sign_fn a Union[CustomPrivateKeySign, CustomPrivateKeySignAsync]) - which may be required if it's common for the HW offloading libraries the do the signing to use async/await pattern (asyncio python).

[sergiitk]

For the reference,

GIL and performance considerations

Unlike the multiprocessing module, which uses separate processes to bypass the global interpreter lock (GIL), the threading module operates within a single process, meaning that all threads share the same memory space. However, the GIL limits the performance gains of threading when it comes to CPU-bound tasks, as only one thread can execute Python bytecode at a time. Despite this, threads remain a useful tool for achieving concurrency in many scenarios.

— https://docs.python.org/3/library/threading.html#gil-and-performance-considerations

[gtcooke94]

Have you had the chance to consider these options? We need to move relatively quickly on this, and that prototype that I made earlier in the year seemed viable? Can we keep this and disable the GIL?

Can we allow for Future or asyncio with the cython integration?

I'll lean on you for these decisions since you are the Python expert

[gtcooke94]

I've been toying around with how we wrap this more

The C++ layers knows it is wrapping Python. What if we just make the C++ wrapper layer responsible for the async.

i.e.

... implementing the interface ...
class PrivateKeySignerPyWrapper : PrivateKeySigner {

... details, assume self_->sign() is calling a cython method that's holding a python callable and is going to call into Python, here's where we _actually_ call it  ...
  
  thread::Detach({}, [s = std::string(input), self = shared_from_this(),
                       context] {
    PyGILState_STATE gstate;
    gstate = PyGILState_Ensure();
    // This is calling into Python
    self->sign_(s, CompletionCallbackForPy, context, self->user_data_);
    PyGILState_Release(gstate);
  });

}

We could go even further in this case, the python sign function doesn't need to know about a callback, calling the callback, or handling async stuff - it just simply returns signed data sync.

  thread::Detach({}, [s = std::string(input), self = shared_from_this(),
                       context] {
    PyGILState_STATE gstate;
    gstate = PyGILState_Ensure();
    // This is calling into Python
    std::string signed_data = self->sign_(s, context, self->user_data_);
    PyGILState_Release(gstate);
    // C++ wrapping layer is responsible for the async and calling the callback
    CompletionCallback(signed_data);
  });

[sergiitk]

We should mention that gevent is not supported, both in the gRFC and the ssl_channel_credentials_with_custom_signer docstring.

[gtcooke94]

Done

[sergiitk]

I don't think this in relevant in the last iteration - we provided a new method instead of modifying the existing one to support private_key

[sergiitk]

note - we require arguments to be passed as keyword arguments now.

Suggested change

	creds = ssl_channel_credentials_with_custom_signer(<some_root>, example_signer, <some_chain>)
	creds = ssl_channel_credentials_with_custom_signer(
	private_key_sign_fn=example_signer,
	root_certificates=b"<some_root>",
	certificate_chain=b"<some_chain>",
	)

[markdroth]

Thanks for writing this up!

Please let me know if you have any questions. Thanks!

[markdroth]

I don't think we actually implemented this metric in C-core. Do we still need to do that?

Implementing this in C-core raises a layering question. The TLS TSI handshaker is the thing that has this data to export, but the non-per-call metrics APIs are gRPC-specific APIs, which means that the TSI implementation would need to use gRPC-specific APIs. Won't that make it harder for this TSI implementation to be used outside of gRPC? Are we okay with that?

[gtcooke94]

We have no security metrics yet (that's happening ~now, we are working on the concept review) - this will be included in that work.

The layering question is one of the prime questions for our concept and design.

[markdroth]

If we haven't implemented this as part of this feature, how about we leave the metric out of this gRFC and include it in the forthcoming gRFC for all of the security-related metrics?

[markdroth]

This is moving in the right direction!

Please let me know if you have any questions. Thanks!

[markdroth]

Please change these lines to:

* [A66: OpenTelemetry Metrics][A66]
* [A79: Non-per-call Metrics Architecture][A79]

[markdroth]

If we haven't implemented this as part of this feature, how about we leave the metric out of this gRFC and include it in the forthcoming gRFC for all of the security-related metrics?

[markdroth]

Just a few comments left.

Note that there are also a couple of unresolved comments from my previous review pass that still need to be addressed.

Thanks!

[markdroth]

Let's be more specific here. What exactly will fail? I assume the failure won't be seen until handshake time, right?

[markdroth]

We've already described the InMemoryCertificateProvider above, so we don't need to introduce the fact that we're adding a new cert provider impl here. Instead, this should just say something like "The private key signer can be configured via the InMemoryCertificateProvider using a new IdentityKeyOrSignerCertPair struct, as shown below."

[markdroth]

This one belongs in the python list, not the C-core/C++ list.