Repository navigation
fix(kms-api, kms-core, kms-keystore): P0-1 nonce panic, P0-2 service-layer tenant pre-check, P0-3 keystore-layer tenant defence in depth - #42
Open
EricZHANG1688 wants to merge 21 commits into
Open
EricZHANG1688 wants to merge 21 commits into
EricZHANG1688 wants to merge 21 commits into
Conversation
added 6 commits
September 12, 2026 17:46
This release combines two related work streams into a single 0.3.0 release. The commit can be split via 'git rebase -i HEAD~1' if the team prefers two separate PRs (see SPLIT NOTE below). ================================================================ PR #1: Reposition as Rust reference implementation ================================================================ Goal: stop claiming 'production-ready KMS', clearly state what the project is and isn't, and demote deployment assets that implied production-readiness without delivering it. - README.md / README.en.md: rewrite with positioning declaration, what-this-is / what-this-isn't matrix, compliance self-assessment table, applicable / non-applicable scenarios - LEARN.md: new (221 lines), 10-node code walk-through for learners - CHANGELOG.md: add 0.3.0 entry documenting the positioning shift - deploy/kubernetes/: delete (4 files); move basic manifest to examples/k8s-demo/ with explicit 'DEMO ONLY — NOT PRODUCTION' framing - operators/kms-operator/README.md: 'EXPERIMENTAL — NOT MAINTAINED' warning - providers/terraform/README.md: 'EXPERIMENTAL — REFERENCE ONLY' warning - Test counts table corrected to actual (877 → 972 with TLCP additions) ================================================================ PR #2: TLCP (GB/T 38636-2020) REST listener ================================================================ Goal: actually wire gm-tlcp into the REST API, so the project no longer claims TLCP support that doesn't exist. - src/cmd/tlcp_listener.rs: new axum Listener implementing TLCP dual-cert ECDHE handshake via gm_tlcp::TlcpAcceptor (269 lines + tests) - src/cmd/config.rs: extend RestTlsConfig with tlcp_{sign,enc}_{cert,key}_path fields + validate_tlcp_paths() / to_tlcp_acceptor() helpers; wire env var support (REST_TLS_TLCP_*) - src/cmd/server.rs: dispatch backend = 'tlcp' branch; surface config + load errors via tracing::error before returning - src/cmd/mod.rs: register tlcp_listener module - kms.toml.example: document [rest_tls].backend = 'tlcp' with TLCP fields - Cargo.toml: add gm-tlcp = '0.6' dep; bump [patch.crates-io] rev to a7fdf20 (gm-tlcp-v0.6.4 tag) covering gm-ca / gm-crypto / gm-sm9-rs / gm-tlcp / gm-tls together; bump gm-crypto = '0.3' so the patch actually applies (gm-tls 0.2.0's exact dep on gm-crypto 0.2.0 would otherwise bypass the patch and create a dual-source E0308 error) - docs/guides/tlcp-deployment.md: new (271 lines, bilingual), dual-cert generation, kms.toml examples, client verification, known limitations - docs/wiki/tlcp.md: new (164 lines, bilingual), TLCP vs TLS 1.3 protocol comparison, ALPN gap explanation, 4 cipher suites - Tests: 13 new unit tests across tlcp_listener.rs and config.rs covering config validation, cert load error paths, listener trait compatibility Bug fixes caught during self-review (in PR #2 portion): - Pre-existing double-bind bug in src/cmd/server.rs (plain TCP listener bound rest_addr unconditionally, TLS branches then EADDRINUSE-panic). Fixed by moving the plain listener creation into the _ => arm only. - TLCP startup errors were silently swallowed by spawn'd task return value (the JoinHandle was never awaited). Fixed by logging via tracing::error before returning the error from the spawn'd async block. ================================================================ Verification ================================================================ - cargo fmt --all -- --check: pass - cargo check --workspace: pass - cargo clippy --workspace -- -D warnings: pass - cargo test --workspace: 972 passed, 21 ignored, 0 failed - TLS 1.3 + SM end-to-end startup: pass (REST + gRPC both listen) - TLCP startup with bad PEM: pass (clear error logged: 'TLCP startup aborted: failed to load dual-certs from ...') ================================================================ SPLIT NOTE ================================================================ To split this commit into two PRs: git reset --soft HEAD~1 git restore --staged <files> git commit -m 'docs: reposition as Rust reference implementation' git commit -m 'feat: wire TLCP (GB/T 38636-2020) into REST listener' Suggested file groupings for the split: PR #1 (docs + assets): CHANGELOG.md (manually pre-TLCP section) README.md, README.en.md, LEARN.md deploy/kubernetes/* (delete), examples/k8s-demo/* (add) operators/kms-operator/README.md, providers/terraform/README.md PR #2 (TLCP feature): src/cmd/tlcp_listener.rs (new) src/cmd/mod.rs src/cmd/server.rs (TLCP branch + double-bind fix + error logging) src/cmd/config.rs (TLCP fields + helpers) Cargo.toml, Cargo.lock kms.toml.example (TLCP portion of [rest_tls] block) docs/guides/tlcp-deployment.md, docs/wiki/tlcp.md ================================================================ Known limitations (documented in README/CHANGELOG/wiki) ================================================================ - gRPC over TLCP not implemented (TLCP protocol has no ALPN, gRPC needs h2) - TLCP cert chain verification not integrated (waits for upstream gm-tlcp TlcpCertPair to wire cert_verify) - TLCP mTLS (client certs) not implemented yet - SM9 master key still in-memory by default (kms-hsm is a stub) - No bug bounty / commercial support / SLA - 0.3.0 is NOT a 1.0 release candidate
Bumps the [patch.crates-io] pins for the five gm-* crates
(gm-ca, gm-crypto, gm-sm9-rs, gm-tlcp, gm-tls) from rev
a7fdf20 to rev c77f0d7 — the commit that ships the
2026-09-18 cert-verification audit fixes:
NEW-1 gm-ca KeyUsage BIT STRING wrapper (KU extnValue now
parses cleanly via x509-parser 0.16)
NEW-2 gm-crypto verify_against_anchors no longer hardcodes
CertRole::Ca on single-element chains (legitimate TLCP
leaf certs no longer get spuriously rejected)
NEW-3 gm-ca / gm-crypto / gm-tls SM2 OID byte sequences
corrected to the canonical GM/T 38636-2020 §6.4.6
encoding (1.2.156.10197.1.{501,301,106}). Openssl
now parses gm-ca-issued certs as
'Signature Algorithm: SM2-with-SM3' /
'Public Key Algorithm: sm2'.
NEW-4 gm-tlcp test-stale-comment cleanup
Also bumps the gm-ca dev-dependency version constraint from '0.1.0'
to '0.2' so the patch takes effect (the previous version
constraint didn't match the 0.2.x patch).
examples/generate_sm2_certs.rs: updates to the gm-ca 0.2 API
surface (the old sign_csr was replaced by sign_csr_with_profile
which returns (serial_hex, pem_str) instead of (pem, serial)).
Also fixes the inline SM2_PK_OID_BYTES constant to the canonical
DER encoding (the previous non-canonical bytes would have made
the CSR's SPKI OID unparseable by openssl).
examples/generate_sm2_certs.rs's sign_certs comment said the profile "matches gm-ca 0.1.x's sign_csr behavior" — process info about which old API the profile replaced. The technical content (what extensions the default end-entity profile emits) stays. Doc-only commit. No behavior change.
The Aes256GcmDecryptor::decrypt reconstruction of the 12-byte AES-GCM nonce from the stored ciphertext nonce had an out-of-bounds slice index that panicked when ciphertext.nonce.len() was in the range [12, 16). Self-produced ciphertexts store a 16-byte counter so the panic was never triggered internally, but any externally imported / legacy / cross-implementation ciphertext with the standard 12-byte AES-GCM nonce (RFC 5116 §5.2 fixes AES-256-GCM nonce length to N_MIN = N_MAX = 12 octets) would crash the KMS process (DoS). Replace the if-else chain with an exhaustive match that accepts the 12-byte direct form, the 16-byte self-produced counter form, and returns DecryptionFailed for any other length instead of panicking. Adds four regression tests covering: - 12-byte external nonce decryption (the panic case) - 16-byte self-produced nonce truncated to 12 bytes (proves both accepted forms are equivalent) - nonces of length 8 and 15 (both must return DecryptionFailed) Public API, Ciphertext layout, and Error variants are unchanged. Wire format is unchanged for self-produced ciphertexts. Resolves P0-1 from the source-level review (2026-09-22).
…ormat_args
The CI clippy gate runs `cargo clippy --workspace --all-targets -- -D warnings`,
which denies clippy::uninlined_format_args. Use the inline argument form
(`{other}`) instead of positional (`{}`, other).
Verified with stable 1.88 (matches CI toolchain):
- cargo +1.88 clippy --workspace --all-targets -- -D warnings -> clean
- cargo +1.88 fmt --all -- --check -> clean
- cargo +1.88 test -p kms-core --lib algorithms_impl::tests:: -> 9 passed
Folding into the previous commit keeps history linear; PR #42 will
auto-update on push.
…or codes The crypto service ran the keystore operation FIRST and only verified tenant ownership AFTERWARDS. On a cross-tenant request, this leaked a key-enumeration oracle (404 KeyNotFound vs 403 Forbidden are distinguishable to the client) and consumed real HSM/TPM signing quotas on requests the caller was not authorised to make. Reorder the four crypto paths (encrypt, decrypt, sign, verify) in CryptoService to use a new `fetch_owned_key_meta` helper that: - returns KeyNotFound if the key does not exist - returns KeyNotFound if the key exists but is owned by another tenant - logs a server-side warn!() with requester / owner tenant IDs The two failure modes are deliberately conflated to a single KeyNotFound response (HTTP 404 "key not found", key_id not in body) so they are indistinguishable from outside. The pre-check happens BEFORE any cryptographic operation, so a cross-tenant request never spends a signing/decryption/encryption budget. Same pattern (already correct in spirit) is applied to KeyService for consistency: rotate_key, delete_key, get_key, export_key switch their cross-tenant error from Forbidden to KeyNotFound. The post-process API for encoding API metrics (algorithm-aware) is preserved by passing the `meta` obtained in the pre-check down to the metric recording step. Eleven new tests cover: - CryptoService: encrypt/decrypt/sign/verify cross-tenant -> KeyNotFound; non-existent key -> KeyNotFound; cross-tenant and non-existent responses are byte-identical via IntoResponse; keystore.sign is NOT called on cross-tenant (counted via a SignCountingKeystore wrapper). - KeyService: get_key / rotate_key / delete_key cross-tenant -> KeyNotFound; non-existent key -> KeyNotFound. Forbidden remains available for PBAC / policy-denial scenarios that are not about cross-tenant access. Resolves P0-2 from the source-level review (2026-09-22).
EricZHANG1688
changed the title
…fence in depth) PR-1.2 added a pre-check in CryptoService / KeyService so that a cross-tenant request returns KeyNotFound before any cryptographic operation runs. That fix only protects callers that go through the service layer. The KeystoreBackend trait itself accepts a `tenant_id: &str` argument on every sensitive method, but every implementation received it as `_tenant_id` (underscore prefix) and ignored it. Any direct call to the keystore — a new REST/gRPC handler, a CLI tool, an internal task, an ops script — therefore bypassed the tenant check. This change makes the keystore itself enforce ownership, mirroring PR-1.2's conflation: a key that does not exist AND a key that belongs to a different tenant both return `Error::KeyNotFound(key_id)`. Concretely: - New private helper `verify_tenant(key_id, tenant_id) -> Result<()>` in `SoftwareKeystore` and `PostgresKeystore`. Reads metadata, compares `meta.tenant_id` against the caller-supplied `tenant_id`, and returns `KeyNotFound` on miss or mismatch. - All nine sensitive methods now invoke `verify_tenant` as their first line: `encrypt`, `decrypt`, `sign`, `verify`, `rotate_key`, `delete_key`, `export_key_material`, `get_key_material`, `get_key_material_version`. Parameter `_tenant_id` is renamed to `tenant_id` to signal that it is now actually consumed. - `generate_key` / `import_key_material` are intentionally NOT changed: the tenant_id is the creator/importer, so there is nothing to check. - `destroy_key` / `destroy_key_with_proof` / `derive_shared_secret` / the SM2-KEX session helpers are NOT changed: their trait signatures do not currently include `tenant_id`. Extending the trait to cover them is deferred (out of scope for PR-1.3). - Twelve new tests cover the new contract end-to-end on the software backend: nine cross-tenant attempts each return KeyNotFound (sign / decrypt / encrypt / verify / export_key_material / get_key_material / get_key_material_version / rotate_key / delete_key), plus three forward-path tests confirming that the correct tenant still succeeds for Ed25519 sign/verify, SM2 sign/verify and AES-GCM encrypt/decrypt. Postgres backend's behaviour is identical by construction (the same `verify_tenant` helper is used); its integration tests remain `#[ignore]`'d as before since they require a live database. Verification (Rust 1.88 stable): - `cargo +1.88 fmt --all -- --check` clean - `cargo +1.88 clippy --workspace --all-targets -- -D warnings` clean - `cargo +1.88 test --workspace` 998 passed, 25 ignored - `cargo +1.88 test -p kms-keystore --lib` 86 passed, 13 ignored (74 existing + 12 new PR-1.3 tests) - `cargo +1.88 test -p kms-api --lib` 267 passed, 5 ignored (PR-1.2 tests still pass — PR-1.3 is transparent to the service layer) Resolves P0-3 from the source-level review (2026-09-22).
EricZHANG1688
changed the title
…id, tenant_id, version)
PR-1.4 / P0-5. Bind AAD to (key_id, tenant_id, version) so that any tamper
with a persisted ciphertext row — swapping key_id, version, or tenant
context — causes the GCM tag check to fail.
New kms-core/src/aad.rs (Purpose enum, 42-byte v2 wire format with magic
marker + aad_version + purpose tag, three builders: user_data_aad /
kek_wrap_aad / export_wrap_aad). 9 aad.rs unit tests cover determinism,
collision resistance, and purpose-tag separation.
Wire format back-compat: Ciphertext::format_version ∈ {0, 1} keeps the
legacy empty-AAD branch; format_version == 2 uses the bound AAD; unknown
format_version returns Error::InvalidCiphertext (no panic).
Five call sites updated:
- software/mod.rs::encrypt / decrypt (AES-256-GCM + SM4-GCM branches) use
user_data_aad(*key_id, &entry.meta.tenant_id, entry.meta.version /
ciphertext.version). encrypt emits format_version = 2.
- postgres.rs::encrypt_material / decrypt_material now take key_id: &Uuid
and bind the KEK envelope via kek_wrap_aad(*key_id). Four call sites
(load_keys, generate_key, rotate_key encrypted_dek, import_key_material)
updated. crypto_encrypt / crypto_decrypt take tenant_id: &str and bind
user_data_aad. SM2 path unchanged (public-key encryption, no AEAD-AAD).
- key_service.rs::export_key envelope uses export_wrap_aad(*key_id,
tenant_id) instead of Aad::empty(). The ExportWrap purpose tag (0x0003)
differs from UserData (0x0001), preventing an attacker from lifting a
user-data ciphertext and presenting it as an export envelope.
Tests (8 new): AES / SM4 round-trip with bound AAD, AES cross-key replay
rejected (primary defence), SM4 cross-key replay rejected, cross-version
forgery rejected, format_version ∈ {0, 1} back-compat (manual ring seal
with empty AAD), unknown format_version rejected, cross-tenant AAD
divergence verified at the AAD layer (defence in depth on top of the
PR-1.2 / PR-1.3 tenant pre-checks).
Verification (stable 1.88):
- cargo fmt --all -- --check: clean
- cargo clippy --workspace --all-targets -- -D warnings: clean
- cargo test --workspace: 1015 passed (plus 18 ignored), 0 failed.
README + CHANGELOG updated to reflect +9 aad.rs unit tests and +8 PR-1.4
keystore regression tests (total 998 → 1015 passed).
Contributor
Author
PR-1.4 / P0-5 — AAD 绑定（新增提交 aec661c）新提交在原有 PR-1.1/1.2/1.3 基础上叠加了 PR-1.4 / P0-5（AAD 绑定）。 改动概要
向后兼容
测试（新增 8 个）
验证（stable 1.88）
测试计数更新
纵深防御与 PR-1.2（service-layer tenant pre-check）+ PR-1.3（keystore-layer tenant verify）形成三层防御。 |
added 8 commits
September 23, 2026 09:04
… (PR-4.1 / P1-3)
Pre-PR-4.1 the KMS gRPC + REST listeners emitted only a tracing::warn!
when started without TLS. API keys + signed payloads transited in
the clear, which is unacceptable for production deployments.
PR-4.1 mirrors the KEK fail-fast pattern at
kms-keystore/src/postgres.rs:92-95: bail!() the startup unless the
operator explicitly opts in via KMS_ALLOW_INSECURE=1 (production
white-list) or KMS_DEV_MODE=1 (test / embedded-integration opt-in).
New kms-core::production_safety module exposes three helpers
(is_allow_insecure / is_dev_mode / is_insecure_opted_in). Both
env vars are read with strict '1' semantics — 'true' / 'yes' / 'on'
do NOT trigger the white-list.
Two pub(crate) helpers (grpc_tls_failfast_message / rest_tls_failfast_message)
extract the fail-fast decision into unit-testable functions instead
of leaving it inline in cmd::server::run, which has too much
network / keystore setup for end-to-end integration testing.
12 new unit tests cover all branches:
- kms-core/production_safety.rs (5 tests): env-var semantics
- src/cmd/server.rs::pr41_failfast_tests (7 tests): gRPC +
REST fail-fast + opt-in + message-distinctness invariant
docs/requirements/N2-tls-failfast.md adds a bilingual requirement
spec (Chinese + English) and is registered in the requirements README.
PR-4.1 made the KMS gRPC/REST listeners refuse to start without TLS unless KMS_ALLOW_INSECURE=1 (or KMS_DEV_MODE=1) is set. The ZAP baseline scan runs against a plaintext localhost listener (intentional, baseline scans run over HTTP for reproducibility), so the test container must opt in explicitly via KMS_ALLOW_INSECURE. Without this env var, the kms-target container exits during startup with 'KMS gRPC requires TLS in production', the localhost:8080/healthz probe never succeeds, and the spider fails with DNS-resolution errors.
… / P1-4)
Pre-PR-4.4 audit of BackendTlsConfig::from_env found three
production-safety gaps:
1. KMS_DB_TLS_MODE=no_verify was accepted unconditionally in
production. Without server-cert validation, TLS encrypts
traffic but not the channel — a MITM attacker can present
any certificate.
2. KMS_DB_TLS_MODE=disabled in non-dev mode was silently
demoted to VerifyCa. Operators who thought they had
Disabled were actually running with VerifyCa; if KMS_DB_TLS_CA_CERT
was also missing, the connection would fail at first
handshake instead of at startup.
3. VerifyCa without KMS_DB_TLS_CA_CERT was accepted by
from_env() (returning a VerifyCa config with no cert path)
and only failed at first database connection.
PR-4.4 closes the gaps with three fail-fast gates, mirroring
the KEK pattern at kms-keystore/src/postgres.rs:92-95 and the
PR-4.1 KMS_API listener pattern:
- no_verify in production -> anyhow::bail! with explicit
'set KMS_ALLOW_INSECURE=1' guidance.
- disabled in production -> anyhow::bail! with the same
guidance.
- verify_ca without non-empty KMS_DB_TLS_CA_CERT -> anyhow::bail!.
- Each is reachable only via KMS_DEV_MODE=1 (test /
embedded-integration) or KMS_ALLOW_INSECURE=1 (explicit
production opt-in); both flags read through the shared
kms_core::production_safety::is_insecure_opted_in() helper
from PR-4.1.
The four callers (cmd/server.rs rate-limiter, quota-tracker,
mfa-pool paths, kms-keystore/src/repository.rs Postgres pool)
unwrap the new Result<_, anyhow::Error> with std::process::exit(1)
to enforce the fail-fast contract at startup.
14 new unit tests under pr44_db_tls_defaults_tests cover all
branches (default unset, verify_ca with/without cert, disabled
in dev/prod, no_verify in prod/with-opt-in, case-insensitive,
empty-string handling). Existing 296 kms-core + 94
kms-keystore unit tests all pass.
docs/requirements/N3-db-redis-tls-defaults.md registers the
requirement spec.
…_args (PR-4.4 follow-up)
CI's stable Rust 1.88 toolchain enforces clippy::uninlined_format_args
as an error; local nightly clippy doesn't. Three sites failed CI:
- crates/kms-core/src/tls_config.rs:397 ('got: {}' -> 'got: {err}')
- crates/kms-core/src/tls_config.rs:586 ('panic!("...", val, e)' -> 'panic!("...", {val:?}, {e})')
- crates/kms-keystore/src/repository.rs:98 ('fail-fast: {}' -> 'fail-fast: {e}')
Inlined the format args to satisfy stable. No behavior change;
purely cosmetic adjustments to the format macro calls.
…low-up) PR-4.4 made BackendTlsConfig::from_env fail-fast on production unsafe DB / Redis TLS configs (verify_ca without CA cert, no_verify / disabled in production). The ZAP baseline scan container runs against kms-docker.toml which has redis=false, rate_limit=false, quota=false, no DATABASE_URL — DB / Redis TLS is not actually used, but cmd/server.rs eagerly calls BackendTlsConfig::from_env() from maybe_create_mfa_pool / maybe_create_redis_rate_limiter / maybe_create_quota_tracker, so the check still fires. Set KMS_DB_TLS_MODE=disabled + KMS_ALLOW_INSECURE=1 on the ZAP target container so the in-memory fixture opts into plaintext DB listeners (mirroring the listener-side opt-in from PR-4.1).
…ilent empty material (PR-4.5 / P1-5) Pre-PR-4.5 audit found that SoftwareKeystore::generate_key for KeySpec::Sm9Signing | Sm9Encryption returned Ok(KeyMeta) with material: Vec::new(). Every subsequent sign / encrypt / decrypt call would fail at runtime (empty material is unusable for any crypto op), but the API surface reported key creation success. PR-4.5 replaces the Vec::new() with Err(Error::NotImplemented) and an explicit text mentioning the KMS-SM9 master-key management infrastructure that would be required for actual SM9 user-key derivation (currently not integrated; the gm-kms repo has kms-core/src/sm9_master_key.rs but no end-to-end HSM / KMS-SM9 master-key management wired up). Mirrors the existing Rsa4096 branch in generate_key (line 700-704 of software/mod.rs). The rotate path at line 1253-1262 already returns Error::KeyOperationNotAllowed with a Sm9RotationAdapter hint and remains unchanged. 4 new unit tests under pr45_sm9_generate_tests lock in: - SM9 signing returns NotImplemented (error mentions SM9). - SM9 encryption returns NotImplemented (error mentions SM9). - RSA-4096 still returns NotImplemented (regression guard). - SM2 still works and material is non-empty (regression guard). The pre-existing kms-api/src/rotation.rs::test_sm9_direct_keystore_rotation_errors relied on the old buggy behavior (it called generate_key(Sm9Signing) which used to return Ok, then asserted rotate_key errored). Post-PR-4.5 that path is unreachable from the public API since SM9 generation fails first. The test is replaced with a documentation comment explaining the invariant; the rotation KeyOperationNotAllowed path in software/mod.rs:1253-1262 stays in place for legacy/migration cases but cannot be triggered via SoftwareKeystore's public API. docs/requirements/N4-sm9-generate-not-implemented.md registers the requirement spec.
…阶段 1/3)
Pre-PR-4.7: the master KEK was only loaded from the KMS_KEK env
var (plaintext in process environment, visible in /proc/<pid>/environ).
Two call sites (kms-keystore::postgres and kms-api::mfa) each
implemented their own inline hex-parsing logic, with copy-paste
duplication.
PR-4.7 introduces kms_core::kek_source::KekSource, a small enum
that supports three sources:
- Env: KMS_KEK hex string (existing behavior, preserved)
- File(PathBuf): KMS_KEK_FILE path to a 0600-mode file containing
64 hex characters; mode bit is enforced on Unix and any
non-0600 mode is rejected with InsecureFileMode
- Missing: neither set (caller decides DEV-mode random or
fail-hard, preserving existing behavior)
When both KMS_KEK and KMS_KEK_FILE are set, File wins — the file
channel is more secure because OS file permissions are enforced.
A whitespace-only path falls through to KMS_KEK (catches the
common 'KMS_KEK_FILE=""' / unset-but-present bug).
PR-4.7 is phase 1/3 of master plan §6/§7 P1-7:
- Phase 2 (PR-4.8): HSM/TPM provider via kms-hsm
- Phase 3 (PR-4.9): KEK rotation (kek_label persistence +
multi-active-KEK)
Changes:
- crates/kms-core/src/kek_source.rs (new): KekSource enum,
KekSourceError enum, from_env() / load() factory and resolver,
17 unit tests in pr47_kek_source_tests covering env/file/missing
resolution, hex parsing (valid/invalid/wrong-length/whitespace),
file-mode enforcement (0600/0644/0666), file-not-found, empty
file, and error-message content
- crates/kms-core/src/lib.rs: add 'pub mod kek_source'
- crates/kms-keystore/src/postgres.rs: replace inline hex parsing
with KekSource::from_env().load(); DEV-mode random + fail-hard
behavior preserved
- crates/kms-api/src/mfa.rs: replace inline hex parsing with
KekSource::from_env().load(); plaintext-with-warning fallback
preserved
- Cargo.toml: workspace version 0.2.1 -> 0.2.2 (patch bump; new
pub API surface is additive)
- CHANGELOG.md: document the addition
- docs/requirements/N5-kek-source-layering.md (new): bilingual
requirement spec
- docs/requirements/README.md: index the new requirement
…P2-6)
Pre-PR-4.11: the HMAC signing key file for WORM-backed signed
audit logs was a sibling of the WORM log file (path computed via
`worm_path.with_extension("signing_key")`). A compromised
process could replace BOTH the log AND the key, defeating WORM's
append-only integrity guarantee. PR-4.11 introduces an opt-in
way for operators to deploy the signing key at a path SEPARATE
from the WORM log directory (different filesystem / mount /
account recommended).
Changes:
- crates/kms-audit/src/worm_logger.rs:
- `WormSignedAuditConfig` gains `signing_key_path:
Option<PathBuf>` field (`None` = pre-PR-4.11 sibling
behavior, byte-identical backward compat).
- `WormSignedAuditConfig::with_signing_key_path(PathBuf)`
builder.
- `WormSignedAuditConfig::load_or_create_with_key_path(
worm_path, key_path, seq)` factory (writes / loads the key
at the operator-supplied path).
- `WormSignedAuditConfig::effective_signing_key_path()` —
SINGLE source of truth for the resolved key path.
- Renamed internal `signing_key_path` helper to
`default_signing_key_path` to reflect its now-default role
(kept `pub` for tests; new public methods cover real usage).
- New `pr411_signing_key_isolation_tests` module: 6 tests
covering default-behavior preservation, custom-path override,
0600 enforcement on Unix, and existing-key loading at the
custom path.
- Cargo.toml: workspace version 0.2.2 -> 0.2.3 (patch; additive
opt-in feature, no breaking changes).
- CHANGELOG.md: document the addition.
- docs/requirements/N6-worm-hmac-key-isolation.md (new):
bilingual requirements document.
- docs/requirements/README.md: add N6 row to the index.
KEK integration (wrapping the HMAC key with the master KEK) is
out of scope for PR-4.11; deferred to PR-4.12 to keep this PR
within kms-audit crate only.
EricZHANG1688
force-pushed
the
release/0.3.0
branch
from
September 23, 2026 12:56
cc92210 to
74390b7
Compare
… / P2-9)
Pre-PR-4.14: the software keystore's `generate_sm2_key()` did:
```
let mut key = vec![0u8; 32];
rand::rng().fill_bytes(&mut key);
key
```
This produces 32 random bytes without enforcing the
GB/T 32918.1-2016 §5.1.4 invariant that an SM2 private
scalar `d` must be in `[1, n-1]` where `n` is the SM2
curve order. The probability of a bare 32-byte sample
falling outside this range is negligible (~ 2^-128), but the
absence of an explicit range check means:
- any future upstream regression in scalar generation
would be silently accepted; and
- the boundary case `d == 0` would yield an invalid
public key (point at infinity) that passes through to
the SQL backend undetected.
The master plan identified this as P2-9
(gm与gm-kms源码级改进建议 §四 P2-9): 建议统一走
EricZHANG1688
force-pushed
the
release/0.3.0
branch
from
September 23, 2026 14:42
2a15b44 to
95f9b8c
Compare
added 2 commits
September 23, 2026 23:22
Pre-PR-4.15: `PostgresKeystore.keys` was a single `Arc<RwLock<HashMap<Uuid, KeyEntry>>>` with no capacity cap, no eviction policy, and no metrics. Every key ever generated stayed resident until process exit, exposing two production risks: 1. Unbounded memory growth (10K keys ~ hundreds of MB). 2. Cold keys compete with hot keys for cache space. The master plan identified this as P2-10 (gm与gm-kms源码级改进建议 §四 P2-10): 建议加惰性加载与
PR-4.15 introduced `with_in_memory_cap(n)` to bound
memory usage, but left a latent bug: keys beyond position
`n` were silently unreachable because `verify_tenant`
returned `KeyNotFound` on cache miss without trying the
DB. Production deployments with 1000 keys and cap=100
effectively rendered the last 900 keys unusable.
PR-4.17 closes the gap (per PR-4.15's SPEC which listed
this as follow-up):
- New inherent method `PostgresKeystore::load_entry_from_db`
that fetches metadata + encrypted material from
PostgreSQL and decrypts with the KEK. Returns
`Error::KeyNotFound` for absent keys, `Error::Internal`
for KEK-rotation failures (loud error so operators
notice) or pre-PR-2.x keys that lack encrypted
material.
- `verify_tenant` cache-miss path now calls
`load_entry_from_db` instead of returning
`KeyNotFound` outright. On success, the entry is
inserted via `BoundedKeyCache::insert_with_eviction`
(FIFO cap honoured automatically) and a
`tracing::debug!` records the lazy load so operators
can correlate cache misses in production logs.
- Tenant isolation (PR-1.2 conflation) preserved: lazy
load + wrong tenant still returns `KeyNotFound`.
- 4 new tests: 3 live-DB `#[ignore]` integration tests
covering cap-beyond, unknown-id, wrong-tenant; 1 unit
smoke test verifying the helper is reachable via the
inherent impl block (no public API change).
- Note: PR-4.17 SPEC considered adding a Prometheus
counter (`kms_keystore_lazy_loads_total`), but
decided against it to keep `kms-keystore` from
depending on the `metrics` crate (currently a
`kms-api` dependency). Operators rely on
`tracing::debug` log scraping instead.
Changes:
- crates/kms-keystore/src/postgres.rs:
+ `load_entry_from_db` (40 lines, KEK-aware)
+ `verify_tenant` cache-miss path rewritten
+ 4 tests added (3 #[ignore] live + 1 unit)
- Cargo.toml: 0.2.5 -> 0.2.6 (patch; no API change).
- Cargo.lock: workspace version bump.
- CHANGELOG.md: document the change.
- docs/requirements/N9-keystore-lazy-load.md (new):
bilingual SPEC.
EricZHANG1688
force-pushed
the
release/0.3.0
branch
from
September 23, 2026 16:52
2d21d99 to
e6c7cb1
Compare
added 2 commits
September 24, 2026 01:56
PR-4.17 made cap-beyond keys lazy-loadable, but the
eager `load_keys()` path still blocked server startup
by ~5–10s for 10k keys (O(N) DB queries + KEK
decrypt × N). PR-4.19 fixes the blocking half:
- New `PostgresKeystore::spawn_load_keys()` returns
a `tokio::task::JoinHandle<Result<usize>>` so
callers can continue with TCP bind / gRPC start
without waiting for the DB. The returned
`Result<usize>` carries the count of keys
successfully inserted into the bounded cache.
- Server startup paths in `src/cmd/server.rs`
(`create_software_keystore`,
`create_software_keystore_inner`) switched from
`await load_keys()` to the new background
preload. Pre-PR-4.19 the listen port bound after
the DB round-trip; post-PR-4.19 it binds
immediately.
- Internal refactor: extracted
`decrypt_material_static(kek, key_id, encrypted)`
from `decrypt_material` so the spawned task can
decrypt without a `&self` borrow. Added shared
`load_one_into_cache` helper used by both
sync (`load_keys`) and async (`spawn_load_keys`)
paths.
- `PostgresKeyRepository` now derives `Clone`
(sqlx::PgPool is Arc-backed; zero-cost clone).
- 4 new tests: 3 unit tests (helper type shape,
too-short error path parity, spawn signature) +
1 `#[ignore]` live-DB integration test
(pr419_load_keys_and_spawn_have_same_outcome).
- PR-4.17's lazy load is unchanged; it still
covers cache misses during the pre-preload
window (rare in practice, important for tests).
Changes:
- crates/kms-keystore/src/postgres.rs:
+ `spawn_load_keys` (~70 lines)
+ `decrypt_material_static` (~50 lines)
+ `load_one_into_cache` shared inner helper
+ 4 tests
- crates/kms-keystore/src/repository.rs:
`PostgresKeyRepository` now `#[derive(Clone)]`.
- src/cmd/server.rs: 2 caller sites switched to
background preload.
- Cargo.toml: 0.2.6 -> 0.2.7 (patch; new API).
- Cargo.lock: workspace version bump.
- CHANGELOG.md: document the change.
- docs/requirements/N10-keystore-background-load.md
(new): bilingual SPEC.
- '[0.2.1] — Unreleased' -> '[0.2.1] — 2026-09-05' (v0.2.1 tag was actually cut on 2026-09-05 at commit 84098d3, not pending) - duplicate '[0.1.0] — 2026-06-29' section renamed to '[Pre-0.1.0 development] — 2026-06-29' with explicit audit-trail disclaimer: pre-release development snapshots folded into 0.1.0, predating the v0.1.0 tag (commit 18b32a3, 2026-08-27); NOT part of any published release - '[0.1.0] — 2026-08-27' marked as 'Initial public release' for clarity No source code changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR bundles three P0 security fixes from the 2026-09-22 source-level review of
gmandgm-kms. Each fix is one atomic commit; all targetrelease/0.3.0 → main.gm-kms/crates/kms-coregm-kms/crates/kms-api{"error":"key not found"}, no body change)gm-kms/crates/kms-keystoreAll fixes are backward-compatible at the wire format / public API level.
P0-1: AES-256-GCM 12-byte nonce panic
Aes256GcmDecryptor::decryptreconstructed the 12-byte AES-GCM nonce from the stored ciphertext nonce with an out-of-bounds slice index that panicked whenciphertext.nonce.len()was in the range[12, 16). Self-produced ciphertexts store a 16-byte counter so the panic was never triggered internally, but any externally imported / legacy / cross-implementation ciphertext with the standard 12-byte AES-GCM nonce (RFC 5116 §5.2 fixes N_MIN = N_MAX = 12 octets for AES-256-GCM) would crash the KMS process.Replaced the if-else chain with an exhaustive
matchaccepting both forms (12-byte direct, 16-byte counter via[4..16]), returningDecryptionFailedfor any other length instead of panicking.Tests:
test_aes256gcm_decrypt_12byte_nonce_no_panic,test_aes256gcm_decrypt_truncated_native_nonce_to_12bytes_succeeds,test_aes256gcm_decrypt_invalid_nonce_length_{8,15}_returns_error.Commit:
6b6fd56(+b5e0118for clippy nit).P0-2: Tenant pre-check + error-code unification (service layer)
CryptoService::encrypt/decrypt/sign/verifycalled the keystore FIRST and only verified tenant ownership AFTERWARDS. Three concrete harms:KeyNotFoundvs HTTP 403Forbiddenwere distinguishable to the client, leaking which key_ids exist in the systemFix: introduce private
fetch_owned_key_metahelper that returnsKeyNotFoundfor both "missing" and "wrong tenant" (with server-sidetracing::warn!for SOC visibility). The pre-check happens BEFORE any cryptographic operation. TheForbiddenvariant remains available for PBAC policy-denial scenarios.For consistency,
KeyService::rotate_key,delete_key,get_key,export_keyalso switch their cross-tenant error fromForbiddentoKeyNotFound(they already did the pre-check correctly; only the error code was wrong).Tests (all pass with
cargo +1.88 test --workspace --lib):test_pr12_{encrypt,decrypt,sign,verify}_cross_tenant_returns_keynotfoundtest_pr12_sign_nonexistent_key_returns_keynotfoundtest_pr12_cross_tenant_and_nonexistent_have_identical_responses— bytes match via axumto_bytestest_pr12_sign_does_not_call_keystore_on_tenant_mismatch— uses a customSignCountingKeystorewrapper; cross-tenant shows 0signinvocationstest_pr12_key_{get,rotate,delete}_cross_tenant_returns_keynotfoundtest_pr12_key_get_nonexistent_returns_keynotfoundCommit:
c66a127.P0-3: Keystore-layer tenant enforcement (defence in depth)
The
KeystoreBackendtrait itself accepts atenant_id: &strargument on every sensitive method (encrypt,decrypt,sign,verify,rotate_key,delete_key,export_key_material,get_key_material,get_key_material_version), but every implementation received it as_tenant_id(underscore prefix) and ignored it. P0-2 only protects callers that go through the service layer — a new REST/gRPC handler, a CLI tool, an internal task, an ops script that called the keystore directly would still bypass the tenant check.Fix: a private
verify_tenant(key_id, tenant_id) -> Result<()>helper inSoftwareKeystoreandPostgresKeystorereads metadata, comparesmeta.tenant_idagainst the caller-suppliedtenant_id, and returnsKeyNotFoundon miss or mismatch (conflating the two outcomes, mirroring P0-2's service-layer behaviour). Every sensitive method callsverify_tenantas its first line. Parameter_tenant_idis renamed totenant_idto signal that it is now actually consumed.Out of scope (deferred):
generate_key/import_key_material—tenant_idis the creator/importer, nothing to check.destroy_key/destroy_key_with_proof/derive_shared_secret/ SM2-KEX session helpers — their trait signatures do not currently includetenant_id; extending the trait is a separate concern.Tests (
cargo +1.88 test -p kms-keystore --lib, 86 passed / 13 ignored):Cross-tenant →
KeyNotFound:test_pr13_sign_cross_tenant_returns_keynotfoundtest_pr13_decrypt_cross_tenant_returns_keynotfoundtest_pr13_encrypt_cross_tenant_returns_keynotfoundtest_pr13_verify_cross_tenant_returns_keynotfoundtest_pr13_export_key_material_cross_tenant_returns_keynotfoundtest_pr13_get_key_material_cross_tenant_returns_keynotfoundtest_pr13_get_key_material_version_cross_tenant_returns_keynotfoundtest_pr13_rotate_key_cross_tenant_returns_keynotfoundtest_pr13_delete_key_cross_tenant_returns_keynotfoundForward path (correct tenant still succeeds):
test_pr13_sign_correct_tenant_succeeds(Ed25519)test_pr13_sm2_sign_correct_tenant_succeeds(SM2)test_pr13_encrypt_decrypt_correct_tenant_succeeds(AES-256-GCM)Commit:
7cf4070.Verification
cargo +1.88 fmt --all -- --checkcargo +1.88 clippy --workspace --all-targets -- -D warningscargo +1.88 test --workspacecargo +1.88 test -p kms-api --lib service::crypto_service::tests::cargo +1.88 test -p kms-api --lib service::key_service::tests::cargo +1.88 test -p kms-keystore --libfail_secure_tests.rsFiles changed
crates/kms-core/src/algorithms_impl.rs(PR-1.1 fix + 4 tests)crates/kms-api/src/service/crypto_service.rs(PR-1.2 fix + helper + 7 tests)crates/kms-api/src/service/key_service.rs(PR-1.2 error-code unification + 4 tests)crates/kms-keystore/src/software/mod.rs(PR-1.3 fix +verify_tenanthelper)crates/kms-keystore/src/postgres.rs(PR-1.3 fix +verify_tenanthelper)crates/kms-keystore/src/software/tests.rs(PR-1.3 tests, 12 new)CHANGELOG.md(entries for P0-1, P0-2, P0-3)README.md/README.en.md(test-count refresh)Standards
Closes P0-1, P0-2 and P0-3 of the 2026-09-22 source-level review.