[PATCH 01/13] auto-t: no hostapd instance graceful failure

[PATCH 01/13] auto-t: no hostapd instance graceful failure

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

If a test does not need any hostapd instances but still loads
hostapd.py for some reason we want to gracefully throw an
exception rather than fail in some other manor.
---
 autotests/util/hostapd.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/autotests/util/hostapd.py b/autotests/util/hostapd.py
index 7c0d8385..cf82c010 100644
--- a/autotests/util/hostapd.py
+++ b/autotests/util/hostapd.py
@@ -32,6 +32,10 @@ class HostapdCLI:
     def _init_hostapd(self, config=None):
         global ctrl_count
         interface = None
+        self.ctrl_sock = None
+
+        if not ctx.hostapd:
+            raise Exception("No hostapd instances are configured")
 
         if not config and len(ctx.hostapd.instances) > 1:
             raise Exception('config must be provided if more than one hostapd instance exists')
@@ -110,6 +114,9 @@ class HostapdCLI:
         raise Exception('timeout waiting for control response')
 
     def _del_hostapd(self, force=False):
+        if not self.ctrl_sock:
+            return
+
         self.ctrl_sock.close()
         os.remove(self.local_ctrl)
 
-- 
2.26.2

[PATCH 02/13] auto-t: add copy_to_ap utility

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

---
 autotests/util/iwd.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index dc4f93a2..fb22412d 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -1027,6 +1027,7 @@ class IWD(AsyncOpAbstract):
     def clear_storage(storage_dir=IWD_STORAGE_DIR):
         os.system('rm -rf ' + storage_dir + '/*')
         os.system('rm -rf ' + storage_dir + '/hotspot/*')
+        os.system('rm -rf ' + storage_dir + '/ap/*')
 
     @staticmethod
     def create_in_storage(file_name, file_content):
@@ -1054,6 +1055,13 @@ class IWD(AsyncOpAbstract):
 
         shutil.copy(source, IWD_STORAGE_DIR + "/hotspot")
 
+    @staticmethod
+    def copy_to_ap(source):
+        if not os.path.exists(IWD_STORAGE_DIR + "/ap"):
+            os.mkdir(IWD_STORAGE_DIR + "/ap")
+
+        IWD.copy_to_storage(source, IWD_STORAGE_DIR + '/ap/')
+
     @staticmethod
     def remove_from_storage(file_name):
         os.system('rm -rf ' + IWD_STORAGE_DIR + '/\'' + file_name + '\'')
-- 
2.26.2

[PATCH 03/13] auto-t: simplify copy_to_hotspot

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

Since copy_to_storage can take an arbitrary destination path now
we can reuse that instead of duplicating code.
---
 autotests/util/iwd.py | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index fb22412d..9bfc8c1a 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -1046,14 +1046,10 @@ class IWD(AsyncOpAbstract):
 
     @staticmethod
     def copy_to_hotspot(source):
-        import shutil
-
-        assert not os.path.isabs(source)
-
         if not os.path.exists(IWD_STORAGE_DIR + "/hotspot"):
             os.mkdir(IWD_STORAGE_DIR + "/hotspot")
 
-        shutil.copy(source, IWD_STORAGE_DIR + "/hotspot")
+        IWD.copy_to_storage(source, IWD_STORAGE_DIR + "/hotspot")
 
     @staticmethod
     def copy_to_ap(source):
-- 
2.26.2

[PATCH 04/13] storage: allow NULL type on storage_network_ssid_from_path

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

This is to allow this API to be used simply to get the file name
without extension. AP will be storing provisioning files similarly
to the network format but the extension will not map to existing
security types. For this case it makes sense to allow a NULL type
which will prevent the security type from being set/parsed.
---
 src/storage.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/storage.c b/src/storage.c
index 00d93933..9ef84e32 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -287,6 +287,11 @@ char *storage_get_network_file_path(enum security type, const char *ssid)
 	return path;
 }
 
+/*
+ * Get filename (without extension) which IWD treats as an SSID. Optionally
+ * parse the extension to get the security type (*.psk, *.open, *.8021x) by
+ * providing a 'type' pointer.
+ */
 const char *storage_network_ssid_from_path(const char *path,
 							enum security *type)
 {
@@ -302,7 +307,10 @@ const char *storage_network_ssid_from_path(const char *path,
 
 	end = strchr(filename, '.');
 
-	if (!end || !security_from_str(end + 1, type))
+	if (!end)
+		return NULL;
+
+	if (type && !security_from_str(end + 1, type))
 		return NULL;
 
 	if (filename[0] != '=') {
-- 
2.26.2

Re: [PATCH 04/13] storage: allow NULL type on storage_network_ssid_from_path

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

Hi James,

On 10/20/20 1:02 PM, James Prestwood wrote:
> This is to allow this API to be used simply to get the file name
> without extension. AP will be storing provisioning files similarly
> to the network format but the extension will not map to existing
> security types. For this case it makes sense to allow a NULL type
> which will prevent the security type from being set/parsed.
> ---
>   src/storage.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/storage.c b/src/storage.c
> index 00d93933..9ef84e32 100644
> --- a/src/storage.c
> +++ b/src/storage.c
> @@ -287,6 +287,11 @@ char *storage_get_network_file_path(enum security type, const char *ssid)
>   	return path;
>   }
>   
> +/*
> + * Get filename (without extension) which IWD treats as an SSID. Optionally
> + * parse the extension to get the security type (*.psk, *.open, *.8021x) by
> + * providing a 'type' pointer.
> + */
>   const char *storage_network_ssid_from_path(const char *path,
>   							enum security *type)

No, please don't do this.  Make up a AP specific API function instead and 
commonize the implementation if needed.

>   {
> @@ -302,7 +307,10 @@ const char *storage_network_ssid_from_path(const char *path,
>   
>   	end = strchr(filename, '.');
>   
> -	if (!end || !security_from_str(end + 1, type))
> +	if (!end)
> +		return NULL;
> +
> +	if (type && !security_from_str(end + 1, type))
>   		return NULL;
>   
>   	if (filename[0] != '=') {
> 

Regards,
-Denis

[PATCH 05/13] storage: add storage_get_ap_path

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

---
 src/storage.c | 30 ++++++++++++++++++++++++++++++
 src/storage.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/src/storage.c b/src/storage.c
index 9ef84e32..b509e161 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -50,6 +50,7 @@
 
 static char *storage_path = NULL;
 static char *storage_hotspot_path = NULL;
+static char *storage_ap_path = NULL;
 
 static int create_dirs(const char *filename)
 {
@@ -221,6 +222,16 @@ bool storage_create_dirs(void)
 		return false;
 	}
 
+	storage_ap_path = l_strdup_printf("%s/ap/", storage_path);
+
+	if (create_dirs(storage_ap_path)) {
+		l_error("Failed to create %s", storage_ap_path);
+
+		l_free(storage_path);
+		l_free(storage_hotspot_path);
+		l_free(storage_ap_path);
+	}
+
 	return true;
 }
 
@@ -228,6 +239,7 @@ void storage_cleanup_dirs(void)
 {
 	l_free(storage_path);
 	l_free(storage_hotspot_path);
+	l_free(storage_ap_path);
 }
 
 char *storage_get_path(const char *format, ...)
@@ -266,6 +278,24 @@ char *storage_get_hotspot_path(const char *format, ...)
 	return str;
 }
 
+char *storage_get_ap_path(const char *format, ...)
+{
+	va_list args;
+	char *fmt, *str;
+
+	if (!format)
+		return l_strdup(storage_ap_path);
+
+	fmt = l_strdup_printf("%s/%s", storage_ap_path, format);
+
+	va_start(args, format);
+	str = l_strdup_vprintf(fmt, args);
+	va_end(args);
+
+	l_free(fmt);
+	return str;
+}
+
 char *storage_get_network_file_path(enum security type, const char *ssid)
 {
 	char *path;
diff --git a/src/storage.h b/src/storage.h
index 80b63c53..54c81d10 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -36,6 +36,7 @@ bool storage_create_dirs(void);
 void storage_cleanup_dirs(void);
 char *storage_get_path(const char *format, ...);
 char *storage_get_hotspot_path(const char *format, ...);
+char *storage_get_ap_path(const char *format, ...);
 
 const char *storage_network_ssid_from_path(const char *path,
 							enum security *type);
-- 
2.26.2

[PATCH 06/13] ap: refactor AP to use provisioning files

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5672 bytes --]

Currently the AP DBus APIs take an SSID and passphrase to start
an access point. This works for basic access points but isn't very
scaleable once other AP options are added.

Instead we can define AP networks similar to how we define station
networks by providing a provisioning file. The provisioning files
for AP will reside in $STATE_DIRECTORY/ap (/var/lib/iwd/ap by
default). The name of the file indicates the SSID and, for now,
the only value will be [Security].Passphrase to match the DBus
API. The file extension does not matter at this point, but in tests
and examples *.ap will be used.

AP provisioning files will be loaded in at startup and remain as
static configs in a queue. When Start() is called the ssid will be
looked up and that config will be used. If no config is found
a "Not Found" error will be returned.
---
 src/ap.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 src/ap.h |   1 +
 2 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 3c4ae907..9df6e14e 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -26,6 +26,7 @@
 
 #include <errno.h>
 #include <linux/if_ether.h>
+#include <dirent.h>
 
 #include <ell/ell.h>
 
@@ -49,6 +50,7 @@
 #include "src/wscutil.h"
 #include "src/eap-wsc.h"
 #include "src/ap.h"
+#include "src/storage.h"
 
 struct ap_state {
 	struct netdev *netdev;
@@ -105,6 +107,7 @@ struct ap_wsc_pbc_probe_record {
 };
 
 static uint32_t netdev_watch;
+static struct l_queue *ap_configs;
 
 void ap_config_free(struct ap_config *config)
 {
@@ -186,7 +189,10 @@ static void ap_reset(struct ap_state *ap)
 	if (ap->rates)
 		l_uintset_free(ap->rates);
 
-	ap_config_free(ap->config);
+
+	if (!ap->config->no_free)
+		ap_config_free(ap->config);
+
 	ap->config = NULL;
 
 	l_queue_destroy(ap->wsc_pbc_probes, l_free);
@@ -2376,6 +2382,14 @@ static const struct ap_ops ap_dbus_ops = {
 	.handle_event = ap_if_event_func,
 };
 
+static bool match_config_ssid(const void *data, const void *user_data)
+{
+	const struct ap_config *config = data;
+	const char *ssid = user_data;
+
+	return !strcmp(config->ssid, ssid);
+}
+
 static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 		struct l_dbus_message *message, void *user_data)
 {
@@ -2393,16 +2407,13 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 						&ssid, &wpa2_passphrase))
 		return dbus_error_invalid_args(message);
 
-	config = l_new(struct ap_config, 1);
-	config->ssid = l_strdup(ssid);
-	l_strlcpy(config->passphrase, wpa2_passphrase,
-			sizeof(config->passphrase));
+	config = l_queue_find(ap_configs, match_config_ssid, ssid);
+	if (!config)
+		return dbus_error_not_found(message);
 
 	ap_if->ap = ap_start(ap_if->netdev, config, &ap_dbus_ops, ap_if);
-	if (!ap_if->ap) {
-		ap_config_free(config);
+	if (!ap_if->ap)
 		return dbus_error_invalid_args(message);
-	}
 
 	ap_if->pending = l_dbus_message_ref(message);
 	return NULL;
@@ -2528,13 +2539,90 @@ static void ap_netdev_watch(struct netdev *netdev,
 	}
 }
 
+static struct ap_config *ap_config_from_settings(struct l_settings *settings,
+							const char *ssid)
+{
+	struct ap_config *config = l_new(struct ap_config, 1);
+	char *passphrase;
+
+	/*
+	 * Dont free when ap interface goes up/down. This is a static config
+	 * that we want to keep around until IWD stops.
+	 */
+	config->no_free = true;
+	config->ssid = l_strdup(ssid);
+
+	passphrase = l_settings_get_string(settings, "Security", "Passphrase");
+	if (passphrase) {
+		if (strlen(passphrase) > 63) {
+			l_error("[Security].Passphrase must not exceed "
+					"63 characters");
+			goto failed;
+		}
+
+		strcpy(config->passphrase, passphrase);
+		l_free(passphrase);
+	}
+
+done:
+	l_info("Loaded AP configuration %s", config->ssid);
+	return config;
+
+failed:
+	ap_config_free(config);
+	return NULL;
+}
+
 static int ap_init(void)
 {
+	DIR *dir;
+	struct dirent *dirent;
+
+	L_AUTO_FREE_VAR(char *, ap_dir) = storage_get_ap_path(NULL);
+
 	netdev_watch = netdev_watch_add(ap_netdev_watch, NULL, NULL);
 
 	l_dbus_register_interface(dbus_get_bus(), IWD_AP_INTERFACE,
 			ap_setup_interface, ap_destroy_interface, false);
 
+	ap_configs = l_queue_new();
+
+	dir = opendir(ap_dir);
+	if (!dir)
+		return -ENOENT;
+
+	while ((dirent = readdir(dir))) {
+		struct ap_config *config;
+		struct l_settings *s;
+		char *filename;
+		const char *ssid;
+
+		if (dirent->d_type != DT_REG && dirent->d_type != DT_LNK)
+			continue;
+
+		filename = l_strdup_printf("%s/%s", ap_dir, dirent->d_name);
+		s = l_settings_new();
+
+		if (!l_settings_load_from_file(s, filename))
+			goto next;
+
+		ssid = storage_network_ssid_from_path(filename, NULL);
+
+		config = ap_config_from_settings(s, ssid);
+		if (!config)
+			goto next;
+
+		l_debug("loaded ap config %s", filename);
+
+		l_queue_push_head(ap_configs, config);
+
+next:
+		l_free(filename);
+		l_settings_free(s);
+	}
+
+	closedir(dir);
+
 	return 0;
 }
 
@@ -2542,6 +2630,8 @@ static void ap_exit(void)
 {
 	netdev_watch_remove(netdev_watch);
 	l_dbus_unregister_interface(dbus_get_bus(), IWD_AP_INTERFACE);
+
+	l_queue_destroy(ap_configs, (l_queue_destroy_func_t) ap_config_free);
 }
 
 IWD_MODULE(ap, ap_init, ap_exit)
diff --git a/src/ap.h b/src/ap.h
index a39797aa..90497008 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -64,6 +64,7 @@ struct ap_config {
 	char *wsc_name;
 	struct wsc_primary_device_type wsc_primary_device_type;
 	bool no_cck_rates : 1;
+	bool no_free : 1;
 };
 
 struct ap_ops {
-- 
2.26.2

[PATCH 07/13] doc: update AP docs with new Start() arguments

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

The 'psk' argument has now been removed and replaced with a
dedicated provisioning file which should contain the psk.
---
 doc/access-point-api.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/doc/access-point-api.txt b/doc/access-point-api.txt
index 9ee06baa..7d504b90 100644
--- a/doc/access-point-api.txt
+++ b/doc/access-point-api.txt
@@ -5,15 +5,17 @@ Service		net.connman.iwd
 Interface	net.connman.iwd.AccessPoint [Experimental]
 Object path	/net/connman/iwd/{phy0,phy1,...}/{1,2,...}
 
-Methods		void Start(string ssid, string psk)
+Methods		void Start(string ssid)
 
-			Start an access point called ssid with a passphrase
-			of psk.
+			Start an access point called ssid. A network
+			provisioning file must exist in /var/lib/iwd/ap/ who's
+			file name matches the ssid in the DBus message.
 
 			Possible errors:	net.connman.iwd.Busy
 						net.connman.iwd.Failed
 						net.connman.iwd.InvalidArguments
 						net.connman.iwd.AlreadyExists
+						net.connman.iwd.NotFound
 
 		void Stop()
 
-- 
2.26.2

[PATCH 08/13] ap: remove 'psk' from Start()

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

This has been replaced by dedicated provisioning files.
---
 src/ap.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 9df6e14e..41de69a9 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -2394,7 +2394,7 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 		struct l_dbus_message *message, void *user_data)
 {
 	struct ap_if_data *ap_if = user_data;
-	const char *ssid, *wpa2_passphrase;
+	const char *ssid;
 	struct ap_config *config;
 
 	if (ap_if->ap && ap_if->ap->started)
@@ -2403,8 +2403,7 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 	if (ap_if->ap || ap_if->pending)
 		return dbus_error_busy(message);
 
-	if (!l_dbus_message_get_arguments(message, "ss",
-						&ssid, &wpa2_passphrase))
+	if (!l_dbus_message_get_arguments(message, "s", &ssid))
 		return dbus_error_invalid_args(message);
 
 	config = l_queue_find(ap_configs, match_config_ssid, ssid);
@@ -2473,7 +2472,7 @@ static bool ap_dbus_property_get_started(struct l_dbus *dbus,
 static void ap_setup_interface(struct l_dbus_interface *interface)
 {
 	l_dbus_interface_method(interface, "Start", 0, ap_dbus_start, "",
-			"ss", "ssid", "wpa2_passphrase");
+			"s", "ssid");
 	l_dbus_interface_method(interface, "Stop", 0, ap_dbus_stop, "", "");
 
 	l_dbus_interface_property(interface, "Started", 0, "b",
-- 
2.26.2

Re: [PATCH 08/13] ap: remove 'psk' from Start()

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

Hi James,

On Tue, 20 Oct 2020 at 20:03, James Prestwood <prestwoj@gmail.com> wrote:
>
> This has been replaced by dedicated provisioning files.

This is going to break Network Manager and an additional problem is
that clients have no way to tell which version of IWD they're talking
to.

Currently clients can get a basic AP set up with just one method call
and with this change they will have to use two different APIs:
filesystem + D-Bus.  Can we make the config file optional?  In my
opinion this is a significant complication for the typical use-case
(Android-style UI), while in most other scenarios hostapd is going to
work better.

Best regards

Re: [PATCH 08/13] ap: remove 'psk' from Start()

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

Hi Andrew,

On Tue, 2020-10-20 at 22:19 +0200, Andrew Zaborowski wrote:
> Hi James,
> 
> On Tue, 20 Oct 2020 at 20:03, James Prestwood <prestwoj@gmail.com>
> wrote:
> > This has been replaced by dedicated provisioning files.
> 
> This is going to break Network Manager and an additional problem is
> that clients have no way to tell which version of IWD they're talking
> to.
> 
> Currently clients can get a basic AP set up with just one method call
> and with this change they will have to use two different APIs:
> filesystem + D-Bus.  Can we make the config file optional?  In my
> opinion this is a significant complication for the typical use-case
> (Android-style UI), while in most other scenarios hostapd is going to
> work better.

Yes Denis and I discussed this and came to the same conclusion. Better
to have a quick and easy Start() API and probably add a sparate API for
starting with a config file.

But if Network Manager is actually using this API we should probably
remove the "experimental" label from the docs. I wasn't aware of this
which is why I thought it was fine modifying it.

Thanks,
James

> 
> Best regards

[PATCH 09/13] auto-t: update start_ap() to remove psk argument

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

---
 autotests/util/iwd.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index 9bfc8c1a..c1349bbd 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -467,7 +467,7 @@ class Device(IWDDBusAbstract):
                                         error_handler=self._failure)
         self._wait_for_async_op()
 
-    def start_ap(self, ssid, psk):
+    def start_ap(self, ssid):
         try:
             self._prop_proxy.Set(IWD_DEVICE_INTERFACE, 'Mode', 'ap')
         except Exception as e:
@@ -476,7 +476,7 @@ class Device(IWDDBusAbstract):
         self._ap_iface = dbus.Interface(self._bus.get_object(IWD_SERVICE,
                                             self.device_path),
                                             IWD_AP_INTERFACE)
-        self._ap_iface.Start(ssid, psk, reply_handler=self._success,
+        self._ap_iface.Start(ssid, reply_handler=self._success,
                                      error_handler=self._failure)
         self._wait_for_async_op()
 
-- 
2.26.2

[PATCH 10/13] auto-t: update AP tests with provisioning files

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4338 bytes --]

Also removed some unused config values which are no longer needed
after the test-runner re-write.
---
 autotests/testAP-no-support/TestAP2.ap         | 2 ++
 autotests/testAP-no-support/connection_test.py | 6 +++---
 autotests/testAP-no-support/hw.conf            | 2 +-
 autotests/testAP/TestAP2.ap                    | 2 ++
 autotests/testAP/connection_test.py            | 5 +++--
 autotests/testAP/failure_test.py               | 5 +++--
 autotests/testAP/hw.conf                       | 3 +--
 7 files changed, 15 insertions(+), 10 deletions(-)
 create mode 100644 autotests/testAP-no-support/TestAP2.ap
 create mode 100644 autotests/testAP/TestAP2.ap

diff --git a/autotests/testAP-no-support/TestAP2.ap b/autotests/testAP-no-support/TestAP2.ap
new file mode 100644
index 00000000..27f086cb
--- /dev/null
+++ b/autotests/testAP-no-support/TestAP2.ap
@@ -0,0 +1,2 @@
+[Security]
+Passphrase=Password2
diff --git a/autotests/testAP-no-support/connection_test.py b/autotests/testAP-no-support/connection_test.py
index 1bbc0bde..ff6fdb25 100644
--- a/autotests/testAP-no-support/connection_test.py
+++ b/autotests/testAP-no-support/connection_test.py
@@ -9,16 +9,16 @@ from iwd import IWD
 
 class Test(unittest.TestCase):
     def test_connection_success(self):
-        wd = IWD()
+        wd = IWD(True)
 
         dev = wd.list_devices(1)[0]
 
         with self.assertRaises(iwd.NotSupportedEx):
-            dev.start_ap('TestAP2', 'Password2')
+            dev.start_ap('TestAP2')
 
     @classmethod
     def setUpClass(cls):
-        pass
+        IWD.copy_to_ap('TestAP2.ap')
 
     @classmethod
     def tearDownClass(cls):
diff --git a/autotests/testAP-no-support/hw.conf b/autotests/testAP-no-support/hw.conf
index 9df3b349..93177bb6 100644
--- a/autotests/testAP-no-support/hw.conf
+++ b/autotests/testAP-no-support/hw.conf
@@ -1,6 +1,6 @@
 [SETUP]
 num_radios=1
-radio_confs=rad0
+start_iwd=0
 
 [rad0]
 iftype_disable=ap
diff --git a/autotests/testAP/TestAP2.ap b/autotests/testAP/TestAP2.ap
new file mode 100644
index 00000000..27f086cb
--- /dev/null
+++ b/autotests/testAP/TestAP2.ap
@@ -0,0 +1,2 @@
+[Security]
+Passphrase=Password2
diff --git a/autotests/testAP/connection_test.py b/autotests/testAP/connection_test.py
index c3744c55..814ed070 100644
--- a/autotests/testAP/connection_test.py
+++ b/autotests/testAP/connection_test.py
@@ -38,13 +38,13 @@ class Test(unittest.TestCase):
         wd.wait_for_object_condition(ordered_network.network_object, condition)
 
     def test_connection_success(self):
-        wd = IWD()
+        wd = IWD(True)
 
         dev1, dev2 = wd.list_devices(2)
 
         self.client_connect(wd, dev1)
 
-        dev1.start_ap('TestAP2', 'Password2')
+        dev1.start_ap('TestAP2')
 
         try:
             condition = 'not obj.scanning'
@@ -96,6 +96,7 @@ class Test(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
         IWD.copy_to_storage('TestAP1.psk')
+        IWD.copy_to_ap('TestAP2.ap')
 
     @classmethod
     def tearDownClass(cls):
diff --git a/autotests/testAP/failure_test.py b/autotests/testAP/failure_test.py
index 4959802f..7c3290b0 100644
--- a/autotests/testAP/failure_test.py
+++ b/autotests/testAP/failure_test.py
@@ -39,13 +39,13 @@ class Test(unittest.TestCase):
         wd.wait_for_object_condition(ordered_network.network_object, condition)
 
     def test_connection_failure(self):
-        wd = IWD()
+        wd = IWD(True)
 
         dev1, dev2 = wd.list_devices(2)
 
         self.client_connect(wd, dev1)
 
-        dev1.start_ap('TestAP2', 'Password2')
+        dev1.start_ap('TestAP2')
 
         try:
             condition = 'not obj.scanning'
@@ -77,6 +77,7 @@ class Test(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
         IWD.copy_to_storage('TestAP1.psk')
+        IWD.copy_to_ap('TestAP2.ap')
 
     @classmethod
     def tearDownClass(cls):
diff --git a/autotests/testAP/hw.conf b/autotests/testAP/hw.conf
index 96a2bbb1..6b8b2aeb 100644
--- a/autotests/testAP/hw.conf
+++ b/autotests/testAP/hw.conf
@@ -1,7 +1,6 @@
 [SETUP]
 num_radios=3
-max_test_exec_interval_sec=40
-tmpfs_extra_stuff=main.conf
+start_iwd=0
 
 [HOSTAPD]
 rad0=psk-ccmp.conf
-- 
2.26.2

[PATCH 11/13] build: add ELL dhcp-util.c to build

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

---
 Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile.am b/Makefile.am
index 1ee414cb..7ed8baa9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -121,6 +121,7 @@ ell_sources = ell/private.h \
 			ell/dhcp-transport.c \
 			ell/dhcp-lease.c \
 			ell/dhcp-util.c \
+			ell/dhcp-server.c \
 			ell/cert-private.h \
 			ell/cert.c \
 			ell/ecc-external.c \
-- 
2.26.2

[PATCH 12/13] ap: add support for DHCPv4 server

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 5120 bytes --]

The DHCP server can be enabled by including an [IPv4] group
in the AP provisioning file and including at least a subnet
mask.
---
 src/ap.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 src/ap.h |   8 +++++
 2 files changed, 110 insertions(+), 6 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 41de69a9..e9f79e5a 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -76,6 +76,8 @@ struct ap_state {
 	uint16_t last_aid;
 	struct l_queue *sta_states;
 
+	struct l_dhcp_server *server;
+
 	bool started : 1;
 	bool gtk_set : 1;
 };
@@ -120,6 +122,13 @@ void ap_config_free(struct ap_config *config)
 	explicit_bzero(config->psk, sizeof(config->psk));
 	l_free(config->authorized_macs);
 	l_free(config->wsc_name);
+
+	l_free(config->address);
+	l_free(config->gateway);
+	l_free(config->netmask);
+	l_strv_free(config->dns_list);
+	l_strv_free(config->ip_range);
+
 	l_free(config);
 }
 
@@ -198,6 +207,9 @@ static void ap_reset(struct ap_state *ap)
 	l_queue_destroy(ap->wsc_pbc_probes, l_free);
 
 	ap->started = false;
+
+	if (ap->server)
+		l_dhcp_server_stop(ap->server);
 }
 
 static void ap_del_station(struct sta_state *sta, uint16_t reason,
@@ -1906,6 +1918,55 @@ static void ap_deauth_cb(const struct mmpdu_header *hdr, const void *body,
 	ap_sta_free(sta);
 }
 
+static bool ap_start_dhcp(struct ap_state *ap)
+{
+	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
+
+	/*
+	 * [IPv4].Netmask is the only required property since .Address and
+	 * .Gateway may inherit from the interfaces IP that was already set.
+	 * If this is not found we assume DHCP is not being used.
+	 */
+	if (!ap->config->netmask)
+		return true;
+
+	ap->server = l_dhcp_server_new(ifindex);
+	if (!ap->server) {
+		l_error("Failed to create DHCP server on %u", ifindex);
+		return false;
+	}
+
+	/* TODO: set address via rtnl */
+	if (ap->config->address)
+		if (!l_dhcp_server_set_ip_address(ap->server,
+							ap->config->address))
+			return false;
+
+	if (ap->config->gateway)
+		if (!l_dhcp_server_set_gateway(ap->server, ap->config->gateway))
+			return false;
+
+	if (ap->config->dns_list)
+		if (!l_dhcp_server_set_dns(ap->server, ap->config->dns_list))
+			return false;
+
+	if (ap->config->lease_time)
+		if (!l_dhcp_server_set_lease_time(ap->server,
+						ap->config->lease_time))
+			return false;
+
+	if (ap->config->ip_range)
+		if (!l_dhcp_server_set_ip_range(ap->server,
+						ap->config->ip_range[0],
+						ap->config->ip_range[1]))
+			return false;
+
+	if (!l_dhcp_server_set_netmask(ap->server, ap->config->netmask))
+		return false;
+
+	return l_dhcp_server_start(ap->server);
+}
+
 static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 {
 	struct ap_state *ap = user_data;
@@ -1915,16 +1976,24 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 	if (l_genl_msg_get_error(msg) < 0) {
 		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
 
-		ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
-					ap->user_data);
-		ap_reset(ap);
-		l_genl_family_free(ap->nl80211);
-		l_free(ap);
-		return;
+		goto failed;
+	}
+
+	if (!ap_start_dhcp(ap)) {
+		l_error("DHCP server failed to start");
+		goto failed;
 	}
 
 	ap->started = true;
 	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
+
+	return;
+
+failed:
+	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap->user_data);
+	ap_reset(ap);
+	l_genl_family_free(ap->nl80211);
+	l_free(ap);
 }
 
 static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
@@ -2563,6 +2632,33 @@ static struct ap_config *ap_config_from_settings(struct l_settings *settings,
 		l_free(passphrase);
 	}
 
+	if (!l_settings_has_group(settings, "IPv4"))
+		goto done;
+
+	config->address = l_settings_get_string(settings, "IPv4", "Address");
+	config->gateway = l_settings_get_string(settings, "IPv4", "Gateway");
+	config->netmask = l_settings_get_string(settings, "IPv4", "Netmask");
+	config->dns_list = l_settings_get_string_list(settings, "IPv4",
+							"DNSList", ',');
+	config->ip_range = l_settings_get_string_list(settings, "IPv4",
+							"IPRange", ',');
+
+	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime",
+					&config->lease_time))
+		config->lease_time = 0;
+
+
+	if (!config->netmask) {
+		l_error("[IPv4].Netmask is a required value for DHCP");
+		goto failed;
+	}
+
+	if (config->ip_range && l_strv_length(config->ip_range) != 2) {
+		l_error("IP range must be 2 values");
+		goto failed;
+	}
+
+
 done:
 	l_info("Loaded AP configuration %s", config->ssid);
 	return config;
diff --git a/src/ap.h b/src/ap.h
index 90497008..277a5a2f 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -63,6 +63,14 @@ struct ap_config {
 	unsigned int authorized_macs_num;
 	char *wsc_name;
 	struct wsc_primary_device_type wsc_primary_device_type;
+
+	char *address;
+	char *gateway;
+	char *netmask;
+	char **dns_list;
+	char **ip_range;
+	uint32_t lease_time;
+
 	bool no_cck_rates : 1;
 	bool no_free : 1;
 };
-- 
2.26.2

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6442 bytes --]

Hi James,

On 10/20/20 1:02 PM, James Prestwood wrote:
> The DHCP server can be enabled by including an [IPv4] group
> in the AP provisioning file and including at least a subnet
> mask.

Any chance we can add this code to the current implementation without changing 
the API just yet?  I think it is safe to assume that if the address is set prior 
to AP starting on a given interface, then we should not start our own DHCP, and 
if the address isn't set, then start DHCP server.  If the address is provided by 
provisioning, then start our own DHCP as well, overriding whatever we picked.

> ---
>   src/ap.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>   src/ap.h |   8 +++++
>   2 files changed, 110 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index 41de69a9..e9f79e5a 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -76,6 +76,8 @@ struct ap_state {
>   	uint16_t last_aid;
>   	struct l_queue *sta_states;
>   
> +	struct l_dhcp_server *server;
> +
>   	bool started : 1;
>   	bool gtk_set : 1;
>   };
> @@ -120,6 +122,13 @@ void ap_config_free(struct ap_config *config)
>   	explicit_bzero(config->psk, sizeof(config->psk));
>   	l_free(config->authorized_macs);
>   	l_free(config->wsc_name);
> +
> +	l_free(config->address);
> +	l_free(config->gateway);
> +	l_free(config->netmask);
> +	l_strv_free(config->dns_list);
> +	l_strv_free(config->ip_range);
> +
>   	l_free(config);
>   }
>   
> @@ -198,6 +207,9 @@ static void ap_reset(struct ap_state *ap)
>   	l_queue_destroy(ap->wsc_pbc_probes, l_free);
>   
>   	ap->started = false;
> +
> +	if (ap->server)
> +		l_dhcp_server_stop(ap->server);
>   }
>   
>   static void ap_del_station(struct sta_state *sta, uint16_t reason,
> @@ -1906,6 +1918,55 @@ static void ap_deauth_cb(const struct mmpdu_header *hdr, const void *body,
>   	ap_sta_free(sta);
>   }
>   
> +static bool ap_start_dhcp(struct ap_state *ap)
> +{
> +	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> +
> +	/*
> +	 * [IPv4].Netmask is the only required property since .Address and
> +	 * .Gateway may inherit from the interfaces IP that was already set.
> +	 * If this is not found we assume DHCP is not being used.

Netmask is also inherited actually.  Its the prefix len...

denkenz(a)localhost ~/iwd-master $ ip addr
...
inet 192.168.1.249/24 brd 192.168.1.255 scope global dynamic noprefixroute
...

/24 = 255.255.255.0

> +	 */
> +	if (!ap->config->netmask)
> +		return true;
> +
> +	ap->server = l_dhcp_server_new(ifindex);
> +	if (!ap->server) {
> +		l_error("Failed to create DHCP server on %u", ifindex);
> +		return false;
> +	}
> +
> +	/* TODO: set address via rtnl */
> +	if (ap->config->address)
> +		if (!l_dhcp_server_set_ip_address(ap->server,
> +							ap->config->address))
> +			return false;
> +
> +	if (ap->config->gateway)
> +		if (!l_dhcp_server_set_gateway(ap->server, ap->config->gateway))
> +			return false;
> +
> +	if (ap->config->dns_list)
> +		if (!l_dhcp_server_set_dns(ap->server, ap->config->dns_list))
> +			return false;
> +
> +	if (ap->config->lease_time)
> +		if (!l_dhcp_server_set_lease_time(ap->server,
> +						ap->config->lease_time))
> +			return false;
> +
> +	if (ap->config->ip_range)
> +		if (!l_dhcp_server_set_ip_range(ap->server,
> +						ap->config->ip_range[0],
> +						ap->config->ip_range[1]))
> +			return false;
> +
> +	if (!l_dhcp_server_set_netmask(ap->server, ap->config->netmask))
> +		return false;
> +
> +	return l_dhcp_server_start(ap->server);
> +}
> +
>   static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
>   {
>   	struct ap_state *ap = user_data;
> @@ -1915,16 +1976,24 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
>   	if (l_genl_msg_get_error(msg) < 0) {
>   		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
>   
> -		ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
> -					ap->user_data);
> -		ap_reset(ap);
> -		l_genl_family_free(ap->nl80211);
> -		l_free(ap);
> -		return;
> +		goto failed;
> +	}
> +
> +	if (!ap_start_dhcp(ap)) {
> +		l_error("DHCP server failed to start");
> +		goto failed;
>   	}
>   
>   	ap->started = true;
>   	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
> +
> +	return;
> +
> +failed:
> +	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap->user_data);
> +	ap_reset(ap);
> +	l_genl_family_free(ap->nl80211);
> +	l_free(ap);
>   }
>   
>   static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
> @@ -2563,6 +2632,33 @@ static struct ap_config *ap_config_from_settings(struct l_settings *settings,
>   		l_free(passphrase);
>   	}
>   
> +	if (!l_settings_has_group(settings, "IPv4"))
> +		goto done;
> +
> +	config->address = l_settings_get_string(settings, "IPv4", "Address");
> +	config->gateway = l_settings_get_string(settings, "IPv4", "Gateway");
> +	config->netmask = l_settings_get_string(settings, "IPv4", "Netmask");
> +	config->dns_list = l_settings_get_string_list(settings, "IPv4",
> +							"DNSList", ',');
> +	config->ip_range = l_settings_get_string_list(settings, "IPv4",
> +							"IPRange", ',');
> +
> +	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime",
> +					&config->lease_time))
> +		config->lease_time = 0;
> +
> +
> +	if (!config->netmask) {
> +		l_error("[IPv4].Netmask is a required value for DHCP");
> +		goto failed;
> +	}
> +
> +	if (config->ip_range && l_strv_length(config->ip_range) != 2) {
> +		l_error("IP range must be 2 values");
> +		goto failed;
> +	}
> +
> +
>   done:
>   	l_info("Loaded AP configuration %s", config->ssid);
>   	return config;
> diff --git a/src/ap.h b/src/ap.h
> index 90497008..277a5a2f 100644
> --- a/src/ap.h
> +++ b/src/ap.h
> @@ -63,6 +63,14 @@ struct ap_config {
>   	unsigned int authorized_macs_num;
>   	char *wsc_name;
>   	struct wsc_primary_device_type wsc_primary_device_type;
> +
> +	char *address;
> +	char *gateway;
> +	char *netmask;
> +	char **dns_list;
> +	char **ip_range;
> +	uint32_t lease_time;
> +

I think we may need Andrew's opinion on whether these should be part of 
ap_config.  P2P might not really care about setting up these variables and would 
prefer for ap.c to just figure things out.

>   	bool no_cck_rates : 1;
>   	bool no_free : 1;
>   };
> 

Regards,
-Denis

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 7916 bytes --]

On Tue, 2020-10-20 at 13:28 -0500, Denis Kenzior wrote:
> Hi James,
> 
> On 10/20/20 1:02 PM, James Prestwood wrote:
> > The DHCP server can be enabled by including an [IPv4] group
> > in the AP provisioning file and including at least a subnet
> > mask.
> 
> Any chance we can add this code to the current implementation without
> changing 
> the API just yet?  I think it is safe to assume that if the address
> is set prior 
> to AP starting on a given interface, then we should not start our own
> DHCP, and 
> if the address isn't set, then start DHCP server.  If the address is
> provided by 
> provisioning, then start our own DHCP as well, overriding whatever we
> picked.

Sure, so keep the 'psk' DBus argument and remove [Security].Passphrase?
It seems weird to have both since we would be ignoring one.

But do we really want to make the decision to use DHCP based on if the
address is set? It really seems like the user would want to decide that
explicitly with a config file. OTOH if all no DHCP options are required
(using defaults) we would need some way of knowing to start DHCP or
not. I just don't see a correlation between the address being set and
the user wanting/not wanting a DHCP server.
> 
> > ---
> >   src/ap.c | 108
> > +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >   src/ap.h |   8 +++++
> >   2 files changed, 110 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/ap.c b/src/ap.c
> > index 41de69a9..e9f79e5a 100644
> > --- a/src/ap.c
> > +++ b/src/ap.c
> > @@ -76,6 +76,8 @@ struct ap_state {
> >   	uint16_t last_aid;
> >   	struct l_queue *sta_states;
> >   
> > +	struct l_dhcp_server *server;
> > +
> >   	bool started : 1;
> >   	bool gtk_set : 1;
> >   };
> > @@ -120,6 +122,13 @@ void ap_config_free(struct ap_config *config)
> >   	explicit_bzero(config->psk, sizeof(config->psk));
> >   	l_free(config->authorized_macs);
> >   	l_free(config->wsc_name);
> > +
> > +	l_free(config->address);
> > +	l_free(config->gateway);
> > +	l_free(config->netmask);
> > +	l_strv_free(config->dns_list);
> > +	l_strv_free(config->ip_range);
> > +
> >   	l_free(config);
> >   }
> >   
> > @@ -198,6 +207,9 @@ static void ap_reset(struct ap_state *ap)
> >   	l_queue_destroy(ap->wsc_pbc_probes, l_free);
> >   
> >   	ap->started = false;
> > +
> > +	if (ap->server)
> > +		l_dhcp_server_stop(ap->server);
> >   }
> >   
> >   static void ap_del_station(struct sta_state *sta, uint16_t
> > reason,
> > @@ -1906,6 +1918,55 @@ static void ap_deauth_cb(const struct
> > mmpdu_header *hdr, const void *body,
> >   	ap_sta_free(sta);
> >   }
> >   
> > +static bool ap_start_dhcp(struct ap_state *ap)
> > +{
> > +	uint32_t ifindex = netdev_get_ifindex(ap->netdev);
> > +
> > +	/*
> > +	 * [IPv4].Netmask is the only required property since .Address
> > and
> > +	 * .Gateway may inherit from the interfaces IP that was already
> > set.
> > +	 * If this is not found we assume DHCP is not being used.
> 
> Netmask is also inherited actually.  Its the prefix len...
> 
> denkenz(a)localhost ~/iwd-master $ ip addr
> ...
> inet 192.168.1.249/24 brd 192.168.1.255 scope global dynamic
> noprefixroute
> ...
> 
> /24 = 255.255.255.0

Ok I'll make this optional as well.

> 
> > +	 */
> > +	if (!ap->config->netmask)
> > +		return true;
> > +
> > +	ap->server = l_dhcp_server_new(ifindex);
> > +	if (!ap->server) {
> > +		l_error("Failed to create DHCP server on %u", ifindex);
> > +		return false;
> > +	}
> > +
> > +	/* TODO: set address via rtnl */
> > +	if (ap->config->address)
> > +		if (!l_dhcp_server_set_ip_address(ap->server,
> > +							ap->config-
> > >address))
> > +			return false;
> > +
> > +	if (ap->config->gateway)
> > +		if (!l_dhcp_server_set_gateway(ap->server, ap->config-
> > >gateway))
> > +			return false;
> > +
> > +	if (ap->config->dns_list)
> > +		if (!l_dhcp_server_set_dns(ap->server, ap->config-
> > >dns_list))
> > +			return false;
> > +
> > +	if (ap->config->lease_time)
> > +		if (!l_dhcp_server_set_lease_time(ap->server,
> > +						ap->config-
> > >lease_time))
> > +			return false;
> > +
> > +	if (ap->config->ip_range)
> > +		if (!l_dhcp_server_set_ip_range(ap->server,
> > +						ap->config-
> > >ip_range[0],
> > +						ap->config-
> > >ip_range[1]))
> > +			return false;
> > +
> > +	if (!l_dhcp_server_set_netmask(ap->server, ap->config-
> > >netmask))
> > +		return false;
> > +
> > +	return l_dhcp_server_start(ap->server);
> > +}
> > +
> >   static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
> >   {
> >   	struct ap_state *ap = user_data;
> > @@ -1915,16 +1976,24 @@ static void ap_start_cb(struct l_genl_msg
> > *msg, void *user_data)
> >   	if (l_genl_msg_get_error(msg) < 0) {
> >   		l_error("START_AP failed: %i",
> > l_genl_msg_get_error(msg));
> >   
> > -		ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
> > -					ap->user_data);
> > -		ap_reset(ap);
> > -		l_genl_family_free(ap->nl80211);
> > -		l_free(ap);
> > -		return;
> > +		goto failed;
> > +	}
> > +
> > +	if (!ap_start_dhcp(ap)) {
> > +		l_error("DHCP server failed to start");
> > +		goto failed;
> >   	}
> >   
> >   	ap->started = true;
> >   	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
> > +
> > +	return;
> > +
> > +failed:
> > +	ap->ops->handle_event(AP_EVENT_START_FAILED, NULL, ap-
> > >user_data);
> > +	ap_reset(ap);
> > +	l_genl_family_free(ap->nl80211);
> > +	l_free(ap);
> >   }
> >   
> >   static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state
> > *ap)
> > @@ -2563,6 +2632,33 @@ static struct ap_config
> > *ap_config_from_settings(struct l_settings *settings,
> >   		l_free(passphrase);
> >   	}
> >   
> > +	if (!l_settings_has_group(settings, "IPv4"))
> > +		goto done;
> > +
> > +	config->address = l_settings_get_string(settings, "IPv4",
> > "Address");
> > +	config->gateway = l_settings_get_string(settings, "IPv4",
> > "Gateway");
> > +	config->netmask = l_settings_get_string(settings, "IPv4",
> > "Netmask");
> > +	config->dns_list = l_settings_get_string_list(settings, "IPv4",
> > +							"DNSList",
> > ',');
> > +	config->ip_range = l_settings_get_string_list(settings, "IPv4",
> > +							"IPRange",
> > ',');
> > +
> > +	if (!l_settings_get_uint(settings, "IPv4", "LeaseTime",
> > +					&config->lease_time))
> > +		config->lease_time = 0;
> > +
> > +
> > +	if (!config->netmask) {
> > +		l_error("[IPv4].Netmask is a required value for DHCP");
> > +		goto failed;
> > +	}
> > +
> > +	if (config->ip_range && l_strv_length(config->ip_range) != 2) {
> > +		l_error("IP range must be 2 values");
> > +		goto failed;
> > +	}
> > +
> > +
> >   done:
> >   	l_info("Loaded AP configuration %s", config->ssid);
> >   	return config;
> > diff --git a/src/ap.h b/src/ap.h
> > index 90497008..277a5a2f 100644
> > --- a/src/ap.h
> > +++ b/src/ap.h
> > @@ -63,6 +63,14 @@ struct ap_config {
> >   	unsigned int authorized_macs_num;
> >   	char *wsc_name;
> >   	struct wsc_primary_device_type wsc_primary_device_type;
> > +
> > +	char *address;
> > +	char *gateway;
> > +	char *netmask;
> > +	char **dns_list;
> > +	char **ip_range;
> > +	uint32_t lease_time;
> > +
> 
> I think we may need Andrew's opinion on whether these should be part
> of 
> ap_config.  P2P might not really care about setting up these
> variables and would 
> prefer for ap.c to just figure things out.

Sure, but they aren't required anyways so p2p doesn't have to include
them and defaults will be used (once netmask is fixed). The ap_config
structure was just a convenient storage object for these but we could
define something else too.

> 
> >   	bool no_cck_rates : 1;
> >   	bool no_free : 1;
> >   };
> > 
> 
> Regards,
> -Denis

Thanks,
James

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

Hi James,

On 10/20/20 1:41 PM, James Prestwood wrote:
> On Tue, 2020-10-20 at 13:28 -0500, Denis Kenzior wrote:
>> Hi James,
>>
>> On 10/20/20 1:02 PM, James Prestwood wrote:
>>> The DHCP server can be enabled by including an [IPv4] group
>>> in the AP provisioning file and including at least a subnet
>>> mask.
>>
>> Any chance we can add this code to the current implementation without
>> changing
>> the API just yet?  I think it is safe to assume that if the address
>> is set prior
>> to AP starting on a given interface, then we should not start our own
>> DHCP, and
>> if the address isn't set, then start DHCP server.  If the address is
>> provided by
>> provisioning, then start our own DHCP as well, overriding whatever we
>> picked.
> 
> Sure, so keep the 'psk' DBus argument and remove [Security].Passphrase?
> It seems weird to have both since we would be ignoring one.

I don't know yet.  I don't think removing the simple 'Start' method which just 
provides an SSID and PSK is a good idea.  It is useful for quick and dirty 
setups.  Provisioning files might be for more advanced users and might need 
their own dedicated Start method.  So something like KnownNetwork(s) but for 
AccessPointConfiguration(s).

> 
> But do we really want to make the decision to use DHCP based on if the
> address is set? It really seems like the user would want to decide that
> explicitly with a config file. OTOH if all no DHCP options are required
> (using defaults) we would need some way of knowing to start DHCP or
> not. I just don't see a correlation between the address being set and
> the user wanting/not wanting a DHCP server.

Well, the user has to first switch the device into AP mode or create the 
required interface first somehow.  So before they actually hit .Start, they have 
a chance to configure the IP or not.  If the IP is configured, then we assume 
the interface is managed by someone else.  If it isn't, we take ownership.

I suppose we can add an explicit setting in main.conf somewhere, or in the 
profile itself.  But I'd keep things simple for now.

>>
>> I think we may need Andrew's opinion on whether these should be part
>> of
>> ap_config.  P2P might not really care about setting up these
>> variables and would
>> prefer for ap.c to just figure things out.
> 
> Sure, but they aren't required anyways so p2p doesn't have to include
> them and defaults will be used (once netmask is fixed). The ap_config
> structure was just a convenient storage object for these but we could
> define something else too.

Yes, true.  But not much sense in actually storing them long-term if that's the 
case.  Generate them / load from settings, give them to dhcp-server and forget.

Regards,
-Denis

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 4037 bytes --]

Hi,

On Tue, 20 Oct 2020 at 20:51, Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi James,
>
> On 10/20/20 1:41 PM, James Prestwood wrote:
> > On Tue, 2020-10-20 at 13:28 -0500, Denis Kenzior wrote:
> >> Hi James,
> >>
> >> On 10/20/20 1:02 PM, James Prestwood wrote:
> >>> The DHCP server can be enabled by including an [IPv4] group
> >>> in the AP provisioning file and including at least a subnet
> >>> mask.
> >>
> >> Any chance we can add this code to the current implementation without
> >> changing
> >> the API just yet?  I think it is safe to assume that if the address
> >> is set prior
> >> to AP starting on a given interface, then we should not start our own
> >> DHCP, and
> >> if the address isn't set, then start DHCP server.  If the address is
> >> provided by
> >> provisioning, then start our own DHCP as well, overriding whatever we
> >> picked.
> >
> > Sure, so keep the 'psk' DBus argument and remove [Security].Passphrase?
> > It seems weird to have both since we would be ignoring one.
>
> I don't know yet.  I don't think removing the simple 'Start' method which just
> provides an SSID and PSK is a good idea.  It is useful for quick and dirty
> setups.  Provisioning files might be for more advanced users and might need
> their own dedicated Start method.  So something like KnownNetwork(s) but for
> AccessPointConfiguration(s).

I replied to a different patch in this series without seeing this
thread, but basically I agree with what Denis said, I prefer if we
keep the DBus arguments unchanged.

>
> >
> > But do we really want to make the decision to use DHCP based on if the
> > address is set? It really seems like the user would want to decide that
> > explicitly with a config file. OTOH if all no DHCP options are required
> > (using defaults) we would need some way of knowing to start DHCP or
> > not. I just don't see a correlation between the address being set and
> > the user wanting/not wanting a DHCP server.
>
> Well, the user has to first switch the device into AP mode or create the
> required interface first somehow.  So before they actually hit .Start, they have
> a chance to configure the IP or not.  If the IP is configured, then we assume
> the interface is managed by someone else.  If it isn't, we take ownership.
>
> I suppose we can add an explicit setting in main.conf somewhere, or in the
> profile itself.  But I'd keep things simple for now.

Maybe we can re-use [General].EnableNetworkConfiguration for the basic
on/off switch and possibly allow individual access point config files
to override this?

In the case of Network Manager users, if they're interfacing with IWD
through NM they will currently not want DHCP as either client or
server, and if they use iwctl they might want DHCP everywhere.

Also for the record, NM only sets the local IP after the AP is started.

>
> >>
> >> I think we may need Andrew's opinion on whether these should be part
> >> of
> >> ap_config.  P2P might not really care about setting up these
> >> variables and would
> >> prefer for ap.c to just figure things out.
> >
> > Sure, but they aren't required anyways so p2p doesn't have to include
> > them and defaults will be used (once netmask is fixed). The ap_config
> > structure was just a convenient storage object for these but we could
> > define something else too.
>
> Yes, true.  But not much sense in actually storing them long-term if that's the
> case.  Generate them / load from settings, give them to dhcp-server and forget.

It does seem logical to put those settings in struct ap_config.  P2P
wouldn't normally need to use them, it should be fine with the
defaults although I'm thinking in the future there may be exceptions
where we need to work around client's bugs (hopefully not).

Note that the P2P Group Owner will need to decide on an IP for itself
and the client before the 4-Way Handshake starts to implement the
optional IP assignment during the handshake.

Best regards

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2561 bytes --]

Hi Andrew,

>> Well, the user has to first switch the device into AP mode or create the
>> required interface first somehow.  So before they actually hit .Start, they have
>> a chance to configure the IP or not.  If the IP is configured, then we assume
>> the interface is managed by someone else.  If it isn't, we take ownership.
>>
>> I suppose we can add an explicit setting in main.conf somewhere, or in the
>> profile itself.  But I'd keep things simple for now.
> 
> Maybe we can re-use [General].EnableNetworkConfiguration for the basic
> on/off switch and possibly allow individual access point config files
> to override this?
> 
> In the case of Network Manager users, if they're interfacing with IWD
> through NM they will currently not want DHCP as either client or
> server, and if they use iwctl they might want DHCP everywhere.
> 
> Also for the record, NM only sets the local IP after the AP is started.
> 

Good points.  I'm fine re-using EnableNetworkConfiguration for this.

>>
>>>>
>>>> I think we may need Andrew's opinion on whether these should be part
>>>> of
>>>> ap_config.  P2P might not really care about setting up these
>>>> variables and would
>>>> prefer for ap.c to just figure things out.
>>>
>>> Sure, but they aren't required anyways so p2p doesn't have to include
>>> them and defaults will be used (once netmask is fixed). The ap_config
>>> structure was just a convenient storage object for these but we could
>>> define something else too.
>>
>> Yes, true.  But not much sense in actually storing them long-term if that's the
>> case.  Generate them / load from settings, give them to dhcp-server and forget.
> 
> It does seem logical to put those settings in struct ap_config.  P2P
> wouldn't normally need to use them, it should be fine with the
> defaults although I'm thinking in the future there may be exceptions
> where we need to work around client's bugs (hopefully not).
> 
> Note that the P2P Group Owner will need to decide on an IP for itself
> and the client before the 4-Way Handshake starts to implement the
> optional IP assignment during the handshake.

Hmm, wouldn't it make sense for AP to figure out the setup and for P2P to just 
use that?  Especially if we start supporting persistent groups eventually.

You do bring up a good point that we'd need a 'reserve' API of sorts on the DHCP 
server side.  That way IP details for the handshaking client can be 
pre-populated if IP configuration through the handshake is supported.

Regards,
-Denis

Re: [PATCH 12/13] ap: add support for DHCPv4 server

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

On Tue, 20 Oct 2020 at 23:03, Denis Kenzior <denkenz@gmail.com> wrote:
> >>>> I think we may need Andrew's opinion on whether these should be part
> >>>> of
> >>>> ap_config.  P2P might not really care about setting up these
> >>>> variables and would
> >>>> prefer for ap.c to just figure things out.
> >>>
> >>> Sure, but they aren't required anyways so p2p doesn't have to include
> >>> them and defaults will be used (once netmask is fixed). The ap_config
> >>> structure was just a convenient storage object for these but we could
> >>> define something else too.
> >>
> >> Yes, true.  But not much sense in actually storing them long-term if that's the
> >> case.  Generate them / load from settings, give them to dhcp-server and forget.
> >
> > It does seem logical to put those settings in struct ap_config.  P2P
> > wouldn't normally need to use them, it should be fine with the
> > defaults although I'm thinking in the future there may be exceptions
> > where we need to work around client's bugs (hopefully not).
> >
> > Note that the P2P Group Owner will need to decide on an IP for itself
> > and the client before the 4-Way Handshake starts to implement the
> > optional IP assignment during the handshake.
>
> Hmm, wouldn't it make sense for AP to figure out the setup and for P2P to just
> use that?  Especially if we start supporting persistent groups eventually.

Yeah, normally P2P doesn't need to fix specific IP vaues, it can let
AP use its defaults.

Even with persistent groups afaik mainly the PSK is persisted, IPs can
differ between each session.

>
> You do bring up a good point that we'd need a 'reserve' API of sorts on the DHCP
> server side.  That way IP details for the handshaking client can be
> pre-populated if IP configuration through the handshake is supported.

Usually we'll just have one client.  Even with multiple clients we can
use disjoint ranges of IPs for DHCP and for the 4-Way handshake IP
setup... but we could instead 'reserve' an IP for the client and offer
it through both methods.

Many standalone APs let you manually assign IPs to MACs, including IPs
in the DHCP range, so it may be useful to have this option.

Best regards

[PATCH 13/13] auto-t: add AP test with DHCP server

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 6142 bytes --]

---
 autotests/testAP-DHCP/TestAP2.ap         | 31 ++++++++++
 autotests/testAP-DHCP/connection_test.py | 76 ++++++++++++++++++++++++
 autotests/testAP-DHCP/failure_test.py    | 53 +++++++++++++++++
 autotests/testAP-DHCP/hw.conf            |  3 +
 autotests/testAP-DHCP/main.conf          |  5 ++
 5 files changed, 168 insertions(+)
 create mode 100644 autotests/testAP-DHCP/TestAP2.ap
 create mode 100644 autotests/testAP-DHCP/connection_test.py
 create mode 100644 autotests/testAP-DHCP/failure_test.py
 create mode 100644 autotests/testAP-DHCP/hw.conf
 create mode 100644 autotests/testAP-DHCP/main.conf

diff --git a/autotests/testAP-DHCP/TestAP2.ap b/autotests/testAP-DHCP/TestAP2.ap
new file mode 100644
index 00000000..dc1a3579
--- /dev/null
+++ b/autotests/testAP-DHCP/TestAP2.ap
@@ -0,0 +1,31 @@
+[Security]
+Passphrase=Password2
+
+[IPv4]
+#
+# IP address of access point interface. If omitted the current interface IP
+# address will be used. If provided the IP will be changed to this address.
+#
+Address=192.168.1.1
+Netmask=255.255.255.0
+
+#
+# Gateway address for AP. If omitted [IPv4].Address will be used.
+#
+Gateway=192.168.1.1
+
+#
+# Optional list of DNS servers
+#
+DNSList=192.168.1.1,192.168.1.254
+
+#
+# Required IP range
+#
+IPRange=192.168.1.2,192.168.1.100
+
+#
+# Optional lease time. If omitted an infinite lease will be used.
+#
+LeaseTime=10
+
diff --git a/autotests/testAP-DHCP/connection_test.py b/autotests/testAP-DHCP/connection_test.py
new file mode 100644
index 00000000..8a79db6d
--- /dev/null
+++ b/autotests/testAP-DHCP/connection_test.py
@@ -0,0 +1,76 @@
+#! /usr/bin/python3
+
+import unittest
+import sys, os
+
+import iwd
+from iwd import IWD
+from iwd import PSKAgent
+from iwd import NetworkType
+import testutil
+
+class Test(unittest.TestCase):
+    def test_connection_success(self):
+
+        os.system('ls /tmp/iwd')
+        os.system('ls /tmp/iwd/ap')
+        wd = IWD(True)
+
+        dev1, dev2 = wd.list_devices(2)
+
+        dev1.start_ap('TestAP2')
+
+        try:
+            condition = 'not obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+            dev2.scan()
+            condition = 'obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+            condition = 'not obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+
+            ordered_networks = dev2.get_ordered_networks()
+
+            networks = { n.name: n for n in ordered_networks }
+            self.assertEqual(networks['TestAP2'].type, NetworkType.psk)
+
+            psk_agent = PSKAgent('Password2')
+            wd.register_psk_agent(psk_agent)
+
+            try:
+                dev2.disconnect()
+
+                condition = 'not obj.connected'
+                wd.wait_for_object_condition(dev2, condition)
+            except:
+                pass
+
+            networks['TestAP2'].network_object.connect()
+
+            condition = 'obj.state == DeviceState.connected'
+            wd.wait_for_object_condition(dev2, condition)
+
+            testutil.test_iface_operstate(dev2.name)
+            testutil.test_ifaces_connected(dev1.name, dev2.name, group=False)
+
+            wd.unregister_psk_agent(psk_agent)
+
+            dev2.disconnect()
+
+            condition = 'not obj.connected'
+            wd.wait_for_object_condition(networks['TestAP2'].network_object,
+                                         condition)
+        finally:
+            dev1.stop_ap()
+
+    @classmethod
+    def setUpClass(cls):
+        IWD.copy_to_ap('TestAP2.ap')
+        os.system('ls /tmp/iwd/ap')
+
+    @classmethod
+    def tearDownClass(cls):
+        IWD.clear_storage()
+
+if __name__ == '__main__':
+    unittest.main(exit=True)
diff --git a/autotests/testAP-DHCP/failure_test.py b/autotests/testAP-DHCP/failure_test.py
new file mode 100644
index 00000000..494fedda
--- /dev/null
+++ b/autotests/testAP-DHCP/failure_test.py
@@ -0,0 +1,53 @@
+#! /usr/bin/python3
+
+import unittest
+import sys, os
+
+sys.path.append('../util')
+import iwd
+from iwd import IWD
+from iwd import PSKAgent
+from iwd import NetworkType
+import testutil
+
+class Test(unittest.TestCase):
+    def test_connection_failure(self):
+        wd = IWD(True)
+
+        dev1, dev2 = wd.list_devices(2)
+
+        dev1.start_ap('TestAP2')
+
+        try:
+            condition = 'not obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+            dev2.scan()
+            condition = 'obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+            condition = 'not obj.scanning'
+            wd.wait_for_object_condition(dev2, condition)
+
+            ordered_networks = dev2.get_ordered_networks()
+            networks = { n.name: n for n in ordered_networks }
+            self.assertEqual(networks['TestAP2'].type, NetworkType.psk)
+
+            psk_agent = PSKAgent('InvalidPassword')
+            wd.register_psk_agent(psk_agent)
+
+            with self.assertRaises(iwd.FailedEx):
+                networks['TestAP2'].network_object.connect()
+
+            wd.unregister_psk_agent(psk_agent)
+        finally:
+            dev1.stop_ap()
+
+    @classmethod
+    def setUpClass(cls):
+        IWD.copy_to_ap('TestAP2.ap')
+
+    @classmethod
+    def tearDownClass(cls):
+        IWD.clear_storage()
+
+if __name__ == '__main__':
+    unittest.main(exit=True)
diff --git a/autotests/testAP-DHCP/hw.conf b/autotests/testAP-DHCP/hw.conf
new file mode 100644
index 00000000..c6553537
--- /dev/null
+++ b/autotests/testAP-DHCP/hw.conf
@@ -0,0 +1,3 @@
+[SETUP]
+num_radios=2
+start_iwd=0
diff --git a/autotests/testAP-DHCP/main.conf b/autotests/testAP-DHCP/main.conf
new file mode 100644
index 00000000..25ebe4ff
--- /dev/null
+++ b/autotests/testAP-DHCP/main.conf
@@ -0,0 +1,5 @@
+[Scan]
+DisableMacAddressRandomization=true
+
+[General]
+EnableNetworkConfiguration=true
-- 
2.26.2

Re: [PATCH 01/13] auto-t: no hostapd instance graceful failure

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

Hi James,

On 10/20/20 1:02 PM, James Prestwood wrote:
> If a test does not need any hostapd instances but still loads
> hostapd.py for some reason we want to gracefully throw an
> exception rather than fail in some other manor.
> ---
>   autotests/util/hostapd.py | 7 +++++++
>   1 file changed, 7 insertions(+)
> 

Patch 1 & 11 applied, thanks.

Regards,
-Denis