Repository: envoyproxy/envoy
File: source/common/tls/context_config_impl.cc (C++)

Code before:

absl::StatusOr<std::unique_ptr<ClientContextConfigImpl>> ClientContextConfigImpl::create(
    const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& config,
    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
  absl::Status creation_status = absl::OkStatus();
  std::unique_ptr<ClientContextConfigImpl> ret = absl::WrapUnique(
      new ClientContextConfigImpl(config, secret_provider_context, creation_status));
  RETURN_IF_NOT_OK(creation_status);
  return ret;
}

ClientContextConfigImpl::ClientContextConfigImpl(
    const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& config,
    Server::Configuration::TransportSocketFactoryContext& factory_context,
    absl::Status& creation_status)
    : ContextConfigImpl(
          config.common_tls_context(), config.auto_sni_san_validation(), DEFAULT_MIN_VERSION,
          DEFAULT_MAX_VERSION, FIPS_mode() ? DEFAULT_CIPHER_SUITES_FIPS : DEFAULT_CIPHER_SUITES,
          FIPS_mode() ? DEFAULT_CURVES_FIPS : DEFAULT_CURVES, factory_context, creation_status),
      server_name_indication_(config.sni()), auto_host_sni_(config.auto_host_sni()),
      allow_renegotiation_(config.allow_renegotiation()),
      enforce_rsa_key_usage_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforce_rsa_key_usage, true)),
      max_session_keys_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_session_keys, 1)) {

  if (!config.has_enforce_rsa_key_usage()) {
    ENVOY_LOG(
        warn,
        "The 'enforce_rsa_key_usage' option is not configured, its default value is changed to "
        "true, and the config option will be removed in the next version. The handshake will "
        "fail if the keyUsage extension is present and incompatible with the TLS usage. Please "
        "update the certificates to be compliant.");
  } else if (!enforce_rsa_key_usage_) {
    ENVOY_LOG(
        warn,
        "The 'enforce_rsa_key_usage' option is set to false, which disables the enforcement of RSA "
        "key usage. This option will be removed in the next version. The handshake will fail "
        "if the keyUsage extension is present and incompatible with the "
        "TLS usage. Please update the certificates to be compliant.");
    factory_context.serverFactoryContext().runtime().countDeprecatedFeatureUse();
  }

  // BoringSSL treats this as a C string, so embedded NULL characters will not
  // be handled correctly.
  if (server_name_indication_.find('\0') != std::string::npos) {
    creation_status = absl::InvalidArgumentError("SNI names containing NULL-byte are not allowed");
    return;
  }

  // TODO(PiotrSikora): Support multiple TLS certificates.
  if ((config.common_tls_context().tls_certificates().size() +
       config.common_tls_context().tls_certificate_sds_secret_configs().size()) > 1 &&

Review comment (refactor):
Maybe we shouldn't warn in this case, and just rely on the release note. With this warning, there's no way to launch envoy without some warning logged: either this message if it's unset, a log of using a deprecated feature if it is set and is true, and a log of using a deprecated feature and the below log message if its set and false. If we remove this case and only warn for `!enforce_rsa_key_usage_` then people who are using the new behavior don't get a warning, which seems fine.

Code after:

absl::StatusOr<std::unique_ptr<ClientContextConfigImpl>> ClientContextConfigImpl::create(
    const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& config,
    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
  absl::Status creation_status = absl::OkStatus();
  std::unique_ptr<ClientContextConfigImpl> ret = absl::WrapUnique(
      new ClientContextConfigImpl(config, secret_provider_context, creation_status));
  RETURN_IF_NOT_OK(creation_status);
  return ret;
}

ClientContextConfigImpl::ClientContextConfigImpl(
    const envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext& config,
    Server::Configuration::TransportSocketFactoryContext& factory_context,
    absl::Status& creation_status)
    : ContextConfigImpl(
          config.common_tls_context(), config.auto_sni_san_validation(), DEFAULT_MIN_VERSION,
          DEFAULT_MAX_VERSION, FIPS_mode() ? DEFAULT_CIPHER_SUITES_FIPS : DEFAULT_CIPHER_SUITES,
          FIPS_mode() ? DEFAULT_CURVES_FIPS : DEFAULT_CURVES, factory_context, creation_status),
      server_name_indication_(config.sni()), auto_host_sni_(config.auto_host_sni()),
      allow_renegotiation_(config.allow_renegotiation()),
      enforce_rsa_key_usage_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforce_rsa_key_usage, true)),
      max_session_keys_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_session_keys, 1)) {

  if (!enforce_rsa_key_usage_) {
    ENVOY_LOG(
        warn,
        "The 'enforce_rsa_key_usage' option is set to false, which disables the enforcement of RSA "
        "key usage. This option will be removed in the next version. The handshake will fail "
        "if the keyUsage extension is present and incompatible with the "
        "TLS usage. Please update the certificates to be compliant.");
  }

  // BoringSSL treats this as a C string, so embedded NULL characters will not
  // be handled correctly.
  if (server_name_indication_.find('\0') != std::string::npos) {
    creation_status = absl::InvalidArgumentError("SNI names containing NULL-byte are not allowed");
    return;
  }

  // TODO(PiotrSikora): Support multiple TLS certificates.
  if ((config.common_tls_context().tls_certificates().size() +
       config.common_tls_context().tls_certificate_sds_secret_configs().size()) > 1 &&
      !config.common_tls_context().has_custom_tls_certificate_selector()) {
    creation_status = absl::InvalidArgumentError(
        "Multiple TLS certificates are not supported for client contexts");
    return;
  }

  if (config.common_tls_context().has_custom_tls_certificate_selector()) {
    const auto& provider_config = config.common_tls_context().custom_tls_certificate_selector();