JosiahWI commented on code in PR #9767:
URL: https://github.com/apache/trafficserver/pull/9767#discussion_r1238421948
##########
iocore/net/TLSSNISupport.cc:
##########
@@ -64,8 +67,12 @@ TLSSNISupport::perform_sni_action()
}
SNIConfig::scoped_config params;
- if (const auto &actions = params->get({servername,
std::strlen(servername)}); !actions.first) {
- Debug("ssl_sni", "%s not available in the map", servername);
+ // should always work in this context of SSL action callbacks
+ SSLNetVConnection *ssl_vc{dynamic_cast<SSLNetVConnection *>(this)};
Review Comment:
I took a look this, and I think it might be time to refactor.
```cpp
class SSLNetVConnection : public UnixNetVConnection,
public ALPNSupport,
public TLSSessionResumptionSupport,
public TLSSNISupport,
public TLSEarlyDataSupport,
public TLSTunnelSupport,
public TLSCertSwitchSupport,
public TLSBasicSupport
```
This is where we obtain the TLSSNIConfig:
```cpp
TLSSNISupport *
TLSSNISupport::getInstance(SSL *ssl)
{
return static_cast<TLSSNISupport *>(SSL_get_ex_data(ssl, _ex_data_index));
}
```
and SSLUtils.cc does the actual calls to perform SNI actions, on the
`TLSSNISupport` object it got from this... apparently some global data stored
using OpenSSL? Am I reading that right?
So there's no `SSLNetVConnection` in sight. The only way we can obtain it is
to `dynamic_cast`. You're not the only person who's noticed this:
```cpp
// TODO: change design to avoid this dynamic_cast
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(snis);
```
@masaori335 put in this TODO, maybe he's thought some about how to do this.
I suggest we aim to do the design change in a separate PR, as it will probably
be nontrivial.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
