A107: TLS Private Key Offloading#524
Conversation
|
|
||
| ``` | ||
|
|
||
| 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.
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.
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>", | |
| ) |
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
Thanks for writing this up!
Please let me know if you have any questions. Thanks!
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This is moving in the right direction!
Please let me know if you have any questions. Thanks!
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
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
changed the title
sergiitk
left a comment
sergiitk
left a comment
There was a problem hiding this comment.
Python: left notes on improving the usage example
|
|
||
| // Down the line update | ||
| provider.UpdateCertificate(&tls.Certificate(<some updated thing>)) | ||
|
|
There was a problem hiding this comment.
For completeness sake, does it also make sense to show how this options struct will be used to create a credentials and how it will be passed to create the grpc channel?
There was a problem hiding this comment.
Yes - I actually removed this line about UpdateCertificate because I think it's just confusing for this example.
sergiitk
left a comment
sergiitk
left a comment
There was a problem hiding this comment.
Python looks good! Just left a few very minor notes / suggestions
| ) | ||
| p.start() | ||
|
|
||
| # Per the Python API, return a callable matching PrivateKeySignCancel |
There was a problem hiding this comment.
Instead, let's explain the flow? Something like "This will be called when ...`
| return cancel | ||
|
|
||
|
|
||
| # In the Python code configuring gRPC, create creds with the custom signer |
There was a problem hiding this comment.
nit: now this is a part of "Concurrent example", which may be a bit confusing. Let's move it to the top, so the flow is as follows:
Usage example:
creds = grpc.experimental.ssl_channel_credentials_with_custom_signer(
private_key_sign_fn=your_signer_fn
certificate_chain=your_cert_chain,
root_certificates=your_root_certs,
)Synchronous (blocking) signer example:
def sync_client_private_key_signer(
data_to_sign,
signature_algorithm,
on_complete,
): ...Concurrent signer example:
import multiprocessing
...There was a problem hiding this comment.
Good call, moved around
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Looks like this has reached consensus. I did notice a few minor things while checking if people's comments were addressed.
| IdentityProvider: provider, | ||
| }, | ||
| } | ||
| clientTLSCreds, err := advancedtls.NewClientCreds(options) |
| crypto/tls does **not** support asynchronous private key signing). | ||
|
|
||
| We are largely restricted by the underlying security libraries in each language. | ||
| In the following sections, each language's API will be discussed as they are |
There was a problem hiding this comment.
"following sections" accidentally left over after this text moved?
There was a problem hiding this comment.
Moved it above each language section
| ---- | ||
| * Author: @gtcooke94 | ||
| * Approver: ejona86 | ||
| * Status: C++ implemented |
There was a problem hiding this comment.
This was the status of the gRFC, with a fixed enumeration of values. So it would have just been "Implemented". The next line would give specifics as to the languages that are complete.
Although with #275 just merged, you could delete this line instead. We removed it because it was essentially always wrong and multiple of the states only applied while it was a PR which didn't provide value.
| * Author: @gtcooke94 | ||
| * Approver: ejona86 | ||
| * Status: C++ implemented | ||
| * Implemented in: C++, Go |
There was a problem hiding this comment.
This is for languages that already have it implemented. Is Go already implemented? I don't see InMemoryCertProvider; was it renamed?
There was a problem hiding this comment.
Just C++, removed Go
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
That should have been approved, since there is consensus. Do fix up the few minor comments, though.
7 participants
gRFC for TLS Private Key Offloading