openconnect: off-by-0=true and 1=false logic

[chunkeey]

Package Name

openconnect

Maintainer

@nmav

OpenWrt Version

SNAPSHOT

OpenWrt Target/Subtarget

all

Steps to Reproduce

packages/net/openconnect/files/openconnect.sh

Lines 89 to 92 in cd5f13e

	while resolveip -t 10 "$server" > "$tmpfile" && [ "$trials" -gt 0 ]; do
	sleep 5
	trials=$((trials - 1))
	done

This took me a really long time to finally understand. But there's a subtle error hiding in plain sight here, that needlessly delays the startup of openconnect (i.e. all tries are exhausted on a system with a working connection).

The issue is that while-loop continues to run if resolveip -t 10 "$server" succeeds (i.e. it returns with 0). But it should stop once it was successful resolving the IP address.

I found that I also suck at explaining things. So please entertain this reproducer example:

while resolveip google.com; do echo repeat; sleep 1;done
We expect that on a system connected to the internet, this should not print "repeat" even once.
But when testing, it will endlessly repeat "repeat" unless it can't resolve the address:

142.251.36.174
repeat
142.251.36.174
repeat
142.251.36.174
repeat
...

whereas adding the "!" in front of resolveip:
while ! resolveip google.com; do echo repeat; sleep 1;done
Will do just what we all want in the same circumstance described above.
The problem here is that this now "looks odd".

From what I can tell, this issue came about in commit:
20ea726

But it's already really late, so sorry for not providing the fix myself at this moment.

Actual Behaviour

Not delay openconnects startup.

Confirmation Checklist

• The package is maintained in this repository.
• I understand that issues related to the base OpenWrt repository or LuCI repository will be closed.
• I am reporting an issue for OpenWrt, not an unsupported fork.

[nmav]

Thanks, your analysis seems right, would you like to submit a patch?

[chunkeey]

I think there's more though. But I don't think it's the script fault.

If the server uses a CNAME that points to an A + AAAA record. In my case, I trigger this with vpn.tik.uni-stuttgart.de which has an IPv6 + (IPv4)), then the VPN never starts and breaks in the proto_add_host_dependency "$config" "$ip" "$interface" loop.

Testcase for /etc/config/network: (yes, the baduser doesn't exists. But even with a baduser the problem can be reproduced)

config interface vpn
        option interface lan
        option proto openconnect
        option uri https://vpn.tik.uni-stuttgart.de
        option username baduser
        option password badpassword

The openconnect.sh proto for netifd always stopped at "adding host dependency for 2001:7c0:..."
and there wasn't never any any "openconnect: executing 'openconnect ' https://vpn.tik.uni-stuttgart.de ...'

Initially, I papered over this by supplying resolveip with -4. But I "think" this is because the IPv6 resolveip returns doesn't come in "[]". So by adding echo $ip | grep -q ':' && ip="[${ip}] in the loop as the first step:

packages/net/openconnect/files/openconnect.sh

Lines 94 to 99 in 57242e5

	if [ -s "$tmpfile" ]; then
	for ip in $(cat "$tmpfile"); do
	logger -t "openconnect" "adding host dependency for $ip at $config"
	proto_add_host_dependency "$config" "$ip" "$interface"
	done
	fi

now becomes:

if [ -s "$tmpfile" ]; then
	for ip in $(cat "$tmpfile"); do
         echo $ip | grep -q ':' && ip="[${ip}]"
	     logger -t "openconnect" "adding host dependency for $ip at $config"
	     proto_add_host_dependency "$config" "$ip" "$interface"
	done
fi

This now works! But couldn't this be the case with more proto_add_host_dependency callers? @nbd168

But in any case. I posted a PR #28023.

(That said: shellcheck also found some warnings/errors. If I get around, I can post a patch for this as well).