feat: support X25519 ECDH - UI - #1171
Open
Manuthor wants to merge 14 commits into
Open
Conversation
Adds a Web UI form for the new asymmetric (X25519 ECDH) DeriveKey capability introduced on the x25519_ecdh backend branch: - New WASM export derive_key_asymmetric_ttlv_request (+ matching response parser), wrapping DeriveKey::new_asymmetric for browser consumers. - New 'X25519 ECDH' derivation method on the Derive Key page, with private-key / peer-public-key selectors built on the existing key-picker component; output is always a fixed 256-bit SecretData object (server-enforced), so the method requires only Derive Key permission on both referenced keys, not a cryptographic usage mask. - en/zh-CN i18n strings for the new form. - Playwright e2e coverage for the new flow (FIPS-mode skipped, since X25519 is non-FIPS only), and an extended EC-key-pair test helper supporting an explicit curve parameter.
Adds an --x25519/--private-key-id/--peer-public-key-id CLI counterpart to the Web UI's asymmetric X25519 ECDH derivation form, per the cli-ui-sync convention (every UI capability needs a ckms subcommand). - New DeriveKeyAction fields (non-fips gated): x25519, private_key_id, peer_public_key_id. salt becomes Option<String> (unused/optional for X25519, still required for PBKDF2/HKDF, validated in run()). - New run_x25519() builds a DeriveKey::new_asymmetric request directly, supporting the optional --derived-key-id output UID (the existing client helper derive_x25519_secret doesn't expose this parameter. - clap constraints: --x25519 requires --private-key-id and --peer-public-key-id (requires_all on the x25519 field itself); all three are mutually exclusive with --key-id/--password. - Update existing derive-key CLI tests for the new struct shape, add test_derive_key_x25519 (happy path + missing-required-arg error), and fix 7 pre-existing test assertions that predated derived_key_id support: the server honors a client-supplied unique_identifier for derived objects (both symmetric and asymmetric paths), so asserting a server-generated derived-* prefix was already incorrect. Also fixes two pre-existing docs-tooling bugs that blocked pre-commit on any change touching clap-derived structs: - docs:generate mise task / smart pre-commit wrapper: a prior commit removed .mise/scripts/audit/crypto_sensor.sh but left generate_docs.sh's step 4 (crypto inventory) calling it unconditionally. Expose the script's existing --skip-crypto flag through the mise task and pass it from the smart regen wrapper, mirroring the existing --skip-cbom handling. - renew_ckms_markdown.sh: the generator emits a trailing space for subcommands with an empty help string (e.g. ), which fought with the trailing-whitespace pre-commit hook in an infinite modify/fix loop. Strip trailing whitespace from the generated file, mirroring the existing usage.md post-processing. Regenerated all auto-derived documentation (ckms CLI reference, server CLI/config reference, KMIP support tables) to bring it back in sync with the current codebase, as required by the docs-regen pre-commit hook. EOF )
Honor explicit HKDF context and digest defaults, preserve sensitive attributes for inline SecretData, and add the X25519 DeriveKey vector.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
The --ceremony flag on `ckms sym keys create-split-key` is a valueless boolean switch (clap bool flag), not a flag that takes a true/false argument. `--ceremony true` fails to parse (true is read as an unexpected positional argument). Document the bare --ceremony flag.
GET /ocsp/ (bare path, no request) 404s: the registered route is
GET /ocsp/{encoded_request}, requiring the base64url-encoded DER OCSP
request in the path (RFC 6960 A.1).
…rity The Derive Key form offered X25519 ECDH unconditionally, but the server rejects derive_key_asymmetric with NotSupported in FIPS mode, directing users toward a request that can never succeed. Query /server-info once and disable the X25519 radio option (with a "Non-FIPS only" tag) when the server runs in FIPS mode; also flag the limitation in the intro copy. Also brings French to full parity with English/Chinese for the deriveKey namespace: the fr bundle was missing all X25519 ECDH strings (methodCard, x25519Card, x25519Intro, private/peer key labels, and the new nonFipsOnly key), so French users saw an untranslated fallback for the entire X25519 section. Chinese was also missing nonFipsOnly. Excludes ui/src/i18n/locales/fr/ from the typos pre-commit hook: the French locale bundles contain hundreds of pre-existing words the English-dictionary spell-checker misflags (objet, raison, contient, ...), and --write-changes was silently corrupting valid French text into English on any commit that staged these files. This mirrors the existing exclude-list pattern already used for other non-English/ non-checkable paths in this repo.
The PR cascade guidance told agents to use gh stack sync to update already-pushed CLI/UI branches after an earlier PR merges, but that tool rebases and force-pushes (--force-with-lease) downstream branches -- directly violating the absolute force-push prohibition elsewhere in the same document. Replace the gh stack sync reference with an explicit merge-based update flow (git merge origin/<upstream> + a regular fast-forward push) that keeps branch history append-only.
Pulls in the 4 review-comment/CI fixes just pushed to the server PR (#1170) base branch: doc fixes, CHANGELOG fix, the asymmetric DeriveKey object_type validation tightening, and the caller-supplied derived_key_id test fixes. Resolves the derive_key_tests.rs conflict by keeping this branch's X25519 CLI test additions alongside the upstream exact-match assertion fix.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are concrete correctness/testability issues (FIPS fallback behavior and brittle placeholder-based Playwright selectors for the new X25519 flow) plus notable scope drift with unrelated CRL/OCSP config changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the client-side DeriveKey feature set by adding an X25519 ECDH (non-FIPS) derivation mode across the Web UI, WASM bindings, and ckms CLI, along with updated Playwright/CLI tests and regenerated documentation.
Changes:
- Add X25519 ECDH mode to the Derive Key UI and Playwright E2E coverage (with FIPS-mode gating).
- Add CLI + WASM support for asymmetric
DeriveKeyrequests/responses (X25519), plus ckms integration tests. - Update generated docs/config templates and KMIP support tables (notably including new
[crl]/[ocsp]sections).
File summaries
| File | Description |
|---|---|
| ui/tests/e2e/README.md | Document the expanded DeriveKey E2E flow to include X25519 ECDH. |
| ui/tests/e2e/helpers.ts | Generalize EC key-pair creation helper to allow selecting X25519. |
| ui/tests/e2e/derive-key-flow.spec.ts | Add Playwright coverage for X25519 ECDH derivation. |
| ui/src/utils/utils.ts | Formatting-only change to docsUrl. |
| ui/src/i18n/locales/zh-CN/actions.json | Add X25519 DeriveKey UI strings (zh-CN). |
| ui/src/i18n/locales/fr/actions.json | Add X25519 DeriveKey UI strings (fr). |
| ui/src/i18n/locales/en/actions.json | Add X25519 DeriveKey UI strings (en). |
| ui/src/actions/Keys/DeriveKey.tsx | Add X25519 mode to DeriveKey UI + FIPS gating via /server-info. |
| README.md | Update KMIP support matrices in the root README. |
| pkg/kms.toml | Add [crl] / [ocsp] configuration blocks (scope expansion beyond X25519). |
| documentation/docs/kms_clients/cli/main_commands.md | Regenerated CLI docs reflecting new ckms flags/subcommands. |
| documentation/docs/kmip_support/support.md | Update KMIP support tables in the docs site. |
| documentation/docs/configuration/server_configuration_file.md | Document new server config sections and adjust Crypto Officer wording. |
| documentation/docs/configuration/server_cli.md | Document new CLI flags (CRL/OCSP) and adjust Crypto Officer wording. |
| crate/server/kms_template.toml | Add [crl] / [ocsp] sections to the server template. |
| crate/clients/wasm/src/wasm.rs | Add WASM export/parser for asymmetric DeriveKey (X25519). |
| crate/clients/clap/src/actions/secret_data/create_secret.rs | Respect --sensitive when creating SecretData from inline value. |
| crate/clients/clap/src/actions/derive_key/mod.rs | Add --x25519 asymmetric DeriveKey path + --info for HKDF context. |
| crate/clients/ckms/src/tests/secret_data/mod.rs | Register new secret_data lifecycle tests. |
| crate/clients/ckms/src/tests/secret_data/lifecycle.rs | Add test ensuring sensitive inline SecretData blocks export. |
| crate/clients/ckms/src/tests/derive_key/derive_key_tests.rs | Add/adjust ckms derive-key tests including X25519 coverage. |
| CHANGELOG/x25519_ecdh_ui.md | Add branch changelog entry for the feature and tests. |
| AGENTS.md | Add/clarify workflow rules (PR cascade + force-push prohibition). |
| .pre-commit-config.yaml | Expand typos exclude list to skip French locale directory. |
| .mise/tasks/docs/generate | Add --skip-crypto passthrough flag. |
| .mise/scripts/pre-commit/smart-docs-regen.sh | Use --skip-crypto during smart docs regen. |
| .mise/scripts/docs/renew_ckms_markdown.sh | Strip trailing whitespace after docs regeneration. |
| .github/copilot-instructions.md | Add PR cascade + force-push prohibition guidance. |
Review details
Suppressed comments (1)
ui/src/actions/Keys/DeriveKey.tsx:232
- The X25519 custom output ID field is currently only identifiable via its placeholder text. Add a
data-testidso E2E tests (and future automation) don’t depend on English copy.
<Form.Item name="derivedKeyId" label={t("deriveKey.derivedKeyId")} help={t("deriveKey.derivedKeyIdHelp")}>
<Input placeholder={t("deriveKey.derivedKeyIdPlaceholder")} />
</Form.Item>
- Files reviewed: 28/28 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…racies
- OCSP: correct endpoint path (GET /ocsp/{encoded_request} + POST /ocsp/),
clarify delegated signing key is not routed through crypto_oracles (HSM
non-extractable keys fail), and clarify --ocsp-include-cert-chain only
embeds the signer cert, not the full chain.
- CRL: clarify crl_default_validity_days is in whole days (min 24h) and can
conflict with the default 24h refresh-overlap; clarify the refresh
scheduler requires kms_public_url to be set to run at all.
- ckms: clarify --ceremony / --total-parts fallback behavior for callers who
are not eligible Crypto Officer candidates.
- KMIP support tables (support.md + README.md): Create Split Key is KMIP 1.3+
(not 1.2); ECDH Private/Public Key is decodable in KMIP 1.4 (aliased to
TransparentECPrivateKey).
All doc fixes are sourced from the corresponding clap doc comments and
regenerated via 'mise run docs:generate server-docs' to keep server_cli.md,
server_configuration_file.md, kms_template.toml and pkg/kms.toml in sync.
…for X25519 DeriveKey - DeriveKey.tsx: default isFips=true (and re-set true in the /server-info catch path) so a failed FIPS-mode probe fails closed (X25519 option stays hidden/disabled) instead of silently allowing a request the server will reject in FIPS mode. - Add stable data-testid props (x25519-private-key-id, x25519-peer-public-key-id, x25519-derived-key-id) to the X25519 form fields so E2E tests don't depend on translated placeholder text. - derive-key-flow.spec.ts: switch the X25519 E2E scenarios to page.getByTestId(...) selectors per the repo's Playwright convention (data-testid over placeholder text).
- Add ocsp_ca_uid (required when ocsp_enabled=true) and ocsp_responder_cert_uid as commented-out example fields in kms_template.toml, since they were entirely absent from the generated example config, silently omitting a field that is mandatory to actually enable the OCSP responder. - crl_default_validity_days: clarify that the documented 1-365 range is enforced by clap only for CLI/env input, and is not currently re-validated when the value comes from a TOML config file. - Regenerated server_cli.md, server_configuration_file.md, pkg/kms.toml from the updated source doc comments.
…y_days doc CrlConfig has no clap env bindings, so the previous fix's wording implying an environment-variable path is also validated was itself inaccurate — only the CLI flag is range-validated.
…a CLI The clap field is a bare boolean switch (ArgAction::SetTrue), so --ocsp-include-cert-chain can only ever set the value to true (already the default); there is no CLI syntax to pass false. Clarified that disabling it requires the TOML config file instead.
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
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.
Add X25519 ECDH key derivation to
DeriveKeyFormUI and CLIDeriveKeyFormReact component that collects private-key and peer-public-key identifiers and sends aderive_key_asymmetricKMIP request. X25519 is disabled when FIPS mode is assumed or reported by/server-info.--x25519,--private-key-id, and--peer-public-key-idflags toDeriveKeyActionin the CLI, with symmetric salt made optional at parse time (still required at runtime for PBKDF2/HKDF). Symmetric HKDF now uses caller-supplied--infobytes instead of a generated timestamp/UUID context.createEcKeyPairhelper to accept an optional curve argument.CreateSecretDataAction.DeriveKeyAction.build_derivation_paramsreplaces the auto-generated HKDF context with--info(default empty); existing callers relying on the old generated context will see different derivation outputs.Macroscope summarized c9533bd.