Skip to content

Add negative cache for failed issuer lookups #178

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

Jump to bottom
Merged
bbockelm merged 3 commits into from
Dec 11, 2025
Merged

Add negative cache for failed issuer lookups #178

bbockelm merged 3 commits into from
Dec 11, 2025
+351 −48

Conversation

Copy link
Contributor

Copilot AI commented Dec 8, 2025 •
edited
Loading

Negative Cache for Missing Issuer Implementation

Successfully implemented negative caching for issuers that fail to be fetched from the web.

Implementation Summary

When an issuer lookup fails (404, 503, DNS error, timeout, or any web fetch failure), a negative cache entry is stored in the SQLite database with:

  • An empty keys array in the JWKS
  • A 5-minute expiration time
  • Standard cache metadata structure

Subsequent requests for the same issuer within 5 minutes will:

  • Detect the negative cache entry in the database
  • Throw InvalidIssuerException indicating it's in negative cache
  • Avoid unnecessary network requests
  • Return empty keys for the get_jwks API

Key Safety Features

Race Condition Protection: The store_negative_cache function now checks if a valid (non-empty) cache entry exists before storing a negative cache entry. This prevents a scenario where:

  1. Thread A starts fetching keys for an issuer
  2. Thread B also starts fetching keys for the same issuer (cache miss)
  3. Thread A succeeds and stores valid keys
  4. Thread B fails and would have overwritten the valid entry with negative cache

The check is done within a transaction to ensure atomicity.

Files Changed

  • src/scitokens_internal.h - Added store_negative_cache declaration
  • src/scitokens_internal.cpp - Enhanced exception handling, updated get_jwks to handle negative cache
  • src/scitokens_cache.cpp - Implemented store_negative_cache with race condition protection and negative cache detection
  • test/main.cpp - Added NegativeCacheTest unit test

Testing Results

  • 28 out of 30 tests pass (2 pre-existing network-related failures)
  • New NegativeCacheTest validates negative cache functionality
  • CodeQL security scan: No vulnerabilities found
  • Linting: All files now pass clang-format checks
Original prompt

This section details on the original issue you should resolve

<issue_title>Negative cache for missing issuer</issue_title>
<issue_description>If a query to the web fails because the issuer doesn't exist (404, 503, etc - any failure to lookup the cache entry that's expired or not pre-existing), then insert a negative cache entry into the sqlite database.</issue_description>

Comments on the Issue (you are @copilot in this section)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Add negative cache for missing issuer in SQLite Add negative cache for failed issuer lookups Dec 8, 2025
Copilot AI requested a review from bbockelm December 8, 2025 22:57
bbockelm
bbockelm requested changes Dec 9, 2025
View reviewed changes
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

@copilot - please make requested changes.

Copy link
Contributor Author

Copilot AI commented Dec 9, 2025

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • demo.scitokens.org
    • Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)
  • invalid-issuer-test.example.com
    • Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest --gtest_filter=KeycacheTest.NegativeCacheTest (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from bbockelm December 9, 2025 02:03
@bbockelm
Copy link
Contributor

bbockelm commented Dec 9, 2025

@copilot - please fix lint issues.

Copy link
Contributor Author

Copilot AI commented Dec 9, 2025 •
edited
Loading

@copilot - please fix lint issues.

Fixed in commit 92830ef - applied clang-format to all modified source files to comply with LLVM style guidelines

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • demo.scitokens.org
    • Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https /usr/lib/apt/methods/https (dns block)
  • invalid-issuer-test.example.com
    • Triggering command: ./test/scitokens-gtest ./test/scitokens-gtest --gtest_filter=KeycacheTest.NegativeCacheTest --build-id --eh-frame-hdr -m elf_x86_64 --hash-style=gnu --as-needed -dynamic-linker /lib64/ld-linux-x86-64.so.2 -pie -z (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

bbockelm
bbockelm approved these changes Dec 9, 2025
View reviewed changes
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

LGTM

@bbockelm bbockelm marked this pull request as ready for review December 9, 2025 11:42
@bbockelm bbockelm requested a review from djw8605 December 9, 2025 16:01
@bbockelm
Add NegativeCacheHitException for negative cache detection
b004979 
- 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.
@bbockelm
Use secure temp directories in tests
0c6cad8 
- Add SecureTempDir helper class using mkdtemp for security
- Create temp directories under build/tests instead of /tmp
- Auto-cleanup temp directories when tests complete
- Update SetGetConfiguredCacheHome test
- Update StringConfigFromEnv test
- Update MonitoringFileOutput integration test
- Update ConcurrentNewIssuerLookup integration test
- Update StressTestValidToken integration test
- Update StressTestInvalidIssuer integration test

This addresses security concerns with predictable /tmp paths and
ensures proper cleanup of test artifacts.
@bbockelm bbockelm force-pushed the copilot/add-negative-cache-logic branch from 92830ef to 0c6cad8 Compare December 11, 2025 14:21
@bbockelm
Refactor SecureTempDir to shared test utility
bd8694c 
- Extract SecureTempDir class to test/test_utils.h
- Remove duplicated code from main.cpp and integration_test.cpp
- Use fork/execv for safe directory removal (prevents shell injection)
- Add documentation comments to the utility class
@bbockelm bbockelm merged commit d14923a into master Dec 11, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djw8605 djw8605 Awaiting requested review from djw8605

1 more reviewer

@bbockelm bbockelm bbockelm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bbockelm bbockelm

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Negative cache for missing issuer

2 participants

@bbockelm