Duansg commented on PR #4293:
URL: https://github.com/apache/hertzbeat/pull/4293#issuecomment-5419808563
Re-reviewed the current head (`ede8be9`). The description and the code now
agree on fail-closed, and every blocking item from my previous two rounds is
resolved — verified each one:
- **`depend: ssl` boolean wiring**: `onParamBooleanChanged` now ends with
`onDependChanged(String(enabled), field)`, and the existing `ngOnChanges` init
dispatch plus `fieldValues.map(String)` make initial visibility, toggle
visibility, hidden-value clearing, and the `hasAdvancedParams` panel collapse
all work on both the new and edit paths, with spec coverage. It also fixes a
pre-existing bug where a persisted `"false"` string was treated as truthy and
rewrote port 21→22 on merely opening the edit form.
- **`isInvalid()` dead code**: `preCheck` delegates to `validationError()` —
one source of truth.
- **Fingerprint verification**: `KeyUtils.checkFingerPrint(expected,
BuiltinDigests.sha256, key)` with the explicit digest; the
regex-vs-manual-compare, `limit: 2048`, the global-state-mutating test, and the
null-session test are all gone. Insecure-skip warnings are deduplicated per
endpoint.
- **Upgrade path**: fail-closed is chosen and documented in `upgrade.md`. CI
is green.
That said, one real regression remains, and it breaks exactly the population
the docs promise is safe.
## Strict boolean validation meets unreplaced placeholders — legacy
plain-FTP monitors go down
`validationError()` has:
```java
if (isNotBlank(ssl) && ssl is neither "true" nor "false") →
error // ①
if (isNotBlank(insecureSkipVerify) && neither "true" nor "false") →
error // ② — before the ssl gate
if (!"true".equalsIgnoreCase(ssl)) return null;
// plain FTP returns here
```
But `CollectUtil.replaceSpecialCharacterIfNeeded()` handles a **missing**
configmap key with `continue` — the `^_^field^_^` placeholder is left in the
value verbatim, not nulled. So any monitor whose param rows lack the new fields
delivers the literal `^_^insecureSkipVerify^_^` into check ②.
Which monitors lack rows? I traced the three job-assembly paths:
| Path | Backfills missing params from `defaultValue`? | Consequence |
|---|---|---|
| Startup `SchedulerInit.run()` | Yes (`insecureSkipVerify` has
`defaultValue: false`) | Legacy plain FTP survives — **except** monitors
created before #3038 (Feb 2025) that were never edited: they have no `ssl` row,
`ssl` has **no** defaultValue, the placeholder hits ① and they **fail at
startup** |
| `addMonitor` — including `importConfig` (`MonitorServiceImpl` builds
configmap without backfill; `validate()` only rejects missing *required*
params, it never backfills optional ones) | No | Importing a 1.8.x export of a
**plain FTP** monitor → placeholder hits ② → "Sftp Protocol skip-verification
option must be true or false." The monitor is down until the next manager
restart backfills it — a confusing self-healing failure referencing an SFTP
field the operator never saw |
| `modifyMonitor` | No | External API edits without the new fields: same as
above |
This contradicts the new `upgrade.md` line "Plain FTP monitors are
unchanged." The UI paths are unaffected (the form always submits all param
rows, and a null-valued row is correctly replaced to null).
**Suggested fix** (small): drop the strict must-be-true-or-false rejection
in ① and ② and use `"true".equalsIgnoreCase(...)` semantics (anything else =
false/disabled), or at minimum move ② behind the `ssl == true` gate. Security
is unaffected — a placeholder or garbage value never equals `"true"`, so it can
never earn the skip-verification exemption; fail-closed is preserved. What's
lost is only an error message for explicitly malformed values; what's gained is
not breaking existing monitors.
## Minor (non-blocking)
1. A legacy SFTP monitor at startup fails with "must use the SHA256:base64
format" — the placeholder makes `hostKeyFingerprint` non-blank, so the
"fingerprint is required" message is never reached, and the operator is told
their (never-entered) value has the wrong format. Falls out of the same
lenient-parsing fix.
2. The regex allows a trailing `=` (`{43}=?`), but MINA's `getFingerPrint`
emits unpadded 43-char base64 — a pasted padded fingerprint passes format
validation yet can never match, yielding only the generic "did not match"
warning. Drop the `=?` or strip padding before comparison.
3. `INSECURE_WARNING_ENDPOINTS` only grows; past 1024 distinct endpoints new
insecure connections log at debug only, and entries are never reclaimed when a
monitor is deleted or gains a fingerprint. Edge-scale only.
4. `serializationDoesNotExposeComputedValidationProperties` asserts on
`"parsedHostKeyFingerprints"`, but the method is named
`parseHostKeyFingerprints` — that assertion can never fire (the
`validationError` one is real).
The core of the PR — the verifier, explicit SHA-256, the rotation window,
the fail-closed contract, and the form wiring — is solid now. Fix the
strict-boolean/placeholder interaction (a few lines) and tune the message, and
I'm happy to approve.
--
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]
