GNOME Bugzilla – Bug 781336
[review] lr/wps2: WPS support
Last modified: 2020-11-12 14:26:51 UTC
https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/wps
> supplicant-interface: add support for WPS enrollment if you call nm_supplicant_interface_enroll_wps() twice, two on_wps_proxy_acquired() callbacks will return. That is, because nm_supplicant_interface_cancel_wps() is unable to cancel enroll_wps() before on_wps_proxy_acquired() returns. Reusing init_cancellable and other_cancellable is confusing. Why use difference cancellable instances for some operations? Maybe introduce a new wps_cancellable? Does the "Cancel" operation even need a cancellable argument? + switch (data->flags) { + case NM_802_11_AP_WPS_PBC: NM80211WpsFlags has the appearance of a "flags" enum, e.g. the values are powers of 2. At various places however, these values are not treated as flags, instead a exclusive mode types. If you need the flags behavior for certain places but not for others, then it seems two distinct types WpsFlags and WpsTypes are appropriate. Also, NMWifiAP:wps-flags and NMSettingWireless:wps have the type flags. But it's not clear that the enum type to configure the setting shall be the same enum type as the supported WPS modes from the AP. If you have enum values that only make sense at some place but not the other, that seems to suggest that you shouldn't reuse the same enum type for both uses. + array = g_variant_get_fixed_array (val, &len, 1); + if (len >= 8 || len <= 32) { + memcpy (psk, array, len); where did you get that the secret is <=32 bytes? Usually, psk is up to 63 characters (or 64 hex). Could you add a comment with a link to the specification that explains this?
(In reply to Thomas Haller from comment #1) > > supplicant-interface: add support for WPS enrollment > > if you call nm_supplicant_interface_enroll_wps() twice, two > on_wps_proxy_acquired() callbacks will return. That is, because > nm_supplicant_interface_cancel_wps() is unable to cancel enroll_wps() before > on_wps_proxy_acquired() returns. > > Reusing init_cancellable and other_cancellable is confusing. Why use > difference cancellable instances for some operations? Maybe introduce a new > wps_cancellable? Does the "Cancel" operation even need a cancellable > argument? Yes, fixed this. > + switch (data->flags) { > + case NM_802_11_AP_WPS_PBC: > > NM80211WpsFlags has the appearance of a "flags" enum, e.g. the values are > powers of 2. > At various places however, these values are not treated as flags, instead a > exclusive mode types. If you need the flags behavior for certain places but > not for others, then it seems two distinct types WpsFlags and WpsTypes are > appropriate. > Also, NMWifiAP:wps-flags and NMSettingWireless:wps have the type flags. But > it's not clear that the enum type to configure the setting shall be the same > enum type as the supported WPS modes from the AP. > If you have enum values that only make sense at some place but not the > other, that seems to suggest that you shouldn't reuse the same enum type for > both uses. Well, I'd hate to a introduce two different types for what seem to be like very similar uses. The only difference is that the "unknown" type doesn't make sense for the connection. Perhaps it doesn't make sense for the AP either though... > + array = g_variant_get_fixed_array (val, &len, 1); > + if (len >= 8 || len <= 32) { > + memcpy (psk, array, len); > > where did you get that the secret is <=32 bytes? Usually, psk is up to 63 > characters (or 64 hex). Could you add a comment with a link to the > specification that explains this? Yeah, it's up to 63 characters; fixed that. The AP always sends the plain PSK, not the SSID-hashed value of 64 characters. Fixed that now. Pushed a couple of updates to the branch.
> ifcfg-rh: don't skip 802-11-security if there's no WPA-PSK - } else if (error) + } else if (*error) goto error; it is really very common to allow for the error argument to be NULL. Don't assume an error is passed in, because it breaks a common pattern. If you care about the error you need to: GError *local = NULL; psk = parse_wpa_psk (ifcfg, file, ssid, &local); if (psk) { ... } else if (local) { g_propagate_error (error, local); goto error; } > dbus: add NM80211WpsFlags type the enum NM80211WpsFlags has all required values "UNKNOWN", "DISABLED", "AUTO" (good!). But please let's have the default be "UNKNOWN", not "AUTO". This way, you leave the connection property unspecified (UNKNOWN) by default, and can fallback to "AUTO" or whatever global policy you have. If you choose "AUTO" as default, you never know whether the user explicitly selected AUTO or whether is was chosen due to being the default. "UNKNOWN" shall be the default an mean: not explicitly set. Choose global policy (quite possibly "AUTO") The default should have value 0. Hence UNKNOWN = 0, DISABLED = 0x1, AUTO = 0x2, > setting-wireless: add wps property commit message seems obsolete: This also is the reason we don't bother exposing the property in CLI or saving by the setting plugins. + (object_class, PROP_WPS, + g_param_spec_uint (NM_SETTING_WIRELESS_SECURITY_WPS, "", "", + NM_802_11_WPS_AUTO, + NM_802_11_WPS_PIN | NM_802_11_WPS_PBC, + NM_802_11_WPS_AUTO, if you restrict the range of values to the know flags at present, we cannot extend the flags later, without causing g_criticals() with old libnm. That is why our GObject properties usually use the full range from 0 to G_MAXINT32 for flags (G_MAXINT32 seems better then G_MAXINT as it's explicit). You shall reject invalid flags during verify(). + if (priv->wps & NM_802_11_WPS_UNKNOWN) { as said before, can we have UNKNOWN the default? Yes, UNKNOWN *should* be treated like AUTO by the server!! But the setting should have this as default, not AUTO. I think verify() should require that "DISABLED" is used alone. Like if (NM_FLAGS_ANY (priv->wps, ~DISABLED)) error. Likewise, does AUTO make sense together with PCB|PIN? +nm_setting_wireless_security_get_wps (NMSettingWirelessSecurity *setting) +{ + g_return_val_if_fail (NM_IS_SETTING_WIRELESS_SECURITY (setting), NM_802_11_WPS_DISABLED); UNKNOWN. > setting-wireless: add wps-pin property + _("WPS PIN needs to me 8 digits long")); ^^^^ + for (i = 0; i < 8; i++) { + if (g_ascii_isdigit (priv->wps_pin[i])) + continue; there would be if (!NM_STRCHAR_ALL (priv->wps_pin, ch, g_ascii_isdigit (ch))) { but I think you would not like that (fine). + for (i = 6; i >= 0; i--) + wps_csum += (i % 2 ? 1 : 3) * g_ascii_digit_value (priv->wps_pin[i]); + if (g_ascii_digit_value (priv->wps_pin[7]) != (10 - wps_csum % 10) % 10) { I really dislike doing module on signed integers. I know, wps_csum cannot get negative (can it?), but could wps_csum be guint? (10 - wps_csum % 10) % 10 interesting, where is the outer %10 useful? > supplicant-interface: add support for WPS enrollment + /* Supersede any previous WPS initiations. */ + nm_supplicant_interface_cancel_wps (self); + + priv->wps_proxy = g_dbus_proxy_new_for_bus_finish (result, &error); this is certainly wrong, because cancel_wps() first does "if (!priv->wps_proxy) return" in nm_supplicant_interface_enroll_wps(), could we do a g_return_if_fail (NM_IS_SUPPLICANT_INTERFACE (self)); priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); g_return_if_fail (priv->object_path); could the log-lines related to WPS have a common prefix? I find such prefixes very useful when wading through logs. For example, "WPS: starting '%s' enrollment". + + g_dbus_proxy_call (priv->wps_proxy, + "org.freedesktop.DBus.Properties.Set", + g_variant_new ("(ssv)", + WPAS_DBUS_IFACE_INTERFACE_WPS, + "ProcessCredentials", + g_variant_new_boolean (TRUE)), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + NULL, + NULL); + + /* Initiate the enrollment. */ is this safe? Shouldn't you call the async methods one at a time, and chain their responses? Especially with NM, we do asynchronous authentication before setting a property. There is no guarantee that a Properties.Set is complete when calling the next operation. + data = g_slice_new0 (WpsEnrollData); + data->self = g_object_ref (self); if you pass a cancellable and do it right, there is no need in this case to take a reference on @self. This allows to automatically cancel the async operation by unrefing self. nm_supplicant_interface_cancel_wps() should always do a nm_clear_g_cancellable (&priv->wps_cancellable); Otherwise, calling together nm_supplicant_interface_enroll_wps() nm_supplicant_interface_cancel_wps() will not actually cancel the operation. > ifcfg-rh: save/restore WPS properties + if (!nm_utils_enum_from_str (nm_802_11_wps_flags_get_type (), value, (int *) &wps, &token)) { + PARSE_WARNING ("Invalid WPS setting '%s'", token); if ifcfg ever would become a library, then printing warnings is not a suitable way to report something. Hence, warnings should be avoided and are mostly for backward compatibility. If a user explicitly(!) configures invalid flags on a new property, the connection should be rejected right away. + else + tmp = _nm_utils_enum_to_ inden(ta)tion :) + wps = nm_setting_wireless_security_get_wps (s_wsec); + if (wps == NM_802_11_WPS_AUTO) + tmp = NULL; here too. The default is "UNKNOWN". + tmp = _nm_utils_enum_to_str_full (nm_802_11_wps_flags_get_type (), wps, " "); The separator " " indicates that for ifcfg-rh we want this kind of behavior. It seems worth to me to establish a common pattern by adding a wrapper function _enum_to_str() (which defines the separator and other aspects regarding). > cli: add WPS properties pushed 2 fixups. + PROPERTY_INFO_WITH_DESC (NM_SETTING_WIRELESS_SECURITY_WPS_PIN, + .is_secret = TRUE, "is_secret" is indeed only used at one place to hide the text. But note, that *all* other properties that have is_secret=TRUE, are ~proper~ secrets. Could we instead add a different flag in this case "is_hidden", and use it below in »···if ( (info->is_secret || info->is_hidden) »··· && !NM_FLAGS_HAS (get_flags, NM_META_ACCESSOR_GET_FLAGS_SHOW_SECRETS))
g_object_set (G_OBJECT (s_wsec), NM_SETTING_WIRELESS_SECURITY_WPS, wps, NULL); can we cast the enum argument to the proper int type to make it explicit? +supplicant_iface_wps_credentials_cb (NMSupplicantInterface *iface, + GVariant *credentials, + NMDeviceWifi *self) supplicant_iface_wps_credentials_cb modifies the settings connection by internal decision, during activation of the connection. That is unprecedented in NM, and seems questionable. E.g. a user may have permissions to activate a connection but not modify it. So, when he activates the connection, it should not modify the settings connection. E.g. the connection might be on immutable disk (well, we handle that badly currently -- but that is another issue). Adding code here to mutate the settings connection during activation goes in the wrong direction -- at least, doing it without all kinds of precautions (like: does the user who activated the connection have permission for this?) It looks fine to mutate the connection during AddAndActivate(). But the code doesn't differentiate between regular activation, auto-activation, and AddAndActivate(). Maybe this *is* indeed fine, but it needs more consideration.
+ /* Now persist the changes in the settings connection. */ + nm_settings_connection_replace_and_commit (settings_connection, applied_connection, NULL, NULL); also, there is no guarantee that applied_connection and settings_connection are still identical. This might overwrite changes in settings_connection that happened in the mean-time. Also, the applied connection usually never has secrets attached. They are always cleared (if not, it's a bug). The secret handling is complicated enough, but let's not also attach secrets to the applied connection.
(In reply to Thomas Haller from comment #3) > > ifcfg-rh: don't skip 802-11-security if there's no WPA-PSK > > - } else if (error) > + } else if (*error) > goto error; > > it is really very common to allow for the error argument to be NULL. Don't > assume an error is passed in, because it breaks a common pattern. If you > care about the error you need to: > > GError *local = NULL; > > psk = parse_wpa_psk (ifcfg, file, ssid, &local); > if (psk) { > ... > } else if (local) { > g_propagate_error (error, local); > goto error; > } > Fixed. > > dbus: add NM80211WpsFlags type > > the enum NM80211WpsFlags has all required values "UNKNOWN", "DISABLED", > "AUTO" (good!). But please let's have the default be "UNKNOWN", not "AUTO". > This way, you leave the connection property unspecified (UNKNOWN) by > default, and can fallback to "AUTO" or whatever global policy you have. There is no "global policy" and I there should not be. AUTO always makes sense if the user didn't specify an explicit method to be used. > If you choose "AUTO" as default, you never know whether the user explicitly > selected AUTO or whether is was chosen due to being the default. > > "UNKNOWN" shall be the default an mean: not explicitly set. Choose global > policy (quite possibly "AUTO") > > The default should have value 0. > > Hence > UNKNOWN = 0, > DISABLED = 0x1, > AUTO = 0x2, Well, the "UNKNOWN" here was supposed to mean that the AP indicates a WPS method that we don't recognize, not that we don't know what value would the property have. That is not too useful and is in fact confusing. I've now renamed it to UNRECOGNIZED, with default being AUTO. I've done one other change -- ordered the UNRECOGNIZED before the AUTO, making the enum signed. That one does not make sense for wifi-security.wps and your nmcli fixup would cause to be offered as an option. This ways we should set the .min of the range to AUTO and only offer valid options. > > setting-wireless: add wps property > > commit message seems obsolete: > > This also is the > reason we don't bother exposing the property in CLI or saving by the > setting plugins. Fixed. > > + (object_class, PROP_WPS, > + g_param_spec_uint (NM_SETTING_WIRELESS_SECURITY_WPS, "", "", > + NM_802_11_WPS_AUTO, > + NM_802_11_WPS_PIN | NM_802_11_WPS_PBC, > + NM_802_11_WPS_AUTO, > > if you restrict the range of values to the know flags at present, we cannot > extend the flags later, without causing g_criticals() with old libnm. That > is why our GObject properties usually use the full range from 0 to > G_MAXINT32 for flags (G_MAXINT32 seems better then G_MAXINT as it's > explicit). You shall reject invalid flags during verify(). Fixed. > + if (priv->wps & NM_802_11_WPS_UNKNOWN) { > > as said before, can we have UNKNOWN the default? Yes, UNKNOWN *should* be > treated like AUTO by the server!! But the setting should have this as > default, not AUTO. Addressed; see above. > I think verify() should require that "DISABLED" is used alone. Like > > if (NM_FLAGS_ANY (priv->wps, ~DISABLED)) > error. Fixed. > Likewise, does AUTO make sense together with PCB|PIN? Yes. That would mean "Use whatever the AP claims to support or PBC|PIN". > +nm_setting_wireless_security_get_wps (NMSettingWirelessSecurity *setting) > +{ > + g_return_val_if_fail (NM_IS_SETTING_WIRELESS_SECURITY (setting), > NM_802_11_WPS_DISABLED); > > UNKNOWN. UNKNOWN (or "UNRECOGNIZED" now) is not allowed in the property. > > setting-wireless: add wps-pin property > > + _("WPS PIN needs to me 8 digits long")); > ^^^^ Fixed. > + for (i = 0; i < 8; i++) { > + if (g_ascii_isdigit (priv->wps_pin[i])) > + continue; > > there would be > if (!NM_STRCHAR_ALL (priv->wps_pin, ch, g_ascii_isdigit (ch))) { > but I think you would not like that (fine). I can't read that. > + for (i = 6; i >= 0; i--) > + wps_csum += (i % 2 ? 1 : 3) * g_ascii_digit_value > (priv->wps_pin[i]); > + if (g_ascii_digit_value (priv->wps_pin[7]) != (10 - wps_csum % 10) > % 10) { > > I really dislike doing module on signed integers. I know, wps_csum cannot > get negative (can it?), but could wps_csum be guint? You're right. Switched do guint. > (10 - wps_csum % 10) % 10 > > interesting, where is the outer %10 useful? This basically takes the last digit of wps_csum and turns 1..9 to 9..1 while 0 stays 0. Thus if last digit of wps_csum is 0, then the () returns 10 and the trailing modulo turns that into a 0. Presumably done so that "00000000" is a valid PIN. > > supplicant-interface: add support for WPS enrollment > > > > + /* Supersede any previous WPS initiations. */ > + nm_supplicant_interface_cancel_wps (self); > + > + priv->wps_proxy = g_dbus_proxy_new_for_bus_finish (result, &error); > > this is certainly wrong, because cancel_wps() first does "if > (!priv->wps_proxy) return" No. The wps_proxy could be set from previous enrollment attempt. > in nm_supplicant_interface_enroll_wps(), could we do a > > g_return_if_fail (NM_IS_SUPPLICANT_INTERFACE (self)); > priv = NM_SUPPLICANT_INTERFACE_GET_PRIVATE (self); > g_return_if_fail (priv->object_path); > > > > could the log-lines related to WPS have a common prefix? I find such > prefixes very useful when wading through logs. For example, "WPS: starting > '%s' enrollment". Fixed in nm-device-wps.c > + g_dbus_proxy_call (priv->wps_proxy, > + "org.freedesktop.DBus.Properties.Set", > + g_variant_new ("(ssv)", > + WPAS_DBUS_IFACE_INTERFACE_WPS, > + "ProcessCredentials", > + g_variant_new_boolean (TRUE)), > + G_DBUS_CALL_FLAGS_NONE, > + -1, > + NULL, > + NULL, > + NULL); > + > + /* Initiate the enrollment. */ > > is this safe? Shouldn't you call the async methods one at a time, and chain > their responses? Especially with NM, we do asynchronous authentication > before setting a property. There is no guarantee that a Properties.Set is > complete when calling the next operation. It probably is. Calling the property set without waiting for it to finish is pretty commonly done (the GDBus generated boilerplate certainly does that). But yes, it won't work for NetworkManager... > + data = g_slice_new0 (WpsEnrollData); > + data->self = g_object_ref (self); > > if you pass a cancellable and do it right, there is no need in this case to > take a reference on @self. This allows to automatically cancel the async > operation by unrefing self. Fixed. > nm_supplicant_interface_cancel_wps() should always do a > nm_clear_g_cancellable (&priv->wps_cancellable); > Otherwise, calling together > nm_supplicant_interface_enroll_wps() > nm_supplicant_interface_cancel_wps() > will not actually cancel the operation. Fixed. > > ifcfg-rh: save/restore WPS properties > > > + if (!nm_utils_enum_from_str (nm_802_11_wps_flags_get_type (), > value, (int *) &wps, &token)) { > + PARSE_WARNING ("Invalid WPS setting '%s'", token); > > > if ifcfg ever would become a library, then printing warnings is not a > suitable way to report something. Hence, warnings should be avoided and are > mostly for backward compatibility. If a user explicitly(!) configures > invalid flags on a new property, the connection should be rejected right > away. Fixed. > + else > + tmp = _nm_utils_enum_to_ > > inden(ta)tion :) Fix(at)ed. > + wps = nm_setting_wireless_security_get_wps (s_wsec); > + if (wps == NM_802_11_WPS_AUTO) > + tmp = NULL; > > here too. The default is "UNKNOWN". It's not! > + tmp = _nm_utils_enum_to_str_full > (nm_802_11_wps_flags_get_type (), wps, " "); > > The separator " " indicates that for ifcfg-rh we want this kind of behavior. > It seems worth to me to establish a common pattern by adding a wrapper > function _enum_to_str() (which defines the separator and other aspects > regarding). Ok. > > cli: add WPS properties > > pushed 2 fixups. Applied 1 and a half. > + PROPERTY_INFO_WITH_DESC (NM_SETTING_WIRELESS_SECURITY_WPS_PIN, > + .is_secret = TRUE, > > "is_secret" is indeed only used at one place to hide the text. But note, > that *all* other properties that have is_secret=TRUE, are ~proper~ secrets. > Could we instead add a different flag in this case "is_hidden", and use it > below in > > »···if ( (info->is_secret || info->is_hidden) > »··· && !NM_FLAGS_HAS (get_flags, > NM_META_ACCESSOR_GET_FLAGS_SHOW_SECRETS)) Dropped that. No longer considering the PIN a secret. (In reply to Thomas Haller from comment #5) > + /* Now persist the changes in the settings connection. */ > + nm_settings_connection_replace_and_commit (settings_connection, > applied_connection, NULL, NULL); > > also, there is no guarantee that applied_connection and settings_connection > are still identical. This might overwrite changes in settings_connection > that happened in the mean-time. + if ( nm_device_get_state (NM_DEVICE (self)) != NM_DEVICE_STATE_NEED_AUTH + || !nm_device_has_unmodified_applied_connection (NM_DEVICE (self), NM_SETTING_COMPARE_FLAG_NONE)) { + _LOGI (LOGD_DEVICE | LOGD_WIFI, "The connection can't be updated with WPS secrets"); + return; + } > Also, the applied connection usually never has secrets attached. They are > always cleared (if not, it's a bug). The secret handling is complicated > enough, but let's not also attach secrets to the applied connection. True. Fixed. (In reply to Thomas Haller from comment #4) > g_object_set (G_OBJECT (s_wsec), NM_SETTING_WIRELESS_SECURITY_WPS, wps, > NULL); > > can we cast the enum argument to the proper int type to make it explicit? Done. > +supplicant_iface_wps_credentials_cb (NMSupplicantInterface *iface, > + GVariant *credentials, > + NMDeviceWifi *self) > > supplicant_iface_wps_credentials_cb modifies the settings connection by > internal decision, during activation of the connection. That is > unprecedented in NM, and seems questionable. It only supplies the secrets now. Removed the SSID handling; it doesn't server any purpose for now since we don't allow activation of a connection with blank wifi.ssid anyway. > E.g. a user may have permissions to activate a connection but not modify it. > So, when he activates the connection, it should not modify the settings > connection. In that case whoever created the connection should have done the pairing themselves or disabled it. Or, well, supplied the PSK. > E.g. the connection might be on immutable disk (well, we handle that badly > currently -- but that is another issue). That is dealt with using the existing error handling. > Adding code here to mutate the settings connection during activation goes in > the wrong direction -- at least, doing it without all kinds of precautions > (like: does the user who activated the connection have permission for this?) > > It looks fine to mutate the connection during AddAndActivate(). But the code > doesn't differentiate between regular activation, auto-activation, and > AddAndActivate(). > > Maybe this *is* indeed fine, but it needs more consideration. Well, certain connection properties can only be learned during the actual activation. Such as the PSK here. In any case, I'm open to more consideration, but please stay constructive. Pushed an updated branch.
in "introspection/org.freedesktop.NetworkManager.AccessPoint.xml", I think there should be no new property "WpsFlags", instead the existing "Flags" should be extended to expose WPS capability too. Also, libnm and cli support for the new AccessPoint's flags is missing. This happens to avoid my previously mentioned dislike about re-using the same NM80211WpsFlags type for org.freedesktop.NetworkManager.AccessPoint and NMSettingWirelessSecurity. NMSettingWirelessSecurityWpsFlags would seem a better name for the flags type used by NMSettingWirelessSecurity::wps. »···NM_802_11_WPS_UNRECOGNIZED = -1, »···NM_802_11_WPS_AUTO = 0, »···NM_802_11_WPS_DISABLED = 1, a negative flag value is strange. Can we avoid bit-operations on signed types, and especially avoid negative flag values? Also, I find it ugly that nm_802_11_wps_flags_get_type does: static const GFlagsValue values[] = { { NM_802_11_WPS_UNRECOGNIZED, "NM_802_11_WPS_UNRECOGNIZED", "unrecognized" }, (thus, implicitly casting UNRECOGNIZED to guint). The use of negative values is not strictly speaking wrong, but seems confusing for no apparent gain. Also, x = NM_802_11_WPS_UNRECOGNIZED; NM_FLAGS_HAS (x, NM_802_11_WPS_PBC) is TRUE. Not all enum values have consistent power-of-2 values, so, when handling the enum you must treat some of them as regular enum values and some as flags. Like: if (x == NM_802_11_WPS_UNRECOGNIZED) ... else { // use bit-operations on x for "flags". } You can all of this avoid by making "UNRECOGNIZED = 0x1000", so UNRECOGNIZED does not collide with the regular flags.
Yes. Should be addressed in lr/wps2
+ /* Enable Credentials processing. */ + _nm_dbus_signal_connect (priv->wps_proxy, "Credentials", G_VARIANT_TYPE ("(a{sv})"), + G_CALLBACK (wpas_iface_wps_credentials), self); the signal handler is never disconnected from priv->wps_proxy. + v = g_variant_lookup_value (properties, "WPS", G_VARIANT_TYPE_VARDICT); + if (v && g_variant_lookup (v, "Type", "&s", &s)) { leaks @v nm_settings_connection_new_secrets(): + nm_connection_update_secrets (applied_connection, setting_name, secrets, NULL); + return TRUE; seems wrong to modify the applied-connection with secrets. Why is this done? + if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO) + type = pin ? "pin" : "pbc"; + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC) + type = "pbc"; + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PIN) + type = "pin"; + else + type = NULL; Seems "AUTO | PBC" or "AUTO | PIN" makes no sense, but NMSettingWirelessSecurity::verify() doesn't reject this combination. > ifcfg-rh: save/restore WPS properties there is svGetValueEnum()
(In reply to Thomas Haller from comment #9) > + /* Enable Credentials processing. */ > + _nm_dbus_signal_connect (priv->wps_proxy, "Credentials", G_VARIANT_TYPE > ("(a{sv})"), > + G_CALLBACK (wpas_iface_wps_credentials), self); > > the signal handler is never disconnected from priv->wps_proxy. That looks correct to me? The signal gets town down along with the proxy, and given the supplicant interface owns it it always happens before the supplicant is gone? > + v = g_variant_lookup_value (properties, "WPS", G_VARIANT_TYPE_VARDICT); > + if (v && g_variant_lookup (v, "Type", "&s", &s)) { > > leaks @v Fixed. > nm_settings_connection_new_secrets(): > > + nm_connection_update_secrets (applied_connection, setting_name, > secrets, NULL); > + return TRUE; > > seems wrong to modify the applied-connection with secrets. Why is this done? Well, I copied it from get_secrets_done_cb(), because the same thing is done when the agent supplies the secret. But you're right -- it's useless. I think it is done because the agents can modify other properties (such as the flags), but we don't want to do that here. Removed. > + if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO) > + type = pin ? "pin" : "pbc"; > + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC) > + type = "pbc"; > + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PIN) > + type = "pin"; > + else > + type = NULL; > > Seems "AUTO | PBC" or "AUTO | PIN" makes no sense, but > NMSettingWirelessSecurity::verify() doesn't reject this combination. It does. "Try either whatever the AP seems to offer or PBC" and "Try either whatever the AP seems to offer or PIN". Not too useful, but also not really wrong. > > ifcfg-rh: save/restore WPS properties > > there is svGetValueEnum() Modified to use that one. Pushed an updated branch.
(In reply to Lubomir Rintel from comment #10) > (In reply to Thomas Haller from comment #9) > > + /* Enable Credentials processing. */ > > + _nm_dbus_signal_connect (priv->wps_proxy, "Credentials", G_VARIANT_TYPE > > ("(a{sv})"), > > + G_CALLBACK (wpas_iface_wps_credentials), self); > > > > the signal handler is never disconnected from priv->wps_proxy. > > That looks correct to me? The signal gets town down along with the proxy, > and given the supplicant interface owns it it always happens before the > supplicant is gone? Unrefing priv-wps_proxy does not mean the proxy is destroyed immediately. The signal should be disconnect to be sure that the callback is not invoked later (at an unexpected time, or when @self is already destroyed). > > nm_settings_connection_new_secrets(): > > > > + nm_connection_update_secrets (applied_connection, setting_name, > > secrets, NULL); > > + return TRUE; > > > > seems wrong to modify the applied-connection with secrets. Why is this done? > > Well, I copied it from get_secrets_done_cb(), because the same thing is done > when the agent supplies the secret. > > But you're right -- it's useless. I think it is done because the agents can > modify other properties (such as the flags), but we don't want to do that > here. I don't think that secret-flags can be modified during a secrets request. That is why I did https://git.gnome.org/browse/network-manager-applet/commit/?id=ac2adab8e1f99417a4b3c32f13d629302fc6aaab > > + if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO) > > + type = pin ? "pin" : "pbc"; > > + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC) > > + type = "pbc"; > > + else if (wps_method & NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PIN) > > + type = "pin"; > > + else > > + type = NULL; > > > > Seems "AUTO | PBC" or "AUTO | PIN" makes no sense, but > > NMSettingWirelessSecurity::verify() doesn't reject this combination. > > It does. "Try either whatever the AP seems to offer or PBC" and "Try either > whatever the AP seems to offer or PIN". Not too useful, but also not really > wrong. In case of g_object_set (s_wsec, NM_SETTING_WIRELESS_SECURITY_WPS_METHOD, NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO | NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC, NULL); handle_auth_or_fail() doesn't do "Try either whatever the AP seems to offer or PBC". It does "whathever the AP seems to offer". Then handle_auth_or_fail() is wrong, because it never does to "or PBC" part. clients/cli/devices.c: In function ‘do_device_wifi_connect_network’: clients/cli/devices.c:3146:4: error: ‘s_wsec’ may be used uninitialized in this function [-Werror=maybe-uninitialized] g_object_set (s_wsec, NM_SETTING_WIRELESS_SECURITY_WPS_PIN, pin, NULL); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > It does. "Try either whatever the AP seems to offer or PBC" and "Try either > > whatever the AP seems to offer or PIN". Not too useful, but also not really > > wrong. > > In case of > > g_object_set (s_wsec, > NM_SETTING_WIRELESS_SECURITY_WPS_METHOD, > NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO > | NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC, > NULL); > > handle_auth_or_fail() doesn't do "Try either whatever the AP seems to offer > or PBC". > > It does "whathever the AP seems to offer". actually, "AUTO|PBC" it does: use "pin" if NM_SETTING_WIRELESS_SECURITY_WPS_PIN is set, otherwise "pbc". Does it even make sense to try "pin", when NM_SETTING_WIRELESS_SECURITY_WPS_PIN is unset?
(In reply to Thomas Haller from comment #11) > (In reply to Lubomir Rintel from comment #10) > > (In reply to Thomas Haller from comment #9) > > > + /* Enable Credentials processing. */ > > > + _nm_dbus_signal_connect (priv->wps_proxy, "Credentials", G_VARIANT_TYPE > > > ("(a{sv})"), > > > + G_CALLBACK (wpas_iface_wps_credentials), self); > > > > > > the signal handler is never disconnected from priv->wps_proxy. > > > > That looks correct to me? The signal gets town down along with the proxy, > > and given the supplicant interface owns it it always happens before the > > supplicant is gone? > > Unrefing priv-wps_proxy does not mean the proxy is destroyed immediately. > The signal should be disconnect to be sure that the callback is not invoked > later (at an unexpected time, or when @self is already destroyed). Okay. Fixed. > > > nm_settings_connection_new_secrets(): > > > > > > + nm_connection_update_secrets (applied_connection, setting_name, > > > secrets, NULL); > > > + return TRUE; > > > > > > seems wrong to modify the applied-connection with secrets. Why is this done? > > > > Well, I copied it from get_secrets_done_cb(), because the same thing is done > > when the agent supplies the secret. > > > > But you're right -- it's useless. I think it is done because the agents can > > modify other properties (such as the flags), but we don't want to do that > > here. > > I don't think that secret-flags can be modified during a secrets request. > That is why I did > https://git.gnome.org/browse/network-manager-applet/commit/ > ?id=ac2adab8e1f99417a4b3c32f13d629302fc6aaab Uh, okay. I'm not really sure now why it's done. Either way, it doesn't matter. > clients/cli/devices.c: In function ‘do_device_wifi_connect_network’: > clients/cli/devices.c:3146:4: error: ‘s_wsec’ may be used uninitialized in > this function [-Werror=maybe-uninitialized] > g_object_set (s_wsec, NM_SETTING_WIRELESS_SECURITY_WPS_PIN, pin, NULL); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ That looks like a false positive. Not sure why doesn't the compiler complain about a similar construct above. Worked around that. (In reply to Thomas Haller from comment #12) > > > It does. "Try either whatever the AP seems to offer or PBC" and "Try either > > > whatever the AP seems to offer or PIN". Not too useful, but also not really > > > wrong. > > > > In case of > > > > g_object_set (s_wsec, > > NM_SETTING_WIRELESS_SECURITY_WPS_METHOD, > > NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_AUTO > > | NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PBC, > > NULL); > > > > handle_auth_or_fail() doesn't do "Try either whatever the AP seems to offer > > or PBC". > > > > It does "whathever the AP seems to offer". > > actually, "AUTO|PBC" it does: > > use "pin" if NM_SETTING_WIRELESS_SECURITY_WPS_PIN is set, otherwise "pbc". Modified to actually take the AP indicated flags into account. > Does it even make sense to try "pin", when > NM_SETTING_WIRELESS_SECURITY_WPS_PIN is unset? wpa_supplicant documentation suggests that it's possible, so it doesn't make sense to me to disallow it. That said, it doesn't specify what actually happens in that case. It could certainly make sense to generate a random PIN, and verify it on the router; but all the bits that would be required to make that work are not in place.
(Updated the branch)
lgtm
> > I don't think that secret-flags can be modified during a secrets request. > That is why I did > https://git.gnome.org/browse/network-manager-applet/commit/ > ?id=ac2adab8e1f99417a4b3c32f13d629302fc6aaab Design decision from a long time back. If you can modify any setting property during a secrets request, that causes connection updates on D-Bus, which caused a cascade of other problems. > setting-wireless: add wps-method property + case PROP_WPS_METHOD: + priv->wps_method = g_value_get_uint (value); + break; do we want g_value_[get|set]_enum() here and g_param_spec_enum()? Do we do 'uint' here to ensure that old libnm won't spew warnings if we extend in the future, because the enum paramspec does internal checking of the values? > setting-wireless: add wps-pin property I've missed some of the back conversation on this one. Do we want this property to be NM_SETTING_PARAM_SECRET or does that complicate things too much? The only time I've used WPS it's been during the normal "enter the password!" dialog on Windows and on the Nokia N900, the same place I'd enter the PSK during the first connect. So I'm not sure what's different about the WPS PIN than a PSK or WEP key from a flow perspective. > supplicant-interface: add support for WPS enrollment Should we disconnect from the wps_proxy Credentials signal when the supplicant interface gets to NM_SUPPLICANT_INTERFACE_STATE_COMPLETED or once we've processed credentials? > wifi: attempt a WPS enroll when secrets are missing what's the operational flow here? Looks like we attempt to enroll_wps(), then proceed with the secrets request. How does the secret agent choose between asking for (a) a PSK, (b) the user to push the button on the AP, or (c) the user to enter the WPS PIN? I don't think the agent can look at the AP flags, because the agent might not have access to the full D-Bus interface for the AP object. We also can't get that info from the connection itself, because that doesn't tell us whether NM actually decided to do WPS or not. Maybe we could pass some hints to the agent so it knows what NM is expecting and can present the right UI?
(In reply to Dan Williams from comment #16) > + case PROP_WPS_METHOD: > + priv->wps_method = g_value_get_uint (value); > + break; > > do we want g_value_[get|set]_enum() here and g_param_spec_enum()? Do we do > 'uint' here to ensure that old libnm won't spew warnings if we extend in the > future, because the enum paramspec does internal checking of the values? Yes. I think if we extended the enum, then the old nmcli would complain. Whereas now it just prints the numeric value. > > setting-wireless: add wps-pin property > > I've missed some of the back conversation on this one. > > Do we want this property to be NM_SETTING_PARAM_SECRET or does that > complicate things too much? The only time I've used WPS it's been during > the normal "enter the password!" dialog on Windows and on the Nokia N900, > the same place I'd enter the PSK during the first connect. So I'm not sure > what's different about the WPS PIN than a PSK or WEP key from a flow > perspective. I didn't manage to get a Windows installation to ask me for the PIN, nor does typing it in the password entry work. A Nokia N9 phone and Thomas' Android phone generate their own PIN instead of asking for one despite the router doesn't have a display and hostapd is configured to use the "label" method. Using the wpa_supplicant seems to work correctly (the BSS indicates support for "pin" method and pairing succeeds). I've now removed the PIN support, since given what you say and what I've observed -- it's really not clear to me how should that work. Presumably this requires me to do some more research. > > supplicant-interface: add support for WPS enrollment > > Should we disconnect from the wps_proxy Credentials signal when the > supplicant interface gets to NM_SUPPLICANT_INTERFACE_STATE_COMPLETED or once > we've processed credentials? I don't think we need to. Upon receiving the credentials we proceed to PREPARE state, and we ignore the credentials when not in NEED_AUTH. If we eventually move back to NEED_AUTH (is that possible?) and the supplicant for some reason sends another set credentials the best thing to use is probably to attempt to use them. > > wifi: attempt a WPS enroll when secrets are missing > > what's the operational flow here? Looks like we attempt to enroll_wps(), > then proceed with the secrets request. How does the secret agent choose > between asking for (a) a PSK, (b) the user to push the button on the AP, or > (c) the user to enter the WPS PIN? > > I don't think the agent can look at the AP flags, because the agent might > not have access to the full D-Bus interface for the AP object. We also > can't get that info from the connection itself, because that doesn't tell us > whether NM actually decided to do WPS or not. > > Maybe we could pass some hints to the agent so it knows what NM is expecting > and can present the right UI? The original idea was that the WPS method to be used will be stored in the connection when it's completed on AddAndActivate. Nevertheless it turned out that it's probably a good idea if there's an "AUTO" setting and the actual method is selected on activation. Setting the secret flag makes sense to me. Added a commit with that. Updated the branch.
+ * description: Used to control the WPS methods to be used + * Valid values are "default", "auto", "disabled", "pin" and "pbc". + * If ommited, whatver the AP announces is used. + * example: WPS_METHOD=disabled, WPS_METHOD="pin pbc" "omitted" if omitted, the default is "default". "default" (currently) falls back to "auto", which in turn is whatever the AP announces. Pushed two fixups. Rest lgtm
(In reply to Lubomir Rintel from comment #17) > (In reply to Dan Williams from comment #16) > > + case PROP_WPS_METHOD: > > + priv->wps_method = g_value_get_uint (value); > > + break; > > > > do we want g_value_[get|set]_enum() here and g_param_spec_enum()? Do we do > > 'uint' here to ensure that old libnm won't spew warnings if we extend in the > > future, because the enum paramspec does internal checking of the values? > > Yes. I think if we extended the enum, then the old nmcli would complain. > Whereas now it just prints the numeric value. OK. > > > setting-wireless: add wps-pin property > > > > I've missed some of the back conversation on this one. > > > > Do we want this property to be NM_SETTING_PARAM_SECRET or does that > > complicate things too much? The only time I've used WPS it's been during > > the normal "enter the password!" dialog on Windows and on the Nokia N900, > > the same place I'd enter the PSK during the first connect. So I'm not sure > > what's different about the WPS PIN than a PSK or WEP key from a flow > > perspective. > > I didn't manage to get a Windows installation to ask me for the PIN, nor > does typing it in the password entry work. A Nokia N9 phone and Thomas' > Android phone generate their own PIN instead of asking for one despite the > router doesn't have a display and hostapd is configured to use the "label" > method. > > Using the wpa_supplicant seems to work correctly (the BSS indicates support > for "pin" method and pairing succeeds). > > I've now removed the PIN support, since given what you say and what I've > observed -- it's really not clear to me how should that work. Presumably > this requires me to do some more research. AFAIK it's usually a PIN printed on the bottom of the AP, though some devices (usually mobile hotspots like https://www.verizonwireless.com/internet-devices/verizon-jetpack-mifi-7730l/ might be able to accept PINs through their touchscreens). But I agree, I've personally never seen this method in the wild, it's always been "pushbutton" on the consumer-type APs I know of. So removal of PIN is probably fine for now. If we get requests for it, we can always add it once we figure out what model of AP the user has. But, there still are some PIN-related things that could disrupt the flow, like in nm-device-wifi.c::handle_auth_or_fail(). I think we should ignore NM_SETTING_WIRELESS_SECURITY_WPS_METHOD_PIN there and just fall back to PSK. Otherwise we'll ask for PIN, but since it's not supported via the D-Bus interface, the agent won't be able to do anything about it. > > > supplicant-interface: add support for WPS enrollment > > > > Should we disconnect from the wps_proxy Credentials signal when the > > supplicant interface gets to NM_SUPPLICANT_INTERFACE_STATE_COMPLETED or once > > we've processed credentials? > > I don't think we need to. Upon receiving the credentials we proceed to > PREPARE state, and we ignore the credentials when not in NEED_AUTH. If we > eventually move back to NEED_AUTH (is that possible?) and the supplicant for > some reason sends another set credentials the best thing to use is probably > to attempt to use them. OK. > > > wifi: attempt a WPS enroll when secrets are missing > > > > what's the operational flow here? Looks like we attempt to enroll_wps(), > > then proceed with the secrets request. How does the secret agent choose > > between asking for (a) a PSK, (b) the user to push the button on the AP, or > > (c) the user to enter the WPS PIN? > > > > I don't think the agent can look at the AP flags, because the agent might > > not have access to the full D-Bus interface for the AP object. We also > > can't get that info from the connection itself, because that doesn't tell us > > whether NM actually decided to do WPS or not. > > > > Maybe we could pass some hints to the agent so it knows what NM is expecting > > and can present the right UI? > > The original idea was that the WPS method to be used will be stored in the > connection when it's completed on AddAndActivate. Nevertheless it turned out > that it's probably a good idea if there's an "AUTO" setting and the actual > method is selected on activation. > > Setting the secret flag makes sense to me. Added a commit with that. > > Updated the branch. Perhaps move "dbus/secret-agent: add a flag indicating WPS PBC is active" before "wifi: attempt a WPS enroll when secrets are missing" so the commits are individually testable? Otherwise seems OK to me, though of course I haven't tested it.
Hi, what is the state of play since this was posted a year ago? I have Fedora 28 which included NM-1.10.6 allegedly including WPS support. Is there anything working which allows push button auth. without need to have root access and hacker skills? I'm setting up a system for an end user that will not have that skill set and I to whom I do not intend to give root access. ( Child user ).
bugzilla.gnome.org is being shut down in favor of a GitLab instance. We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time. If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/ Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).