Skip to content

Add mutex protection and std::expected get() to all key stores (issue #36) - #183

Merged
dnovick merged 4 commits into
mainfrom
claude-issue-36-key-store-thread-safety
May 21, 2026
Merged

Add mutex protection and std::expected get() to all key stores (issue #36)#183
dnovick merged 4 commits into
mainfrom
claude-issue-36-key-store-thread-safety

Conversation

@dnovick

@dnovick dnovick commented May 20, 2026

Copy link
Copy Markdown
Owner

What

Adds std::mutex protection and copy-under-lock get() functions returning std::expected<TView, CryptoError> to all seven key stores that were previously unprotected.

Why

Issue #36: concurrent import/destroy/get on the same key store had no synchronisation, creating data races. The PQC key stores already had a mutex but returned std::optional; this PR upgrades them to std::expected for consistency with the rest of the architecture.

Changes

  • arm_asm/key_store.hpp, ia_asm/key_store.hpp: Added sym_store_mutex(), KeyView struct, key_store_get()std::expected<KeyView, CryptoError> copying bytes under lock; import and destroy now locked
  • arm_asm/ec_key_store.hpp: Added ec_store_mutex(), EcKeyView struct, ec_key_store_get()std::expected<EcKeyView, CryptoError>
  • arm_asm/rsa.hpp: Added rsa_store_mutex(), RsaKeyView struct, rsa_key_store_get()std::expected<RsaKeyView, CryptoError>
  • openssl/openssl_key_store.hpp: Added ossl_asym_store_mutex() and ossl_raw_store_mutex(); asym get() calls EVP_PKEY_up_ref() under lock (caller must EVP_PKEY_free()); raw get() copies bytes into OpenSslRawView; ossl_asym_store_alg() also locked
  • arm_asm/pqc_key_store.hpp, psa_mbedtls/pqc_key_store.hpp: pqc_key_store_get_private/public upgraded from std::optional<PqcKeyView> to std::expected<PqcKeyView, CryptoError>
  • arm_asm/arm_asm_backend.hpp, ia_asm/ia_asm_backend.hpp, arm_asm/hkdf.hpp, ia_asm/hkdf.hpp, openssl/openssl_backend.hpp: All call sites updated to use the new std::expected return types; OpenSSL asym call sites now call EVP_PKEY_free() at all exit paths
  • safe-crypto-lib/crypto_error.hpp: Added CryptoErrorCode::InternalError for allocation/up_ref failures

Closes #36

…36)

Seven previously unprotected key stores now serialise import, destroy, and
get via a per-store std::mutex.  All _get functions copy key material under
the lock into an owning View struct and return std::expected<TView,
CryptoError>, matching the rest of the architecture.  The OpenSSL asym store
uses EVP_PKEY_up_ref() instead of copying; callers must call EVP_PKEY_free().
CryptoErrorCode::InternalError added for allocation/up_ref failures.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dnovick

dnovick commented May 20, 2026

Copy link
Copy Markdown
Owner Author

Blocking: this PR currently does not compile.

  1. safe-crypto-lib-test/arm_asm_tests.hpp still calls the old pointer-out getter APIs after the PR changes the stores to expected-returning getters. The failing call sites include key_store_get(id, &key, &len) at lines 1264/1270/1277, ec_key_store_get(id, &curve, &kind, &key, &len) at lines 1330/1341/1349, and rsa_key_store_get(..., &kind, &bits, &key, &len) at lines 2308/2320/2329. These need to be updated to assert on the new std::expected return value instead of the old bool API.

  2. The IA_ASM backend now calls rsa_key_store_get(id) as if the IA RSA store has the new view API, but providers/ia_asm/rsa.hpp still defines only the old pointer-out signature at line 78. CI fails in the IA_ASM compile job at the RSA export/sign/verify/encrypt/decrypt call sites in providers/ia_asm/ia_asm_backend.hpp (for example lines 310, 345, 551, 647, 783, and 805) with too-few-arguments errors. Either update the IA RSA key store to the new mutex-protected RsaKeyView/std::expected API or keep those backend call sites on the old signature.

Because every relevant CI job is failing at compile time, I did not run further local tests or merge.

…updates

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dnovick

dnovick commented May 21, 2026

Copy link
Copy Markdown
Owner Author

Blocking review on the updated #183: the previous API-mismatch blockers are fixed, but this still is not ready to merge.

  1. CryptoError::message() now returns std::string_view, which breaks existing CLI string concatenation. A local scli build fails with invalid operands to binary expression (const char[...] and std::string_view) at the PQC CLI error paths, e.g. safe-crypto-cli/cmd_ml_kem.hpp:50, :90, :139, safe-crypto-cli/cmd_ml_dsa.hpp:49, :97, and safe-crypto-cli/cmd_slh_dsa.hpp:49, :100. Either keep message() returning const std::string&, or update these callers to explicitly build a std::string before concatenating.

  2. The OpenSSL asymmetric store still has a consistency race between the key reference and the stored algorithm. ossl_asym_store_get() and ossl_asym_store_alg() each lock independently (providers/openssl/openssl_key_store.hpp:80-111), but sign/verify read them in separate calls (providers/openssl/openssl_backend.hpp:1191-1196 and :1251-1256). If another thread destroys/reimports the same slot between those calls, the operation can use an EVP_PKEY reference from the old slot together with the algorithm from the new slot. For issue Thread safety: symmetric, EC, RSA, and OpenSSL key stores have no synchronization #36, the OpenSSL asym getter should return a single view/reference object containing both the up-ref’d EVP_PKEY* and the alg captured under the same lock.

Verification: cmake -S /private/tmp/claude-crypto-pr183-worktree -B /private/tmp/claude-crypto-pr183-build -DSAFE_CRYPTO_ACTIVE_PROVIDER=PSA_MBEDTLS -DSAFE_CRYPTO_PQC=LIBOQS -DCMAKE_BUILD_TYPE=Debug configured successfully, but cmake --build /private/tmp/claude-crypto-pr183-build --target scli --parallel 4 fails on the std::string_view concatenation. GitHub currently shows OPENSSL / Debug failing as well, with some other checks still pending. Not merging.

string_view return broke string concatenation on macOS clang CI
("const char[N] + std::string_view" is not valid standard C++).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dnovick

dnovick commented May 21, 2026

Copy link
Copy Markdown
Owner Author

Not ready to merge yet. CI is now green and the earlier CryptoError::message() compile blocker is fixed, but the OpenSSL asymmetric key store still has the issue #36 consistency race I called out.

ossl_asym_store_get() and ossl_asym_store_alg() each take ossl_asym_store_mutex() separately (providers/openssl/openssl_key_store.hpp:80-110). Callers then read the key reference and algorithm in two separate operations, for example sign at providers/openssl/openssl_backend.hpp:1191-1196 and verify at providers/openssl/openssl_backend.hpp:1251-1256. If another thread destroys and reimports the same slot between those calls, the operation can hold an up-ref to the old EVP_PKEY while comparing against the new slot algorithm.

For the thread-safety issue this PR is closing, the OpenSSL asymmetric getter should capture both values under one lock, e.g. return a view/reference object containing the up-ref’d EVP_PKEY* plus the stored alg, and callers should use that single snapshot.

sign, verify, encapsulate, and decapsulate previously called
ossl_asym_store_get() and ossl_asym_store_alg() under separate locks,
creating a window where another thread could destroy/reimport the slot
between the two calls. The new ossl_asym_store_get_with_alg() captures
both the up_ref'd EVP_PKEY* and the stored alg atomically under one lock.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dnovick

dnovick commented May 21, 2026

Copy link
Copy Markdown
Owner Author

Reviewed latest head 3220379. I do not see any remaining code blockers.

The previous issues look fixed:

  • CryptoError::message() is back to returning std::string, and the scli target builds locally.
  • OpenSSL asymmetric key operations now use ossl_asym_store_get_with_alg(), capturing the key pointer and stored algorithm under one lock before sign/verify/KEM checks.

Local verification passed:

  • cmake --build /private/tmp/claude-crypto-pr183-build --target scli --parallel 4
  • OpenSSL provider configure/build in /private/tmp/claude-crypto-pr183-openssl-build
  • ctest --test-dir /private/tmp/claude-crypto-pr183-openssl-build --output-on-failure -E safe_crypto_cli_test_NOT_BUILT passed 529/529

GitHub CI is almost green: OpenSSL, PSA debug, PSA sanitize, TSan, IA compile, and all clang-tidy jobs have passed. ARM_ASM / Debug is still pending, so I would wait for that final required check before merging.

@dnovick
dnovick merged commit 94c6598 into main May 21, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread safety: symmetric, EC, RSA, and OpenSSL key stores have no synchronization

1 participant