Skip to content

Don't rely on SSLNetVC when HttpSM gathers info about SSL #7961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maskit merged 1 commit into from
Jun 25, 2021
+15 −9
Merged

Don't rely on SSLNetVC when HttpSM gathers info about SSL #7961

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions proxy/http/HttpSM.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)

// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);

is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());

if (ssl_vc != nullptr) {
if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the multiple dynamic_cast operations are a concern. One alternative would be to put a virtual method on UnixNetVConnection like

virtual TLSBasicSupport * tls_services() { return nullptr; }

Copy link
Copy Markdown
Member Author

@maskit maskit Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also fine with it. One thing I don't like very much is that we'd need to have a function for each service. Is it possible to have an universal function like this?

T *getService<T>();

Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I might need to bring in a bit of meta programming support from libSWOC. You'd want something like

template<typename T> auto get_service_for() -> EnableForServiceTypes<T, T*> { return static_cast<T*>(this); }

This requires a type list backing it up, which lists all of the types for which there is a service. This compiles for those types and not for other types.

A problem is template functions/methods cannot be also virtual. You must choose between the cost of dynamic_cast or a fatter base API.

It would be possible for the mixins to provide the methods, but it would still be necessary to put a using in the inheriting class

class QUIC : public TicketService {
  using TicketService::ticket_service();
};

Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this more, I'm not sure why feature_for<TLSBasicSupport>() is much better than tls_feature(). It's only three methods which isn't much bloat and in the end, these feature sets are chunky and important enough that it's reasonable to promote virtual methods for them to UnixNetVConnection.

client_connection_is_ssl = true;
client_ssl_reused = ssl_vc->getSSLSessionCacheHit();
const char *protocol = ssl_vc->get_tls_protocol_name();
const char *protocol = tbs->get_tls_protocol_name();
client_sec_protocol = protocol ? protocol : "-";
const char *cipher = ssl_vc->get_tls_cipher_suite();
const char *cipher = tbs->get_tls_cipher_suite();
client_cipher_suite = cipher ? cipher : "-";
const char *curve = ssl_vc->get_tls_curve();
const char *curve = tbs->get_tls_curve();
client_curve = curve ? curve : "-";
client_alpn_id = ssl_vc->get_negotiated_protocol_id();

if (!client_tcp_reused) {
// Copy along the TLS handshake timings
milestones[TS_MILESTONE_TLS_HANDSHAKE_START] = ssl_vc->get_tls_handshake_begin_time();
milestones[TS_MILESTONE_TLS_HANDSHAKE_END] = ssl_vc->get_tls_handshake_end_time();
milestones[TS_MILESTONE_TLS_HANDSHAKE_START] = tbs->get_tls_handshake_begin_time();
milestones[TS_MILESTONE_TLS_HANDSHAKE_END] = tbs->get_tls_handshake_end_time();
}
}

if (auto as = dynamic_cast<ALPNSupport *>(netvc)) {
client_alpn_id = as->get_negotiated_protocol_id();
}

if (auto tsrs = dynamic_cast<TLSSessionResumptionSupport *>(netvc)) {
client_ssl_reused = tsrs->getSSLSessionCacheHit();
}

const char *protocol_str = client_vc->get_protocol_string();
client_protocol = protocol_str ? protocol_str : "-";

Expand Down Expand Up @@ -603,6 +608,7 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
t_state.hdr_info.client_request.create(HTTP_TYPE_REQUEST);

// Prepare raw reader which will live until we are sure this is HTTP indeed
SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
if (is_transparent_passthrough_allowed() || (ssl_vc && ssl_vc->decrypt_tunnel())) {
ua_raw_buffer_reader = ua_txn->get_remote_reader()->clone();
}
Expand Down