[PATCH 1/8] test-runner: remove reference to missing class member

[PATCH 1/8] test-runner: remove reference to missing class member

[James Prestwood]

The print statement was referencing self.name, which doesn't exist. Use
self.args[0] instead.
---
 tools/utils.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/utils.py b/tools/utils.py
index 66002f77..857aa1eb 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -259,7 +259,7 @@ class Process(subprocess.Popen):
 		try:
 			self.wait(timeout=15)
 		except:
-			print("Process %s did not complete in 15 seconds!" % self.name)
+			print("Process %s did not complete in 15 seconds!" % self.args[0])
 			super().kill()
 
 		self._cleanup()
-- 
2.34.1

[PATCH 2/8] test-runner: make is_process_running more accurate

[James Prestwood]

This function was checking if the process object exists, which can
persist long after a process is killed, or dies unexpectedly. Check
that the actual PID exists by sending signal 0.
---
 tools/utils.py | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/utils.py b/tools/utils.py
index 857aa1eb..7272c1f9 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -342,10 +342,24 @@ class Namespace:
 	def stop_process(self, p, force=False):
 		p.kill(force)
 
+	def _is_running(self, pid):
+		try:
+			os.kill(pid, 0)
+		except OSError:
+			return False
+
+		return True
+
 	def is_process_running(self, process):
 		for p in Process.get_all():
-			if p.namespace == self.name and p.args[0] == process:
-				return True
+			# Namespace processes are actually started by 'ip' where
+			# the actual process name is at index 4 of the arguments.
+			idx = 0 if not p.namespace else 4
+
+			if p.namespace == self.name and p.args[idx] == process:
+				# The process object exists, but make sure its
+				# actually running.
+				return self._is_running(p.pid)
 		return False
 
 	def _cleanup_dbus(self):
-- 
2.34.1

[PATCH 3/8] test-runner: fix UML blocking on wait_for_socket/service

[James Prestwood]

In UML if any process dies while test-runner is waiting for the DBus
service or some socket to be available it will block forever. This
is due to the way the non_block_wait works.

Its not optimal but it essentially polls over some input function
until the conditions are met. And, depending on the input function,
this can cause UML to hang since it never has a chance to go idle
and advance the time clock.

This can be fixed, at least for services/sockets, by sleeping in
the input function allowing time to pass. This will then allow
test-runner to bail out with an exception.

This patch adds a new wait_for_service function which handles this
automatically, and wait_for_socket was refactored to behave
similarly.
---
 autotests/util/iwd.py |  3 ---
 tools/run-tests       |  5 ++---
 tools/utils.py        | 26 ++++++++++++++++++++++++--
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index f609e37e..aae57733 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -1097,9 +1097,6 @@ class IWD(AsyncOpAbstract):
             self._iwd_proc = self.namespace.start_iwd(iwd_config_dir,
                                                         iwd_storage_dir)
 
-        ctx.non_block_wait(self._bus.name_has_owner, 20, IWD_SERVICE,
-                                exception=TimeoutError('IWD has failed to start'))
-
         self._devices = DeviceList(self)
 
         # Weak to make sure the test's reference to @self is the only counted
diff --git a/tools/run-tests b/tools/run-tests
index 2b3b1f2e..565847df 100755
--- a/tools/run-tests
+++ b/tools/run-tests
@@ -326,9 +326,8 @@ class TestContext(Namespace):
 			# register hwsim as medium
 			args.extend(['--no-register'])
 
-		self.start_process(args)
-		self.non_block_wait(self._bus.name_has_owner, 20, 'net.connman.hwsim',
-					exception=TimeoutError('net.connman.hwsim did not appear'))
+		proc = self.start_process(args)
+		proc.wait_for_service(self, 'net.connman.hwsim', 20)
 
 		for i in range(nradios):
 			name = 'rad%u' % i
diff --git a/tools/utils.py b/tools/utils.py
index 7272c1f9..f3e12a85 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -9,6 +9,7 @@ import dbus
 from gi.repository import GLib
 from weakref import WeakValueDictionary
 from re import fullmatch
+from time import sleep
 
 from runner import RunnerCoreArgParse
 
@@ -213,7 +214,24 @@ class Process(subprocess.Popen):
 		self.write_fds.append(f)
 
 	def wait_for_socket(self, socket, wait):
-		Namespace.non_block_wait(os.path.exists, wait, socket)
+		def _wait(socket):
+			if not os.path.exists(socket):
+				sleep(0.1)
+				return False
+			return True
+
+		Namespace.non_block_wait(_wait, wait, socket,
+				exception=Exception("Timed out waiting for %s" % socket))
+
+	def wait_for_service(self, ns, service, wait):
+		def _wait(ns, service):
+			if not ns._bus.name_has_owner(service):
+				sleep(0.1)
+				return False
+			return True
+
+		Namespace.non_block_wait(_wait, wait, ns, service,
+				exception=Exception("Timed out waiting for %s" % service))
 
 	# Wait for both process termination and HUP signal
 	def __wait(self, timeout):
@@ -423,7 +441,11 @@ class Namespace:
 		if Process.is_verbose('iwd-rtnl'):
 			env['IWD_RTNL_DEBUG'] = '1'
 
-		return self.start_process(args, env=env)
+		proc = self.start_process(args, env=env)
+
+		proc.wait_for_service(self, 'net.connman.iwd', 20)
+
+		return proc
 
 	@staticmethod
 	def non_block_wait(func, timeout, *args, exception=True):
-- 
2.34.1

[PATCH 4/8] auto-t: fix testEncryptedProfiles mtime check

[James Prestwood]

Yet another weird UML quirk. The intent of this tests was to ensure
the profile gets encrypted, and to check this both the mtime and
contents of the profile were checked.

But under UML the profile is copied, IWD started, and the profile
is encrypted all without any time passing. The (same) mtime was
then updated without any changes which fails the mtime check.

This puts a sleep after copying the profile to ensure the system
time differs once IWD encrypts the profile.
---
 autotests/testEncryptedProfiles/connection_test.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/autotests/testEncryptedProfiles/connection_test.py b/autotests/testEncryptedProfiles/connection_test.py
index 2b846596..ab5a0ae7 100644
--- a/autotests/testEncryptedProfiles/connection_test.py
+++ b/autotests/testEncryptedProfiles/connection_test.py
@@ -6,6 +6,7 @@ import sys
 sys.path.append('../util')
 import iwd
 import os
+from time import sleep
 from iwd import IWD
 from iwd import NetworkType
 from iwd import PSKAgent
@@ -48,6 +49,8 @@ class Test(unittest.TestCase):
         mtime = os.path.getmtime('/tmp/iwd/' + 'ssidCCMP.psk')
         self.assertFalse(self.profile_is_encrypted('ssidCCMP.psk'))
 
+        sleep(1)
+
         wd = IWD(True)
 
         # Make sure profile was accepted
-- 
2.34.1

[PATCH 5/8] auto-t: fix rekey/reauth logic in a few ways

[James Prestwood]

The rekey/reauth logic was broken in a few different ways.

For rekeys the event list was not being reset so any past 4-way
handshake would allow the call to pass. This actually removes
the need for the sleep in the extended key ID test because the
actual handshake event is waited for correctly.

For both rekeys and reauths, just waiting for the EAP/handshake
events was not enough. Without checking if the client got
disconnected we essentially allow a full disconnect and reconnect,
meaning the rekey/reauth failed.

Now a 'disallow' array can be passed to wait_for_event which will
throw an exception if any events in that array are encountered
while waiting for the target event.
---
 autotests/testWPA2-ext-key-id/connection_test.py |  2 --
 autotests/util/hostapd.py                        | 16 ++++++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/autotests/testWPA2-ext-key-id/connection_test.py b/autotests/testWPA2-ext-key-id/connection_test.py
index d77a216b..afd48149 100644
--- a/autotests/testWPA2-ext-key-id/connection_test.py
+++ b/autotests/testWPA2-ext-key-id/connection_test.py
@@ -50,8 +50,6 @@ class Test(unittest.TestCase):
             testutil.test_iface_operstate()
             testutil.test_ifaces_connected()
 
-            wd.wait(0.5)
-
         device.disconnect()
 
         condition = 'not obj.connected'
diff --git a/autotests/util/hostapd.py b/autotests/util/hostapd.py
index ff5a97c1..758427fe 100644
--- a/autotests/util/hostapd.py
+++ b/autotests/util/hostapd.py
@@ -102,23 +102,26 @@ class HostapdCLI(object):
 
         return True
 
-    def _poll_event(self, event):
+    def _poll_event(self, event, disallow):
         # Look through the list (most recent is first) until the even is found.
         # Once found consume this event and any older ones as to not
         # accidentally trigger a false positive later on.
         for idx, e in enumerate(self.events):
+            for d in disallow:
+                if d in e:
+                    raise Exception('Event %s found while waiting for %s' % (d, event))
             if event in e:
                 self.events = self.events[:idx]
                 return e
 
         return False
 
-    def wait_for_event(self, event, timeout=10):
+    def wait_for_event(self, event, timeout=10, disallow=[]):
         if event == 'AP-ENABLED':
             if self.enabled:
                 return 'AP-ENABLED'
 
-        return ctx.non_block_wait(self._poll_event, timeout, event,
+        return ctx.non_block_wait(self._poll_event, timeout, event, disallow,
                                     exception=TimeoutError("waiting for event"))
 
     def _data_available(self):
@@ -166,8 +169,8 @@ class HostapdCLI(object):
         self.events = []
         cmd = 'EAPOL_REAUTH ' + client_address
         self.ctrl_sock.sendall(cmd.encode('utf-8'))
-        self.wait_for_event('CTRL-EVENT-EAP-STARTED')
-        self.wait_for_event('CTRL-EVENT-EAP-SUCCESS')
+        self.wait_for_event('CTRL-EVENT-EAP-STARTED', disallow=['AP-STA-DISCONNECTED'])
+        self.wait_for_event('CTRL-EVENT-EAP-SUCCESS', disallow=['AP-STA-DISCONNECTED'])
 
     def reload(self):
         # Seemingly all three commands needed for the instance to notice
@@ -234,7 +237,8 @@ class HostapdCLI(object):
         if address:
             cmd = 'REKEY_PTK %s' % address
             self.ctrl_sock.sendall(cmd.encode('utf-8'))
-            self.wait_for_event('EAPOL-4WAY-HS-COMPLETED')
+            self.events = []
+            self.wait_for_event('EAPOL-4WAY-HS-COMPLETED', disallow=['AP-STA-DISCONNECTED'])
             return
 
         cmd = 'REKEY_GTK'
-- 
2.34.1

[PATCH 6/8] auto-t: remove sleep in testAgent

[James Prestwood]

The test here is verifying that a DBus Connect() call will still
work with 'other' agents registered. In this case it uses iwctl to
set a password, then call Connect() manually.

The problem here is that we have no way of knowing when iwctl fully
starts and registers its agent. There was a sleep in there but that
is unreliable and we occationally were still getting past that without
iwctl having started fully.

To fix this properly we need to wait for iwctl's agent service to appear
on the bus. Since the bus name is unknown we must first find all names,
then cross reference their PID's against the iwctl PID. This is done
using ListNames, and GetConnectionUnixProcessID APIs.
---
 autotests/testAgent/agent_test.py | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/autotests/testAgent/agent_test.py b/autotests/testAgent/agent_test.py
index e2d6e49e..c1dc8c7c 100644
--- a/autotests/testAgent/agent_test.py
+++ b/autotests/testAgent/agent_test.py
@@ -2,6 +2,7 @@
 
 import unittest
 import sys
+import dbus
 
 sys.path.append('../util')
 import iwd
@@ -56,11 +57,26 @@ class Test(unittest.TestCase):
         IWD.clear_storage()
 
     def test_connection_with_other_agent(self):
+        def wait_for_service_pid(pid):
+            dbus_object = ctx._bus.get_object('org.freedesktop.DBus',
+                                 '/org/freedesktop/DBus')
+            dbus_iface = dbus.Interface(dbus_object, 'org.freedesktop.DBus')
+
+            services = dbus_iface.ListNames()
+
+            for service in services:
+                bus_iface = dbus.Interface(dbus_object, "org.freedesktop.DBus")
+                if pid == int(bus_iface.GetConnectionUnixProcessID(service)):
+                    return True
+
+            return False
+
         wd = IWD()
 
         iwctl = ctx.start_process(['iwctl', '-P', 'secret_ssid2'])
+
         # Let iwctl to start and register its agent.
-        wd.wait(2)
+        ctx.non_block_wait(wait_for_service_pid, 10, iwctl.pid)
 
         self.check_connection(wd, 'ssid2')
 
-- 
2.34.1

[PATCH 7/8] eapol: allow 'secure' to be set on rekeys

[James Prestwood]

About a month ago hostapd was changed to set the secure bit on
eapol frames during rekeys (bc36991791). The spec is ambiguous
about this and has conflicting info depending on the sections you
read (12.7.2 vs 12.7.6). According to the hostapd commit log TGme
is trying to clarify this and wants to set secure=1 in the case
of rekeys. Because of this, IWD is completely broken with rekeys
since its disallows secure=1 on PTK 1/4 and 2/4.

Now, a bool is passed to the verify functions which signifies if
the PTK has been negotiated already. If secure differs from this
the key frame is not verified.
---
 src/eapol.c | 14 ++++++++------
 src/eapol.h |  5 +++--
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index 9f885d02..e8bd5cdb 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -443,7 +443,8 @@ static void eapol_key_data_append(struct eapol_key *ek,
 	if (ek->error)		\
 		return false	\
 
-bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len)
+bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len,
+				bool ptk_complete)
 {
 	/* Verify according to 802.11, Section 11.6.6.2 */
 	VERIFY_PTK_COMMON(ek);
@@ -457,7 +458,7 @@ bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len)
 	if (ek->key_mic)
 		return false;
 
-	if (ek->secure)
+	if (ek->secure != ptk_complete)
 		return false;
 
 	if (ek->encrypted_key_data)
@@ -475,7 +476,7 @@ bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len)
 	return true;
 }
 
-bool eapol_verify_ptk_2_of_4(const struct eapol_key *ek)
+bool eapol_verify_ptk_2_of_4(const struct eapol_key *ek, bool ptk_complete)
 {
 	uint16_t key_len;
 
@@ -491,7 +492,7 @@ bool eapol_verify_ptk_2_of_4(const struct eapol_key *ek)
 	if (!ek->key_mic)
 		return false;
 
-	if (ek->secure)
+	if (ek->secure != ptk_complete)
 		return false;
 
 	if (ek->encrypted_key_data)
@@ -1151,7 +1152,8 @@ static void eapol_handle_ptk_1_of_4(struct eapol_sm *sm,
 
 	l_debug("ifindex=%u", sm->handshake->ifindex);
 
-	if (!eapol_verify_ptk_1_of_4(ek, sm->mic_len))
+	if (!eapol_verify_ptk_1_of_4(ek, sm->mic_len,
+					sm->handshake->ptk_complete))
 		return;
 
 	if (sm->handshake->ptk_complete && unencrypted) {
@@ -1523,7 +1525,7 @@ static void eapol_handle_ptk_2_of_4(struct eapol_sm *sm,
 
 	l_debug("ifindex=%u", sm->handshake->ifindex);
 
-	if (!eapol_verify_ptk_2_of_4(ek))
+	if (!eapol_verify_ptk_2_of_4(ek, sm->handshake->ptk_complete))
 		return;
 
 	if (L_BE64_TO_CPU(ek->key_replay_counter) != sm->replay_counter)
diff --git a/src/eapol.h b/src/eapol.h
index 53ae6e8f..8d8d5252 100644
--- a/src/eapol.h
+++ b/src/eapol.h
@@ -65,8 +65,9 @@ uint8_t *eapol_decrypt_key_data(enum ie_rsn_akm_suite akm, const uint8_t *kek,
 				const struct eapol_key *frame,
 				size_t *decrypted_size, size_t mic_len);
 
-bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len);
-bool eapol_verify_ptk_2_of_4(const struct eapol_key *ek);
+bool eapol_verify_ptk_1_of_4(const struct eapol_key *ek, size_t mic_len,
+				bool ptk_complete);
+bool eapol_verify_ptk_2_of_4(const struct eapol_key *ek, bool ptk_complete);
 bool eapol_verify_ptk_3_of_4(const struct eapol_key *ek, bool is_wpa,
 				size_t mic_len);
 bool eapol_verify_ptk_4_of_4(const struct eapol_key *ek, bool is_wpa);
-- 
2.34.1

[PATCH 8/8] unit: update test-eapol to use the new ptk verify APIs

[James Prestwood]

---
 unit/test-eapol.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/unit/test-eapol.c b/unit/test-eapol.c
index d3cfac11..227e485f 100644
--- a/unit/test-eapol.c
+++ b/unit/test-eapol.c
@@ -1768,13 +1768,13 @@ static void eapol_4way_test(const void *data)
 	step1 = eapol_key_validate(eapol_key_data_3,
 					sizeof(eapol_key_data_3), 16);
 	assert(step1);
-	assert(eapol_verify_ptk_1_of_4(step1, 16));
+	assert(eapol_verify_ptk_1_of_4(step1, 16, false));
 	memcpy(anonce, step1->key_nonce, sizeof(step1->key_nonce));
 
 	step2 = eapol_key_validate(eapol_key_data_4,
 					sizeof(eapol_key_data_4), 16);
 	assert(step2);
-	assert(eapol_verify_ptk_2_of_4(step2));
+	assert(eapol_verify_ptk_2_of_4(step2, false));
 	memcpy(snonce, step2->key_nonce, sizeof(step2->key_nonce));
 
 	assert(!crypto_psk_from_passphrase(passphrase, (uint8_t *) ssid,
@@ -1853,13 +1853,13 @@ static void eapol_wpa2_handshake_test(const void *data)
 	ptk_step1 = eapol_key_validate(eapol_key_data_7,
 					sizeof(eapol_key_data_7), 16);
 	assert(ptk_step1);
-	assert(eapol_verify_ptk_1_of_4(ptk_step1, 16));
+	assert(eapol_verify_ptk_1_of_4(ptk_step1, 16, false));
 	memcpy(anonce, ptk_step1->key_nonce, sizeof(ptk_step1->key_nonce));
 
 	ptk_step2 = eapol_key_validate(eapol_key_data_8,
 					sizeof(eapol_key_data_8), 16);
 	assert(ptk_step2);
-	assert(eapol_verify_ptk_2_of_4(ptk_step2));
+	assert(eapol_verify_ptk_2_of_4(ptk_step2, false));
 	memcpy(snonce, ptk_step2->key_nonce, sizeof(ptk_step2->key_nonce));
 
 	assert(!crypto_psk_from_passphrase(passphrase, (uint8_t *) ssid,
@@ -1983,13 +1983,13 @@ static void eapol_wpa_handshake_test(const void *data)
 	ptk_step1 = eapol_key_validate(eapol_key_data_13,
 					sizeof(eapol_key_data_13), 16);
 	assert(ptk_step1);
-	assert(eapol_verify_ptk_1_of_4(ptk_step1, 16));
+	assert(eapol_verify_ptk_1_of_4(ptk_step1, 16, false));
 	memcpy(anonce, ptk_step1->key_nonce, sizeof(ptk_step1->key_nonce));
 
 	ptk_step2 = eapol_key_validate(eapol_key_data_14,
 					sizeof(eapol_key_data_14), 16);
 	assert(ptk_step2);
-	assert(eapol_verify_ptk_2_of_4(ptk_step2));
+	assert(eapol_verify_ptk_2_of_4(ptk_step2, false));
 	memcpy(snonce, ptk_step2->key_nonce, sizeof(ptk_step2->key_nonce));
 
 	assert(!crypto_psk_from_passphrase(passphrase, (uint8_t *) ssid,
-- 
2.34.1

Re: [PATCH 1/8] test-runner: remove reference to missing class member

[Denis Kenzior]

Hi James,

On 6/24/22 18:07, James Prestwood wrote:
> The print statement was referencing self.name, which doesn't exist. Use
> self.args[0] instead.
> ---
>   tools/utils.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

All applied, thanks.

Regards,
-Denis