Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not cover successfully matching the requested host from a hashed entry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes libssh2 known-host key selection for hashed host entries.
Changes:
- Verifies hashed entries against the requested host and port.
- Adds regression test 3500 and build registrations.
File summaries
| File | Description |
|---|---|
lib/vssh/libssh2.c |
Validates hashed known-host entries. |
tests/libtest/test3500.pl |
Generates regression-test known-host data. |
tests/libtest/Makefile.am |
Distributes the helper script. |
tests/data/test3500 |
Defines the SFTP regression test. |
tests/data/Makefile.am |
Registers test 3500. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
libssh2 does not expose the plaintext name for hashed entries. Check each entry key against the requested hostname before using its key type. Otherwise an unrelated hashed entry can select the wrong algorithm. Add an SFTP regression test with an unrelated hashed entry before the requested host.
7423024 to
7c33c9e
Compare
|
Thanks! |
|
Pushed a follow-up: ssh_force_knownhost_key_type() was calling libssh2_knownhost_checkp() once per hashed known_hosts entry, and each call rescans the whole known_hosts set on its own, so a file with many hashed entries did more work than needed. Added a small cache of key types already confirmed absent for the connecting host (NOTFOUND for a given host/port/type does not depend on which entry's key bytes were passed in, so it is safe to skip repeat entries of that type). The MATCH path and the switch on store->typemask afterward are unchanged. Reran runtests.pl 3500, 1459, 600, the full SFTP/SCP keyword set (58/58), and checksrc.pl. All pass. |
|
If you pushed this after my merge, you need to make that a new pull-request! |
|
yep my bad, that went up after you'd merged. put it up properly as #22905 now, just the perf bit. |
libssh2_knownhost_get() does not return a plaintext name for hashed entries. The current scan treats the first such entry as the requested host and selects its key type, even when the hash belongs to a different host.
Use libssh2_knownhost_checkp() to verify each hashed entry against the requested hostname and port before selecting its key type.
Test 3500 puts an unrelated hashed entry before a valid entry for the requested host. It fails before this change and passes afterward.
Tests: