maintenance: classify service credentials as passwords - #4281
Conversation
|
Author remediation update: The change now covers existing data as well as new definitions. Stored Ollama and HTTP service-discovery credentials migrate through the existing encryption boundary, API responses remain masked, masked edits preserve the prior secret, runtime use decrypts it, and migration is destination-bound so copied ciphertext is not accepted for another parameter. Persistence, masking, edit, runtime-decrypt, and legacy-upgrade contracts passed (53 focused tests plus startup packaging). The current GitHub backend, E2E, docs, license, and label checks are green. The requested migration gap is resolved; maintainer review is still required. |
|
Thanks for tackling this — apiKey and sd_token being stored as text is a real problem worth fixing. However, I think the current implementation has three blocking issues.
MonitorServiceImpl.java:309 only resolves param definitions from monitor.app: List paramDefines = appService.getAppParamDefines(monitor.getApp()); For an HTTP-SD monitor, app is the discovered application (e.g. linux), while sd_token is declared in app-http_sd.yml and keyed off monitor.scrape. So sd_token never appears in this paramDefines loop, which means:
Meanwhile MonitorParam.fromEntity masks on the stored Param.type, which this PR's migration sets to PARAM_TYPE_PASSWORD. Net effect: edit an HTTP-SD monitor and the string ****** is written back as the real access token, breaking service discovery. sd_password (app-http_sd.yml:102) is already type: password, so it hits the same path today. Repro: create an HTTP-SD monitor with an access token → run the migration → open and save the monitor → inspect hzb_param. Suggestion: resolve param definitions for the scrape/SD app as well, not just monitor.app.
The new restore branch deliberately accepts ciphertext under the legacy default key: boolean legacyCiphertext = !AesUtil.DEFAULT_ENCODE_RULES.equals(AesUtil.getDefaultSecretKey()) But the restored value continues through the same loop into paramValidatorManager.validate(...), and PasswordParamValidator.java:40 re-checks with the current key: if (!AesUtil.isCiphertext(passwordValue)) { // false for legacy-key ciphertext WheelTimerTask.initJobMetrics only performs one AesUtil.aesDecode(), so the collector ends up with the legacy ciphertext instead of the plaintext credential. This affects exactly the key-rotation path the branch was added to support. Suggestion: skip the password validator for values restored from storage, or make the ciphertext check key-aware.
Export goes exportConfig → getMonitorDto() → MonitorServiceImpl.java:521 setParams() → MonitorParam.fromEntity (masks) → AbstractImExportServiceImpl.java:126, so exported files now contain ******. Import goes importConfig → validateImportBatch → validate(dto, false), and with isModify != TRUE a masked value hits "The credential mask cannot be used as a new value." — the whole batch fails. This is not limited to Ollama and HTTP-SD; it applies to MySQL, Redis, SSH, Oracle and every other app with a password param, so the existing export/migrate workflow stops working. Masking secrets in exports may well be the right call, but import needs a matching story (an explicit "include secrets" option, or skip-and-prompt on import). Non-blocking ServiceCredentialMigration runs as a CommandLineRunner and rescans the table on every startup. A versioned migration would be a better fit and would align with #4280, which uses Flyway V182. |
Summary
This update completes the storage and API lifecycle for the Ollama API key and HTTP service-discovery access token.
******in monitor API responses and exportsThe migration is idempotent and logs only the number of migrated rows. It never logs a credential value. It executes before
SchedulerInit, so collectors receive encrypted type-2 config and decrypt it only inside the runtime collection path.Regression evidence
The previous head had no database migration, no response-mask contract, and no mask-resolution behavior. The added contracts cover:
WheelTimerTaskruntime decryption and protocol placeholder replacementValidation
AI assistance: used for draft implementation and test iteration.
Human validation: ran the real H2 migration contract, API and edit-path regressions, collector runtime decryption proof, and the 24-module startup source package proof; all completed successfully.
Risk notes: startup performs a bounded query for only the two reclassified parameter identities. A migration failure aborts before scheduling rather than dispatching an ambiguously typed credential. Exported masked credentials must be re-entered when imported as a new monitor.