diff --git a/.github/README_WORKFLOWS.md b/.github/README_WORKFLOWS.md index 4230e82c34..f4cd743337 100644 --- a/.github/README_WORKFLOWS.md +++ b/.github/README_WORKFLOWS.md @@ -128,6 +128,13 @@ flowchart TB | proteccio | ✓ | ✗ | FIPS only | | softhsm2 | ✓ | ✓ | | +> **Upstream-only jobs**: test types that need upstream-only secrets or infrastructure run in +> dedicated jobs gated by `if: github.repository == 'Cosmian/kms'`, so they are skipped on +> forks: `test-nix-upstream` (`google-cse`, `secret_vault`, `secret_aws`, `secret_azure`), +> `hsm-upstream` (`proteccio`, `crypt2pay`, `aws-cloudhsm` hardware HSMs) and `xks-remote` +> (AWS XKS — remote server). They are separate jobs +> because a job-level `if` cannot read the `matrix` context. + --- ## 5. Windows Test Workflow (`test_windows.yml`) diff --git a/.github/workflows/test_all.yml b/.github/workflows/test_all.yml index 9f8eb042cc..f88a6c34c3 100644 --- a/.github/workflows/test_all.yml +++ b/.github/workflows/test_all.yml @@ -23,7 +23,6 @@ jobs: - mariadb - psql - otel - - google-cse - redis - pykmip - wasm @@ -38,9 +37,6 @@ jobs: - iris - db2 - ase - - secret_vault - - secret_aws - - secret_azure - secret_cosmian_kms - spire - kmip-go @@ -89,13 +85,8 @@ jobs: - type: ase features: fips # secret_cosmian_kms runs against a local KMS server — works with both fips and non-fips - # secret_vault, secret_aws, secret_azure require external services — run non-fips only - - type: secret_vault - features: fips - - type: secret_aws - features: fips - - type: secret_azure - features: fips + # google-cse, secret_vault, secret_aws and secret_azure need upstream-only secrets: + # they run in the `test-nix-upstream` job, which is skipped on forks. # spire relies on the Vault API which is non-fips only - type: spire features: fips @@ -222,16 +213,75 @@ jobs: run: | mise run test:pkcs11:support --variant non-fips + # Test types that depend on upstream-only secrets / external services. Kept out of + # `test-nix` because a job-level `if` cannot read the `matrix` context: the whole + # job is skipped on forks, where those secrets do not exist. + test-nix-upstream: + name: Test on ${{ matrix.type }} - ${{ matrix.features }} + runs-on: ubuntu-latest + if: github.repository == 'Cosmian/kms' + strategy: + fail-fast: false + matrix: + type: + - google-cse + - secret_vault + - secret_aws + - secret_azure + features: [fips, non-fips] + exclude: + # secret_vault, secret_aws, secret_azure require external services — run non-fips only + - type: secret_vault + features: fips + - type: secret_aws + features: fips + - type: secret_azure + features: fips + + steps: + - uses: actions/checkout@v7 + with: + submodules: recursive + + - uses: ./.github/actions/cleanup-runner + + - uses: ./.github/actions/setup-nix + + - uses: ./.github/actions/install-mise + + - name: Test + env: + # Google variables (google-cse test type) + TEST_GOOGLE_OAUTH_CLIENT_ID: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_ID }} + TEST_GOOGLE_OAUTH_CLIENT_SECRET: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_SECRET }} + TEST_GOOGLE_OAUTH_REFRESH_TOKEN: ${{ secrets.TEST_GOOGLE_OAUTH_REFRESH_TOKEN }} + GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY: ${{ secrets.GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY }} + + # AWS secret backend variables (secret_aws test type) + AWS_ACCESS_KEY_ID: ${{ secrets.KMS_CI_AWS_ACCESS_KEY_ID }} + AWS_SECRET_ACCESS_KEY: ${{ secrets.KMS_CI_AWS_SECRET_ACCESS_KEY }} + AWS_REGION: ${{ secrets.KMS_CI_AWS_REGION }} + + # Azure Key Vault secret backend variables (secret_azure test type) + AZURE_TENANT_ID: ${{ secrets.KMS_CI_AZURE_TENANT_ID }} + AZURE_CLIENT_ID: ${{ secrets.KMS_CI_AZURE_CLIENT_ID }} + AZURE_CLIENT_SECRET: ${{ secrets.KMS_CI_AZURE_CLIENT_SECRET }} + AZURE_KV_NAME: ${{ secrets.KMS_CI_AZURE_KV_NAME }} + + # Provide an authenticated token so mise can install tools from + # GitHub releases without hitting the unauthenticated rate limit. + GITHUB_TOKEN: ${{ github.token }} + run: | + set -ex + mise run test:${{ matrix.type }} --variant ${{ matrix.features }} + hsm: name: HSM ${{ matrix.hsm-type }} - ${{ matrix.features }} runs-on: ubuntu-latest - # proteccio, crypt2pay, and aws-cloudhsm hardware do not support concurrent connections - # from multiple CI runs; give them fixed concurrency groups so only one job runs at a time - # across all PRs. utimaco and softhsm2 use a per-run group so they are never blocked by - # other PRs. + # Software/simulated HSMs: a per-run group so they are never blocked by other PRs. + # Hardware HSMs (proteccio, crypt2pay, aws-cloudhsm) run in the `hsm-upstream` job. concurrency: - group: ${{ (matrix.hsm-type == 'proteccio' && 'hsm-proteccio') || (matrix.hsm-type == 'crypt2pay' && 'hsm-crypt2pay') || (matrix.hsm-type == 'aws-cloudhsm' - && 'hsm-aws-cloudhsm') || format('hsm-{0}-{1}', matrix.hsm-type, github.run_id) }} + group: ${{ format('hsm-{0}-{1}', matrix.hsm-type, github.run_id) }} queue: max cancel-in-progress: false strategy: @@ -239,23 +289,69 @@ jobs: matrix: hsm-type: - utimaco - - proteccio - softhsm2 - kryoptic + features: [fips, non-fips] + + steps: + - uses: actions/checkout@v7 + with: + submodules: recursive + + - uses: ./.github/actions/cleanup-runner + + - uses: ./.github/actions/setup-nix + + - uses: ./.github/actions/install-mise + + - name: Test + env: + # Google variables + TEST_GOOGLE_OAUTH_CLIENT_ID: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_ID }} + TEST_GOOGLE_OAUTH_CLIENT_SECRET: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_SECRET }} + TEST_GOOGLE_OAUTH_REFRESH_TOKEN: ${{ secrets.TEST_GOOGLE_OAUTH_REFRESH_TOKEN }} + GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY: ${{ secrets.GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY }} + run: | + mise run test:hsm-${{ matrix.hsm-type }} --variant ${{ matrix.features }} + + # PKCS#11 v3 conformance tests: signs through OpenSC's pkcs11-tool (a real, + # independently-implemented external PKCS#11 v3 client), not just our own + # in-process Rust tests above. SoftHSM2-only: it is the only backend the CI + # is allowed to reconfigure with an HSM-KEK for this check. + # Prerequisite warning: `pkcs11-tool` (OpenSC) is a hard requirement — the + # task fails loudly if it is missing, with no silent fallback. It does not + # need a separate install step here: shell.nix already adds `pkgs.opensc` to + # the nix-shell PATH on Linux whenever `WITH_HSM=1` is set, which this task + # sets before entering the nix shell (see .mise/tasks/test/hsm-pkcs11-tool). + - name: Test (PKCS#11 v3 conformance via pkcs11-tool) + if: matrix.hsm-type == 'softhsm2' + run: | + mise run test:hsm-pkcs11-tool --variant ${{ matrix.features }} + + # Hardware HSMs reachable only with upstream-only secrets and infrastructure (Proteccio + # network HSM, Crypt2Pay over OpenVPN, the AWS CloudHSM CI cluster). They live in their + # own job (a job-level `if` cannot read the `matrix` context) and are skipped on forks. + hsm-upstream: + name: HSM ${{ matrix.hsm-type }} - ${{ matrix.features }} + runs-on: ubuntu-latest + if: github.repository == 'Cosmian/kms' + # This hardware does not support concurrent connections from multiple CI runs: a fixed + # group per HSM so only one job runs at a time across all PRs. + concurrency: + group: ${{ format('hsm-{0}', matrix.hsm-type) }} + queue: max + cancel-in-progress: false + strategy: + fail-fast: false + matrix: + hsm-type: + - proteccio - crypt2pay - aws-cloudhsm - features: [fips, non-fips] - exclude: - # parallel connections on proteccio is not supported - - hsm-type: proteccio - features: fips - # Not required - testing non-fips is sufficient - - hsm-type: crypt2pay - features: fips - # Not required until FIPS-mode compatibility is confirmed against the cluster's - # supported mechanism list - testing non-fips is sufficient for now - - hsm-type: aws-cloudhsm - features: fips + # non-fips only: parallel connections on proteccio are not supported, non-fips is + # sufficient for crypt2pay, and aws-cloudhsm FIPS-mode compatibility is not yet + # confirmed against the cluster's supported mechanism list. + features: [non-fips] steps: - uses: actions/checkout@v7 @@ -270,11 +366,13 @@ jobs: - name: Test env: - # HSM + # Proteccio PROTECCIO_IP: ${{ secrets.PROTECCIO_IP }} PROTECCIO_PASSWORD: ${{ secrets.PROTECCIO_PASSWORD }} PROTECCIO_SLOT: ${{ secrets.PROTECCIO_SLOT }} + # Crypt2Pay (reached through OpenVPN) CRYPT2PAY_PASSWORD: ${{ secrets.CRYPT2PAY_PASSWORD }} + OVPN_CONF: ${{ secrets.OVPN_CONF }} # AWS CloudHSM: persistent CI cluster (see crate/hsm/aws_cloudhsm/README.md) AWS_CLOUDHSM_CLUSTER_ID: ${{ secrets.KMS_CI_AWS_CLOUDHSM_CLUSTER_ID }} AWS_CLOUDHSM_CU_USERNAME: ${{ secrets.KMS_CI_AWS_CLOUDHSM_CU_USERNAME }} @@ -290,30 +388,9 @@ jobs: AWS_ACCESS_KEY_ID: ${{ secrets.KMS_CI_AWS_ACCESS_KEY_ID }} AWS_SECRET_ACCESS_KEY: ${{ secrets.KMS_CI_AWS_SECRET_ACCESS_KEY }} AWS_REGION: ${{ secrets.KMS_CI_AWS_REGION }} - # Google variables - TEST_GOOGLE_OAUTH_CLIENT_ID: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_ID }} - TEST_GOOGLE_OAUTH_CLIENT_SECRET: ${{ secrets.TEST_GOOGLE_OAUTH_CLIENT_SECRET }} - TEST_GOOGLE_OAUTH_REFRESH_TOKEN: ${{ secrets.TEST_GOOGLE_OAUTH_REFRESH_TOKEN }} - GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY: ${{ secrets.GOOGLE_SERVICE_ACCOUNT_PRIVATE_KEY }} - # OVPN for Crypt2Pay HSM tests - OVPN_CONF: ${{ secrets.OVPN_CONF }} run: | mise run test:hsm-${{ matrix.hsm-type }} --variant ${{ matrix.features }} - # PKCS#11 v3 conformance tests: signs through OpenSC's pkcs11-tool (a real, - # independently-implemented external PKCS#11 v3 client), not just our own - # in-process Rust tests above. SoftHSM2-only: it is the only backend the CI - # is allowed to reconfigure with an HSM-KEK for this check. - # Prerequisite warning: `pkcs11-tool` (OpenSC) is a hard requirement — the - # task fails loudly if it is missing, with no silent fallback. It does not - # need a separate install step here: shell.nix already adds `pkgs.opensc` to - # the nix-shell PATH on Linux whenever `WITH_HSM=1` is set, which this task - # sets before entering the nix shell (see .mise/tasks/test/hsm-pkcs11-tool). - - name: Test (PKCS#11 v3 conformance via pkcs11-tool) - if: matrix.hsm-type == 'softhsm2' - run: | - mise run test:hsm-pkcs11-tool --variant ${{ matrix.features }} - helm: name: Helm chart E2E (minikube) runs-on: ubuntu-latest @@ -351,7 +428,9 @@ jobs: permissions: contents: read environment: xks-remote-approval + # Upstream only: needs the XKS test server and its secrets, which forks do not have. if: >- + github.repository == 'Cosmian/kms' && github.actor != 'dependabot[bot]' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) @@ -410,8 +489,13 @@ jobs: cargo-publish: needs: - test-nix + - test-nix-upstream - hsm + - hsm-upstream - helm + # The upstream-only jobs are skipped on forks: a skipped dependency must not skip the + # publish dry-run, but any failed or cancelled dependency still blocks it. + if: ${{ !failure() && !cancelled() }} uses: ./.github/workflows/cargo-publish.yml with: toolchain: 1.97.0 diff --git a/CHANGELOG/claude_optimistic-carson-li1owh.md b/CHANGELOG/claude_optimistic-carson-li1owh.md new file mode 100644 index 0000000000..e21c2c6050 --- /dev/null +++ b/CHANGELOG/claude_optimistic-carson-li1owh.md @@ -0,0 +1,40 @@ +# HSM delegation review fixes + +## Security + +- Reject SHA-1 RSA signatures in FIPS mode on the HSM-delegated pre-hashed path too: a + 20-byte `digested_data` was signed as a SHA-1 PKCS#1 v1.5 `DigestInfo` through raw + `CKM_RSA_PKCS`, bypassing the FIPS gate that only blocked `CKM_SHA1_RSA_PKCS` + +## Bug Fixes + +### HSM + +- Only infer the RSA PKCS#1 v1.5 hash from the input length when the input is a digest + (`digested_data`); a raw message with no explicit hash always signs with SHA-256 instead + of silently switching to SHA-1/SHA-384/SHA-512 for 20/48/64-byte messages +- Reject a `CryptographicAlgorithm` that does not match the key family (e.g. `ECDSA` on an + RSA key) with a clear KMIP error instead of an opaque PKCS#11 mechanism error +- Report a malformed DER ECDSA signature as invalid in HSM `SignatureVerify` instead of + returning an operation error +- Pool only read-write PKCS#11 sessions (`SlotManager::checkout_session` no longer takes an + ignored `read_write` flag) + +### DB + +- Keyset re-key eligibility and HSM latest-generation selection bypass the `RotateNameCache`, + so they are never decided on stale keyset state (e.g. a rotation done by another KMS node) +- `RotateNameCache` is invalidated by keyset name for all owners and generation filters, and + by member UID on update, state change (revoke/destroy), delete and HSM re-label; empty + results are no longer cached + +### PKCS#11 + +- A signature cached by a `C_Sign` length query is only reused for the same data; a + follow-up call over different data is signed again instead of returning the earlier + signature + +## Documentation + +- Document the `RotateNameCache` invalidation and multi-node consistency model, and the + virtual-memory cost of the server's 16 MiB `RUST_MIN_STACK` default diff --git a/crate/clients/pkcs11/module/src/sessions.rs b/crate/clients/pkcs11/module/src/sessions.rs index b066f09bd6..cc3196a836 100644 --- a/crate/clients/pkcs11/module/src/sessions.rs +++ b/crate/clients/pkcs11/module/src/sessions.rs @@ -43,8 +43,8 @@ use crate::{ objects_store::{OBJECTS_STORE, ObjectsStore}, profiling::{self, SignPhase}, traits::{ - DecryptContext, EncryptContext, KeyAlgorithm, SearchOptions, SignContext, SignOperation, - VerifyContext, backend, use_pin_as_access_token, + DecryptContext, EncryptContext, KeyAlgorithm, PendingSignature, SearchOptions, SignContext, + SignOperation, VerifyContext, backend, use_pin_as_access_token, }, }; @@ -665,7 +665,12 @@ impl Session { // length reported by an earlier query call inconsistent with the bytes // actually produced later - causing a spurious CKR_BUFFER_TOO_SMALL on // the caller's second, real-buffer call. - let signature = if let Some(cached) = sign_ctx.pending_signature.clone() { + let cached = sign_ctx + .pending_signature + .as_ref() + .filter(|pending| pending.data == data) + .map(|pending| pending.signature.clone()); + let signature = if let Some(cached) = cached { cached } else { let private_key_sign = profiling::phase(SignPhase::PrivateKeySign); @@ -684,7 +689,10 @@ impl Session { } }; drop(private_key_sign); - sign_ctx.pending_signature = Some(signature.clone()); + sign_ctx.pending_signature = Some(PendingSignature { + data: data.to_vec(), + signature: signature.clone(), + }); signature }; if pSignature.is_null() { @@ -1181,6 +1189,126 @@ mod tests { assert!(session.sign_ctx.is_none()); } + /// Variable-length (ECDSA) test key whose "signature" is the signed data itself, so a + /// test can tell which data a returned signature was computed over. + #[derive(Debug)] + struct EchoEcdsaPrivateKey { + sign_calls: Arc, + } + + impl crate::traits::PrivateKey for EchoEcdsaPrivateKey { + fn remote_id(&self) -> &'static str { + "echo-ecdsa" + } + + fn sign( + &self, + _algorithm: &crate::traits::SignatureAlgorithm, + data: &[u8], + ) -> ModuleResult> { + self.sign_calls.fetch_add(1, AtomicOrdering::Relaxed); + Ok(data.to_vec()) + } + + fn algorithm(&self) -> KeyAlgorithm { + KeyAlgorithm::EccP256 + } + + fn key_size(&self) -> usize { + 256 + } + + fn pkcs8_der_bytes(&self) -> ModuleResult>> { + Err(ModuleError::FunctionNotSupported) + } + + fn rsa_public_exponent(&self) -> ModuleResult> { + Err(ModuleError::FunctionNotSupported) + } + } + + #[test] + fn variable_length_cached_signature_is_bound_to_its_data() { + let sign_calls = Arc::new(AtomicUsize::new(0)); + let mut session = Session { + sign_ctx: Some(SignContext { + algorithm: crate::traits::SignatureAlgorithm::Ecdsa, + private_key: Arc::new(EchoEcdsaPrivateKey { + sign_calls: Arc::clone(&sign_calls), + }), + operation: SignOperation::Classic, + payload: None, + pending_signature: None, + }), + ..Default::default() + }; + let first = [0x11_u8; 8]; + let second = [0x22_u8; 8]; + let mut signature_len: CK_ULONG = 0; + + // SAFETY: the data slice and output-length pointer remain valid for the call; + // a null signature pointer is the standard PKCS#11 length-query convention. + unsafe { + session + .sign(Some(&first), std::ptr::null_mut(), &raw mut signature_len) + .unwrap(); + } + assert_eq!(signature_len, 8); + assert_eq!(sign_calls.load(AtomicOrdering::Relaxed), 1); + + // The follow-up call changes the data: the signature cached for `first` must not + // be returned for `second`. + let mut signature = [0_u8; 8]; + // SAFETY: `signature` has the queried capacity and both pointers remain valid. + unsafe { + session + .sign( + Some(&second), + signature.as_mut_ptr(), + &raw mut signature_len, + ) + .unwrap(); + } + assert_eq!(signature, second); + assert_eq!(sign_calls.load(AtomicOrdering::Relaxed), 2); + assert!(session.sign_ctx.is_none()); + } + + #[test] + fn variable_length_cached_signature_is_reused_for_the_same_data() { + let sign_calls = Arc::new(AtomicUsize::new(0)); + let mut session = Session { + sign_ctx: Some(SignContext { + algorithm: crate::traits::SignatureAlgorithm::Ecdsa, + private_key: Arc::new(EchoEcdsaPrivateKey { + sign_calls: Arc::clone(&sign_calls), + }), + operation: SignOperation::Classic, + payload: None, + pending_signature: None, + }), + ..Default::default() + }; + let data = [0x33_u8; 8]; + let mut signature_len: CK_ULONG = 0; + + // SAFETY: see `variable_length_cached_signature_is_bound_to_its_data`. + unsafe { + session + .sign(Some(&data), std::ptr::null_mut(), &raw mut signature_len) + .unwrap(); + } + let mut signature = [0_u8; 8]; + // SAFETY: `signature` has the queried capacity and both pointers remain valid. + unsafe { + session + .sign(Some(&data), signature.as_mut_ptr(), &raw mut signature_len) + .unwrap(); + } + assert_eq!(signature, data); + assert_eq!(sign_calls.load(AtomicOrdering::Relaxed), 1); + } + #[test] fn ed25519_message_sign_keeps_context_for_multiple_messages() { let sign_calls = Arc::new(AtomicUsize::new(0)); diff --git a/crate/clients/pkcs11/module/src/traits/backend.rs b/crate/clients/pkcs11/module/src/traits/backend.rs index 193bf3f8af..b652792907 100644 --- a/crate/clients/pkcs11/module/src/traits/backend.rs +++ b/crate/clients/pkcs11/module/src/traits/backend.rs @@ -30,7 +30,18 @@ pub struct SignContext { /// to leading-zero bytes in r/s): re-signing on the second call could /// produce a different-length signature than what was reported by the /// length query, causing a spurious `CKR_BUFFER_TOO_SMALL`. - pub pending_signature: Option>, + pub pending_signature: Option, +} + +/// A signature cached between the two calls of the PKCS#11 length-query convention, +/// bound to the exact data it was computed over. +#[derive(Debug)] +pub struct PendingSignature { + /// The data that was signed. The cached signature is only reused for a follow-up + /// call over identical data; a caller that changes the data between the length + /// query and the real call gets a fresh signature, never one over the old data. + pub data: Vec, + pub signature: Vec, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/crate/clients/pkcs11/module/src/traits/mod.rs b/crate/clients/pkcs11/module/src/traits/mod.rs index 1018dffb2a..1553a1e506 100644 --- a/crate/clients/pkcs11/module/src/traits/mod.rs +++ b/crate/clients/pkcs11/module/src/traits/mod.rs @@ -18,9 +18,9 @@ // limitations under the License. pub use backend::{ - Backend, DecryptContext, EncryptContext, SignContext, SignOperation, VerifyContext, backend, - clear_backend, invoke_login_fn, register_backend, register_backend_if_absent, - register_login_fn, register_pin_mode, use_pin_as_access_token, + Backend, DecryptContext, EncryptContext, PendingSignature, SignContext, SignOperation, + VerifyContext, backend, clear_backend, invoke_login_fn, register_backend, + register_backend_if_absent, register_login_fn, register_pin_mode, use_pin_as_access_token, }; pub use certificate::Certificate; pub use data_object::DataObject; diff --git a/crate/hsm/base_hsm/src/kms_hsm.rs b/crate/hsm/base_hsm/src/kms_hsm.rs index 9cca728586..cbc5fff8fd 100644 --- a/crate/hsm/base_hsm/src/kms_hsm.rs +++ b/crate/hsm/base_hsm/src/kms_hsm.rs @@ -275,7 +275,7 @@ impl HSM for BaseHsm

{ let data = data.to_vec(); let iv_counter_nonce = iv_counter_nonce.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session.session()?.encrypt( handle, @@ -301,7 +301,7 @@ impl HSM for BaseHsm

{ let key_id = key_id.to_vec(); let data = data.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session .session()? @@ -324,7 +324,7 @@ impl HSM for BaseHsm

{ let key_id = key_id.to_vec(); let data = data.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session.session()?.sign(handle, algorithm.into(), &data)?; session.checkin(); @@ -347,7 +347,7 @@ impl HSM for BaseHsm

{ let data = data.to_vec(); let signature = signature.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session .session()? @@ -367,7 +367,7 @@ impl HSM for BaseHsm

{ let slot = self.get_slot(slot_id)?; let key_id = key_id.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session.session()?.get_key_type(handle)?; session.checkin(); @@ -385,7 +385,7 @@ impl HSM for BaseHsm

{ let slot = self.get_slot(slot_id)?; let key_id = key_id.to_vec(); tokio::task::spawn_blocking(move || { - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let handle = session.session()?.get_object_handle(&key_id)?; let result = session.session()?.get_key_metadata(handle)?; session.checkin(); @@ -397,7 +397,7 @@ impl HSM for BaseHsm

{ async fn generate_random(&self, slot_id: usize, len: usize) -> InterfaceResult> { let slot = self.get_slot(slot_id)?; - let session = SessionGuard::new(&slot, slot.checkout_session(true)?); + let session = SessionGuard::new(&slot, slot.checkout_session()?); let result = session.session()?.generate_random(len)?; session.checkin(); Ok(result) diff --git a/crate/hsm/base_hsm/src/session/session_impl.rs b/crate/hsm/base_hsm/src/session/session_impl.rs index 60d0bdc8f7..e3c2969922 100644 --- a/crate/hsm/base_hsm/src/session/session_impl.rs +++ b/crate/hsm/base_hsm/src/session/session_impl.rs @@ -176,7 +176,15 @@ const fn is_encryption_algorithm_supported(_: HsmEncryptionAlgorithm) -> bool { #[cfg(not(feature = "non-fips"))] const fn is_signing_algorithm_supported(algorithm: HsmSigningAlgorithm) -> bool { - !matches!(algorithm, HsmSigningAlgorithm::Sha1WithRsa) + // Both the hashing mechanism and the pre-hashed `DigestInfo` path must be rejected: + // otherwise a 20-byte SHA-1 digest signed through raw `CKM_RSA_PKCS` bypasses the gate. + !matches!( + algorithm, + HsmSigningAlgorithm::Sha1WithRsa + | HsmSigningAlgorithm::RsaPkcsV15Digest { + hashing_algorithm: HashingAlgorithm::SHA1 + } + ) } #[cfg(feature = "non-fips")] @@ -1426,9 +1434,9 @@ impl Session { data: &[u8], ) -> HResult> { if !is_signing_algorithm_supported(algorithm) { - return Err(HError::Default( - "RSA signatures with SHA-1 are unavailable in FIPS mode".to_owned(), - )); + return Err(HError::Default(format!( + "Signing algorithm {algorithm:?} is unavailable in FIPS mode" + ))); } match algorithm { HsmSigningAlgorithm::RsaPkcsV15 => { @@ -1497,12 +1505,7 @@ impl Session { // domain separator, even with a zero-length context), which not every // conformant library implements — do not pass params unless a context // string or the prehash flag is actually required. - let mut mechanism = CK_MECHANISM { - mechanism: CKM_EDDSA, - pParameter: ptr::null_mut(), - ulParameterLen: 0, - }; - self.sign_with_mechanism(key_handle, &mut mechanism, data) + self.sign_with_simple_mechanism(key_handle, CKM_EDDSA, data) } } } @@ -1801,9 +1804,9 @@ impl Session { signature: &[u8], ) -> HResult { if !is_signing_algorithm_supported(algorithm) { - return Err(HError::Default( - "RSA signatures with SHA-1 are unavailable in FIPS mode".to_owned(), - )); + return Err(HError::Default(format!( + "Signing algorithm {algorithm:?} is unavailable in FIPS mode" + ))); } match algorithm { HsmSigningAlgorithm::RsaPkcsV15 => { @@ -1864,7 +1867,12 @@ impl Session { // (matching the software ECDSA verify convention). Convert it back, // using the key's own `CKA_EC_PARAMS` to determine the field size. let curve = self.ec_curve_for_key(key_handle)?; - let raw_signature = Self::ecdsa_der_to_raw(signature, curve_byte_size(curve))?; + // A signature that is not a well-formed `ECDSA-Sig-Value` for this curve is + // simply invalid (as in the software verify path), not an operation error. + let Ok(raw_signature) = Self::ecdsa_der_to_raw(signature, curve_byte_size(curve)) + else { + return Ok(false); + }; self.verify_with_simple_mechanism(key_handle, mechanism, data, &raw_signature) } // EdDSA (Ed25519/Ed448) is a pure, un-hashed signature scheme (RFC 8032): the raw @@ -1876,12 +1884,7 @@ impl Session { HsmSigningAlgorithm::Eddsa => { // See the matching comment in `sign()`: omit `CK_EDDSA_PARAMS` to // request the pure Ed25519 variant (RFC 8032), not `Ed25519ctx`. - let mut mechanism = CK_MECHANISM { - mechanism: CKM_EDDSA, - pParameter: ptr::null_mut(), - ulParameterLen: 0, - }; - self.verify_with_mechanism(key_handle, &mut mechanism, data, signature) + self.verify_with_simple_mechanism(key_handle, CKM_EDDSA, data, signature) } } } diff --git a/crate/hsm/base_hsm/src/slots.rs b/crate/hsm/base_hsm/src/slots.rs index 5cbf4eab51..425b64deb3 100644 --- a/crate/hsm/base_hsm/src/slots.rs +++ b/crate/hsm/base_hsm/src/slots.rs @@ -330,8 +330,11 @@ impl SlotManager { ) } - /// Check out a pooled session if available, otherwise open a new one. - pub fn checkout_session(&self, read_write: bool) -> HResult { + /// Check out a pooled read-write session if available, otherwise open a new one. + /// + /// The pool only ever holds read-write sessions, so callers can never receive a + /// session with different access rights than the ones it was opened with. + pub fn checkout_session(&self) -> HResult { let pooled = { let mut pool = self .session_pool @@ -339,7 +342,7 @@ impl SlotManager { .map_err(|e| HError::Default(format!("Failed to lock session pool: {e}")))?; pool.pop() }; - pooled.map_or_else(|| self.open_session(read_write), Ok) + pooled.map_or_else(|| self.open_session(true), Ok) } /// Return a healthy session to the pool for reuse. diff --git a/crate/hsm/base_hsm/src/tests_shared.rs b/crate/hsm/base_hsm/src/tests_shared.rs index a5fb67f999..e5428f0d06 100644 --- a/crate/hsm/base_hsm/src/tests_shared.rs +++ b/crate/hsm/base_hsm/src/tests_shared.rs @@ -1332,7 +1332,7 @@ pub fn concurrent_sign_does_not_degrade(slot: &Arc) -> HResult<()> let handle = thread::spawn(move || -> HResult<()> { while !stop.load(Ordering::Relaxed) { - let session = slot.checkout_session(true)?; + let session = slot.checkout_session()?; let _sig = { let res = session.sign(sk, sign_algorithm, &signing_input)?; slot.checkin_session(session); diff --git a/crate/interfaces/src/crypto_oracle.rs b/crate/interfaces/src/crypto_oracle.rs index c6dcb8b14e..f9be7c4f28 100644 --- a/crate/interfaces/src/crypto_oracle.rs +++ b/crate/interfaces/src/crypto_oracle.rs @@ -275,6 +275,27 @@ impl SigningAlgorithm { } } + // Same up-front key-family guard as for an explicit `digital_signature_algorithm` + // above: an RSA key requested with `EC`/`ECDSA` (or an EC key with `RSA`) must fail + // with a clear KMIP error rather than an opaque PKCS#11 mechanism error from the HSM. + let is_rsa_key = matches!(key_type, KeyType::RsaPrivateKey | KeyType::RsaPublicKey); + let is_ec_key = matches!(key_type, KeyType::EcPrivateKey | KeyType::EcPublicKey); + match params.cryptographic_algorithm { + Some(algorithm @ (CryptographicAlgorithm::EC | CryptographicAlgorithm::ECDSA)) + if !is_ec_key => + { + return Err(InterfaceError::InvalidRequest(format!( + "Cryptographic algorithm {algorithm:?} does not match the {key_type:?} key" + ))); + } + Some(algorithm @ CryptographicAlgorithm::RSA) if !is_rsa_key => { + return Err(InterfaceError::InvalidRequest(format!( + "Cryptographic algorithm {algorithm:?} does not match the {key_type:?} key" + ))); + } + _ => {} + } + // 3. cryptographic_algorithm + hashing_algorithm (EC/ECDSA) if matches!( params.cryptographic_algorithm, @@ -313,13 +334,13 @@ impl SigningAlgorithm { Some(HashingAlgorithm::SHA1) => { Self::rsa_pkcs1_from_hash(HashingAlgorithm::SHA1, input_is_digest) } - Some(HashingAlgorithm::SHA256) | None => { - let hash = params - .hashing_algorithm - .or_else(|| Self::infer_hash_from_digest_len(input_len)) - .unwrap_or(HashingAlgorithm::SHA256); - Self::rsa_pkcs1_from_hash(hash, input_is_digest) + Some(HashingAlgorithm::SHA256) => { + Self::rsa_pkcs1_from_hash(HashingAlgorithm::SHA256, input_is_digest) } + None => Self::rsa_pkcs1_from_hash( + Self::default_rsa_hash(input_is_digest, input_len), + input_is_digest, + ), Some(HashingAlgorithm::SHA384) => { Self::rsa_pkcs1_from_hash(HashingAlgorithm::SHA384, input_is_digest) } @@ -346,11 +367,10 @@ impl SigningAlgorithm { // default mechanism selection: `signature_verify` has no `input_is_digest` // parameter of its own, so this also covers the `SignatureVerify` KMIP operation // delegating to a public/private key with no explicit algorithm requested. - KeyType::RsaPrivateKey | KeyType::RsaPublicKey => { - let hash = - Self::infer_hash_from_digest_len(input_len).unwrap_or(HashingAlgorithm::SHA256); - Self::rsa_pkcs1_from_hash(hash, input_is_digest) - } + KeyType::RsaPrivateKey | KeyType::RsaPublicKey => Self::rsa_pkcs1_from_hash( + Self::default_rsa_hash(input_is_digest, input_len), + input_is_digest, + ), KeyType::EcPrivateKey | KeyType::EcPublicKey => match curve { Some(crate::EcCurve::P384) => Ok(Self::Ecdsa { hashing_algorithm: HashingAlgorithm::SHA384, @@ -402,10 +422,54 @@ impl SigningAlgorithm { } } + /// Default hash for RSA PKCS#1 v1.5 when the request does not name one. + /// + /// The digest length is only meaningful when the caller supplied a digest + /// (`digested_data`): for a raw message the length says nothing about the hash, so the + /// default is always SHA-256 (the software RSA signing default). Inferring from a raw + /// message length would silently pick SHA-1 for a 20-byte message, or SHA-384/SHA-512 for + /// a 48/64-byte one, producing signatures no default-configured verifier accepts. + const fn default_rsa_hash(input_is_digest: bool, input_len: usize) -> HashingAlgorithm { + if input_is_digest { + if let Some(hash) = Self::infer_hash_from_digest_len(input_len) { + return hash; + } + } + HashingAlgorithm::SHA256 + } + + /// FIPS: SHA-1 RSA signatures are not approved (SP 800-131A). Checked for both the + /// hashing mechanism (`CKM_SHA1_RSA_PKCS`) and the pre-hashed `DigestInfo` variant, so a + /// 20-byte `digested_data` cannot bypass the gate by going through raw `CKM_RSA_PKCS`. + #[cfg(not(feature = "non-fips"))] + fn check_rsa_signature_hash_allowed( + hashing_algorithm: HashingAlgorithm, + ) -> Result<(), InterfaceError> { + if hashing_algorithm == HashingAlgorithm::SHA1 { + return Err(InterfaceError::InvalidRequest( + "RSA signatures with SHA-1 are unavailable in FIPS mode".to_owned(), + )); + } + Ok(()) + } + + /// Non-FIPS: every hash `rsa_pkcs1_from_hash` otherwise supports is allowed. + #[cfg(feature = "non-fips")] + #[expect( + clippy::unnecessary_wraps, + reason = "signature must match the FIPS variant of this function" + )] + const fn check_rsa_signature_hash_allowed( + _hashing_algorithm: HashingAlgorithm, + ) -> Result<(), InterfaceError> { + Ok(()) + } + fn rsa_pkcs1_from_hash( hashing_algorithm: HashingAlgorithm, input_is_digest: bool, ) -> Result { + Self::check_rsa_signature_hash_allowed(hashing_algorithm)?; if input_is_digest { return match hashing_algorithm { HashingAlgorithm::SHA1 @@ -998,4 +1062,94 @@ mod tests { SigningAlgorithm::Ed25519 ); } + + #[test] + fn from_kmip_raw_message_length_never_selects_the_rsa_hash() { + // A raw (non-digest) message of a digest-like length must still default to SHA-256: + // the length of a message says nothing about the hash to use. + let rsa_params = params_with(None, Some(CryptographicAlgorithm::RSA), None, None, None); + for len in [20, 32, 48, 64] { + for params in [None, Some(&rsa_params)] { + assert_eq!( + SigningAlgorithm::from_kmip(params, KeyType::RsaPrivateKey, None, false, len) + .expect("should resolve"), + SigningAlgorithm::Sha256WithRsa, + "len: {len}, params: {params:?}" + ); + } + } + } + + #[test] + fn from_kmip_digest_length_selects_the_rsa_digest_info_hash() { + for (len, hashing_algorithm) in [ + (32, HashingAlgorithm::SHA256), + (48, HashingAlgorithm::SHA384), + (64, HashingAlgorithm::SHA512), + ] { + assert_eq!( + SigningAlgorithm::from_kmip(None, KeyType::RsaPrivateKey, None, true, len) + .expect("should resolve"), + SigningAlgorithm::RsaPkcsV15Digest { hashing_algorithm }, + "len: {len}" + ); + } + } + + #[cfg(not(feature = "non-fips"))] + #[test] + fn from_kmip_fips_rejects_sha1_rsa_signatures() { + // Pre-hashed 20-byte digest: must not bypass the FIPS gate via raw `CKM_RSA_PKCS`. + SigningAlgorithm::from_kmip(None, KeyType::RsaPrivateKey, None, true, 20) + .expect_err("SHA-1 digest must be rejected in FIPS mode"); + let params = params_with( + Some(DigitalSignatureAlgorithm::SHA1WithRSAEncryption), + None, + None, + None, + None, + ); + for input_is_digest in [false, true] { + SigningAlgorithm::from_kmip( + Some(¶ms), + KeyType::RsaPrivateKey, + None, + input_is_digest, + 20, + ) + .expect_err("SHA1WithRSAEncryption must be rejected in FIPS mode"); + } + } + + #[cfg(feature = "non-fips")] + #[test] + fn from_kmip_non_fips_accepts_sha1_rsa_digest() { + assert_eq!( + SigningAlgorithm::from_kmip(None, KeyType::RsaPrivateKey, None, true, 20) + .expect("should resolve"), + SigningAlgorithm::RsaPkcsV15Digest { + hashing_algorithm: HashingAlgorithm::SHA1, + } + ); + } + + #[test] + fn from_kmip_rejects_cryptographic_algorithm_key_family_mismatch() { + let ec_params = params_with(None, Some(CryptographicAlgorithm::ECDSA), None, None, None); + let err = + SigningAlgorithm::from_kmip(Some(&ec_params), KeyType::RsaPrivateKey, None, false, 0) + .expect_err("ECDSA on an RSA key must be rejected"); + assert!(matches!(err, InterfaceError::InvalidRequest(_))); + + let rsa_params = params_with(None, Some(CryptographicAlgorithm::RSA), None, None, None); + let err = SigningAlgorithm::from_kmip( + Some(&rsa_params), + KeyType::EcPublicKey, + Some(crate::EcCurve::P256), + false, + 0, + ) + .expect_err("RSA on an EC key must be rejected"); + assert!(matches!(err, InterfaceError::InvalidRequest(_))); + } } diff --git a/crate/interfaces/src/hsm/hsm_store.rs b/crate/interfaces/src/hsm/hsm_store.rs index 0f7ab0ac6e..54fde48246 100644 --- a/crate/interfaces/src/hsm/hsm_store.rs +++ b/crate/interfaces/src/hsm/hsm_store.rs @@ -916,7 +916,8 @@ impl CryptoOracle for HsmStore { } }; let curve = metadata.curve; - // here is always the original signed message, never a caller-supplied digest. + // `data` is the caller-supplied digest when `input_is_digest` is set (KMIP + // `digested_data`), otherwise the original signed message. let algorithm = SigningAlgorithm::from_kmip( cryptographic_parameters, key_type, diff --git a/crate/server/src/core/operations/rekey/common.rs b/crate/server/src/core/operations/rekey/common.rs index 1bb0c36d5d..afa5be4075 100644 --- a/crate/server/src/core/operations/rekey/common.rs +++ b/crate/server/src/core/operations/rekey/common.rs @@ -97,7 +97,12 @@ impl KMS { return Ok(true); }; let current_gen = attrs.rotate_generation.unwrap_or(0); - let all = self.database.find_by_rotate_name(name, None, user).await?; + // Uncached: re-key eligibility must reflect the current keyset state, including + // rotations performed by other KMS nodes sharing this database. + let all = self + .database + .find_by_rotate_name_uncached(name, None, user) + .await?; Ok(!all.iter().any(|(other_uid, other_attrs)| { other_uid != uid && other_attrs.rotate_generation.unwrap_or(0) > current_gen })) @@ -522,14 +527,9 @@ pub(crate) async fn execute_rekey( )) }) .collect(); + // `Database::atomic` invalidates the keyset-resolution cache for every created + // member's `rotate_name`, so the new generation is visible immediately. kms.database.atomic(user, &persist_ops).await?; - for r in replacements.as_ref() { - if let Some(rotate_name) = &r.attributes.rotate_name { - kms.database - .invalidate_rotate_name_cache(rotate_name, user) - .await; - } - } op.finalize_dependants(kms, user, &candidates, &replacements) .await?; Ok(op.build_response(&replacements)) diff --git a/crate/server/src/core/operations/rekey/symmetric/hsm.rs b/crate/server/src/core/operations/rekey/symmetric/hsm.rs index 41ebddb5ef..83b0c9aa8d 100644 --- a/crate/server/src/core/operations/rekey/symmetric/hsm.rs +++ b/crate/server/src/core/operations/rekey/symmetric/hsm.rs @@ -30,7 +30,7 @@ impl KMS { user: &UserId, ) -> Option { self.database - .find_by_rotate_name(rotate_name, None, user) + .find_by_rotate_name_uncached(rotate_name, None, user) .await .ok() .and_then(|keys| { @@ -270,7 +270,7 @@ impl KMS { // Make the new generation visible to Sign/Verify/Encrypt/Decrypt immediately // instead of waiting out the cache's TTL (see `RotateNameCache` module docs) — // this is the commit point: CKA_LABEL on both keys now reflects the rotation. - self.database.invalidate_rotate_name_cache(name, user).await; + self.database.invalidate_rotate_name_cache(name); } trace!("HSM ReKey: old={uid} → new={new_uid} (slot={slot_id}, gen={new_gen}), user={user}"); diff --git a/crate/server/src/main.rs b/crate/server/src/main.rs index 36519b2010..8c896bc9cd 100644 --- a/crate/server/src/main.rs +++ b/crate/server/src/main.rs @@ -53,6 +53,13 @@ fn main() { // deep KMIP/TTLV + middleware call chains under concurrent load otherwise overflow // the small platform default stack (observed: "actix-server worker N ... stack // overflow" under >16 concurrent clients). + // + // This minimum applies to *every* std-spawned thread without an explicit stack size, + // including each actix worker's Tokio blocking pool (where HSM PKCS#11 calls run via + // `spawn_blocking`; ~512 threads in total by default). Stacks are reserved as virtual + // memory and only committed on use, so the resident cost is unchanged, but hosts that + // cap address space (`ulimit -v`, `RLIMIT_AS`) must budget for it. Operators can set + // `RUST_MIN_STACK` explicitly to override this default. if std::env::var_os("RUST_MIN_STACK").is_none() { unsafe { std::env::set_var("RUST_MIN_STACK", (16 * 1024 * 1024).to_string()); diff --git a/crate/server_database/src/core/database_objects.rs b/crate/server_database/src/core/database_objects.rs index 831af3d004..ef89c0cb61 100644 --- a/crate/server_database/src/core/database_objects.rs +++ b/crate/server_database/src/core/database_objects.rs @@ -210,13 +210,18 @@ impl Database { if let Some(ref uid) = uid { reject_reserved_uid(uid)?; } - self.record("create", async move { - let db = self - .get_object_store(uid.as_deref().unwrap_or_default()) - .await?; - Ok(db.create(uid, owner, object, attributes, tags).await?) - }) - .await + let uid = self + .record("create", async move { + let db = self + .get_object_store(uid.as_deref().unwrap_or_default()) + .await?; + Ok(db.create(uid, owner, object, attributes, tags).await?) + }) + .await?; + if let Some(name) = attributes.rotate_name.as_deref() { + self.rotate_name_cache.invalidate_name(name); + } + Ok(uid) } /// Retrieve objects from the database. @@ -421,6 +426,8 @@ impl Database { .await?; // Invalidate the object cache since attributes or key material may have changed. self.object_cache.invalidate(uid).await; + self.rotate_name_cache + .invalidate_member(uid, attributes.rotate_name.as_deref()); // Validate the unwrapped cache: if the object fingerprint changed (e.g. a // re-wrap), evict the stale unwrapped entry so the next get_unwrapped call // performs a fresh unwrap instead of returning stale key material. @@ -436,6 +443,7 @@ impl Database { }) .await?; self.object_cache.invalidate(uid).await; + self.rotate_name_cache.invalidate_member(uid, None); Ok(()) } @@ -448,6 +456,7 @@ impl Database { .await?; self.object_cache.invalidate(uid).await; self.unwrapped_cache.clear_cache(uid).await; + self.rotate_name_cache.invalidate_member(uid, None); Ok(()) } @@ -680,8 +689,11 @@ impl Database { /// `C_FindObjects` scan plus a `C_GetAttributeValue` round-trip per object on *every* /// cryptographic operation. Rotation is a rare, explicit administrative action, so a /// worst-case few-second staleness window before a new generation becomes visible is an - /// accepted trade-off (`rotate` operations additionally call - /// [`crate::core::RotateNameCache::invalidate`] to shrink that window in practice). + /// accepted trade-off for key *resolution*. Local writes invalidate the cache eagerly; + /// writes from other KMS nodes sharing the database are only seen after the TTL. + /// + /// Paths whose correctness depends on the current keyset state (re-key eligibility, + /// next-generation allocation) must use [`Self::find_by_rotate_name_uncached`] instead. pub async fn find_by_rotate_name( &self, name: &str, @@ -691,6 +703,23 @@ impl Database { if let Some(cached) = self.rotate_name_cache.get(name, generation, owner).await { return Ok(cached); } + let results = self + .find_by_rotate_name_uncached(name, generation, owner) + .await?; + self.rotate_name_cache + .insert(name, generation, owner, results.clone()) + .await; + Ok(results) + } + + /// Same as [`Self::find_by_rotate_name`], but always queries the object stores, + /// bypassing (and not populating) the [`crate::core::RotateNameCache`]. + pub async fn find_by_rotate_name_uncached( + &self, + name: &str, + generation: Option, + owner: &UserId, + ) -> DbResult> { let map = self.objects.read().await; let mut results: Vec<(String, Attributes)> = Vec::new(); for db in map.values() { @@ -700,19 +729,16 @@ impl Database { .unwrap_or_default(), ); } - drop(map); - self.rotate_name_cache - .insert(name, generation, owner, results.clone()) - .await; Ok(results) } - /// Invalidate the cached `find_by_rotate_name` entry for `name`/`owner`. + /// Invalidate every cached `find_by_rotate_name` entry for keyset `name` + /// (all owners, all generation filters). /// /// Call this after a rotation (rekey) creates a new generation so the next /// resolution sees it immediately instead of waiting out the cache's TTL. - pub async fn invalidate_rotate_name_cache(&self, name: &str, owner: &UserId) { - self.rotate_name_cache.invalidate(name, owner).await; + pub fn invalidate_rotate_name_cache(&self, name: &str) { + self.rotate_name_cache.invalidate_name(name); } /// Set the `CKA_LABEL` (or equivalent) on a key identified by `uid`. @@ -720,7 +746,10 @@ impl Database { /// Routes to the object store responsible for `uid`. SQL stores silently ignore this. pub async fn set_key_label(&self, uid: &str, label: &str) -> DbResult<()> { let store = self.get_object_store(uid).await?; - store.set_key_label(uid, label).await.map_err(Into::into) + store.set_key_label(uid, label).await?; + // On HSM stores the label carries the keyset name and generation. + self.rotate_name_cache.invalidate_member(uid, None); + Ok(()) } /// Rewrite the PKCS#11 rotation dates on an HSM key identified by `uid`. @@ -783,23 +812,30 @@ impl Database { // invalidate of clear cache for all operations for op in operations { match op { - AtomicOperation::Create((uid, _owner, object, ..)) => { + AtomicOperation::Create((uid, _owner, object, attributes, ..)) => { self.object_cache.invalidate(uid).await; self.unwrapped_cache.validate_cache(uid, object).await?; + if let Some(name) = attributes.rotate_name.as_deref() { + self.rotate_name_cache.invalidate_name(name); + } } - AtomicOperation::UpdateObject((uid, object, ..)) - | AtomicOperation::Upsert((uid, object, ..)) => { + AtomicOperation::UpdateObject((uid, object, attributes, ..)) + | AtomicOperation::Upsert((uid, object, attributes, ..)) => { self.object_cache.invalidate(uid).await; self.unwrapped_cache.validate_cache(uid, object).await?; + self.rotate_name_cache + .invalidate_member(uid, attributes.rotate_name.as_deref()); } AtomicOperation::Delete(uid) => { self.object_cache.invalidate(uid).await; self.unwrapped_cache.clear_cache(uid).await; + self.rotate_name_cache.invalidate_member(uid, None); } AtomicOperation::UpdateState((uid, _)) => { // Evict the stale object so the new lifecycle state is // visible immediately on the next retrieve_object call. self.object_cache.invalidate(uid).await; + self.rotate_name_cache.invalidate_member(uid, None); } } } diff --git a/crate/server_database/src/core/rotate_name_cache.rs b/crate/server_database/src/core/rotate_name_cache.rs index 89c405b87b..d34e760af2 100644 --- a/crate/server_database/src/core/rotate_name_cache.rs +++ b/crate/server_database/src/core/rotate_name_cache.rs @@ -21,10 +21,21 @@ //! administrative action, so a worst-case few-second delay before a freshly //! rotated generation becomes visible to new Sign/Verify calls is an accepted //! trade-off for removing a full HSM slot scan from the per-operation hot path. +//! +//! # Invalidation +//! +//! Local writes invalidate eagerly and by keyset *name* (every owner and every +//! generation filter), or by member UID when the name is not known (delete, +//! state change). Empty results are never cached, so a freshly created keyset +//! is visible immediately. Writes made by *other* KMS nodes sharing the same +//! database are only picked up once the TTL expires, which is why correctness- +//! critical paths (re-key eligibility and generation allocation) must bypass +//! this cache via `Database::find_by_rotate_name_uncached`. -use std::{num::NonZeroUsize, time::Duration}; +use std::{num::NonZeroUsize, sync::Arc, time::Duration}; use cosmian_kmip::kmip_2_1::kmip_attributes::Attributes; +use cosmian_logger::warn; use moka::future::Cache; /// Bounded staleness window for cached `find_by_rotate_name` results. @@ -46,12 +57,14 @@ struct RotateNameKey { owner: String, } +type RotateNameResults = Arc>; + /// Concurrent, short-TTL cache for [`crate::Database::find_by_rotate_name`] results. /// /// Backed by [`moka::future::Cache`] — lookups are lock-free, so concurrent /// Sign/Verify calls on distinct HSM sessions never serialize on a shared lock. pub struct RotateNameCache { - inner: Cache>, + inner: Cache, } impl RotateNameCache { @@ -70,6 +83,7 @@ impl RotateNameCache { inner: Cache::builder() .max_capacity(max_capacity) .time_to_live(ttl) + .support_invalidation_closures() .build(), } } @@ -86,10 +100,13 @@ impl RotateNameCache { generation, owner: owner.to_owned(), }; - self.inner.get(&key).await + self.inner.get(&key).await.map(|results| (*results).clone()) } /// Insert a freshly computed result for `(name, generation, owner)`. + /// + /// Empty results are not cached: a keyset that does not exist yet must become + /// visible as soon as its first member is created, not after the TTL. pub async fn insert( &self, name: &str, @@ -97,31 +114,52 @@ impl RotateNameCache { owner: &str, results: Vec<(String, Attributes)>, ) { + if results.is_empty() { + return; + } let key = RotateNameKey { name: name.to_owned(), generation, owner: owner.to_owned(), }; - self.inner.insert(key, results).await; + self.inner.insert(key, Arc::new(results)).await; } - /// Invalidate every cached generation-filter variant for `name`/`owner`. + /// Invalidate every cached entry for keyset `name`, for all owners and all + /// generation filters. /// /// Called after a rotation (rekey) so the next resolution sees the new - /// generation immediately instead of waiting out the TTL. Since the - /// generation filter is part of the key but rotations only add a new - /// generation (they never need `invalidate_entries_if` to be exhaustive - /// for correctness — the TTL bounds worst-case staleness regardless), - /// this clears the unfiltered (`None`) entry that `resolve_keyset_to_single_uid` - /// and `walk_keyset_chain` actually populate for `SingleLatest`/`Bare`/`Latest` - /// lookups, which is the entry every delegated Sign/Verify call reads. - pub async fn invalidate(&self, name: &str, owner: &str) { - let key = RotateNameKey { - name: name.to_owned(), - generation: None, - owner: owner.to_owned(), - }; - self.inner.invalidate(&key).await; + /// generation immediately instead of waiting out the TTL. + pub fn invalidate_name(&self, name: &str) { + let name = name.to_owned(); + self.invalidate_if(move |key, _| key.name == name); + } + + /// Invalidate every cached entry that may be affected by a local write to `uid`: + /// entries for keyset `rotate_name` (when known) and any entry listing `uid` as a member. + /// + /// Used for writes where the keyset name is unknown or may have changed (delete, + /// state change, attribute update), so that e.g. a destroyed or revoked generation + /// is not returned as the keyset's latest member for the rest of the TTL. + pub fn invalidate_member(&self, uid: &str, rotate_name: Option<&str>) { + let uid = uid.to_owned(); + let rotate_name = rotate_name.map(ToOwned::to_owned); + self.invalidate_if(move |key, results| { + rotate_name.as_deref() == Some(key.name.as_str()) + || results.iter().any(|(member, _)| *member == uid) + }); + } + + fn invalidate_if(&self, predicate: F) + where + F: Fn(&RotateNameKey, &RotateNameResults) -> bool + Send + Sync + 'static, + { + if let Err(e) = self.inner.invalidate_entries_if(predicate) { + // Only possible if invalidation closures were not enabled at build time; + // fall back to dropping everything rather than serving stale results. + warn!("RotateNameCache: predicate invalidation failed ({e}); clearing cache"); + self.inner.invalidate_all(); + } } } @@ -141,23 +179,34 @@ mod tests { Attributes::default() } - #[tokio::test] - async fn hit_miss_and_key_isolation() { - let cache = RotateNameCache::with_config(Duration::from_secs(60), NonZeroUsize::MIN); - assert!(cache.get("keyset-a", None, "alice").await.is_none()); + fn cache() -> RotateNameCache { + RotateNameCache::with_config( + Duration::from_secs(60), + NonZeroUsize::new(100).unwrap_or(NonZeroUsize::MIN), + ) + } + async fn seed(cache: &RotateNameCache, name: &str, generation: Option, owner: &str) { cache .insert( - "keyset-a", - None, - "alice", - vec![("uid-1".to_owned(), attrs())], + name, + generation, + owner, + vec![(format!("{name}-uid"), attrs())], ) .await; + } + + #[tokio::test] + async fn hit_miss_and_key_isolation() { + let cache = cache(); + assert!(cache.get("keyset-a", None, "alice").await.is_none()); + + seed(&cache, "keyset-a", None, "alice").await; assert_eq!( cache.get("keyset-a", None, "alice").await, - Some(vec![("uid-1".to_owned(), attrs())]) + Some(vec![("keyset-a-uid".to_owned(), attrs())]) ); // Different owner, different generation, different name: all distinct keys. assert!(cache.get("keyset-a", None, "bob").await.is_none()); @@ -166,31 +215,54 @@ mod tests { } #[tokio::test] - async fn invalidate_clears_the_bare_entry() { - let cache = RotateNameCache::with_config(Duration::from_secs(60), NonZeroUsize::MIN); - cache - .insert( - "keyset-a", - None, - "alice", - vec![("uid-1".to_owned(), attrs())], - ) - .await; - cache.invalidate("keyset-a", "alice").await; + async fn empty_results_are_not_cached() { + let cache = cache(); + cache.insert("keyset-a", Some(2), "alice", vec![]).await; + assert!(cache.get("keyset-a", Some(2), "alice").await.is_none()); + } + + #[tokio::test] + async fn invalidate_name_clears_all_owners_and_generations() { + let cache = cache(); + seed(&cache, "keyset-a", None, "alice").await; + seed(&cache, "keyset-a", None, "bob").await; + seed(&cache, "keyset-a", Some(0), "alice").await; + seed(&cache, "keyset-b", None, "alice").await; + + cache.invalidate_name("keyset-a"); + assert!(cache.get("keyset-a", None, "alice").await.is_none()); + assert!(cache.get("keyset-a", None, "bob").await.is_none()); + assert!(cache.get("keyset-a", Some(0), "alice").await.is_none()); + assert!(cache.get("keyset-b", None, "alice").await.is_some()); + } + + #[tokio::test] + async fn invalidate_member_clears_entries_listing_the_uid() { + let cache = cache(); + seed(&cache, "keyset-a", None, "alice").await; + seed(&cache, "keyset-b", None, "alice").await; + + cache.invalidate_member("keyset-a-uid", None); + + assert!(cache.get("keyset-a", None, "alice").await.is_none()); + assert!(cache.get("keyset-b", None, "alice").await.is_some()); + } + + #[tokio::test] + async fn invalidate_member_clears_entries_for_the_given_name() { + let cache = cache(); + seed(&cache, "keyset-a", Some(3), "bob").await; + + cache.invalidate_member("some-new-uid", Some("keyset-a")); + + assert!(cache.get("keyset-a", Some(3), "bob").await.is_none()); } #[tokio::test] async fn entries_expire_after_ttl() { let cache = RotateNameCache::with_config(Duration::from_millis(20), NonZeroUsize::MIN); - cache - .insert( - "keyset-a", - None, - "alice", - vec![("uid-1".to_owned(), attrs())], - ) - .await; + seed(&cache, "keyset-a", None, "alice").await; assert!(cache.get("keyset-a", None, "alice").await.is_some()); tokio::time::sleep(Duration::from_millis(80)).await; assert!(cache.get("keyset-a", None, "alice").await.is_none()); diff --git a/documentation/docs/configuration/object-cache.md b/documentation/docs/configuration/object-cache.md index c6589b4102..f331096ea0 100644 --- a/documentation/docs/configuration/object-cache.md +++ b/documentation/docs/configuration/object-cache.md @@ -198,20 +198,30 @@ used to resolve `RotateName` and generation selectors. This is distinct from | Default capacity | 10,000 entries | | TTL | 2 seconds | | Concurrency | Lock-free `moka::future::Cache` | -| Invalidation | Bare/latest entry invalidated after rotation commit | +| Invalidation | By keyset name (all owners and generations) or by member UID on local writes | +| Empty results | Never cached | On a miss, the existing multi-store lookup runs unchanged. On a hit, the database query or HSM slot scan is skipped. The owner is part of the key to preserve access isolation, and `generation` is part of the key to prevent an explicit historical lookup from sharing the bare/latest result. -### Rotation visibility +### Invalidation and consistency -The SQL re-key/re-certify orchestrator and the dedicated HSM symmetric re-key -path invalidate the keyset's bare/latest entry after the new generation is -committed. New operations therefore see the new head immediately through those -paths. The 2-second TTL bounds staleness for any future rotation path that does -not yet perform explicit invalidation. +Every local write through `Database` invalidates the affected entries eagerly: + +- creating an object with a `RotateName` (including a re-key's new generation) + clears every entry for that keyset name, for all owners and generation filters; +- updating, re-labelling (HSM `CKA_LABEL`), changing the state of (revoke, + destroy) or deleting an object clears every entry that lists it as a member. + +Empty results are not cached, so a newly created keyset is visible at once. + +Writes performed by *other* KMS nodes sharing the same database are only seen +once the 2-second TTL expires. Paths whose correctness depends on the current +keyset state — re-key eligibility (`enforce_keyset_latest`) and HSM +latest-generation selection — therefore bypass the cache through +`Database::find_by_rotate_name_uncached`. `RotateNameCache` is internal and is not configurable through the server configuration file or command-line options.