PR #1078 and #1430 audit: confirmed token-auth defects (cache, JWKS concurrency, OpenID discovery)

[vzakaznikov]

Summary

Deep static audit of PR #1078 (token authentication and authorization) identified multiple confirmed defects, including high-severity issues in cache handling and JWKS concurrency.

A detailed report has been prepared locally in:

• PR-1078-audit-report.md

Scope reviewed

• HTTP/TCP token auth entrypoints
• Token validation/cache (ExternalAuthenticators)
• JWT/JWKS path (TokenProcessorsJWT, JWKSProvider)
• OpenID/Opaque provider path (TokenProcessorsOpaque)
• Token user directory mapping (TokenAccessStorage)
• Config/parser/load integration

Confirmed defects

High

1. Use-after-erase in token cache expiry cleanup

   • src/Access/ExternalAuthenticators.cpp
   • Potential UB/crash in expired-entry cleanup path.

2. Data race in JWKS refresh/cache writes

   • src/Access/Common/JWKSProvider.cpp
   • Shared-lock path writes mutable state (last_request_send, cached_jwks).

3. OpenID discovery constructor does not assign discovered fallback endpoints

   • src/Access/TokenProcessorsOpaque.cpp
   • Discovery-configured OpenID processor can fail fallback path.

Medium

4. token_cache_lifetime units mismatch (seconds vs minutes)

   • src/Access/ExternalAuthenticators.cpp

5. cache_entry.expires_at can remain unset in one branch

   • src/Access/ExternalAuthenticators.cpp

6. jwt_static_key parser requires static_key unconditionally

   • src/Access/TokenProcessorsParse.cpp

7. Invalid roles_filter regex falls back to unfiltered role mapping

   • src/Access/TokenAccessStorage.cpp

8. Inconsistent malformed-token handling (false vs throw) across JWT processors

   • src/Access/TokenProcessorsJWT.cpp

Low

9. Diagnostics consistency issues in token/JWKS paths

Recommended next actions

• Fix high-severity issues first (cache UB, JWKS race, OpenID discovery endpoint assignment).
• Add focused regression tests for:
  • expired cache cleanup branch,
  • concurrent JWKS refresh,
  • OpenID discovery fallback behavior,
  • jwt_static_key parser behavior for asymmetric algorithms.

Notes

This issue intentionally tracks only confirmed defects from static reasoning. Runtime stress/TSAN validation is still recommended for concurrency manifestation frequency.

[vzakaznikov]

PR #1078 Audit Report (From Scratch)

Scope and partitions

Reviewed PR #1078 token-authentication feature across these partitions:

1. HTTP/TCP auth entrypoints and scheme routing
2. Token validation and cache handling
3. JWT/JWKS verification path
4. Opaque/OpenID provider path
5. Token access storage role/profile mapping
6. Config/parse/load integration
7. Tests/docs sanity

Call graph (high-level)

• authenticateUserByHTTP() -> ExternalAuthenticators::checkTokenCredentials() -> processor validate -> cache -> session.authenticate(...)
• TCPHandler JWT branch -> ExternalAuthenticators::checkTokenCredentials() -> processor validate -> cache -> session.authenticate(...)
• TokenAccessStorage::authenticateImpl() -> checkTokenCredentials(token, provider_name) -> role/profile mapping -> auth result
• JwksJwtProcessor::resolveAndValidate() -> JWKSClient::getJWKS()

Confirmed defects

High

1. Use-after-erase in token cache expiry cleanup
   • src/Access/ExternalAuthenticators.cpp
   • Risk: undefined behavior/crash during expired token cleanup.

if (cached_entry_iter->second.expires_at <= std::chrono::system_clock::now())
{
    access_token_to_username_cache.erase(cached_entry_iter);
    username_to_access_token_cache.erase(cached_entry_iter->second.user_name);
}

2. Data race in JWKS refresh/cache writes
   • src/Access/Common/JWKSProvider.cpp
   • Risk: concurrent auth instability due to writes under shared lock.

std::shared_lock lock(mutex);
...
last_request_send = std::chrono::high_resolution_clock::now();
cached_jwks = std::move(parsed_jwks);

3. OpenID discovery constructor does not assign discovered fallback endpoints
   • src/Access/TokenProcessorsOpaque.cpp
   • Risk: discovery-configured OpenID processors can fail fallback path.

const picojson::object openid_config = getObjectFromURI(Poco::URI(openid_config_endpoint_));
if (!openid_config.contains("userinfo_endpoint") || !openid_config.contains("introspection_endpoint"))
    throw Exception(...);
// discovered endpoints are validated but not assigned to member URIs in this ctor

Medium

4. token_cache_lifetime unit mismatch (seconds vs minutes)
   • src/Access/ExternalAuthenticators.cpp

auto default_expiration_ts = std::chrono::system_clock::now()
                             + std::chrono::minutes(processor.getTokenCacheLifetime());

5. cache_entry.expires_at can remain unset in a branch

   • src/Access/ExternalAuthenticators.cpp
   • In expires_at >= default_expiration_ts case, branch logs but does not assign cache_entry.expires_at.

6. jwt_static_key parser requires static_key unconditionally

   • src/Access/TokenProcessorsParse.cpp
   • Rejects asymmetric configurations unexpectedly.

7. Invalid roles_filter regex falls back to unfiltered role mapping

   • src/Access/TokenAccessStorage.cpp
   • Fail-open authorization behavior on bad regex.

8. Inconsistent malformed-token handling across JWT processors

   • src/Access/TokenProcessorsJWT.cpp
   • Static-key path catches and returns false; JWKS path can throw.

Low

9. Diagnostics consistency issues in token/JWKS paths
   • Mostly wording/contract consistency, not primary correctness failures.

Fault-category completion matrix

• Auth entrypoint mixups: Executed
• Token credential shape faults: Executed
• JWT validation faults (static key): Executed
• JWKS faults: Executed
• Opaque/OpenID/Azure provider faults: Executed
• Cache behavior faults: Executed
• TokenAccessStorage faults: Executed
• Role/profile mapping faults: Executed
• Config/load-time faults: Executed
• Transport/protocol parity faults: Executed
• Observability/diagnostics faults: Executed
• Performance/resource failure classes (static): Executed

No categories were skipped; runtime-only stress manifestation frequency remains a dynamic-test follow-up.

Assumptions and limits

• Static reasoning only (no TSAN/stress runs in this pass).
• Findings above are code-path-confirmed defects.

Confidence

• Overall confidence: High (static defect identification).
• Confidence increases with:
  • TSAN/stress for cache/JWKS concurrency,
  • integration test for OpenID discovery fallback path,
  • parser tests for jwt_static_key asymmetric configs.

[Selfeer]

PR #1430 Audit Review (Second Pass)

1) Scope and partitions

PR size is large (44 files), so review is partitioned:

• P1: Token validation core (TokenProcessors*, JWKSProvider*, parser/build glue)
• P2: Auth orchestration/cache (ExternalAuthenticators*, Authentication*, Credentials*)
• P3: Token user directory (TokenAccessStorage*, AccessControl*, config wiring)
• P4: Entrypoints/docs/tests (HTTP/TCP handlers, docs, integration tests)

2) Call graph

• HTTP Bearer and TCP JWT entrypoints construct TokenCredentials.
• session->authenticate(...) reaches Authentication::areCredentialsValid.
• Token credentials route into ExternalAuthenticators::checkTokenCredentials.
• Cache path:
  • cache hit -> hydrate username/groups from cache
  • cache miss -> checkCredentialsAgainstProcessor -> ITokenProcessor::resolveAndValidate
• Processor families:
  • local JWT (StaticKeyJwtProcessor, JwksJwtProcessor)
  • remote providers (GoogleTokenProcessor, AzureTokenProcessor, OpenIdTokenProcessor)
• JWKS-backed JWT path calls JWKSClient::getJWKS for key retrieval/cache refresh.

3) Transition matrix

• T1: incoming token -> parse -> TokenCredentials
• T2: cache lookup -> cache reuse or eviction
• T3: processor validation -> cache insert
• T4: configured JWT claim policy -> enforcement decision
• T5: OpenID discovery -> fallback /userinfo retrieval
• T6: JWKS refresh -> shared state update

4) Logical code-path testing summary (static)

• Covered success/failure branches for cache hit/miss/expired token cleanup.
• Covered JWT/static-key, dynamic/static JWKS, and OpenID discovery branches.
• Covered config parser constraints vs documented config contract.
• Covered concurrency-sensitive shared-state paths (JWKSClient, token cache).
• Runtime execution was not performed in this pass.

5) Fault categories and injection results

Category	Status	Outcome	Defects
Memory lifetime / iterator invalidation	Executed	Fail	1
Concurrency / shared-state synchronization	Executed	Fail	1
Security policy enforcement	Executed	Fail	1
Config-to-runtime contract consistency	Executed	Fail	2
Cache lifetime/TTL correctness	Executed	Fail	2
Integer/signedness	Not Applicable	N/A	0
Ownership/leak/RAII	Not Applicable	N/A	0

6) Confirmed defects (High/Medium/Low)

High

H1: Use-after-erase in expired token cache cleanup

• Impact: Undefined behavior/crash in auth path under realistic expired-token traffic.
• File/function: src/Access/ExternalAuthenticators.cpp, ExternalAuthenticators::checkTokenCredentials
• Fault-injection trigger: cache contains token whose expires_at <= now.
• Transition mapping: T2 (cache eviction path).
• Why defect: iterator is erased, then dereferenced (cached_entry_iter->second.user_name).
• Smallest repro: create expired cache entry, call token auth once.
• Likely fix direction: store username in local variable before erasing iterator.
• Regression test direction: unit test expired-entry cleanup with ASan/UBSan.
• Subsystem/blast radius: token auth cache; all Bearer/JWT requests.

// src/Access/ExternalAuthenticators.cpp
if (cached_entry_iter->second.expires_at <= std::chrono::system_clock::now())
{
    access_token_to_username_cache.erase(cached_entry_iter);
    username_to_access_token_cache.erase(cached_entry_iter->second.user_name); // invalid iterator access
}

H2: Data race in JWKS refresh path (writes under shared lock)

• Impact: Undefined behavior or corrupted JWKS cache under concurrent JWT validation.
• File/function: src/Access/Common/JWKSProvider.cpp, JWKSClient::getJWKS
• Fault-injection trigger: concurrent calls to getJWKS while refresh needed.
• Transition mapping: T6 (JWKS refresh/update).
• Why defect: function acquires std::shared_lock but mutates last_request_send and cached_jwks.
• Smallest repro: multi-threaded concurrent JWT validation against shared dynamic JWKS processor.
• Likely fix direction: promote to exclusive lock for refresh/write section (with double-check).
• Regression test direction: TSAN stress test for parallel getJWKS().
• Subsystem/blast radius: all dynamic-JWKS authentication.

// src/Access/Common/JWKSProvider.cpp
std::shared_lock lock(mutex);
...
last_request_send = std::chrono::high_resolution_clock::now();
cached_jwks = std::move(parsed_jwks);

H3: Per-user JWT claims policy bypass

• Impact: security policy bypass; users configured with JWT claims restrictions are authenticated without claim enforcement.
• File/function: src/Access/UsersConfigParser.cpp, src/Access/Authentication.cpp
• Fault-injection trigger: configure user with JWT claims restriction and present valid token lacking required claims.
• Transition mapping: T4 (policy enforcement path).
• Why defect: user config parser only adds AuthenticationType::JWT; auth runtime checks token validity but does not validate configured user claims.
• Smallest repro: user with JWT claim constraints in config + token missing constraints still authenticates.
• Likely fix direction: parse/store claims in user auth metadata and enforce on token auth path.
• Regression test direction: integration test for claims mismatch rejection.
• Subsystem/blast radius: JWT-authenticated users relying on claim-based restrictions.

// src/Access/UsersConfigParser.cpp
else if (has_jwt)
{
    user->authentication_methods.emplace_back(AuthenticationType::JWT);
}

// src/Access/Authentication.cpp
if (const auto * token_credentials = typeid_cast<const TokenCredentials *>(&credentials))
{
    if (authentication_method.getType() != AuthenticationType::JWT)
        return false;
    return external_authenticators.checkTokenCredentials(*token_credentials); // no per-user claims check
}

Medium

M1: token_cache_lifetime interpreted as minutes instead of seconds

• Impact: token cache entries live 60x longer than configured/documented; stale/revoked token acceptance window and unexpected memory retention.
• File/function: src/Access/ExternalAuthenticators.cpp, checkCredentialsAgainstProcessor
• Fault-injection trigger: set token_cache_lifetime=N expecting seconds.
• Transition mapping: T3 (cache insert TTL calculation).
• Why defect: config/docs treat value as seconds, code uses std::chrono::minutes.
• Smallest repro: token_cache_lifetime=60 yields 60 minutes, not 60 seconds.
• Likely fix direction: replace std::chrono::minutes(...) with std::chrono::seconds(...).
• Regression test direction: unit test asserting TTL unit for cache entry.
• Subsystem/blast radius: all token processors using cache.

// src/Access/ExternalAuthenticators.cpp
auto default_expiration_ts = std::chrono::system_clock::now()
                             + std::chrono::minutes(processor.getTokenCacheLifetime());

<!-- docs/en/operations/external-authenticators/tokens.md -->
- `token_cache_lifetime` -- maximum lifetime of cached token (in seconds). Optional, default: 3600.

M2: Cache entry expiration not assigned when token expiry is later than default

• Impact: undefined/default-initialized expiry behavior for entries, likely immediate eviction and repeated remote validation load.
• File/function: src/Access/ExternalAuthenticators.cpp, checkCredentialsAgainstProcessor
• Fault-injection trigger: token has exp later than default cache lifetime.
• Transition mapping: T3 (cache insert).
• Why defect: branch logs but does not set cache_entry.expires_at.
• Smallest repro: validate long-lived token and inspect immediate cache behavior.
• Likely fix direction: set expires_at = default_expiration_ts in that branch.
• Regression test direction: tests for both token_exp < default and token_exp >= default.
• Subsystem/blast radius: cache effectiveness and auth latency.

// src/Access/ExternalAuthenticators.cpp
if (credentials.getExpiresAt().value() < default_expiration_ts)
    cache_entry.expires_at = credentials.getExpiresAt().value();
else
    LOG_TRACE(...); // no assignment to cache_entry.expires_at

M3: OpenID discovery constructor validates endpoints but does not persist them

• Impact: fallback userinfo validation path fails for discovery-configured OpenID processor when local JWT path is unavailable/fails.
• File/function: src/Access/TokenProcessorsOpaque.cpp, OpenIdTokenProcessor discovery ctor and resolveAndValidate
• Fault-injection trigger: discovery-based OpenID setup + fallback required.
• Transition mapping: T5 (discovery -> fallback).
• Why defect: constructor checks keys but does not assign userinfo_endpoint / token_introspection_endpoint members.
• Smallest repro: configure with configuration_endpoint, force local JWT validation to fail, observe fallback failure.
• Likely fix direction: assign endpoint members from discovery JSON.
• Regression test direction: integration test for discovery-mode fallback path.
• Subsystem/blast radius: OpenID token authentication reliability.

// src/Access/TokenProcessorsOpaque.cpp (discovery ctor)
if (!openid_config.contains("userinfo_endpoint") || !openid_config.contains("introspection_endpoint"))
    throw Exception(...);
// missing assignment to member URIs

// src/Access/TokenProcessorsOpaque.cpp (fallback path)
user_info_json = getObjectFromURI(userinfo_endpoint, token);

Low

• No additional low-severity confirmed defect in this second pass.

7) Coverage accounting + stop-condition status

• Call-graph nodes covered: 100% for token-auth nodes touched by this PR.
• Transitions reviewed: T1..T6 all covered.
• Fault categories: all executed or explicitly marked Not Applicable.
• Coverage stop condition: met for static audit scope.

8) Assumptions & limits

• Static review only; no live token/JWKS/IdP execution.
• No sanitizer runs in this pass.
• Behavioral confidence is based on code-path reasoning from PR diffs.

9) Confidence rating and confidence-raising evidence

• Overall confidence: High for identified defects.
• Would raise confidence further: targeted TSAN/ASAN tests and integration scenarios for OpenID discovery fallback and JWT claims enforcement.

10) Residual risks and untested paths

• Remote provider latency/failure under load while global authenticator mutex is held.
• Cross-thread behavior under very high auth concurrency in mixed processor setups.
• Policy interactions between token user-directory role mapping and dynamic role changes under concurrent updates.