Inefficiency in SNI policy parsing

[zwoop]

It seems that rather than parsing these types of "text" configs once, we parse it over and over again (strcmp()) on each transaction hitting this code:

std::string_view
HttpSM::get_outbound_sni() const
{
  using namespace ts::literals;
  ts::TextView zret;
  ts::TextView policy{t_state.txn_conf->ssl_client_sni_policy, ts::TextView::npos};

  if (policy.empty() || !strcmp(policy, "host"_tv)) {
    // By default the host header field value is used for the SNI.
    int len;
    char const *ptr = t_state.hdr_info.server_request.host_get(&len);
    zret.assign(ptr, len);
  } else if (ua_txn && !strcmp(policy, "server_name"_tv)) {
    zret.assign(ua_txn->get_netvc()->get_server_name(), ts::TextView::npos);
  } else if (policy.front() == '@') { // guaranteed non-empty from previous clause
    zret = policy.remove_prefix(1);
  } else {
    // If other is specified, like "remap" and "verify_with_name_source", the remapped origin name is used for the SNI value
    zret.assign(t_state.server_info.name, ts::TextView::npos);
  }
  return zret;
}

[masaori335]

set_tls_options has the same issue (proxy.config.ssl.client.verify.server.policy/proxy.config.ssl.client.verify.server.properties), I think.

trafficserver/proxy/http/HttpSM.cc

Lines 4810 to 4849 in d4fc16f

	static void
	set_tls_options(NetVCOptions &opt, const OverridableHttpConfigParams *txn_conf)
	{
	char *verify_server = nullptr;
	if (txn_conf->ssl_client_verify_server_policy == nullptr) {
	opt.verifyServerPolicy = YamlSNIConfig::Policy::UNSET;
	} else {
	verify_server = txn_conf->ssl_client_verify_server_policy;
	if (strcmp(verify_server, "DISABLED") == 0) {
	opt.verifyServerPolicy = YamlSNIConfig::Policy::DISABLED;
	} else if (strcmp(verify_server, "PERMISSIVE") == 0) {
	opt.verifyServerPolicy = YamlSNIConfig::Policy::PERMISSIVE;
	} else if (strcmp(verify_server, "ENFORCED") == 0) {
	opt.verifyServerPolicy = YamlSNIConfig::Policy::ENFORCED;
	} else {
	Warning("%s is invalid for proxy.config.ssl.client.verify.server.policy. Should be one of DISABLED, PERMISSIVE, or ENFORCED",
	verify_server);
	opt.verifyServerPolicy = YamlSNIConfig::Policy::UNSET;
	}
	}
	if (txn_conf->ssl_client_verify_server_properties == nullptr) {
	opt.verifyServerProperties = YamlSNIConfig::Property::UNSET;
	} else {
	verify_server = txn_conf->ssl_client_verify_server_properties;
	if (strcmp(verify_server, "SIGNATURE") == 0) {
	opt.verifyServerProperties = YamlSNIConfig::Property::SIGNATURE_MASK;
	} else if (strcmp(verify_server, "NAME") == 0) {
	opt.verifyServerProperties = YamlSNIConfig::Property::NAME_MASK;
	} else if (strcmp(verify_server, "ALL") == 0) {
	opt.verifyServerProperties = YamlSNIConfig::Property::ALL_MASK;
	} else if (strcmp(verify_server, "NONE") == 0) {
	opt.verifyServerProperties = YamlSNIConfig::Property::NONE;
	} else {
	Warning("%s is invalid for proxy.config.ssl.client.verify.server.properties. Should be one of SIGNATURE, NAME, or ALL",
	verify_server);
	opt.verifyServerProperties = YamlSNIConfig::Property::NONE;
	}
	}
	}

[SolidWallOfCode]

The tricky bit here is the value is not strictly an enumeration. It can have an arbitrary value as long as that value starts with "@". In such a case, the "@" is removed and the literal value is used as the outbound SNI. This means a string is required in some cases. What would be needed is an enumeration and a string, with the string set only when it's explicitly specified by a plugin.

[ywkaras]

Agreed https://github.com/ywkaras/trafficserver/blob/e21287f4da9bdb7b670e94c97736f5dd30b2a8ac/include/tscpp/util/CodeOrStr.h#L456 .