Duansg commented on PR #4293: URL: https://github.com/apache/hertzbeat/pull/4293#issuecomment-5162837117
Thanks — pinning host keys here makes sense. SshClient.setUpDefaultClient() inherits ClientBuilder.DEFAULT_SERVER_KEY_VERIFIER, which is AcceptAllServerKeyVerifier.INSTANCE (verified in sshd-core 2.13.1), so today any server key is accepted. A few points on the implementation. The compatibility migration isn't one-time SftpHostKeyCompatibilityMigration is a CommandLineRunner with no version record or completion marker, so it re-scans all ftp monitors on every startup and writes insecureSkipVerify=true into any SFTP monitor lacking both hostKeyFingerprint and insecureSkipVerify. That's correct for the upgrade moment, but it keeps applying afterwards: a monitor that reaches the database without a host-key policy — via config import, a direct API call, or SQL — gets converted to "skip verification" at the next restart. The normal UI path is likely unaffected, since insecureSkipVerify has defaultValue: false and produces a param row, but the import/API paths bypass that. Suggestion: make it a Flyway Java migration, or persist a completion flag so the backfill runs once. New monitors should opt into the unsafe mode explicitly rather than inherit it. "Required" is declared three different ways In app-ftp.yml, hostKeyFingerprint is required: false and hide: true, its display name says "（必填）/ Required", and the collector treats it as mandatory in both FtpProtocol.isInvalid() and preCheck(). hide: true puts it in the collapsed Advanced Parameters panel (monitor-form.component.html:122-131) rather than hiding it outright, but a user who doesn't expand that panel gets a rejected job referencing a field they never saw. Worth making the three agree — either mark it required and surface it, or drop "必填" from the label. Pin the digest explicitly KeyUtils.getFingerPrint(PublicKey) resolves through DEFAULT_DIGEST_HOLDER, an AtomicReference that's mutable via setDefaultFingerPrintFactory(...) and the KEY_FINGERPRINT_FACTORY_PROP system property. Since the docs and placeholder promise SHA256:, the explicit overload would be safer: KeyUtils.getFingerPrint(BuiltinDigests.sha256, serverKey) Fingerprint comparison is format-sensitive Raw byte equality against the trimmed input means a fingerprint pasted without the SHA256: prefix, or with different base64 casing, fails with the generic "did not match" warning and no hint that the format was the issue. Normalizing, or rejecting malformed entries up front with a clear message, would save operators some guessing. Minor: MessageDigest.isEqual is unnecessary here — fingerprints are public, there's no secret to leak through timing. -- 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]
