-
Notifications
You must be signed in to change notification settings - Fork 249
A107 - TLS Private Key Offloading #524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| // Note that when a signature of a hash of a larger message is needed, | ||
| // the caller is responsible for hashing the larger message and passing | ||
| // the hash (as digest) and the hash function (as opts) to Sign. | ||
| Sign(rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's an obscure usecase for supporting MessageSigner here for restricted TPM keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| The most complex piece of this is the implementation \- C-Core/Cython/Python must handle the calling of the user provided Python signing function from C which must invoke a callback that is passed to it. This will involve creating bridge types between the Python user sign function and the expected `absl::AnyInvocable` as well as bridging the callback that is passed to the user sign function while managing the GIL and asynchronous nature of the signing. This is technically feasible with Cython. A proof-of-concept of this structure [is written here.](https://source.corp.google.com/piper///depot/google3/experimental/users/gregorycooke/python_cpp_wrapping/) | ||
|
|
||
| ```py | ||
| # Example Usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_bytesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Futureshould make it easier for the user to work with variousPoolExecutors. - Whether we should also allow an
asyncmethod signature (makeprivate_key_sign_fnaUnion[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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| // To use, set the provider on the TlsCredentialsOptions | ||
| ``` | ||
|
|
||
| ### Python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention that gevent is not supported, both in the gRFC and the ssl_channel_credentials_with_custom_signer docstring.
|
|
||
| ``` | ||
|
|
||
| We won't significantly refactor the Python API surface \- instead we will allow the `private_key` input to be a signing function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
|
||
| # Now the user is in their application configuring gRPC | ||
| # Create creds with the custom signer | ||
| creds = ssl_channel_credentials_with_custom_signer(<some_root>, example_signer, <some_chain>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note - we require arguments to be passed as keyword arguments now.
| 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>", | |
| ) |
gRFC for TLS Private Key Offloading