Re: [LTP] [PATCH v2 1/1] ssh-stress: disable resource penalties

[Petr Vorel <pvorel@suse.cz>]

Hi Vasileios,

> Our tests create a number of ssh sessions in the
> background which are immediately killed. Some of
> them haven't finished the authentication stage yet
> and they close the connection incurring penalties from
> the ssh daemon.

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> Then when we try to reconnect to the daemon we are bounced
> because of the active penalty which leads to a failed test.

> ssh-stress 1 TINFO: Killing all ssh sessions
> kex_exchange_identification: read: Connection reset by peer
> Connection reset by fd00:1:1:1::2 port 58373
> ssh-stress 1 TFAIL: SSH not reachable

> From the sshd logs we can see

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> This feature was added to OpenSSH 9.8 in the 2024 release in the
> 81c1099d2 commit. Lets disable penalties for the versions that
> support them.

+1 for a nice description!

> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
> ---
> Changes in v2:
> - Removed unnecessary parenthesis around if statements.
> ---
>  testcases/network/stress/ssh/ssh-stress.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

> diff --git a/testcases/network/stress/ssh/ssh-stress.sh b/testcases/network/stress/ssh/ssh-stress.sh
> index c27c27a28..d5db24835 100755
> --- a/testcases/network/stress/ssh/ssh-stress.sh
> +++ b/testcases/network/stress/ssh/ssh-stress.sh
> @@ -39,8 +39,12 @@ cleanup()

>  setup()
>  {
> -	local port rc
> +	local port rc version major minor

> +	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
> +	set -- $version
> +	major=$1
> +	minor=$2

>  	port=$(tst_rhost_run -c "tst_get_unused_port ipv${TST_IPVER} stream")

> @@ -60,6 +64,13 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
>  HostKey $TST_TMPDIR/ssh_host_ed25519_key
>  EOF

> +	if [ -n "$major" ] && [ -n "$minor" ]; then
This could be simplified by assigning both to zero.

> +		if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then

This is wrong. It should be as I suggested at v1:

if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then

otherwise it does not work for > 9. Using () in v1 was also correct, just
unnecessary complicated.

If you agree I can merge this:

setup()
{
	local port rc version
	local major=0 minor=0

	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
	set -- $version
	major=$1
	minor=$2
	...
	if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then
		cat << EOF >> sshd_config
PerSourcePenalties no
EOF
	fi

(See full diff from your v2 below.)
---
Why? && and || have equal precedence (unlike C like languages where && has
higher preference):

$ echo 1 || echo 2 && echo 3
1
3
=> In C like evaluation it would be just:
1

If you wonder why, see section "Lists" in man bash(1) [1]:

	A list is a sequence of one or more pipelines separated by one of
	the operators ;, &, &&, or ||, and optionally terminated by one of
	;, &, or <newline>.

	Of these list operators, && and || have equal precedence, followed
	by ; and &, which have equal precedence.

Just to double check this basic functionality works the same across POSIX shell
see section "Short-Circuit List Operators" in man dash(1) [2]:

	“&&” and “||” are AND-OR list operators.  “&&” executes the first
	command, and then executes the second command if and only if the
	exit status of the first command is zero.  “||” is similar, but
	executes the second command if and only if the exit status of the
	first command is nonzero.  “&&” and “||” both have the same
	priority.

Therefore 10.1 version will not be selected in your new code:

$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then echo "found"; else echo "not found"; fi
not found

But using just 2x [ ] (the second with "-a") will behave like 
$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then echo "foo"; fi
foo

$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then echo "found"; else echo "not found"; fi
found

Using () in v1 was correct:
$ major=10 minor=1; if [ "$major" -gt 9 ] || ([ "$major" -eq 9 ] && [ "$minor" -ge 8 ]); then echo "found"; else echo "not found"; fi
found

it just looked me unnecessary complicated, because ( ... ) executes evaluation
in a subshell (see "Grouping Commands Together" in dash(1) or "Compound
Commands" in bash(1)).

[1] https://man7.org/linux/man-pages/man1/bash.1.html
[2] https://man7.org/linux/man-pages/man1/dash.1.html

diff --git testcases/network/stress/ssh/ssh-stress.sh testcases/network/stress/ssh/ssh-stress.sh
index d5db24835d..14a4af8213 100755
--- testcases/network/stress/ssh/ssh-stress.sh
+++ testcases/network/stress/ssh/ssh-stress.sh
@@ -39,7 +39,8 @@ cleanup()
 
 setup()
 {
-	local port rc version major minor
+	local port rc version
+	local major=0 minor=0
 
 	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
 	set -- $version
@@ -64,12 +65,10 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
 HostKey $TST_TMPDIR/ssh_host_ed25519_key
 EOF
 
-	if [ -n "$major" ] && [ -n "$minor" ]; then
-		if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then
-			cat << EOF >> sshd_config
+	if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then
+		cat << EOF >> sshd_config
 PerSourcePenalties no
 EOF
-		fi
 	fi
 	ssh-keygen -q -N "" -t rsa -b 4096 -f $TST_TMPDIR/ssh_host_rsa_key
 	ssh-keygen -q -N "" -t ecdsa -f $TST_TMPDIR/ssh_host_ecdsa_key

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp