Per discuss in the internal CL, we could move
#if defined(TLS1_3_VERSION) right after line 913.
Then we keep the original switch flow (except removing lines 918, 922, 932, 936). The code will be much readable and extensible.

Basically, we only set min & max proto version if OPENSSL_VERSION_NUMBER >= 0x10100000 and TLS1_3_VERSION is defined. Otherwise, it will be no-op.

To confirm, are you proposing the following code?

#if OPENSSL_VERSION_NUMBER >= 0x10100000
#if defined(TLS1_3_VERSION)
  switch (min_tls_version) {
    case tsi_tls_version::TSI_TLS1_2:
      SSL_CTX_set_min_proto_version(ssl_context, TLS1_2_VERSION);
      break;
    case tsi_tls_version::TSI_TLS1_3:
      SSL_CTX_set_min_proto_version(ssl_context, TLS1_3_VERSION);
      break;
    default:
      // Some logging.
      break;
  }

  switch (max_tls_version) {
    case tsi_tls_version::TSI_TLS1_2:
      SSL_CTX_set_max_proto_version(ssl_context, TLS1_2_VERSION);
      break;
    case tsi_tls_version::TSI_TLS1_3:
      SSL_CTX_set_max_proto_version(ssl_context, TLS1_3_VERSION);
      break
    default:
      // Some logging.
      break;
  }
#endif
#endif

If so, that's more than just a readability change - that's a functional change. Consider the case when OPENSSL_VERSION_NUMBER >= 0x10100000 and TLS1_3_VERSION is not defined. Then, this method is a no-op, so there are no restrictions on the minimum TLS version. And since we've instantiated the SSL_CTX using TLS_method rather than TLSv1_2_method (see here), we have opened ourselves up to a downgrade attack where the peer could try to negotiate TLS 1.1 or worse.

Please correct me if I misunderstood your proposal. :)

@davidben to keep me honest on the fact that we must set a min TLS version if we use TLS_method.

If you do not set a minimum version with TLS_method, it'll use the library's default. That default, for BoringSSL, is currently TLS 1.0. So, yeah, if your aim is to reject TLS 1.0 and 1.1, you need to set a minimum version.

This PR is a little odd though. So, if I understand right, the issue is that OpenSSLs which lack TLS 1.3 will break when you set the maximum to TSI_TLS1_3, rather than silently going down to TLS 1.2. But this also loses the TSI_FAILED_PRECONDITION logic, which seems valuable because if someone uses a totally random constant, you'd like for the library to fail.

It seems to me the desired logic should be something like:

Leave this block unchanged. If the caller requested TLS 1.3 and the underlying
library is incapable of providing TLS 1.3, the connection should fail, rather than
silently allowing TLS 1.2.

  // Set the min TLS version of the SSL context.
  switch (min_tls_version) {
    case tsi_tls_version::TSI_TLS1_2:
      SSL_CTX_set_min_proto_version(ssl_context, TLS1_2_VERSION);
      break;
// If the library does not support TLS 1.3, and the caller requested a minimum
// of TLS 1.3, return an error. The caller's request cannot be satisfied.
#if defined(TLS1_3_VERSION)
    case tsi_tls_version::TSI_TLS1_3:
      SSL_CTX_set_min_proto_version(ssl_context, TLS1_3_VERSION);
      break;
#endif
    default:
      gpr_log(GPR_INFO, "TLS version is not supported.");
      return TSI_FAILED_PRECONDITION;
  }

Change this block so if the library doesn't support TLS 1.3, set a maximum of
TLS 1.2, which is still compatible with what they requested.

  // Set the max TLS version of the SSL context.
  switch (max_tls_version) {
    case tsi_tls_version::TSI_TLS1_2:
      SSL_CTX_set_max_proto_version(ssl_context, TLS1_2_VERSION);
      break;
    case tsi_tls_version::TSI_TLS1_3:
#if defined(TLS1_3_VERSION)
      SSL_CTX_set_max_proto_version(ssl_context, TLS1_3_VERSION);
#else
      // The library doesn't support TLS 1.3, so set a maximum of
      // TLS 1.2 instead.
      SSL_CTX_set_max_proto_version(ssl_context, TLS1_2_VERSION);
#endif
      break;
    default:
      gpr_log(GPR_INFO, "TLS version is not supported.");
      return TSI_FAILED_PRECONDITION;
  }

(There may be a cleaner way to express this.)

Thanks for the suggestion David! I've added it in the newest commit.

Some context: A user with LibreSSL encountered a failure due to this method: the gRPC stack requested min=1.2, max=1.3, and LibreSSL satisfied OPENSSL_VERSION_NUMBER >= 0x10100000 but did not have TLS1_3_VERSION defined. As I understand it, LibreSSL is actively working on adopting OpenSSL's TLS 1.3 semantics, but this fix is needed in the meantime.

To avoid this kind of problem in the future, @veblush will add e2e tests against different SSL libraries (see #24960).

Thanks @davidben for suggestion! The new logic makes sense to me.

The reason we introduced min/max TLS version was mainly for testing. Since TLS 1.2 and TLS 1.3 behave differently. We do not have customer request for only wanting a specific version of TLS.

nit. s/libraary/library

Good catch, thanks!

Thanks @davidben for suggestion! The new logic makes sense to me.

The reason we introduced min/max TLS version was mainly for testing. Since TLS 1.2 and TLS 1.3 behave differently. We do not have customer request for only wanting a specific version of TLS.