feat(sse): enable local KMS evaluation workflows - #189
Conversation
cxymds
left a comment
cxymds
left a comment
There was a problem hiding this comment.
Reviewed head eaa84a9d6c637e64bc4e0075337d39d93a7a9934. I found three P1 security/correctness issues and two P2 recovery/test-coverage issues. The current RustFS contract from rustfs/rustfs#5418 does preserve an existing Local master key, optional file permissions, and development-mode setting during reconfiguration, so that contract is not a blocker here. CI and focused local checks are green, but the current source-regex tests do not exercise the state and payload transitions described below.
| "Local filesystem KMS configuration is read-only in Console until safe master-key rotation is available.", | ||
| ), | ||
| payload: { | ||
| backend_type: "Local", |
There was a problem hiding this comment.
[P1] Fail closed for absent or unsupported backend types
normalizeBackendType currently maps both the normal NotConfigured response (backend_type: null) and every future/unknown backend to local. Now that Local is writable, a fresh instance opens with Local selected and this payload opts into plaintext development mode even though the user never chose Local. An older Console talking to a server with a newer backend is worse: editing any field can submit backend_type: "Local"; the server currently permits a non-Local → Local transition and then forbids switching away from Local, potentially making data encrypted through the original backend unavailable. Return the initial/unselected state for NotConfigured, use an explicit read-only unsupported state for unknown values, and emit this payload only after an explicit Local selection. Please add behavior tests for both null and a future backend value.
| backend_type: "Local", | ||
| key_dir: values.keyDir.trim(), | ||
| file_permissions: | ||
| localKmsConfigured && !hasStoredLocalFilePermissions ? undefined : (filePermissions ?? 384), |
There was a problem hiding this comment.
[P1] Reject permissions that expose plaintext key material
This passes any parsed integer through as the Unix mode. With the new initial Local flow, no master key is present, so RustFS stores the key material as plaintext-dev-only; entering decimal 438 (0o666) or 511 (0o777) makes the resulting .key files readable by other local users when the directory is traversable. The form uses noValidate, and the server currently applies Permissions::from_mode(mode) without checking owner/group/other bits. Keep this owner-only (the smallest safe choice is fixed 384/0o600), or reject group/other and special bits before sending the request; the server should enforce the same final-boundary invariant. Tests should prove unsafe modes do not issue a request.
| const statusKind = React.useMemo(() => (statusError ? "Error" : getStatusKind(status)), [status, statusError]) | ||
| const isRunning = statusKind === "Running" | ||
| const hasConfiguration = !statusError && statusKind !== "NotConfigured" | ||
| const localKmsConfigured = hasConfiguration && status?.backend_type === "Local" |
There was a problem hiding this comment.
[P1] Keep status and the form baseline in the same snapshot
This lock is derived from the latest status, while handleRefresh and mutation reconciliation call loadStatus(false) and leave formState/baselineFormState untouched. For example, if another administrator configures Local while this page is open, Refresh makes localKmsConfigured true but leaves the old or empty form in place; the new backend/path locks then make those stale values unrecoverable through Reset/Save. Concurrent changes to default/cache settings can also be overwritten by the next full-payload submission. When the form is clean, refresh should rebuild both form and baseline from the returned status; when it is dirty and the server baseline changed, surface a persistent conflict and block a silent overwrite. Please cover the external-Local-config and committed-POST/failed-response cases.
| const mutationInFlight = Boolean(activeMutation || submittingConfig || creatingKey || processingKeyAction) | ||
| const canSetCreatedKeyAsDefault = | ||
| status?.backend_type !== "Local" && status?.backend_type !== "Static" && !isConfigDirty | ||
| const canSetCreatedKeyAsDefault = status?.backend_type !== "Static" && !isConfigDirty |
There was a problem hiding this comment.
[P2] Preserve recovery state after a create/set-default partial failure
Allowing Local here exposes it to the existing two-stage flow, but handleCreateKey closes the dialog and clears its state immediately after key creation, before the configuration update finishes. If the reconfigure request then fails—and especially if the list refresh also fails or the user is on a later page—the generated key ID is no longer visible, while the toast tells the user to save a form that does not contain that ID. Persist a partial-success state with the created ID and a retry/copy action, or populate defaultKeyId plus a durable inline warning after confirming that the second stage did not commit. Add an interaction test for create success + reconfigure failure + list failure.
| assert.match(source, /const localKmsReadOnly = hasConfiguration && formState\.backendType === "local"/) | ||
| assert.match(source, /const localKmsConfigured = hasConfiguration && status\?\.backend_type === "Local"/) | ||
| assert.match(source, /backend_type: "Local"/) | ||
| assert.match(source, /allow_insecure_dev_defaults: !hasStoredLocalMasterKey/) |
There was a problem hiding this comment.
[P2] Test the state and payload behavior, not the source spelling
This assertion locks in one implementation string but never executes the payload builder or renders the form. The suite still passes when NotConfigured defaults to writable Local, when an unsupported backend is coerced to Local, when unsafe file modes are submitted, or when one of the immutable Local inputs is accidentally unlocked. Extract the payload/state transition into a testable function or add component tests with a mocked useSSE; cover the backend null/unknown matrix, safe and unsafe modes, all three Local immutable controls, refresh conflicts, and the create/set-default partial-failure path.
2 participants
Pull Request
Description
Enable Local KMS configuration and safe day-to-day key management in Console for evaluation environments. The UI explicitly warns that Local KMS is not for production, opts into plaintext development mode only when no server-managed local master key exists, and locks fields that cannot be changed safely after initial configuration.
Type of Change
Testing
Checklist
Related Issues
Related to rustfs/rustfs#5063.
Screenshots (if applicable)
N/A.
Additional Notes
Local KMS remains explicitly evaluation-only. After initial configuration, Console prevents changing the backend, key directory, and file permissions. It does not send local master-key material during updates. Default-key, cache, and timeout settings remain editable, and key creation can safely set the new key as the default.