Fix client_ssl_test and server_ssl_test when built against OpenSSL 1.0.2.#25865
Fix client_ssl_test and server_ssl_test when built against OpenSSL 1.0.2.#25865matthewstevenson88 merged 25 commits intogrpc:masterfrom
Conversation
This reverts commit 383c247.
matthewstevenson88
added
area/security
area/test
release notes: no
|
@veblush FYI: still getting one weird failure, will look into it this week. Sorry for the delay. |
|
@matthewstevenson88 No worry and please take your time! |
| // All cipher suites for default are compliant with HTTP2. | ||
| GPR_GLOBAL_CONFIG_DEFINE_STRING( | ||
| grpc_ssl_cipher_suites, | ||
| "TLS_AES_128_GCM_SHA256:" |
There was a problem hiding this comment.
Is it okay to drop this? gRPC Java still use it though. https://github.com/netty/netty/blob/d34212439068091bcec29a8fad4df82f0a82c638/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2SecurityUtil.java#L69
There was a problem hiding this comment.
@davidben told me a while ago it was ok to drop this. David: could you confirm?
There was a problem hiding this comment.
Java and OpenSSL don't have the same API. Cipher suite configuration is a TLS implementation thing, not a TLS protocol thing. The TLS protocol doesn't have opinions on which part is configurable by callers. (Making them configurable was probably a mistake, but so it goes.)
So it depends on what this list is for. I see it's not defined in an OpenSSL-specific file, so gRPC seems to think it's a generic cross-TLS-implementation thing? Yet it uses OpenSSL-specific cipher suite names (the odd shortened spelling with hyphens is unique to OpenSSL), so there seems to be something strange going on here.
Anyway, the short answer is yes, they're unnecessary in the OpenSSL API. The long answer is:
- In BoringSSL, we do not let you configure TLS 1.3 cipher suites. That configurability was a mistake. So any you pass into
SSL_CTX_set_cipher_listare just ignored. - In OpenSSL 1.0.2 and 1.1.0, TLS 1.3 is not supported, so any you pass into
SSL_CTX_set_cipher_listare just ignored. - In OpenSSL 1.1.1,
SSL_CTX_set_cipher_listdoes not impact TLS 1.3 cipher suites, so any you pass intoSSL_CTX_set_cipher_listare irrelevant. If you callSSL_CTX_set_ciphersuites, the TLS 1.3 cipher suites do matter. (This is necessary for compatibility so TLS 1.3 doesn't break in old applications that only configure TLS 1.2 ciphers.)
However, all of this is specific to the OpenSSL API. If this is meant to be a cross-TLS thing, you probably shouldn't make that assumption. But the format you all use isn't suitable for a cross-TLS thing so I dunno.
There was a problem hiding this comment.
Thanks for clarifying David! The only use of this cipher list is to pass in to the SSL_CTX_set_cipher_list API. However, this is poorly documented and this list is used very far away from where it is defined. To be safe, I've removed this change from this PR and will send a follow-up PR that changes the cipher list and adds documentation.
There was a problem hiding this comment.
Thank you for the detailed context!
veblush
left a comment
veblush
left a comment
There was a problem hiding this comment.
Thank you for the fix! I'll try to rerun the test once this is merged!
5 participants
This should hopefully fix the remaining bugs we found in #24960. More precisely, it does the following:
In
test/core/handshake/server_ssl_common.cc, defer cleanup of the SSL libraries until all tests have finished. This is needed because cleanup of the SSL libraries must be done manually when building against OpenSSL 1.0.2, whereas it is done automatically for newer OpenSSL versions.In
test/core/handshake/client_ssl.cc, prevent a race condition in initializing the SSL libraries and similarly defer cleanup of the SSL libraries, add substantial logging to the test server, add a check for compatibility of the cert and private key to the test server, and have the test server do automatic curve selection (otherwise, the test server may not have a shared cipher with the client).InThis will be done in a follow-up PR.src/core/lib/security/security_connector/ssl_utils.cc, remove the TLS 1.3-specific ciphers from the list of allowed ciphers, because they are not needed. This was suggested by @davidben in the review of Enable TLS 1.3 in the C-core and all wrapped languages. #23165.