[DO NOT MERGE] Add min and max TLS version options#23527
[DO NOT MERGE] Add min and max TLS version options#23527Ryanfsdf wants to merge 4 commits intogrpc:masterfrom
Conversation
c39f885 to
8f223e3
Compare
8f223e3 to
dbe83e2
Compare
dbe83e2 to
9fca7e1
Compare
97a662c to
c8ec8f2
Compare
6282563 to
19dbc88
Compare
19dbc88 to
407127b
Compare
matthewstevenson88
left a comment
There was a problem hiding this comment.
Thanks Ryan! I've left some more comments.
matthewstevenson88
left a comment
There was a problem hiding this comment.
Approved, with 1 nit.
f1f0f28 to
b229aea
Compare
|
Please hold on this PR until we know if we need to temporarily disable TLS 1.3 in grpc core. |
|
Currently user cannot control TLS version. It is not a good idea if we explicitly expose an API to allow user to set TLS max/min version, but it won't work due to PR #23608. Shall we hold on this PR until PR #23608 has been reverted? @matthewstevenson88 Any suggestion? |
|
Just to check: you mean it is a good idea if we (eventually) expose an API to allow setting min/max versions, correct? :) (I assume this is what you meant because both gRPC-Java and gRPC-Go expose such an API already.) I think we should do the following:
WDYT? |
|
SG. I agree the value of such API. |
jiangtaoli2016
left a comment
There was a problem hiding this comment.
Let's first agree on API changes with gRPC folks cc @markdroth
| - creds: an instance of |grpc_ssl_credentials|. If |creds| is nullptr, then | ||
| this is a no-op. | ||
| - min_tls_version: the minimum TLS version. */ | ||
| GRPCAPI void grpc_ssl_credentials_set_min_tls_version( |
There was a problem hiding this comment.
I do not fully understand why we want to create new APIs for setting min and max TLS version after credentials object has been created. It seems that we should pass min_tls_version and max_tls_version during create, passing min and max TLS version as credentials options to create().
grpc_ssl_credentials_create_with_options (replacing grpc_ssl_credentials_create_ex)
grpc_ssl_server_credentials_create_with_options
@markdroth what is your take on this?
| const char* pem_root_certs, grpc_ssl_pem_key_cert_pair* pem_key_cert_pair, | ||
| const verify_peer_options* verify_options, void* reserved); | ||
|
|
||
| /** Sets the minimum TLS version that will be negotiated during the TLS |
There was a problem hiding this comment.
We should clarify in the comment that min and max TLS version only work for OpenSSL and BoringSSL 1.1 and above.
| const verify_peer_options* verify_options, void* reserved); | ||
|
|
||
| /** Sets the minimum TLS version that will be negotiated during the TLS | ||
| handshake. The caller MUST ensure that |creds| is an instance of |
There was a problem hiding this comment.
nit: it is not a grpc tradition to wrap parameter with "|" in comment. you can just remove "|".
markdroth
left a comment
There was a problem hiding this comment.
I would prefer not to add any new features to the old SSL creds API at this point. Instead, we should add this to the TLS creds API. That way, we're not encouraging more people to use the old interface.
Note that all non-experimental API changes require a gRFC (see https://github.com/grpc/proposal/), for both C-core and C++ APIs. In addition, C++ API changes must be backward-compatible. What this means is that it's going to be a lot easier to add this to the TLS creds API, since that API is still experimental. We can structure the API however we want, even if it changes the current API. Whatever we come up with here will wind up being part of the gRFC that @yihuazhang is writing for the TLS creds API.
Reviewed 9 of 16 files at r1, 1 of 7 files at r2, 6 of 7 files at r3.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @jiangtaoli2016, @matthewstevenson88, and @Ryanfsdf)
include/grpc/grpc_security.h, line 1 at r3 (raw file):
/*
Looks like you've accidentally made a number of files executable. Please revert those changes.
include/grpc/grpc_security.h, line 249 at r3 (raw file):
Previously, jiangtaoli2016 (Jiangtao Li) wrote…
I do not fully understand why we want to create new APIs for setting min and max TLS version after credentials object has been created. It seems that we should pass min_tls_version and max_tls_version during create, passing min and max TLS version as credentials options to create().
grpc_ssl_credentials_create_with_options(replacing grpc_ssl_credentials_create_ex)
grpc_ssl_server_credentials_create_with_options@markdroth what is your take on this?
I agree. This should be part of the credential options that are set at creation time. We should not need to introduce any new methods for this.
markdroth
left a comment
There was a problem hiding this comment.
With regard to #23608, we should be ready to revert it fairly soon, so I'd way wait for the revert before moving forward with this PR.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @jiangtaoli2016, @matthewstevenson88, and @Ryanfsdf)
|
@ZhenLian will own the gRFC on new TLS creds. |
Added the plumbing for specifying the min and max TLS versions. This was implemented in the grpc core but not for the cpp wrapper. This PR exposes the ability to specify the min and max TLS version in the cpp wrapper.
This builds on PR #23165.
The
TlsVersionRangestruct was created on the client side because we needed to create a default value for the min and max TLS versions while maintaining API stability. Without the struct, we would not be able to set a default value for min and max TLS versions because adding a constructor to set default values would break struct initialization, as no constructor is currently defined. This was not needed for the server side because a constructor was already defined and we could define the defaults in the constructors.@matthewstevenson88
This change is