Skip to content

michal/ssh/fix-compat-suite #10375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: maint
Choose a base branch
from
Draft

michal/ssh/fix-compat-suite #10375

wants to merge 1 commit into from
+906 −378

Conversation

@Mikaka27
Copy link
Contributor

@Mikaka27 Mikaka27 commented Nov 14, 2025 •
edited
Loading

Changes:

  1. Get list of versions from file versions.txt instead of built docker images
  2. Build docker image automatically if missing
  3. Support multiple versions of base image (ubuntu versions)
  4. Support openssl3.x
  5. Support dropbear
  6. User's keys are present in both openssl and dropbear formats
  7. Properly return exit codes from scripts on failure
  8. Download archives from github if present there (faster download speed)
  9. When otp is server try to make client connect using docker bridge interface address

TODO:

  1. Skip tests on slow machines??
  2. Use docker registry to pull image instead of building it
  3. Allow running single group from command line if possible

@Mikaka27 Mikaka27 self-assigned this Nov 14, 2025
@Mikaka27 Mikaka27 added the team:PS Assigned to OTP team PS label Nov 14, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 14, 2025 •
edited
Loading

CT Test Results

    2 files     29 suites   19m 53s ⏱️
  482 tests   475 ✅  7 💤 0 ❌
1 687 runs  1 660 ✅ 27 💤 0 ❌

Results for commit cb64acf.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@u3s u3s self-requested a review November 14, 2025 14:13
@Mikaka27 Mikaka27 force-pushed the michal/ssh/fix-compat-suite branch 2 times, most recently from 9b1141e to 87143ab Compare November 14, 2025 17:05
@Mikaka27 Mikaka27 force-pushed the michal/ssh/fix-compat-suite branch 5 times, most recently from ecf4e0c to 0ff81e9 Compare November 21, 2025 18:06
@Mikaka27 Mikaka27 force-pushed the michal/ssh/fix-compat-suite branch from 0ff81e9 to cb64acf Compare November 21, 2025 18:11
u3s
u3s reviewed Nov 24, 2025
View reviewed changes
lib/ssh/test/ssh_compat_SUITE_data/build_scripts/create-dropbear-image
dropbear)
FAMssh=dropbear
VERssh=$2
LINK=https://matt.ucc.asn.au/dropbear/releases/dropbear-${VERssh}.tar.bz2
Copy link
Contributor

@u3s u3s Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we verifying what has been downloaded? should we check some repo hardcoded checksums? should we host binaries ourselves?

lib/ssh/test/ssh_compat_SUITE_data/versions.txt
#
# %CopyrightEnd%

openssh 4.4p1 openssl 0.9.8c 16.04
Copy link
Contributor

@u3s u3s Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be used for running test suite for a single group only? for example from terminal?
can CT anyhow parse group name with a space? I failed so far ...

lib/ssh/test/ssh_compat_SUITE.erl
{otp_server, [], [login_otp_is_server,
renegotiation_otp_is_server
]} |
[{G, [], [{group,otp_client}, {group,otp_server}]} || G <- ssh_image_versions()]
Copy link
Contributor

@u3s u3s Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewrite slightly, so that ssh_image_versions/0 is called only once across whole test suite?
nest generated groups under some umbrella group, and call it explicitly from all/0 ?

also reduce in rest of test suite code. read once, store it in Config?

maybe not relevant when we move image creation to other CI job ?

lib/ssh/test/ssh_compat_SUITE_data/versions.txt Outdated
dropbear 2020.81 22.04
dropbear 2022.83 22.04
dropbear 2024.86 22.04
dropbear 2025.88 22.04 No newline at end of file
Copy link
Contributor

@u3s u3s Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add something from Ubuntu 24 ? reduce Ubuntu 16?

lib/ssh/src/ssh.hrl
recv_ext_info, %% Expect ext-info from peer

kex_strict_negotiated = false,
ignore_next_kex_message = false, %% RFC 4253 section 7, peer's guess was wrong
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't mix compat test fix PR with fix for Dropbear? have separate PR for PR-8676

lib/ssh/src/ssh_fsm_kexinit.erl

%%% ######## {key_exchange, client|server, init|renegotiate} ####
%%%---- RFC 4253 section 7 guess was wrong
handle_event(internal, Msg, {key_exchange,server,_ReNeg},
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation, so that eyes don't bleed that much ;-)

lib/ssh/src/ssh_transport.erl
IsGuessWrong = is_guess_wrong(CounterGuess, CounterPart, Own),
key_exchange_first_msg(Algos#alg.kex,
Ssh#ssh{algorithms = Algos})
Ssh#ssh{algorithms = Algos, ignore_next_kex_message = IsGuessWrong})
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to
ignore_initial_kex_message ?

lib/ssh/src/ssh_transport.erl
is_different_algorithm(CounterPreferredKexAlgo, OwnPreferredKexAlgo) orelse
is_different_algorithm(CounterPreferredHostKeyAlgo, OwnPreferredHostKeyAlgo).

is_different_algorithm(none, none) ->
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this function really better than some not equal operator?

lib/ssh/src/ssh_transport.erl
Comment on lines +458 to +466
get_preferred_kex_algorithm(#ssh_msg_kexinit{kex_algorithms = [Preferred | _]}) ->
Preferred;
get_preferred_kex_algorithm(_) ->
none.

get_preferred_host_key_algorithm(#ssh_msg_kexinit{server_host_key_algorithms = [Preferred | _]}) ->
Preferred;
get_preferred_host_key_algorithm(_) ->
none.
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combine 2 get_preferred functions into one returning tuple {Kex, Host} ?

then simplify

    is_different_algorithm(CounterPreferredKexAlgo, OwnPreferredKexAlgo) orelse
        is_different_algorithm(CounterPreferredHostKeyAlgo, OwnPreferredHostKeyAlgo).

into {Kex1, Host1} =/ {Kex2, Host2} ?

lib/ssh/src/ssh_transport.erl
crypto:generate_key(ecdh, Args);
generate_key(dh, [P,G,Sz2]) ->
{Public,Private} = crypto:generate_key(dh, [P, G, max(Sz2,?MIN_DH_KEY_SIZE)] ),
BitSize = fun(N) -> bit_size(binary:encode_unsigned(N)) end,
Copy link
Contributor

@u3s u3s Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed in final PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@u3s u3s u3s left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Mikaka27 Mikaka27

Labels

team:PS Assigned to OTP team PS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Mikaka27 @u3s