Skip to content

Commit 69e18aa

Browse files
committed
Add NegativeCacheHitException for negative cache detection
- Add NegativeCacheHitException class inheriting from InvalidIssuerException - Add negative_cache_hits counter to IssuerStats for monitoring - Detect negative cache entries in get_public_keys_from_db() and throw NegativeCacheHitException with descriptive message including the issuer - Add negative_cache_hits field to monitoring JSON output - Add NegativeCacheTest unit test that verifies: - Negative cache entries are created for invalid issuers - NegativeCacheHitException is thrown on subsequent access - negative_cache_hits counter increments correctly (only for hits, not misses) - Use unique issuer per test run to avoid interference from cache persistence This allows callers to distinguish negative cache hits from other validation failures and enables monitoring of negative cache effectiveness.
1 parent 47fda99 commit 69e18aa

File tree

5 files changed

+161
-2
lines changed

5 files changed

+161
-2
lines changed

src/scitokens_cache.cpp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,35 @@ bool scitokens::Validator::get_public_keys_from_db(const std::string issuer,
209209
return false;
210210
}
211211
auto keys_local = iter->second;
212+
213+
// Check if this is a negative cache entry (empty keys array)
214+
if (keys_local.is<picojson::object>()) {
215+
auto jwks_obj = keys_local.get<picojson::object>();
216+
auto keys_iter = jwks_obj.find("keys");
217+
if (keys_iter != jwks_obj.end() &&
218+
keys_iter->second.is<picojson::array>()) {
219+
auto keys_array = keys_iter->second.get<picojson::array>();
220+
if (keys_array.empty()) {
221+
// Check if negative cache has expired
222+
iter = top_obj.find("expires");
223+
if (iter != top_obj.end() && iter->second.is<int64_t>()) {
224+
auto expiry = iter->second.get<int64_t>();
225+
if (now > expiry) {
226+
// Negative cache expired, remove and return false
227+
if (remove_issuer_entry(db, issuer, true) != 0) {
228+
return false;
229+
}
230+
sqlite3_close(db);
231+
return false;
232+
}
233+
}
234+
// Negative cache still valid - throw exception
235+
sqlite3_close(db);
236+
throw NegativeCacheHitException(issuer);
237+
}
238+
}
239+
}
240+
212241
iter = top_obj.find("expires");
213242
if (iter == top_obj.end() || !iter->second.is<int64_t>()) {
214243
if (remove_issuer_entry(db, issuer, true) != 0) {

src/scitokens_internal.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -921,8 +921,14 @@ std::string Validator::get_jwks(const std::string &issuer) {
921921
auto now = std::time(NULL);
922922
picojson::value jwks;
923923
int64_t next_update;
924-
if (get_public_keys_from_db(issuer, now, jwks, next_update)) {
925-
return jwks.serialize();
924+
try {
925+
if (get_public_keys_from_db(issuer, now, jwks, next_update)) {
926+
return jwks.serialize();
927+
}
928+
} catch (const NegativeCacheHitException &) {
929+
// Negative cache hit - return empty keys without incrementing counter
930+
// (counter is incremented elsewhere for validation failures)
931+
return std::string("{\"keys\": []}");
926932
}
927933
return std::string("{\"keys\": []}");
928934
}

src/scitokens_internal.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,9 @@ struct IssuerStats {
264264
std::atomic<uint64_t> background_successful_refreshes{0};
265265
std::atomic<uint64_t> background_failed_refreshes{0};
266266

267+
// Negative cache statistics
268+
std::atomic<uint64_t> negative_cache_hits{0};
269+
267270
// Increment methods for atomic counters (use relaxed ordering for stats)
268271
void inc_successful_validation() {
269272
successful_validations.fetch_add(1, std::memory_order_relaxed);
@@ -301,6 +304,9 @@ struct IssuerStats {
301304
void inc_background_failed_refresh() {
302305
background_failed_refreshes.fetch_add(1, std::memory_order_relaxed);
303306
}
307+
void inc_negative_cache_hit() {
308+
negative_cache_hits.fetch_add(1, std::memory_order_relaxed);
309+
}
304310

305311
// Time setters that accept std::chrono::duration (use relaxed ordering)
306312
template <typename Rep, typename Period>
@@ -476,6 +482,14 @@ class InvalidIssuerException : public std::runtime_error {
476482
InvalidIssuerException(const std::string &msg) : std::runtime_error(msg) {}
477483
};
478484

485+
class NegativeCacheHitException : public InvalidIssuerException {
486+
public:
487+
explicit NegativeCacheHitException(const std::string &issuer)
488+
: InvalidIssuerException("Issuer is in negative cache (recently failed "
489+
"to retrieve keys): " +
490+
issuer) {}
491+
};
492+
479493
class JsonException : public std::runtime_error {
480494
public:
481495
JsonException(const std::string &msg) : std::runtime_error(msg) {}
@@ -1350,6 +1364,8 @@ class Validator {
13501364
const std::exception &e) {
13511365
if (dynamic_cast<const TokenExpiredException *>(&e)) {
13521366
stats.inc_expired_token();
1367+
} else if (dynamic_cast<const NegativeCacheHitException *>(&e)) {
1368+
stats.inc_negative_cache_hit();
13531369
}
13541370

13551371
stats.inc_unsuccessful_validation();

src/scitokens_monitoring.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ std::string MonitoringStats::get_json() const {
134134
static_cast<int64_t>(stats.background_failed_refreshes.load(
135135
std::memory_order_relaxed)));
136136

137+
// Negative cache statistics
138+
issuer_obj["negative_cache_hits"] =
139+
picojson::value(static_cast<int64_t>(
140+
stats.negative_cache_hits.load(std::memory_order_relaxed)));
141+
137142
std::string sanitized_issuer = sanitize_issuer_for_json(issuer);
138143
issuers_obj[sanitized_issuer] = picojson::value(issuer_obj);
139144
}

test/main.cpp

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -843,6 +843,109 @@ TEST_F(KeycacheTest, RefreshExpiredTest) {
843843
EXPECT_EQ(jwks_str, "{\"keys\": []}");
844844
}
845845

846+
TEST_F(KeycacheTest, NegativeCacheTest) {
847+
// This test verifies that failed issuer lookups are cached as negative
848+
// entries and that subsequent attempts fail quickly with the right counter
849+
char *err_msg = nullptr;
850+
851+
// Reset monitoring stats for clean baseline
852+
scitoken_reset_monitoring_stats(&err_msg);
853+
if (err_msg) {
854+
free(err_msg);
855+
err_msg = nullptr;
856+
}
857+
858+
// Create a token with an issuer that will fail to lookup
859+
std::unique_ptr<void, decltype(&scitoken_key_destroy)> mykey(
860+
scitoken_key_create("1", "ES256", ec_public, ec_private, &err_msg),
861+
scitoken_key_destroy);
862+
ASSERT_TRUE(mykey.get() != nullptr) << err_msg;
863+
864+
std::unique_ptr<void, decltype(&scitoken_destroy)> mytoken(
865+
scitoken_create(mykey.get()), scitoken_destroy);
866+
ASSERT_TRUE(mytoken.get() != nullptr);
867+
868+
// Use a unique issuer that doesn't exist (will fail to fetch keys)
869+
// Include timestamp to avoid interference from previous test runs
870+
std::string invalid_issuer = "https://invalid-issuer-negative-cache-" +
871+
std::to_string(std::time(nullptr)) +
872+
".example.com";
873+
auto rv = scitoken_set_claim_string(mytoken.get(), "iss",
874+
invalid_issuer.c_str(), &err_msg);
875+
ASSERT_TRUE(rv == 0) << err_msg;
876+
877+
char *token_value = nullptr;
878+
rv = scitoken_serialize(mytoken.get(), &token_value, &err_msg);
879+
ASSERT_TRUE(rv == 0) << err_msg;
880+
std::unique_ptr<char, decltype(&free)> token_value_ptr(token_value, free);
881+
882+
// First attempt should fail to fetch keys (DNS failure or connection
883+
// refused). This is a cache MISS (creates negative cache entry).
884+
std::unique_ptr<void, decltype(&scitoken_destroy)> read_token(
885+
scitoken_create(nullptr), scitoken_destroy);
886+
ASSERT_TRUE(read_token.get() != nullptr);
887+
888+
rv = scitoken_deserialize_v2(token_value, read_token.get(), nullptr,
889+
&err_msg);
890+
ASSERT_FALSE(rv == 0); // Should fail
891+
if (err_msg) {
892+
free(err_msg);
893+
err_msg = nullptr;
894+
}
895+
896+
// Check that a negative cache entry was created (returns empty keys)
897+
char *jwks;
898+
rv = keycache_get_cached_jwks(invalid_issuer.c_str(), &jwks, &err_msg);
899+
ASSERT_TRUE(rv == 0) << err_msg;
900+
ASSERT_TRUE(jwks != nullptr);
901+
std::string jwks_str(jwks);
902+
free(jwks);
903+
904+
// Should return empty keys array (negative cache)
905+
EXPECT_EQ(jwks_str, "{\"keys\": []}");
906+
907+
// Second attempt should fail quickly using negative cache
908+
rv = scitoken_deserialize_v2(token_value, read_token.get(), nullptr,
909+
&err_msg);
910+
ASSERT_FALSE(rv == 0); // Should still fail
911+
ASSERT_TRUE(err_msg != nullptr);
912+
std::string error_msg(err_msg);
913+
free(err_msg);
914+
err_msg = nullptr;
915+
916+
// Error message should indicate it's from negative cache
917+
EXPECT_NE(error_msg.find("negative cache"), std::string::npos)
918+
<< "Error message should mention negative cache: " << error_msg;
919+
920+
// Third attempt to verify counter increments correctly
921+
rv = scitoken_deserialize_v2(token_value, read_token.get(), nullptr,
922+
&err_msg);
923+
ASSERT_FALSE(rv == 0);
924+
if (err_msg) {
925+
free(err_msg);
926+
err_msg = nullptr;
927+
}
928+
929+
// Get monitoring stats and verify negative_cache_hits counter
930+
char *json_out = nullptr;
931+
rv = scitoken_get_monitoring_json(&json_out, &err_msg);
932+
ASSERT_TRUE(rv == 0) << err_msg;
933+
ASSERT_TRUE(json_out != nullptr);
934+
std::string json_str(json_out);
935+
free(json_out);
936+
937+
// Parse JSON and check negative_cache_hits
938+
// Only the second and third attempts should hit the negative cache:
939+
// - First attempt: creates negative cache (cache miss, not hit)
940+
// - Second and third attempts: hit existing negative cache
941+
EXPECT_NE(json_str.find("\"negative_cache_hits\""), std::string::npos)
942+
<< "JSON should contain negative_cache_hits field";
943+
944+
// Verify 2 negative cache hits (attempts 2 and 3 only)
945+
EXPECT_NE(json_str.find("\"negative_cache_hits\":2"), std::string::npos)
946+
<< "Should have 2 negative cache hits. JSON: " << json_str;
947+
}
948+
846949
class IssuerSecurityTest : public ::testing::Test {
847950
protected:
848951
void SetUp() override {

0 commit comments

Comments
 (0)