docs: add host key verification best-practice guidance#1939
docs: add host key verification best-practice guidance#1939orbisai0security wants to merge 1 commit into
Conversation
Add SECURITY sections to libssh2_session_handshake.md and libssh2_knownhost_checkp.md documenting that applications should verify host keys and reject non-matching results. Update the ssh2_exec.c example to actually refuse connections when host key verification fails, rather than just printing and continuing. Addresses feedback from PR libssh2#1932 where a maintainer suggested documentation improvements over a new API. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| "Refusing to connect.\n", check); | ||
| libssh2_knownhost_free(nh); | ||
| goto shutdown; | ||
| } |
There was a problem hiding this comment.
If doing this, why only in this particular example and not in ssh2_echo.c
which has the same comment?
I take issue with unleashing the LLM on the libssh2 codebase, to add
generated text with trivia or just for the sake of it. FAILURE and
the rest are also non-existing return values. It's also trivial that all
error values should be checked if security and correctness is a goal.
The above check in the example may be fine, but did you (as a human
person) actually try if it works as intented?
There was a problem hiding this comment.
Thanks, that’s fair feedback.
I agree that this should not be presented as a library vulnerability or as broadly generated documentation. The intended scope is only to make the examples/docs safer for copy-paste use: libssh2_knownhost_checkp() documents four possible outcomes, and only LIBSSH2_KNOWNHOST_CHECK_MATCH means the host/key matched. Continuing after NOTFOUND or MISMATCH is therefore not a good example pattern.
You’re right that changing only ssh2_exec.c is inconsistent if ssh2_echo.c has the same pattern. I can revise the PR to either:
- update both examples consistently, or
- Drop the example code change and keep only a short man-page note, depending on what you prefer.
Also agreed on the return-value wording: I’ll use the full LIBSSH2_KNOWNHOST_CHECK_* names rather than shorthand like FAILURE/NOTFOUND/MISMATCH.
I’ll also build and run the affected example locally before updating the PR, and I’ll remove any broad/generated wording so the change is limited to concrete secure-usage guidance.
2 participants
Summary
libssh2_session_handshake(3)andlibssh2_knownhost_checkp(3)man pages documenting that applications should verify host keys after handshake and reject non-matching resultsssh2_exec.cexample to actually refuse connections when host key verification fails, instead of just printing and continuingMotivation
Follows up on feedback from #1932 where @willco007 noted that a documentation note about best practices would be appropriate. This PR takes the documentation-only approach rather than introducing a new API.
The existing
ssh2_exec.cexample performed thelibssh2_knownhost_checkp()call but had only a comment saying "we could verify... or bail out" without actually doing so. This made it easy for developers to copy the example without realizing they should reject non-matching keys.Test plan
ssh2_exec.cstill compiles with the project's build system🤖 Generated with Claude Code