Skip to content

Fix origin session related crashes #7808

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
duke8253 merged 1 commit into from
May 12, 2021
Merged

Fix origin session related crashes #7808

Show file tree
Hide file tree
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
6 changes: 6 additions & 0 deletions doc/admin-guide/monitoring/statistics/core/ssl.en.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,18 @@ SSL/TLS
.. ts:stat:: global proxy.process.ssl.ssl_session_cache_hit integer
:type: counter

.. ts:stat:: global proxy.process.ssl.ssl_origin_session_cache_hit integer
:type: counter

.. ts:stat:: global proxy.process.ssl.ssl_session_cache_lock_contention integer
:type: counter

.. ts:stat:: global proxy.process.ssl.ssl_session_cache_miss integer
:type: counter

.. ts:stat:: global proxy.process.ssl.ssl_origin_session_cache_miss integer
:type: counter

.. ts:stat:: global proxy.process.ssl.ssl_session_cache_new_session integer
:type: counter

Expand Down
2 changes: 1 addition & 1 deletion iocore/net/SSLClientUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ ssl_new_session_callback(SSL *ssl, SSL_SESSION *sess)
if (!sni_addr.empty()) {
std::string lookup_key;
ts::bwprint(lookup_key, "{}:{}:{}", sni_addr.c_str(), SSL_get_SSL_CTX(ssl), get_verify_str(ssl));
origin_sess_cache->insert_session(lookup_key, sess);
origin_sess_cache->insert_session(lookup_key, sess, ssl);
return 1;
} else {
if (is_debug_tag_set("ssl.origin_session_cache")) {
Expand Down
88 changes: 49 additions & 39 deletions iocore/net/SSLSessionCache.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,58 +317,68 @@ SSLSessionBucket::~SSLSessionBucket() {}

SSLOriginSessionCache::SSLOriginSessionCache() {}

SSLOriginSessionCache::~SSLOriginSessionCache()
{
for (auto &x : origin_sessions) {
SSL_SESSION_free(x.second);
}
}
SSLOriginSessionCache::~SSLOriginSessionCache() {}

void
SSLOriginSessionCache::insert_session(const std::string &lookup_key, SSL_SESSION *sess)
SSLOriginSessionCache::insert_session(const std::string &lookup_key, SSL_SESSION *sess, SSL *ssl)
{
if (is_debug_tag_set("ssl.origin_session_cache")) {
Debug("ssl.origin_session_cache", "insert session: %s = %p", lookup_key.c_str(), sess);
}

std::unique_lock lock(mutex);
auto node = origin_sessions.find(lookup_key);
if (node != origin_sessions.end()) {
SSL_SESSION_free(node->second);
} else if (origin_sessions.size() >= SSLConfigParams::origin_session_cache_size) {
remove_oldest_session(lock);
}
origin_sessions[lookup_key] = sess;
}
size_t len = i2d_SSL_SESSION(sess, nullptr);

void
SSLOriginSessionCache::remove_session(const std::string &lookup_key)
{
if (is_debug_tag_set("ssl.origin_session_cache")) {
Debug("ssl.origin_session_cache", "remove session: %s", lookup_key.c_str());
}
Ptr<IOBufferData> buf;
Ptr<IOBufferData> buf_exdata;
size_t len_exdata = sizeof(ssl_session_cache_exdata);
buf = new_IOBufferData(buffer_size_to_index(len, MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
i2d_SSL_SESSION(sess, &loc);
buf_exdata = new_IOBufferData(buffer_size_to_index(len, MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
ink_release_assert(static_cast<size_t>(buf_exdata->block_size()) >= len_exdata);
ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata *>(buf_exdata->data());
// This could be moved to a function in charge of populating exdata
exdata->curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl);

ats_scoped_obj<SSLOriginSession> ssl_orig_session(new SSLOriginSession(lookup_key, buf, len, buf_exdata));
auto new_node = ssl_orig_session.release();

std::unique_lock lock(mutex);
auto node = origin_sessions.find(lookup_key);
if (node != origin_sessions.end()) {
SSL_SESSION_free(node->second);
origin_sessions.erase(node);
auto entry = orig_sess_map.find(lookup_key);
if (entry != orig_sess_map.end()) {
auto node = entry->second;
orig_sess_que.remove(node);
orig_sess_map.erase(entry);
delete node;
} else if (orig_sess_map.size() >= SSLConfigParams::origin_session_cache_size) {
remove_oldest_session(lock);
}

orig_sess_que.enqueue(new_node);
orig_sess_map[lookup_key] = new_node;
}

SSL_SESSION *
SSLOriginSessionCache::get_session(const std::string &lookup_key)
bool
SSLOriginSessionCache::get_session(const std::string &lookup_key, SSL_SESSION **sess, ssl_session_cache_exdata **data)
{
if (is_debug_tag_set("ssl.origin_session_cache")) {
Debug("ssl.origin_session_cache", "get session: %s", lookup_key.c_str());
}

std::shared_lock lock(mutex);
auto node = origin_sessions.find(lookup_key);
if (node == origin_sessions.end()) {
return nullptr;
auto entry = orig_sess_map.find(lookup_key);
if (entry == orig_sess_map.end()) {
return false;
}
return node->second;

const unsigned char *loc = reinterpret_cast<const unsigned char *>(entry->second->asn1_data->data());
*sess = d2i_SSL_SESSION(nullptr, &loc, entry->second->len_asn1_data);
if (data != nullptr) {
ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata *>(entry->second->extra_data->data());
*data = exdata;
}
return true;
}

void
Expand All @@ -377,12 +387,12 @@ SSLOriginSessionCache::remove_oldest_session(const std::unique_lock<std::shared_
// Caller must hold the bucket shared_mutex with unique_lock.
ink_assert(lock.owns_lock());

auto node = origin_sessions.begin();

if (is_debug_tag_set("ssl.origin_session_cache")) {
Debug("ssl.origin_session_cache", "remove oldest session: %s = %p", node->first.c_str(), node->second);
while (orig_sess_que.head && orig_sess_que.size >= static_cast<int>(SSLConfigParams::origin_session_cache_size)) {
auto node = orig_sess_que.pop();
if (is_debug_tag_set("ssl.origin_session_cache")) {
Debug("ssl.origin_session_cache", "remove oldest session: %s", node->key.c_str());
}
orig_sess_map.erase(node->key);
delete node;
}

SSL_SESSION_free(node->second);
origin_sessions.erase(node);
}
29 changes: 23 additions & 6 deletions iocore/net/SSLSessionCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,19 +183,36 @@ class SSLSessionCache
size_t nbuckets;
};

class SSLOriginSession
{
public:
std::string key;
Ptr<IOBufferData> asn1_data; /* this is the ASN1 representation of the SSL_CTX */
size_t len_asn1_data;
Ptr<IOBufferData> extra_data;

SSLOriginSession(const std::string &lookup_key, const Ptr<IOBufferData> &ssl_asn1_data, size_t len_asn1,
Ptr<IOBufferData> &exdata)
: key(lookup_key), asn1_data(ssl_asn1_data), len_asn1_data(len_asn1), extra_data(exdata)
{
}

LINK(SSLOriginSession, link);
};

class SSLOriginSessionCache
{
public:
SSLOriginSessionCache();
~SSLOriginSessionCache();

void insert_session(const std::string &lookup_key, SSL_SESSION *sess);
void remove_session(const std::string &lookup_key);
SSL_SESSION *get_session(const std::string &lookup_key);
void insert_session(const std::string &lookup_key, SSL_SESSION *sess, SSL *ssl);
bool get_session(const std::string &lookup_key, SSL_SESSION **sess, ssl_session_cache_exdata **data);

private:
mutable std::shared_mutex mutex;
std::map<std::string, SSL_SESSION *> origin_sessions;

void remove_oldest_session(const std::unique_lock<std::shared_mutex> &lock);

mutable std::shared_mutex mutex;
CountQueue<SSLOriginSession> orig_sess_que;
std::map<std::string, SSLOriginSession *> orig_sess_map;
};
6 changes: 6 additions & 0 deletions iocore/net/SSLStats.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,18 @@ SSLInitializeStatistics()
RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_session_cache_hit", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_session_cache_hit, RecRawStatSyncCount);

RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_origin_session_cache_hit", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_origin_session_cache_hit, RecRawStatSyncCount);

RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_session_cache_new_session", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_session_cache_new_session, RecRawStatSyncCount);

RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_session_cache_miss", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_session_cache_miss, RecRawStatSyncCount);

RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_origin_session_cache_miss", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_origin_session_cache_miss, RecRawStatSyncCount);

RecRegisterRawStat(ssl_rsb, RECT_PROCESS, "proxy.process.ssl.ssl_session_cache_eviction", RECD_COUNTER, RECP_PERSISTENT,
(int)ssl_session_cache_eviction, RecRawStatSyncCount);

Expand Down
2 changes: 2 additions & 0 deletions iocore/net/SSLStats.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ enum SSL_Stats {
ssl_total_dyn_max_tls_record_count,
ssl_total_dyn_redo_tls_record_count,
ssl_session_cache_hit,
ssl_origin_session_cache_hit,
ssl_session_cache_miss,
ssl_origin_session_cache_miss,
ssl_session_cache_eviction,
ssl_session_cache_lock_contention,
ssl_session_cache_new_session,
Expand Down
7 changes: 6 additions & 1 deletion iocore/net/SSLUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2034,7 +2034,12 @@ SSLConnect(SSL *ssl)

Debug("ssl.origin_session_cache", "origin session cache lookup key = %s", lookup_key.c_str());

sess = origin_sess_cache->get_session(lookup_key);
TLSSessionResumptionSupport *srs = TLSSessionResumptionSupport::getInstance(ssl);
Copy link
Member

@maskit maskit May 12, 2021

Choose a reason for hiding this comment

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

Adding these lines here is actually a bit of sad thing, because SSLConnect was originally independent from NetVC. You could use it for any SSL connections. SSLReadBuffer too, before supporting early data. I think that's the reason those functions are in SSLUtils.

Now those two functions depend on NetVC, and there is no longer a reason to have those in SSLUtils as non-member functions. Although I don't think we should do this on this PR, we should probably make the two functions (and SSLWriteBuffer) member functions of SSLNetVC and move them to P_SSLNetVConnection.h. Then we don't need to call getInstance to call getOriginSession.

Copy link
Contributor Author

@duke8253 duke8253 May 12, 2021

Choose a reason for hiding this comment

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

I agree, was trying to stay away from this, but looks like we'll have to do things like this for now.

ink_assert(srs);
if (srs) {
sess = srs->getOriginSession(ssl, lookup_key);
}

if (sess) {
SSL_set_session(ssl, sess);
}
Expand Down
24 changes: 24 additions & 0 deletions iocore/net/TLSSessionResumptionSupport.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,30 @@ TLSSessionResumptionSupport::getSession(SSL *ssl, const unsigned char *id, int l
return session;
}

SSL_SESSION *
TLSSessionResumptionSupport::getOriginSession(SSL *ssl, const std::string &lookup_key)
Copy link
Member

@maskit maskit May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm OK with adding this for now, but I wonder if we can have only one getSession and lookup different caches inside it depends on the connection direction (SSL_is_server). I t think we never call getOriginSession on client sessions (inbound connections), and vice versa.

{
SSL_SESSION *session = nullptr;
ssl_session_cache_exdata *exdata = nullptr;
if (origin_sess_cache->get_session(lookup_key, &session, &exdata)) {
ink_assert(session);
ink_assert(exdata);

// Double check the timeout
if (is_ssl_session_timed_out(session)) {
SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_miss);
session = nullptr;
} else {
SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_hit);
this->_setSSLSessionCacheHit(true);
this->_setSSLCurveNID(exdata->curve);
}
} else {
SSL_INCREMENT_DYN_STAT(ssl_origin_session_cache_miss);
}
return session;
}

void
TLSSessionResumptionSupport::clear()
{
Expand Down
1 change: 1 addition & 0 deletions iocore/net/TLSSessionResumptionSupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class TLSSessionResumptionSupport
ssl_curve_id getSSLCurveNID() const;

SSL_SESSION *getSession(SSL *ssl, const unsigned char *id, int len, int *copy);
SSL_SESSION *getOriginSession(SSL *ssl, const std::string &lookup_key);

protected:
void clear();
Expand Down