[MAJOR] §8: std::getenv() called with concurrency-mt-unsafe suppression in NATS code

[mvillmow]

Finding

Severity: MAJOR
Section: 8
Evidence: src/transport/nats_connection.cpp:23
Principle: SOLID

getenv() is called at connection construction time with a NOLINT suppression for concurrency-mt-unsafe. POSIX specifies getenv() is not thread-safe if another thread modifies the environment concurrently. In a multi-threaded NATS application where connections may be created from worker threads, this is a realistic race condition risk during initialization.

---

Part of #504

[mvillmow]

Implementation Plan

Now I have a complete picture of the code. Here is the implementation plan:

---

Implementation Plan: Issue #518 — Thread-safe std::getenv() in NATS Connection

1. Objective

Replace the // NOLINT(concurrency-mt-unsafe) suppression around std::getenv() in src/transport/nats_connection.cpp with a genuinely thread-safe implementation. The three environment variables (KEYSTONE_NATS_TLS_CA_PATH, KEYSTONE_NATS_TLS_CERT_PATH, KEYSTONE_NATS_TLS_KEY_PATH) must be read exactly once, before any concurrent access, and the results cached for all subsequent uses.

---

2. Approach

The getEnvVar() helper at line 22–25 is called from both NatsTlsConfig::validate() (line 83–84) and NatsConnection::applyTlsOptions() (lines 159, 172–173) — potentially from worker threads if connections are established concurrently. The fix is to cache the values using std::call_once + std::once_flag inside a function-scope static initializer (thread-safe under C++11/20 guaranteed static initialization), eliminating the concurrent getenv() race entirely.

Key decisions:

• Use a file-local anonymous-namespace struct with three std::string members initialized via std::call_once. This mirrors the C++20 idiom for lazy, one-time initialization and is safe against concurrent first-call scenarios.
• Do not add a new public API or change the header — this is a purely internal implementation fix.
• The NOLINT(concurrency-mt-unsafe) suppression is removed once the race is gone; the single call inside the std::call_once lambda runs at most once and is protected by the once_flag.
• NatsTlsConfig::validate() currently also calls getEnvVar() independently. It will be updated to call the same cached accessor, removing its own getenv calls.

---

3. Files to Create

None.

---

4. Files to Modify

src/transport/nats_connection.cpp

Changes:

1. Add #include <mutex> — already transitively present via the class member, but make it explicit.
2. Replace the anonymous-namespace getEnvVar() free function with a new cachedTlsEnvVars() function that uses std::call_once:

namespace {

struct TlsEnvVars {
  std::string ca_path;
  std::string cert_path;
  std::string key_path;
};

const TlsEnvVars& cachedTlsEnvVars() {
  static std::once_flag flag;
  static TlsEnvVars vars;
  std::call_once(flag, []() {
    const char* ca   = std::getenv("KEYSTONE_NATS_TLS_CA_PATH");
    const char* cert = std::getenv("KEYSTONE_NATS_TLS_CERT_PATH");
    const char* key  = std::getenv("KEYSTONE_NATS_TLS_KEY_PATH");
    vars.ca_path   = ca   ? ca   : "";
    vars.cert_path = cert ? cert : "";
    vars.key_path  = key  ? key  : "";
  });
  return vars;
}

}  // namespace

3. Update NatsTlsConfig::validate() (lines 83–84) to use cachedTlsEnvVars() instead of getEnvVar(...).
4. Update NatsConnection::applyTlsOptions() (lines 159, 172–173) to use cachedTlsEnvVars() instead of getEnvVar(...).
5. Remove the old getEnvVar() helper entirely.

The three NOLINT(concurrency-mt-unsafe) suppressions disappear with the helper removal.

tests/unit/test_nats_connection.cpp

Changes:

Add a new test fixture NatsTlsEnvVarTest with tests that verify environment variable override behavior. Because cachedTlsEnvVars() uses a static + once_flag, env-var tests must be structured carefully — use setenv/unsetenv only before the cache is populated (i.e., in a fresh process, or with std::call_once broken up by linking a testable seam). The practical approach given the existing test structure:

• Add tests that set env vars before constructing a NatsConnection and calling connect()-adjacent code paths (without a live server). Specifically test:
  • KEYSTONE_NATS_TLS_CA_PATH env var feeds into NatsTlsConfig::validate() correctly (validate does not throw when only ca_path is set via env).
  • KEYSTONE_NATS_TLS_CERT_PATH + KEYSTONE_NATS_TLS_KEY_PATH pair set via env does not cause validate() to throw.
  • Mismatched env var (cert but no key via env, both struct fields empty) causes validate() to throw.

Note on static cache in tests: Since the once_flag fires once per process, env-var tests that need different values for different test cases must avoid fighting over the static. The practical solution is to document that these tests rely on env-var state at test binary startup time, and order them accordingly, or move the env-var override logic into an injectable dependency for unit testing (see optional step below). For now, add tests that verify the behavior assuming clean-slate env (CI does not set these vars).

---

5. Implementation Order

1. Modify src/transport/nats_connection.cpp — replace getEnvVar() with cachedTlsEnvVars(), update all three call sites (validate(), applyTlsOptions() CA path, applyTlsOptions() cert/key paths), remove the suppression.
2. Verify compilation — just build confirms no new warnings or errors.
3. Add env-var tests to tests/unit/test_nats_connection.cpp — new NatsTlsEnvVarTest fixture covering env-var fallback behavior, cert+key parity via env, and missing-key-with-cert-in-env throwing.
4. Run unit tests — ctest --preset asan --output-on-failure to exercise the changed code under ASan.
5. Run TSan — cmake --preset tsan && cmake --build --preset tsan && ctest --preset tsan --output-on-failure to confirm the thread-safety fix eliminates any TSan data-race report on getenv.

---

6. Verification

Check	Command	Expected
Build clean	just build	Zero warnings, zero errors
Formatting	just format-check	No violations
Static analysis	just lint	No concurrency-mt-unsafe suppression remaining
Unit tests (ASan)	ctest --preset asan --output-on-failure	All pass
Thread-safety (TSan)	ctest --preset tsan --output-on-failure	No data-race reports on getenv
No NOLINT remaining	grep -r 'concurrency-mt-unsafe' src/	Empty output

---

7. Skills Used

Directly invoked during planning:

• None (direct file reads and grep — no skill invocations required for this targeted fix)

Team knowledge base skills referenced (from Prior Learnings):

• natsc-fetchcontent-cpp20-integration — confirmed nats.c C++20 patterns; established that connection setup is expected at service initialization time, supporting the design decision to cache env vars once at first use rather than per-call
• quality-security-scan — confirmed that NOLINT suppressions for unsafe patterns are findings to eliminate, not accept; validates that removing concurrency-mt-unsafe suppression is the correct outcome, not merely re-suppressing it

---

Generated by Claude Code Planner