Duansg commented on PR #4293:
URL: https://github.com/apache/hertzbeat/pull/4293#issuecomment-5232507473
The underlying issue is real — SshClient.setUpDefaultClient() uses
AcceptAllServerKeyVerifier.INSTANCE, so SFTP collection currently accepts any
host key. Scoping the change to ssl=true so plain FTP is untouched is right,
and supporting two fingerprints for a rotation window plus documenting that
ssh-keyscan alone doesn't authenticate a server are both good calls.
That said, I don't think this can go in as-is.
1. The upgrade path described in the PR body isn't in the diff, and existing
monitors will break.
The description says:
> Existing ftp monitors with SFTP enabled and neither a fingerprint nor an
explicit verification policy receive insecureSkipVerify=true once, before
scheduler initialization. … SftpHostKeyCompatibilityMigrationTest
There's no migration class in the 7 changed files, and the docs actually
state the opposite ("HertzBeat does not enable that option automatically"). So
the code and docs agree on fail-closed; the description is stale.
I traced what actually happens to a pre-upgrade monitor. In
CollectUtil.replaceSpecialCharacterIfNeeded():
Configmap param = configmap.get(replaceField);
if (param == null) { continue; } // placeholder is left in place verbatim
Existing monitors have no rows for the new params, so:
- hostKeyFingerprint resolves to the literal "^_^hostKeyFingerprint^_^" —
non-blank, so Assert.hasText passes
- Boolean.parseBoolean("^_^insecureSkipVerify^_^") is false
- Assert.isTrue(hasValidHostKeyFingerprints()) throws
- MetricsCollect:201 catches it and sets Code.FAIL
Every existing SFTP monitor goes unavailable right after upgrade, with the
message "host key fingerprints must use the SHA256:base64 format" — for a field
the operator never filled in. Either land the migration for real (plus a
home/docs/start/upgrade.md note), or make the first release warn-only and
enforce in the next one.
2. depend: ssl: [true] has no effect in the UI.
monitor-form.component.html:110-115 only routes radio parents to
onDependChanged; boolean parents go to onParamBooleanChanged, which contains no
depend handling at all (just the api/ftp ssl↔port defaulting). Every existing
depend: in the repo is keyed on a radio field (snmpVersion, httpMethod,
__sd_authType__) — a boolean parent is new here.
Combined with hide: true, both params land in the advanced list
(monitor-new.component.ts:125) where Param.display defaults to true and the
section renders on display !== false. Net effect: both fields are always
visible, so plain-FTP users see a "DANGER: skip SFTP host key verification"
checkbox. The "conditional on SFTP" claim isn't achieved.
Simplest fix is to drop the depend block and say "SFTP only" in the param
name/tip, unless you want to add boolean-parent support to the form first.
3. FtpProtocol.isInvalid() is dead code.
There are only six .isInvalid() call sites in main (consul_sd, nacos_sd,
nginx, registry, pop3, dns); FTP isn't one of them — FtpCollectImpl enforces
via preCheck(). So the +48 lines in FtpProtocol and +48 in FtpProtocolTest
cover a method that never runs in production, while the six lines in preCheck
do the real work. The two have already diverged: isInvalid() rejects a
malformed insecureSkipVerify value and rejects a bad fingerprint even when skip
is on; preCheck does neither. Having preCheck delegate to isInvalid() would
give one source of truth and make the tests cover the live path.
Smaller things:
- createServerKeyVerifier runs on every collection inside handleSftpCollect,
so with insecureSkipVerify=true the warning fires once per interval per
monitor. Worth deduplicating, or warn once and debug afterwards.
- The SHA256:[A-Za-z0-9+/]{43}=? pattern depends on KeyUtils.getFingerPrint
emitting the SHA256: prefix, even though BuiltinDigests.sha256.getAlgorithm()
is SHA-256 and MINA strips the hyphen internally — a quiet break waiting for a
MINA upgrade. KeyUtils.checkFingerPrint(String, PublicKey) handles both the
SHA256:base64 and MD5:xx:xx: forms and would remove the regex and the manual
comparison.
- limit: 2048 is inert here: ParamDefine.limit is a Short documented as max
255, and only type: textarea-limit reads it (extra.textarea_limit) — plain
textarea ignores it. (paramValue is varchar(8126), so capacity itself is fine.)
- serverKeyVerifierAlwaysUsesSha256 mutates JVM-global state via
KeyUtils.setDefaultFingerPrintFactory. It's restored in finally, but it will
interfere with other tests if surefire ever runs them in parallel in the same
JVM.
- verifyServerKey(null, null, key) passes a null session and address,
coupling the test to the fact that the current implementation ignores them.
Happy to re-review once the upgrade path and the depend wiring are sorted.
--
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]
