Correctly distinguish ssh banner and protocol string#597
Merged
michalvasko merged 3 commits intodevelfrom Apr 21, 2026
Merged
Conversation
michalvasko
requested changes
Apr 17, 2026
There was a problem hiding this comment.
Pull request overview
This PR clarifies the distinction between the SSH protocol identification string (RFC 4253 §4.2) and the SSH pre-auth “issue banner” (RFC 4252 §5.4) in libnetconf2, adding support for configuring/sending the actual SSH banner while renaming the previously misnamed “banner” API to “protocol string”.
Changes:
- Rename
nc_session_ssh_get_banner()tonc_session_ssh_get_protocol_string()and update error messaging accordingly. - Add server API
nc_server_ssh_set_protocol_string()and use it to set the SSH identification string via libssh. - Implement sending/receiving the real SSH issue banner (libssh >= 0.10) and add/adjust tests and YANG model documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ssh.c | Updates tests for protocol string getter/setter and adds banner test + config setup. |
| src/session_server_ssh.c | Adds protocol string forging/setter; configures identification string; sends SSH issue banner during auth. |
| src/session_server.h | Declares new public server API for setting the SSH protocol identification string. |
| src/session_server.c | Frees the new server_opts.ssh_protocol_string during server teardown. |
| src/session_p.h | Clarifies banner semantics and adds ssh_protocol_string to server options. |
| src/session_client_ssh.c | Retrieves and logs received SSH issue banner (libssh >= 0.10). |
| src/session.h | Renames public session getter API and updates documentation. |
| src/session.c | Implements renamed getter and updates error text. |
| modules/libnetconf2-netconf-server@2025-11-11.yang | Updates banner leaf semantics to real SSH issue banner and adjusts references/constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
michalvasko
requested changes
Apr 20, 2026
Roytak
force-pushed
the
ssh-banner-fix
branch
2 times, most recently
from
ac0f6db to
db40132
Compare
Rename previous banner to protocol string and add an API setter for it. Add actual SSH banner and send/receive it. Deprecated nc_session_ssh_get_banner API by introducing nc_session_ssh_get_protocol_string, to better reflect what it's actually getting Fixes #592
michalvasko
approved these changes
Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rename previous banner to protocol string and add an API setter for it.
Add actual SSH banner and send/receive it.
NBC: renamed nc_session_ssh_get_banner API to
nc_session_ssh_get_protocol_string, to better reflect what it's actually getting
Fixes #592