Duansg commented on PR #4281:
URL: https://github.com/apache/hertzbeat/pull/4281#issuecomment-5162732927
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.
> 1. __sd_token__ can never be restored — the literal ****** gets persisted
MonitorServiceImpl.java:309 only resolves param definitions from monitor.app:
List<ParamDefineInfo> 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:
- the restoresMaskedCredential pre-check never sees it;
- the mask-restore branch (guarded by
"password".equals(paramDefine.getType())) never fires.
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.
> 2. Legacy-key ciphertext gets encrypted twice
The new restore branch deliberately accepts ciphertext under the legacy
default key:
boolean legacyCiphertext =
!AesUtil.DEFAULT_ENCODE_RULES.equals(AesUtil.getDefaultSecretKey())
&& AesUtil.isCiphertext(storedValue, AesUtil.DEFAULT_ENCODE_RULES);
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
passwordValue = AesUtil.aesEncode(passwordValue); // encrypted a
second time
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.
> 3. Export → import round-trip is broken for every app with a password param
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
