michal/ssh/fix-each-side-may-guess/OTP-19864 #10575
Conversation
CT Test Results 2 files 29 suites 20m 7s ⏱️ Results for commit 916a93b. ♻️ 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 |
Mikaka27
force-pushed
the
michal/ssh/fix-each-side-may-guess/OTP-19864
branch
2 times, most recently
from
64ac615 to
7a43077
Compare
Mikaka27
added
the
testing
|
This caused failure in the tests, will have to do better filtering of algorithm selected, to ensure that we try only with algorithms supported by available openssh version. Will fix that. |
Mikaka27
force-pushed
the
michal/ssh/fix-each-side-may-guess/OTP-19864
branch
from
7a43077 to
243cfb9
Compare
Mikaka27
force-pushed
the
michal/ssh/fix-each-side-may-guess/OTP-19864
branch
from
243cfb9 to
916a93b
Compare
|
|
||
| kex_strict_negotiated = false, | ||
|
|
||
| ignore_initial_kex_message = false, %% RFC 4253 section 7, if true peer's guess was wrong |
| D = #data{ssh_params = Ssh0 = #ssh{ignore_initial_kex_message = true}}) when | ||
| is_record(Msg, ssh_msg_kexdh_init); | ||
| is_record(Msg, ssh_msg_kex_ecdh_init) -> | ||
| DebugMsg = ["server ignored ", element(1, Msg), " message due to wrong guess."], |
There was a problem hiding this comment.
use ?SELECT_MSG + fun() macro to avoid processing of debug lines if level is not debug.
| server_host_key_algorithms = [CounterHKey | _]}, | ||
| #ssh_msg_kexinit{kex_algorithms = [OwnKex | _], | ||
| server_host_key_algorithms = [OwnHKey | _]}) -> | ||
| {CounterKex, CounterHKey} /= {OwnKex, OwnHKey}. |
There was a problem hiding this comment.
maybe less processing and more erlangish if you match in function head for equalness? like:
fun(false, _, _) -> false;
fun(true, A, A) -> false;
fun(true, _, _ -> true. | WrongKex = {kex, lists:reverse(KexAlgs)}, | ||
| WrongPubKey = {public_key, lists:reverse(PubKeyAlgs)}, | ||
|
|
||
| ClientAlgsWrongKex = [WrongKex, PubKey], |
There was a problem hiding this comment.
use list comprehension and reduce empty lines pls. avoid copy/paste pls.
| WrongKex = {kex, lists:reverse(KexAlgs)}, | ||
| WrongPubKey = {public_key, lists:reverse(PubKeyAlgs)}, | ||
|
|
||
| ClientAlgsWrongKex = [WrongKex, PubKey], |
There was a problem hiding this comment.
as above. please reduce copy/paste and empty lines.
| client_guess_test(false, Config, ClientAlgsWrong, ServerAlgs). | ||
|
|
||
| %%% RFC 4253 section 7, client guesses correctly in renegotiation | ||
| client_guesses_correctly_renegotiation(Config) -> |
There was a problem hiding this comment.
are we repeating the same test cases here again?
can you reduce 4 testcases to 2? just combine 2 helper calls in list comprehension.
AFAIK, you have same inut data.
Just make sure to sprinkle some test logs in helper functions so it is clear in logs where the failure happened.
(?CT_LOG is not present in maint-27, maint-26 but maybe its a good occasion to port it there?)
| get_specific_ops(GuessCorrect, Variant); | ||
|
|
||
| %%% RFC 4253 section 7, each side may guess | ||
| get_specific_ops(true, dh) -> |
There was a problem hiding this comment.
please try to further reduce this helper code, possibly avoiding repetitions.
maybe something like this or better?
get_specific_ops(GuessCorrect, Variant) ->
{InitGuess, Init, Reply} = kex_messages(Variant),
[{send, ssh_msg_kexinit_guess},
{match, #ssh_msg_kexinit{_='_'}, receive_msg},
{send, InitGuess}] ++
[{send, Init} || GuessCorrect =:= false] ++
[{match, Reply, receive_msg}].
kex_messages(dh) ->
{ssh_msg_kexdh_init_guess, ssh_msg_kexdh_init, #ssh_msg_kexdh_reply{_='_'}};
kex_messages(ecdh) ->
{ssh_msg_kex_ecdh_init_guess, ssh_msg_kex_ecdh_init, #ssh_msg_kex_ecdh_reply{_='_'}}.| {"Send (reconstructed)~n~s~n",[format_msg(Msg)]} | ||
| end), | ||
| send_bytes(NextKexMsgBin, S#s{ssh = C}); | ||
| send(S0, MsgType) when ?role(S0) == client, |
There was a problem hiding this comment.
please try to reduce send code you have added to avoid repetitions of same code fragments. remember you will be maintaining this ;-) !
I forgot that when adding clause for send(S0, ssh_msg_kexdh_init_dup) ... now I regret it... maybe it could also be reduced in one go?
2 participants
Fixes: #8676
If merged after #10512 we need to add support for hybrid key exchange here as well.
If merged before, we need to add support for this in #10512