Repository navigation
[cmake]: disable ssh protocols with HTTP_ONLY - #22744
fredrikblau wants to merge 3 commits into
Conversation
| set(CURL_USE_LIBSSH2 OFF) | ||
| set(CURL_DISABLE_SMTP ON) | ||
| set(CURL_USE_LIBSSH OFF) |
There was a problem hiding this comment.
Perhaps add them in their alpha-sorted positions?
There was a problem hiding this comment.
Zooming out, this issue also needs to be fixed in curl_setup.h,
for non-cmake builds. Also to fix it cleanly I think this'd need a
distinct DISABLE flag for SCP/SFTP.
I'd also consider sunsetting these options, as they seem to be
somewhat problematic to maintain, and arguably rarely used
ones, and also CMake-specific.
More about this here:
#22740 (comment)
This PR as-is now is fine for a firefighting fix, but the issue is not
urgent or grave. I'd personally go either with a full fix or drop the
HTTP_ONLY options, with preference for the latter.
|
Thanks for the detailed review. I moved the new options into alphabetical order. I kept this PR focused on the reported CMake behavior. I agree that covering curl_setup.h and non-CMake builds cleanly would require distinct SCP/SFTP disable flags, or a decision to deprecate/remove these options, which is a broader design change than this fix. The current patch addresses the CMake path without changing the existing HTTP_ONLY macro semantics. |
65fb27d to
76941b7
Compare
| set(CURL_USE_LIBSSH OFF) | ||
| set(CURL_USE_LIBSSH2 OFF) | ||
| set(CURL_DISABLE_TELNET ON) | ||
| set(CURL_DISABLE_TFTP ON) | ||
| set(CURL_DISABLE_WEBSOCKETS ON) |
There was a problem hiding this comment.
| set(CURL_USE_LIBSSH OFF) | |
| set(CURL_USE_LIBSSH2 OFF) | |
| set(CURL_DISABLE_TELNET ON) | |
| set(CURL_DISABLE_TFTP ON) | |
| set(CURL_DISABLE_WEBSOCKETS ON) | |
| set(CURL_DISABLE_TELNET ON) | |
| set(CURL_DISABLE_TFTP ON) | |
| set(CURL_DISABLE_WEBSOCKETS ON) | |
| set(CURL_USE_LIBSSH OFF) | |
| set(CURL_USE_LIBSSH2 OFF) |
Why not alpha-sort the whole list?
|
Updated as suggested: the full HTTP_ONLY option list is now alphabetized, with CURL_USE_LIBSSH and CURL_USE_LIBSSH2 after the CURL_DISABLE_* entries. |
We have not added disable flags for protocols that already can be disabled simply by switching off the 3rd party libraries. I think perhaps because adding a disable option mostly adds confusion then. |
Yep, maybe overkill. With or without it, it's a mess to sort out internally. |
|
I wouldn't mind just dropping this option... |
|
thanks @fredrikblau for this, but after some discussion I think @vszakats and I both prefer this route: #22764 - removing the option |
|
Thanks for the guidance. I understand the HTTP_ONLY removal is the preferred path. I enjoy contributing to curl—if there is another well-scoped issue you would recommend, I would be happy to work on it. |
HTTP_ONLY promises to disable all protocols except HTTP, but CMake still enabled libssh2/libssh and therefore SCP and SFTP whenever those libraries were installed.
Set both SSH backend options off when HTTP_ONLY is enabled. This keeps the existing HTTP_ONLY behavior while making the resulting protocol list match the documented contract.
Verified with libssh2 installed:
cmake -S . -B /tmp/curl-http-only -DHTTP_ONLY=ON -DCURL_USE_LIBSSH2=OFF -DCURL_USE_LIBSSH=OFF -DBUILD_TESTING=OFF -DCMAKE_BUILD_TYPE=Releasecmake --build /tmp/curl-http-only -j2curl --versionadvertises onlyhttp httpsgit diff --checkFixes #22740.