Skip to content

Commit 8a87347

Browse files
Copilotbbockelm
andcommitted
Implement per-issuer lock to prevent thundering herd on new issuers
Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>
1 parent 7eac47e commit 8a87347

File tree

2 files changed

+94
-5
lines changed

2 files changed

+94
-5
lines changed

src/scitokens_internal.cpp

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include <memory>
44
#include <sstream>
55
#include <sys/stat.h>
6+
#include <unordered_map>
67

78
#include <jwt-cpp/base.h>
89
#include <jwt-cpp/jwt.h>
@@ -33,6 +34,44 @@ CurlRaii myCurl;
3334

3435
std::mutex key_refresh_mutex;
3536

37+
// Per-issuer mutex map for preventing thundering herd on new issuers
38+
std::mutex issuer_mutex_map_lock;
39+
std::unordered_map<std::string, std::shared_ptr<std::mutex>> issuer_mutexes;
40+
constexpr size_t MAX_ISSUER_MUTEXES = 1000;
41+
42+
// Get or create a mutex for a specific issuer
43+
std::shared_ptr<std::mutex> get_issuer_mutex(const std::string &issuer) {
44+
std::lock_guard<std::mutex> guard(issuer_mutex_map_lock);
45+
46+
auto it = issuer_mutexes.find(issuer);
47+
if (it != issuer_mutexes.end()) {
48+
return it->second;
49+
}
50+
51+
// Prevent resource exhaustion: limit the number of cached mutexes
52+
if (issuer_mutexes.size() >= MAX_ISSUER_MUTEXES) {
53+
// Remove mutexes that are no longer in use
54+
for (auto iter = issuer_mutexes.begin(); iter != issuer_mutexes.end(); ) {
55+
if (iter->second.use_count() == 1) {
56+
// Only we hold a reference, safe to remove
57+
iter = issuer_mutexes.erase(iter);
58+
} else {
59+
++iter;
60+
}
61+
}
62+
63+
// If still at capacity, clear the oldest entries
64+
// Note: In a production system, we might use an LRU cache
65+
if (issuer_mutexes.size() >= MAX_ISSUER_MUTEXES) {
66+
issuer_mutexes.clear();
67+
}
68+
}
69+
70+
auto mutex_ptr = std::make_shared<std::mutex>();
71+
issuer_mutexes[issuer] = mutex_ptr;
72+
return mutex_ptr;
73+
}
74+
3675
} // namespace
3776

3877
namespace scitokens {
@@ -854,16 +893,29 @@ Validator::get_public_key_pem(const std::string &issuer, const std::string &kid,
854893
result->m_done = true;
855894
}
856895
} else {
857-
// No keys in the DB, or they are expired
896+
// No keys in the DB, or they are expired, so get them from the web.
858897
// Record that we had expired keys if the issuer was previously known
859-
// (This is tracked by having an entry in issuer stats)
860898
auto &issuer_stats =
861899
internal::MonitoringStats::instance().get_issuer_stats(issuer);
862900
issuer_stats.inc_expired_key();
863901

864-
// Get keys from the web.
865-
result = get_public_keys_from_web(
866-
issuer, internal::SimpleCurlGet::default_timeout);
902+
// Use per-issuer lock to prevent thundering herd for new issuers
903+
auto issuer_mutex = get_issuer_mutex(issuer);
904+
std::lock_guard<std::mutex> lock(*issuer_mutex);
905+
906+
// Check again if keys are now in DB (another thread may have fetched
907+
// them)
908+
if (get_public_keys_from_db(issuer, now, result->m_keys,
909+
result->m_next_update)) {
910+
// Keys are now available, use them
911+
result->m_continue_fetch = false;
912+
result->m_do_store = false;
913+
result->m_done = true;
914+
} else {
915+
// Still no keys, fetch them from the web
916+
result = get_public_keys_from_web(
917+
issuer, internal::SimpleCurlGet::default_timeout);
918+
}
867919
}
868920
result->m_issuer = issuer;
869921
result->m_kid = kid;

test/main.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,43 @@ TEST_F(EnvConfigTest, StringConfigFromEnv) {
10281028
free(err_msg);
10291029
}
10301030

1031+
// Test for thundering herd prevention with per-issuer locks
1032+
TEST_F(IssuerSecurityTest, ThunderingHerdPrevention) {
1033+
char *err_msg = nullptr;
1034+
1035+
// Create tokens for a new issuer and pre-populate the cache
1036+
std::string test_issuer = "https://thundering-herd-test.example.org/gtest";
1037+
1038+
auto rv = scitoken_set_claim_string(m_token.get(), "iss",
1039+
test_issuer.c_str(), &err_msg);
1040+
ASSERT_TRUE(rv == 0) << err_msg;
1041+
1042+
// Store public key for this issuer in the cache
1043+
rv = scitoken_store_public_ec_key(test_issuer.c_str(), "1", ec_public,
1044+
&err_msg);
1045+
ASSERT_TRUE(rv == 0) << err_msg;
1046+
1047+
char *token_value = nullptr;
1048+
rv = scitoken_serialize(m_token.get(), &token_value, &err_msg);
1049+
ASSERT_TRUE(rv == 0) << err_msg;
1050+
std::unique_ptr<char, decltype(&free)> token_value_ptr(token_value, free);
1051+
1052+
// Successfully deserialize - the per-issuer lock should prevent thundering
1053+
// herd Since we pre-populated the cache, this should succeed without
1054+
// network access
1055+
rv = scitoken_deserialize_v2(token_value, m_read_token.get(), nullptr,
1056+
&err_msg);
1057+
ASSERT_TRUE(rv == 0) << err_msg;
1058+
1059+
// Verify the issuer claim
1060+
char *value;
1061+
rv = scitoken_get_claim_string(m_read_token.get(), "iss", &value, &err_msg);
1062+
ASSERT_TRUE(rv == 0) << err_msg;
1063+
ASSERT_TRUE(value != nullptr);
1064+
std::unique_ptr<char, decltype(&free)> value_ptr(value, free);
1065+
EXPECT_STREQ(value, test_issuer.c_str());
1066+
}
1067+
10311068
int main(int argc, char **argv) {
10321069
::testing::InitGoogleTest(&argc, argv);
10331070
return RUN_ALL_TESTS();

0 commit comments

Comments
 (0)