Add MAC address randomization functionality

Add two D-Bus methods:
* EnableMACAddressRandomization: (ay : mask) -> nothing
* DisableMACAddressRandomization: nothing -> nothing

which configure random MAC address functionality in the Wi-Fi
driver via netlink. This also enables random MAC addresses on
timer-based scans and fixes weird pointer ownership that was
causing memory issues.

Signed-off-by: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Kevin Cernekee <cernekee@chromium.org>
---
 wpa_supplicant/ctrl_iface.c             | 51 +----------------
 wpa_supplicant/dbus/dbus_new.c          | 13 +++++
 wpa_supplicant/dbus/dbus_new_handlers.c | 43 +++++++++++++++
 wpa_supplicant/dbus/dbus_new_handlers.h |  5 ++
 wpa_supplicant/scan.c                   | 73 ++++++++++++-------------
 wpa_supplicant/wpa_supplicant.c         | 56 +++++++++++++++++++
 wpa_supplicant/wpa_supplicant_i.h       |  4 ++
 7 files changed, 158 insertions(+), 87 deletions(-)

On Mon, 2019-04-22 at 15:58 -0700, Eric Caruso wrote:
> Add two D-Bus methods:
> * EnableMACAddressRandomization: (ay : mask) -> nothing
> * DisableMACAddressRandomization: nothing -> nothing

I'd almost rather see a single method call, like
SetMACAddressRandomization(ay : mask) -> nothing where a zero-length
mask means disabling randomization (since it appears a mask is
required?)

But also, is a mask really required? Can't we just pick a suitable
default (and allow len(mask)==0) and only if the user wants a different
behavior, then they have to pass a full mask?

Dan

> which configure random MAC address functionality in the Wi-Fi
> driver via netlink. This also enables random MAC addresses on
> timer-based scans and fixes weird pointer ownership that was
> causing memory issues.
> 
> Signed-off-by: Eric Caruso <ejcaruso@chromium.org>
> Tested-by: Eric Caruso <ejcaruso@chromium.org>
> Reviewed-by: Kevin Cernekee <cernekee@chromium.org>
> ---
>  wpa_supplicant/ctrl_iface.c             | 51 +----------------
>  wpa_supplicant/dbus/dbus_new.c          | 13 +++++
>  wpa_supplicant/dbus/dbus_new_handlers.c | 43 +++++++++++++++
>  wpa_supplicant/dbus/dbus_new_handlers.h |  5 ++
>  wpa_supplicant/scan.c                   | 73 ++++++++++++-----------
> --
>  wpa_supplicant/wpa_supplicant.c         | 56 +++++++++++++++++++
>  wpa_supplicant/wpa_supplicant_i.h       |  4 ++
>  7 files changed, 158 insertions(+), 87 deletions(-)
> 
> diff --git a/wpa_supplicant/ctrl_iface.c
> b/wpa_supplicant/ctrl_iface.c
> index d814fdf7f..113a6dcda 100644
> --- a/wpa_supplicant/ctrl_iface.c
> +++ b/wpa_supplicant/ctrl_iface.c
> @@ -8566,55 +8566,10 @@ static int
> wpas_ctrl_iface_mac_rand_scan(struct wpa_supplicant *wpa_s,
>  		return -1;
>  	}
>  
> -	if (!enable) {
> -		wpas_mac_addr_rand_scan_clear(wpa_s, type);
> -		if (wpa_s->pno) {
> -			if (type & MAC_ADDR_RAND_PNO) {
> -				wpas_stop_pno(wpa_s);
> -				wpas_start_pno(wpa_s);
> -			}
> -		} else if (wpa_s->sched_scanning &&
> -			   (type & MAC_ADDR_RAND_SCHED_SCAN)) {
> -			wpas_scan_restart_sched_scan(wpa_s);
> -		}
> -		return 0;
> -	}
> +	if (!enable)
> +		return wpas_disable_mac_addr_randomization(wpa_s,
> type);
>  
> -	if ((addr && !mask) || (!addr && mask)) {
> -		wpa_printf(MSG_INFO,
> -			   "CTRL: MAC_RAND_SCAN invalid addr/mask
> combination");
> -		return -1;
> -	}
> -
> -	if (addr && mask && (!(mask[0] & 0x01) || (addr[0] & 0x01))) {
> -		wpa_printf(MSG_INFO,
> -			   "CTRL: MAC_RAND_SCAN cannot allow multicast
> address");
> -		return -1;
> -	}
> -
> -	if (type & MAC_ADDR_RAND_SCAN) {
> -		wpas_mac_addr_rand_scan_set(wpa_s, MAC_ADDR_RAND_SCAN,
> -					    addr, mask);
> -	}
> -
> -	if (type & MAC_ADDR_RAND_SCHED_SCAN) {
> -		wpas_mac_addr_rand_scan_set(wpa_s,
> MAC_ADDR_RAND_SCHED_SCAN,
> -					    addr, mask);
> -
> -		if (wpa_s->sched_scanning && !wpa_s->pno)
> -			wpas_scan_restart_sched_scan(wpa_s);
> -	}
> -
> -	if (type & MAC_ADDR_RAND_PNO) {
> -		wpas_mac_addr_rand_scan_set(wpa_s, MAC_ADDR_RAND_PNO,
> -					    addr, mask);
> -		if (wpa_s->pno) {
> -			wpas_stop_pno(wpa_s);
> -			wpas_start_pno(wpa_s);
> -		}
> -	}
> -
> -	return 0;
> +	return wpas_enable_mac_addr_randomization(wpa_s, type, addr,
> mask);
>  }
>  
>  
> diff --git a/wpa_supplicant/dbus/dbus_new.c
> b/wpa_supplicant/dbus/dbus_new.c
> index 27b3012ae..e58632ef9 100644
> --- a/wpa_supplicant/dbus/dbus_new.c
> +++ b/wpa_supplicant/dbus/dbus_new.c
> @@ -2808,6 +2808,19 @@ static const struct wpa_dbus_method_desc
> wpas_dbus_interface_methods[] = {
>  		  END_ARGS
>  	  }
>  	},
> +	{ "EnableMACAddressRandomization",
> WPAS_DBUS_NEW_IFACE_INTERFACE,
> +	  (WPADBusMethodHandler)
> &wpas_dbus_handler_enable_mac_address_randomization,
> +	  {
> +		  { "mac_mask", "ay", ARG_IN },
> +		  END_ARGS
> +	  }
> +	},
> +	{ "DisableMACAddressRandomization",
> WPAS_DBUS_NEW_IFACE_INTERFACE,
> +	  (WPADBusMethodHandler)
> &wpas_dbus_handler_disable_mac_address_randomization,
> +	  {
> +		  END_ARGS
> +	  }
> +	},
>  #ifdef CONFIG_WPS
>  	{ "Start", WPAS_DBUS_NEW_IFACE_WPS,
>  	  (WPADBusMethodHandler) wpas_dbus_handler_wps_start,
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c
> b/wpa_supplicant/dbus/dbus_new_handlers.c
> index e11dd36ca..ed2adbd97 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers.c
> +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -2005,6 +2005,49 @@ DBusMessage *
> wpas_dbus_handler_remove_blob(DBusMessage *message,
>  #endif /* CONFIG_NO_CONFIG_BLOBS */
>  
>  
> +DBusMessage * wpas_dbus_handler_enable_mac_address_randomization(
> +	DBusMessage *message, struct wpa_supplicant *wpa_s)
> +{
> +	DBusMessageIter	iter, array_iter;
> +	u8 *mask;
> +	int mask_len;
> +
> +	dbus_message_iter_init(message, &iter);
> +	dbus_message_iter_recurse(&iter, &array_iter);
> +
> +	dbus_message_iter_get_fixed_array(&array_iter, &mask,
> &mask_len);
> +	if (mask_len != ETH_ALEN) {
> +		return wpas_dbus_error_invalid_args(
> +			message, "Malformed MAC address mask");
> +	}
> +
> +	if (wpas_enable_mac_addr_randomization(
> +	    wpa_s, MAC_ADDR_RAND_SCAN | MAC_ADDR_RAND_SCHED_SCAN,
> +	    wpa_s->perm_addr, mask)) {
> +		return wpas_dbus_error_unknown_error(
> +			message, "Couldn't enable MAC address
> randomization");
> +	}
> +
> +	wpa_printf(MSG_DEBUG, "Enabled MAC address randomization with
> mask: "
> +		   MACSTR, MAC2STR(mask));
> +
> +	return NULL;
> +}
> +
> +DBusMessage * wpas_dbus_handler_disable_mac_address_randomization(
> +	DBusMessage *message, struct wpa_supplicant *wpa_s)
> +{
> +	if (wpas_disable_mac_addr_randomization(
> +	    wpa_s, MAC_ADDR_RAND_SCAN | MAC_ADDR_RAND_SCHED_SCAN)) {
> +		return wpas_dbus_error_unknown_error(
> +			message, "Couldn't disable MAC address
> randomization");
> +	}
> +
> +	wpa_printf(MSG_DEBUG, "Disabled MAC address randomization");
> +
> +	return NULL;
> +}
> +
>  /*
>   * wpas_dbus_handler_flush_bss - Flush the BSS cache
>   * @message: Pointer to incoming dbus message
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h
> b/wpa_supplicant/dbus/dbus_new_handlers.h
> index 1d6235d6f..7700b003d 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers.h
> +++ b/wpa_supplicant/dbus/dbus_new_handlers.h
> @@ -117,6 +117,11 @@ DBusMessage *
> wpas_dbus_handler_remove_blob(DBusMessage *message,
>  DBusMessage * wpas_dbus_handler_set_pkcs11_engine_and_module_path(
>  	DBusMessage *message, struct wpa_supplicant *wpa_s);
>  
> +DBusMessage * wpas_dbus_handler_enable_mac_address_randomization(
> +	DBusMessage *message, struct wpa_supplicant *wpa_s);
> +DBusMessage * wpas_dbus_handler_disable_mac_address_randomization(
> +	DBusMessage *message, struct wpa_supplicant *wpa_s);
> +
>  DBusMessage * wpas_dbus_handler_flush_bss(DBusMessage *message,
>  					  struct wpa_supplicant
> *wpa_s);
>  
> diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
> index fb8ebdf2e..b4bf4421e 100644
> --- a/wpa_supplicant/scan.c
> +++ b/wpa_supplicant/scan.c
> @@ -79,6 +79,27 @@ static int wpas_wps_in_use(struct wpa_supplicant
> *wpa_s,
>  #endif /* CONFIG_WPS */
>  
>  
> +static int wpa_setup_mac_addr_rand_params(struct
> wpa_driver_scan_params *params,
> +					  const u8 *mac_addr)
> +{
> +	u8 *tmp;
> +
> +	if (!mac_addr)
> +		return 0;
> +
> +	params->mac_addr_rand = 1;
> +
> +	tmp = os_malloc(2 * ETH_ALEN);
> +	if (!tmp)
> +		return -1;
> +
> +	os_memcpy(tmp, mac_addr, 2 * ETH_ALEN);
> +	params->mac_addr = tmp;
> +	params->mac_addr_mask = tmp + ETH_ALEN;
> +	return 0;
> +}
> +
> +
>  /**
>   * wpa_supplicant_enabled_networks - Check whether there are enabled
> networks
>   * @wpa_s: Pointer to wpa_supplicant data
> @@ -175,6 +196,10 @@ static void wpas_trigger_scan_cb(struct
> wpa_radio_work *work, int deinit)
>  			   "Request driver to clear scan cache due to
> local BSS flush");
>  		params->only_new_results = 1;
>  	}
> +
> +	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN)
> +		wpa_setup_mac_addr_rand_params(params, wpa_s-
> >mac_addr_scan);
> +
>  	ret = wpa_drv_scan(wpa_s, params);
>  	wpa_scan_free_params(params);
>  	work->ctx = NULL;
> @@ -1047,13 +1072,8 @@ ssid_list_set:
>  	}
>  #endif /* CONFIG_P2P */
>  
> -	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN) {
> -		params.mac_addr_rand = 1;
> -		if (wpa_s->mac_addr_scan) {
> -			params.mac_addr = wpa_s->mac_addr_scan;
> -			params.mac_addr_mask = wpa_s->mac_addr_scan +
> ETH_ALEN;
> -		}
> -	}
> +	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCAN)
> +		wpa_setup_mac_addr_rand_params(&params, wpa_s-
> >mac_addr_scan);
>  
>  	if (!is_zero_ether_addr(wpa_s->next_scan_bssid)) {
>  		struct wpa_bss *bss;
> @@ -1469,14 +1489,8 @@ scan:
>  
>  	wpa_setband_scan_freqs(wpa_s, scan_params);
>  
> -	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCHED_SCAN) {
> -		params.mac_addr_rand = 1;
> -		if (wpa_s->mac_addr_sched_scan) {
> -			params.mac_addr = wpa_s->mac_addr_sched_scan;
> -			params.mac_addr_mask = wpa_s-
> >mac_addr_sched_scan +
> -				ETH_ALEN;
> -		}
> -	}
> +	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_SCHED_SCAN)
> +		wpa_setup_mac_addr_rand_params(&params, wpa_s-
> >mac_addr_sched_scan);
>  
>  	ret = wpa_supplicant_start_sched_scan(wpa_s, scan_params);
>  	wpabuf_free(extra_ie);
> @@ -2319,23 +2333,9 @@ wpa_scan_clone_params(const struct
> wpa_driver_scan_params *src)
>  		params->sched_scan_plans_num = src-
> >sched_scan_plans_num;
>  	}
>  
> -	if (src->mac_addr_rand) {
> -		params->mac_addr_rand = src->mac_addr_rand;
> -
> -		if (src->mac_addr && src->mac_addr_mask) {
> -			u8 *mac_addr;
> -
> -			mac_addr = os_malloc(2 * ETH_ALEN);
> -			if (!mac_addr)
> -				goto failed;
> -
> -			os_memcpy(mac_addr, src->mac_addr, ETH_ALEN);
> -			os_memcpy(mac_addr + ETH_ALEN, src-
> >mac_addr_mask,
> -				  ETH_ALEN);
> -			params->mac_addr = mac_addr;
> -			params->mac_addr_mask = mac_addr + ETH_ALEN;
> -		}
> -	}
> +	if (src->mac_addr_rand &&
> +	    wpa_setup_mac_addr_rand_params(params, (const u8 *)src-
> >mac_addr))
> +		goto failed;
>  
>  	if (src->bssid) {
>  		u8 *bssid;
> @@ -2516,13 +2516,8 @@ int wpas_start_pno(struct wpa_supplicant
> *wpa_s)
>  		params.freqs = wpa_s->manual_sched_scan_freqs;
>  	}
>  
> -	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_PNO) {
> -		params.mac_addr_rand = 1;
> -		if (wpa_s->mac_addr_pno) {
> -			params.mac_addr = wpa_s->mac_addr_pno;
> -			params.mac_addr_mask = wpa_s->mac_addr_pno +
> ETH_ALEN;
> -		}
> -	}
> +	if (wpa_s->mac_addr_rand_enable & MAC_ADDR_RAND_PNO)
> +		wpa_setup_mac_addr_rand_params(&params, wpa_s-
> >mac_addr_pno);
>  
>  	ret = wpa_supplicant_start_sched_scan(wpa_s, &params);
>  	os_free(params.filter_ssids);
> diff --git a/wpa_supplicant/wpa_supplicant.c
> b/wpa_supplicant/wpa_supplicant.c
> index 7361ee96d..94e3bcb67 100644
> --- a/wpa_supplicant/wpa_supplicant.c
> +++ b/wpa_supplicant/wpa_supplicant.c
> @@ -6907,3 +6907,59 @@ int wpa_is_bss_tmp_disallowed(struct
> wpa_supplicant *wpa_s, const u8 *bssid)
>  		   MAC2STR(bss->bssid), age.sec, age.usec);
>  	return 1;
>  }
> +
> +int wpas_enable_mac_addr_randomization(struct wpa_supplicant *wpa_s,
> +				       int type, u8 *addr, u8 *mask)
> +{
> +	if ((addr && !mask) || (!addr && mask)) {
> +		wpa_printf(MSG_INFO,
> +			   "MAC_ADDR_RAND_SCAN invalid addr/mask
> combination");
> +		return -1;
> +	}
> +
> +	if (addr && mask && (!(mask[0] & 0x01) || (addr[0] & 0x01))) {
> +		wpa_printf(MSG_INFO,
> +			   "MAC_ADDR_RAND_SCAN cannot allow multicast
> address");
> +		return -1;
> +	}
> +
> +	if (type & MAC_ADDR_RAND_SCAN) {
> +		wpas_mac_addr_rand_scan_set(wpa_s, MAC_ADDR_RAND_SCAN,
> +					    addr, mask);
> +	}
> +
> +	if (type & MAC_ADDR_RAND_SCHED_SCAN) {
> +		wpas_mac_addr_rand_scan_set(wpa_s,
> MAC_ADDR_RAND_SCHED_SCAN,
> +					    addr, mask);
> +
> +		if (wpa_s->sched_scanning && !wpa_s->pno)
> +			wpas_scan_restart_sched_scan(wpa_s);
> +	}
> +
> +	if (type & MAC_ADDR_RAND_PNO) {
> +		wpas_mac_addr_rand_scan_set(wpa_s, MAC_ADDR_RAND_PNO,
> +					    addr, mask);
> +		if (wpa_s->pno) {
> +			wpas_stop_pno(wpa_s);
> +			wpas_start_pno(wpa_s);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +int wpas_disable_mac_addr_randomization(struct wpa_supplicant
> *wpa_s, int type)
> +{
> +	wpas_mac_addr_rand_scan_clear(wpa_s, type);
> +	if (wpa_s->pno) {
> +		if (type & MAC_ADDR_RAND_PNO) {
> +			wpas_stop_pno(wpa_s);
> +			wpas_start_pno(wpa_s);
> +		}
> +	} else if (wpa_s->sched_scanning && (type &
> MAC_ADDR_RAND_SCHED_SCAN)) {
> +		wpas_scan_restart_sched_scan(wpa_s);
> +	}
> +
> +	return 0;
> +}
> diff --git a/wpa_supplicant/wpa_supplicant_i.h
> b/wpa_supplicant/wpa_supplicant_i.h
> index ef9273d09..809791e14 100644
> --- a/wpa_supplicant/wpa_supplicant_i.h
> +++ b/wpa_supplicant/wpa_supplicant_i.h
> @@ -1208,6 +1208,10 @@ void wpas_mbo_update_cell_capa(struct
> wpa_supplicant *wpa_s, u8 mbo_cell_capa);
>  struct wpabuf * mbo_build_anqp_buf(struct wpa_supplicant *wpa_s,
>  				   struct wpa_bss *bss);
>  
> +int wpas_enable_mac_addr_randomization(struct wpa_supplicant *wpa_s,
> +				       int type, u8 *addr, u8 *mask);
> +int wpas_disable_mac_addr_randomization(struct wpa_supplicant
> *wpa_s, int type);
> +
>  /**
>   * wpa_supplicant_ctrl_iface_ctrl_rsp_handle - Handle a control
> response
>   * @wpa_s: Pointer to wpa_supplicant data

On Mon, Apr 22, 2019 at 5:42 PM Dan Williams <dcbw@redhat.com> wrote:
>
> On Mon, 2019-04-22 at 15:58 -0700, Eric Caruso wrote:
> > Add two D-Bus methods:
> > * EnableMACAddressRandomization: (ay : mask) -> nothing
> > * DisableMACAddressRandomization: nothing -> nothing
>
> I'd almost rather see a single method call, like
> SetMACAddressRandomization(ay : mask) -> nothing where a zero-length
> mask means disabling randomization (since it appears a mask is
> required?)
>
> But also, is a mask really required? Can't we just pick a suitable
> default (and allow len(mask)==0) and only if the user wants a different
> behavior, then they have to pass a full mask?

I'm afraid I don't really understand what you mean here. Are you
suggesting a single method with three possible effects, where a 6-byte
mask enables randomization with that mask exactly, a 0-length mask
enables randomization with a default mask (say, keeping the OUI and
randomizing the rest), and no argument to disable randomization?

I think this sort of API design makes it really hard to understand
what's going on and how to use it correctly, personally...

>
> Dan

On Tue, 2019-04-23 at 12:39 -0700, Eric Caruso wrote:
> On Mon, Apr 22, 2019 at 5:42 PM Dan Williams <dcbw@redhat.com> wrote:
> > On Mon, 2019-04-22 at 15:58 -0700, Eric Caruso wrote:
> > > Add two D-Bus methods:
> > > * EnableMACAddressRandomization: (ay : mask) -> nothing
> > > * DisableMACAddressRandomization: nothing -> nothing
> > 
> > I'd almost rather see a single method call, like
> > SetMACAddressRandomization(ay : mask) -> nothing where a zero-
> > length
> > mask means disabling randomization (since it appears a mask is
> > required?)
> > 
> > But also, is a mask really required? Can't we just pick a suitable
> > default (and allow len(mask)==0) and only if the user wants a
> > different
> > behavior, then they have to pass a full mask?
> 
> I'm afraid I don't really understand what you mean here. Are you
> suggesting a single method with three possible effects, where a 6-
> byte
> mask enables randomization with that mask exactly, a 0-length mask
> enables randomization with a default mask (say, keeping the OUI and
> randomizing the rest), and no argument to disable randomization?
> 
> I think this sort of API design makes it really hard to understand
> what's going on and how to use it correctly, personally...

I was sort-of getting at this as a perfect example of a *property* on
the Interface object, not a method. The object should just have a
"MACAddressRandomization" property that you can use the standard D-Bus
Properties interface to get & set. We have a lot of those already.

The downside of properties is that in some language bindings you cannot
receive errors from the Get & Set operations, but those language
bindings are kinda broken.

So yes, while having these as method calls works, and (as I had
suggested) a single method call is perhaps less desirable, the best-
case for this is a property. Any time there's a get/set pattern, or an
enable/disable pattern, that begs for a property.

Dan

> > Dan
> 
> _______________________________________________
> Hostap mailing list
> Hostap@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/hostap

> I was sort-of getting at this as a perfect example of a *property* on
> the Interface object, not a method. The object should just have a
> "MACAddressRandomization" property that you can use the standard D-Bus
> Properties interface to get & set. We have a lot of those already.
>
> The downside of properties is that in some language bindings you cannot
> receive errors from the Get & Set operations, but those language
> bindings are kinda broken.

I think a property sounds fine and am not worried about the bindings issue here.

> So yes, while having these as method calls works, and (as I had
> suggested) a single method call is perhaps less desirable, the best-
> case for this is a property. Any time there's a get/set pattern, or an
> enable/disable pattern, that begs for a property.

This seems right, but I'm not sure how to make this understandable
when the interface is going to have three states: disabled, default,
and enabled with an explicitly specified mask.

If the suggestion was not to have the user set a default but to have
wpa_supplicant start up that way, then I'm not sure I want to do that
because it will probably break any users who are using a MAC whitelist
on their AP. I think it's best if this is disabled by default and
users can turn it on from whatever UI they have. If the default is
only meant for the initial setting, then having this as a property
with len(mask) == 0 to disable and len(mask) == 6 to set the mask
seems fine. Otherwise, if users can request the default mask through
some other fashion, this might get messy.

Thanks!
-Eric

> > I was sort-of getting at this as a perfect example of a *property* on
> > the Interface object, not a method. The object should just have a
> > "MACAddressRandomization" property that you can use the standard D-Bus
> > Properties interface to get & set. We have a lot of those already.
> >
> > The downside of properties is that in some language bindings you cannot
> > receive errors from the Get & Set operations, but those language
> > bindings are kinda broken.
>
> I think a property sounds fine and am not worried about the bindings issue here.
>
> > So yes, while having these as method calls works, and (as I had
> > suggested) a single method call is perhaps less desirable, the best-
> > case for this is a property. Any time there's a get/set pattern, or an
> > enable/disable pattern, that begs for a property.
>
> This seems right, but I'm not sure how to make this understandable
> when the interface is going to have three states: disabled, default,
> and enabled with an explicitly specified mask.

One other thing about this: getting a mask value for the property is
actually not trivial, because we could potentially have different
masks set up for normal scans, scheduled scans, and PNO. I'm not sure
under which circumstances these would differ, but if they do, it's not
clear which to return.

The property could be a{say} of string keys ("scan", "sched_scan",
"pno") or a{uay} with some enum representing the same, mapping the
scan type to a mask, and we would disable it for any scan types not
present in the map. Then an empty map would fully disable MAC address
randomization. It would also make it much clearer what to return no
matter which scan types have randomization enabled, or if they are
different. This also has the benefit of an empty array not carrying
any special meaning.

> If the suggestion was not to have the user set a default but to have
> wpa_supplicant start up that way, then I'm not sure I want to do that
> because it will probably break any users who are using a MAC whitelist
> on their AP. I think it's best if this is disabled by default and
> users can turn it on from whatever UI they have. If the default is
> only meant for the initial setting, then having this as a property
> with len(mask) == 0 to disable and len(mask) == 6 to set the mask
> seems fine. Otherwise, if users can request the default mask through
> some other fashion, this might get messy.
>
> Thanks!
> -Eric

On Thu, 2019-04-25 at 17:29 -0700, Eric Caruso wrote:
> > > I was sort-of getting at this as a perfect example of a
> > > *property* on
> > > the Interface object, not a method. The object should just have a
> > > "MACAddressRandomization" property that you can use the standard
> > > D-Bus
> > > Properties interface to get & set. We have a lot of those
> > > already.
> > > 
> > > The downside of properties is that in some language bindings you
> > > cannot
> > > receive errors from the Get & Set operations, but those language
> > > bindings are kinda broken.
> > 
> > I think a property sounds fine and am not worried about the
> > bindings issue here.
> > 
> > > So yes, while having these as method calls works, and (as I had
> > > suggested) a single method call is perhaps less desirable, the
> > > best-
> > > case for this is a property. Any time there's a get/set pattern,
> > > or an
> > > enable/disable pattern, that begs for a property.
> > 
> > This seems right, but I'm not sure how to make this understandable
> > when the interface is going to have three states: disabled,
> > default,
> > and enabled with an explicitly specified mask.
> 
> One other thing about this: getting a mask value for the property is
> actually not trivial, because we could potentially have different
> masks set up for normal scans, scheduled scans, and PNO. I'm not sure
> under which circumstances these would differ, but if they do, it's
> not
> clear which to return.
> 
> The property could be a{say} of string keys ("scan", "sched_scan",
> "pno") or a{uay} with some enum representing the same, mapping the
> scan type to a mask, and we would disable it for any scan types not
> present in the map. Then an empty map would fully disable MAC address
> randomization. It would also make it much clearer what to return no
> matter which scan types have randomization enabled, or if they are
> different. This also has the benefit of an empty array not carrying
> any special meaning.

Sounds fine to me.

Dan

> > If the suggestion was not to have the user set a default but to
> > have
> > wpa_supplicant start up that way, then I'm not sure I want to do
> > that
> > because it will probably break any users who are using a MAC
> > whitelist
> > on their AP. I think it's best if this is disabled by default and
> > users can turn it on from whatever UI they have. If the default is
> > only meant for the initial setting, then having this as a property
> > with len(mask) == 0 to disable and len(mask) == 6 to set the mask
> > seems fine. Otherwise, if users can request the default mask
> > through
> > some other fashion, this might get messy.
> > 
> > Thanks!
> > -Eric