Correctly distinguish ssh banner and protocol string#597
Draft
Correctly distinguish ssh banner and protocol string#597
Conversation
michalvasko
requested changes
Apr 17, 2026
|
|
||
| API const char * | ||
| nc_session_ssh_get_banner(const struct nc_session *session) | ||
| nc_session_ssh_get_protocol_string(const struct nc_session *session) |
Member
There was a problem hiding this comment.
To avoid NBC changes, keep the old function as well. Let it just call nc_session_ssh_get_protocol_string() and doxygen should say that it is deprecated and the new function should be used instead.
| It can be used to provide information about the server or legal notices. | ||
| Note that the banner is sent before authentication, so it should not contain any sensitive information. | ||
|
|
||
| This feature requires libssh version 0.10.0 or later. If built with an older |
Member
There was a problem hiding this comment.
Such very implementation-specific information should not be in YANG. It is fine to just remove it and print a WRN message in case an old libssh version is used and the banner is set but cannot be sent.
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.
Comment on lines
1684
to
1688
| * 0 return indicates success, 1 fail (msg not yet replied to), -1 fail (msg was replied to) */ | ||
| if (method == SSH_AUTH_METHOD_NONE) { | ||
| ret = nc_server_ssh_auth_none(local_users_supported, auth_client, msg); | ||
| ret = nc_server_ssh_auth_none(session, opts->banner, local_users_supported, auth_client, msg); | ||
| } else if (method == SSH_AUTH_METHOD_PASSWORD) { | ||
| ret = nc_server_ssh_auth_password(session, local_users_supported, auth_client, msg); |
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
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