Remove usage of deprecated libssh functions #4408

tobe2098 · 2025-10-06T09:53:08Z

The main purpose of this PR is to replace the deprecated functions in libssh, which are unmantained, by our own code, since those functions do not have replacements. This gives us control over those sections of code and allows us to maintain them.

In terms of tests, the changes were as minimal as possible without changing outside the PREMOCK framework that is used for all libssh functions.

Some of the deprecated warnings came from the building of libssh itself, so those warnings are silenced at the submodule level.

Fixes #4227

MULTI-2243

codecov · 2025-10-06T10:16:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.99%. Comparing base (a940f07) to head (eee2f5c).
⚠️ Report is 145 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4408      +/-   ##
==========================================
- Coverage   89.78%   88.99%   -0.80%     
==========================================
  Files         257      238      -19     
  Lines       16884    15273    -1611     
==========================================
- Hits        15160    13592    -1568     
+ Misses       1724     1681      -43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tests/test_ssh_client.cpp

tobe2098 · 2025-10-06T15:33:21Z

3rd-party/libssh/CMakeLists.txt

    set(BUILD_SHARED_LIBS OFF)
    set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+elseif(MSVC)
+    set(BUILD_SHARED_LIBS OFF)


To be able to build with libssh internal C functions that are not exposed via LIBSSH_API, it is necessary to not build shared libs in windows.

Instead of disabling the shared lib altogether, would something like WINDOWS_EXPORT_ALL_SYMBOLS work here?

tobe2098

Built and tested in Windows and Linux.

Sploder12

Overall pretty good! Just some concerns about using private SSH headers and some nitpicks

3rd-party/libssh/CMakeLists.txt

src/ssh/ssh_client.cpp

Sploder12 · 2025-10-09T16:31:28Z

src/sshfs_mount/sftp_server.cpp

-    mp::SSH::throw_on_error(sftp_server_session,
-                            session,
-                            "[sftp] server init failed",
-                            sftp_server_init);


From libssh source /* @deprecated in favor of sftp_server_new() and callbacks based sftp server */, maybe we should move to a callbacks based sftp server? Although, that'd require a ton more work with little benefit.

I'm not really a fan of including the private header, could make updates painful in the future. Also reimplementing the deprecated functions doesn't seem much better than just defining SSH_SUPPRESS_DEPRECATED IMO. I don't know, I'd like to get other's thoughts on this

I agree, but the comment plus the commit message leave us with little information. It is not ideal to reimplement, but at least now those functions are "maintained" now. I would be happy to work on moving to the callbacks based sftp server, and that was what I initally researched, but I found no existing documentation on that option. If anyone finds anything I will be happy to take a look at what changes are necessary.

In the case of get_exit_state, there is no other option really. Until we deal with SSH return codes with more consistency, reimplementing seemed like the only viable option.

Yeah the documentation is pretty much nonexistent for sftp server functionality from what I can tell. I could only find this example implementation but that itself isn't well commented or documented

Yeah, apart from the default subsystem request and default data callback... besides, it does say that those two are not supported in windows (ifdef + comment), so I do not think they are an option in this case. Sadly, until we do more research or someone finds some documentation we will have to go with the reimplementations. Is it possible that existing sftp servers all rely on the deprecated functions or old versions?

Our implementation is a pretty popular reference implementation so it's possible

Perhaps just add a comment here explaining the rationale and linking to an issue for moving to a callback-based version?

src/sshfs_mount/sftp_server.h

Sploder12 · 2025-10-09T16:52:45Z

tests/test_sftpserver.cpp

                                                        : exit_status_mock.success_status);

-        return nullptr;
+        return (calls++ & 1) ? nullptr : message.get();


this check seems odd (pun intended)

I know, but I reached it through trial and error. In the test it seems like there are 2 sftp server inits with a message_get in between, that is why the lambda looks so odd :D

How is calls different from num_calls? Could we use the latter value here? It looks like that would match what's happening with exit_status_mock.set_exit_status just above.

tests/test_ssh_client.cpp

tests/test_sshfsmount.cpp

tobe2098 · 2025-10-14T12:45:53Z

After some research, I have found in Gitlab the following libssh mirror that does implement an sample_sftpserver.c: https://gitlab.com/libssh/libssh-mirror/-/blob/master/examples/sample_sftpserver.c And in the PR they did they comment more on it. From a first (inexperienced) look, moving to callbacks may require a major refactor. Now at least we have a working (?) example.

I am unsure if this is a fork of our libssh or viceversa, but I think it would be nice to have this example.
Original PR that created example:
https://gitlab.com/libssh/libssh-mirror/-/merge_requests/263/diffs#cadf2ffbd25228d28ef40122660aa3c8ffb7d0d4
Merged PR:
https://gitlab.com/libssh/libssh-mirror/-/merge_requests/340/diffs

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

Co-authored-by: Mădălin-Florin GOIAN <79133273+MadalinGOIAN@users.noreply.github.com> Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

The deprecated function ssh_get_exit_status was replaced with the supported ssh_get_exit_state by replicating the ssh_get_exit_status within our codebase until we decide to use the additional information that the new function makes accessible (e.g. SIG info, the get_exit_state retcode...). Within the tests due to the more complex behavior of ssh_get_exit_state multiple adjustments had to be made. Moving them to SetUp does not work within SetUp() without further adjustments, so for now the lowest energy solution has been applied.

Most of the existing warnings and errors coming from the deprecated function declarations from the libssh library come from the library itself. It includes the legacy.h header in its main header libssh.h and that header contains all the deprecated functions. Since it has to be built and the warnings come from internal use in deprecated functions of other deprecated functions, the most pragmatic and correct solution is to silence the warnings coming from building libssh itself. The usage of deprecated functions outside libssh will still raise a warning or error.

Also moved the make_sftp_server to the class namespace for future mocking.

The function is replace with its existing internal functionality. Because in this case we cannot rely on the abstraction of S SSH:throw_on_error due to the libssh functions not setting the ssh_error on failure (previously the role of the replaced function), the exception management is done directly.

tobe2098 · 2025-10-17T09:44:33Z

On the test, the solution is not very clean. The execution order of the functions is exec->init->msg->exec->exec(not sshfs)->init->msg, (where after the PR init becomes a different msg) so a single index cannot account for behavior of both functions very cleanly

jimporter · 2025-10-18T01:35:19Z

On the test, the solution is not very clean. The execution order of the functions is exec->init->msg->exec->exec(not sshfs)->init->msg, (where after the PR init becomes a different msg) so a single index cannot account for behavior of both functions very cleanly

Perhaps pass in a queue and pop the front element off each time? That would be similar to how side_effect works in Python mocks.

tobe2098 · 2025-10-18T09:39:00Z

The queue does not seem cleaner than just checking num_calls against 2 and 5 like in the solution I pushed yesterday. I found a shortcut to make it cleaner than just checking against every number, since 2 and 5 share that (num_calls + 1)%3==0. Does this solution follow what you had in mind about calls vsnum_calls?

PD: I also added a new test to cover the termination signal logging.

tobe2098 · 2025-10-21T07:22:28Z

ssh_init had to be added to the windows daemon and to the client due to static linking. It could be excluded from linux and macos where it is not necessary via macros, but it has no negative effects (as tested in linux).

Copilot

Pull Request Overview

This PR removes usage of deprecated libssh functions by replacing them with custom implementations or alternative APIs. The primary changes involve replacing sftp_server_init with manual initialization logic and ssh_channel_get_exit_status with ssh_channel_get_exit_state to handle signal termination properly.

Key changes:

Replaced deprecated sftp_server_init with manual SFTP initialization handling
Replaced ssh_channel_get_exit_status with ssh_channel_get_exit_state for better exit state management
Added SSH library initialization/finalization calls in client and daemon entry points

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/sshfs_mount/sftp_server.cpp	Implemented manual SFTP initialization replacing deprecated `sftp_server_init`
src/ssh/ssh_client.cpp	Added `ssh_channel_get_exit_status` wrapper using `ssh_channel_get_exit_state`
tests/test_ssh_client.cpp	Updated tests to mock `ssh_channel_get_exit_state` instead of deprecated function
tests/test_sftpserver.cpp	Added SFTP init message handling to all test cases
tests/test_sshfsmount.cpp	Added mock SFTP client message setup for mount tests
tests/test_cli_client.cpp	Updated mocks to use `ssh_channel_get_exit_state`
tests/sftp_server_test_fixture.h	Changed fixture to use `sftp_server_free` instead of deprecated `sftp_free`
tests/mock_ssh_test_fixture.h	Updated to mock `ssh_channel_get_exit_state`
tests/mock_ssh.h	Changed mock declaration from `ssh_channel_get_exit_status` to `ssh_channel_get_exit_state`
tests/mock_ssh.cpp	Updated mock implementation signature for exit state
tests/mock_sftpserver.h	Added `sftp_reply_version` and `sftp_server_free` mocks
tests/mock_sftpserver.cpp	Implemented new mock functions
tests/c_mock_defines.cmake	Updated C mock definitions for new function signatures
src/sshfs_mount/sftp_server.h	Updated type alias to use `sftp_server_free`
src/daemon/daemon_main_win.cpp	Added `ssh_init`/`ssh_finalize` calls
src/client/cli/client.h	Made destructor virtual and non-default
src/client/cli/client.cpp	Added `ssh_init`/`ssh_finalize` in constructor/destructor
include/multipass/ssh/ssh_client.h	Added private `ssh_channel_get_exit_status` helper method
CMakeLists.txt	Added compiler flags to suppress deprecation warnings for libssh
3rd-party/libssh/CMakeLists.txt	Updated build configuration for Windows

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/daemon/daemon_main_win.cpp

tests/test_sftpserver.cpp

xmkg · 2025-10-21T09:20:00Z

3rd-party/libssh/CMakeLists.txt

    set(BUILD_SHARED_LIBS OFF)
    set(CMAKE_POSITION_INDEPENDENT_CODE ON)
+elseif(MSVC)
+    set(BUILD_SHARED_LIBS OFF)


Instead of disabling the shared lib altogether, would something like WINDOWS_EXPORT_ALL_SYMBOLS work here?

CMakeLists.txt

src/client/cli/client.cpp

src/daemon/daemon_main_win.cpp

After the stfp_server_init change the libssh premock requires that a new function be mocked in the tests and some adjustments are made. The function to be mocked is sftp_reply_version, which requires minor changes. The adjustment of sftp_get_client_message requires more adjustments due to how many times it is called in the codebase. test_sftpserver.cpp already has a framework that simplifies the changes, while test_sshfsmount.cpp requires the introduction of stateful lambdas to make the tests possible.

A comment was also added in case we want to log part of the sftp server initialization.

Unfortunately it seems that both clang and LLVM diverge in the treatment of captured constants, and the only fix is to either input manually or to use macros.

Unfortunately, it seems like MSVC requires exposing all symbols to be able to access the functions without the LIBSSH_API prefix, which in this case we are only interested in sftp_reply_version, which used to happen internally inside sftp_server_init. Seems like the current best solution, but still could be suboptimal. In this commit it is done by making the libssh library static, but later a CMake option will be set to expose these symbols only on Windows.

The logging framework was included, and the if statement logic was simplified.

tobe2098 · 2025-10-21T14:34:34Z

In the end, the CMake flag you suggested works, so I am closing the related threads. Thanks!

xmkg

LGTM

jimporter

LGTM!

tobe2098 commented Oct 6, 2025

View reviewed changes

tests/test_ssh_client.cpp Show resolved Hide resolved

tobe2098 commented Oct 6, 2025

View reviewed changes

tobe2098 marked this pull request as ready for review October 6, 2025 15:34

tobe2098 self-assigned this Oct 6, 2025

levkropp mentioned this pull request Oct 6, 2025

Recent package doesn't work on macOS #4409

Closed

sharder996 requested review from Sploder12, jimporter and xmkg October 9, 2025 13:10

Sploder12 requested changes Oct 9, 2025

View reviewed changes

tobe2098 requested a review from Sploder12 October 10, 2025 12:28

tobe2098 force-pushed the update-libssh-functions branch from 6504417 to 6e412ed Compare October 14, 2025 13:36

vedant-z and others added 14 commits October 14, 2025 17:15

[ssh] Update ssh_client.cpp

dd8673c

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update ssh_client.cpp

a8fb83f

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update src/ssh/ssh_client.cpp

10bc20d

Co-authored-by: Mădălin-Florin GOIAN <79133273+MadalinGOIAN@users.noreply.github.com> Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update ssh_client.cpp

bf4bd86

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update ssh_client.cpp

13c24e1

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update ssh_client.cpp

5872b41

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[ssh] Update ssh_client.cpp

29c7483

Signed-off-by: Vedant Borkar <vedantborkar1234@gmail.com>

[sshfs] Fix usage of sftp_server_free

cc70a35

Also moved the make_sftp_server to the class namespace for future mocking.

[sftp] Undo function placement inside server class

5d0294b

[ssh] Formatting of previous changes

025f2b8

[sftp] Fix compilation errors

2c88805

tobe2098 force-pushed the update-libssh-functions branch from 6e412ed to 02ddf0b Compare October 14, 2025 15:32

tobe2098 mentioned this pull request Oct 14, 2025

Update ssh_client.cpp #4307

Open

tobe2098 requested a review from jimporter October 17, 2025 09:54

tobe2098 force-pushed the update-libssh-functions branch from 6d6fde1 to 22f70c6 Compare October 18, 2025 09:35

tobe2098 force-pushed the update-libssh-functions branch 3 times, most recently from 0732a75 to cdd4dbd Compare October 20, 2025 13:46

xmkg requested a review from Copilot October 21, 2025 08:45

Copilot AI reviewed Oct 21, 2025

View reviewed changes

src/daemon/daemon_main_win.cpp Outdated Show resolved Hide resolved

tests/test_sftpserver.cpp Outdated Show resolved Hide resolved

tests/test_sftpserver.cpp Outdated Show resolved Hide resolved

xmkg requested changes Oct 21, 2025

View reviewed changes

tobe2098 added 5 commits October 21, 2025 15:55

[sftp] Improve exception message

f636f85

A comment was also added in case we want to log part of the sftp server initialization.

[tests] Fix unused reference capture in lambdas

0c0400a

[tests] Increase test code coverage in sftpserver

5513f4b

[tests] Fix lambda capture of constants

63fcf2a

Unfortunately it seems that both clang and LLVM diverge in the treatment of captured constants, and the only fix is to either input manually or to use macros.

tobe2098 force-pushed the update-libssh-functions branch from cdd4dbd to 558340d Compare October 21, 2025 14:12

tobe2098 added 7 commits October 21, 2025 16:20

[build] Make consistent libssh building with macos

b3310d4

[tests] Simplify lambda functions

9d11e18

[ssh] Amend the exit status function

8e768e4

The logging framework was included, and the if statement logic was simplified.

[sftp] Correct the sftp_server_init function

dc3f8c6

[tests] Unify execution count in sftp test

b3aafe9

[tests] Add test for termination signal logging

eee2f5c

tobe2098 force-pushed the update-libssh-functions branch from 558340d to eee2f5c Compare October 21, 2025 14:20

tobe2098 requested a review from xmkg October 21, 2025 14:38

xmkg approved these changes Oct 21, 2025

View reviewed changes

jimporter approved these changes Oct 21, 2025

View reviewed changes

Remove usage of deprecated libssh functions #4408

Are you sure you want to change the base?

Remove usage of deprecated libssh functions #4408

Uh oh!

Conversation

tobe2098 commented Oct 6, 2025

Uh oh!

codecov bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobe2098 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sploder12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tobe2098 commented Oct 14, 2025

Uh oh!

tobe2098 commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimporter commented Oct 18, 2025

Uh oh!

tobe2098 commented Oct 18, 2025

Uh oh!

tobe2098 commented Oct 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobe2098 commented Oct 21, 2025

Uh oh!

xmkg left a comment

Choose a reason for hiding this comment

Uh oh!

jimporter left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

codecov bot commented Oct 6, 2025 •

edited

Loading

tobe2098 left a comment •

edited

Loading

tobe2098 commented Oct 17, 2025 •

edited

Loading