From c5f46bae43caa1816ec43d7f0bbda1fad89329ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 22 Jan 2024 15:54:54 +0100 Subject: [PATCH 1/2] libnmc: fix secrets request on 2nd stage of 2FA authentication Clients using nm-secret-agent-simple always asked for some default VPN secrets, which are dependent on the VPN service, when the auth dialog can't be used and the fallback method is used instead. When using 2FA this has to be avoided in the 2nd step because those default secrets were already requested and validated in the 1st step. Fix it by adding a new "x-dynamic-challenge" prefix tag that can be used in the hints received from the VPN plugin. This tag indicates that we are in the 2nd step of a 2FA authentication. This way we know that we don't have to request the default secrets this time. Note that the tag name doesn't explicitly mention VPNs so it can be reused for other type of connections in the future. As the default secrets were requested always unconditionally when using the fallback method, there is no possible workaround to this problem that avoids having to change libnm-client. The change is backwards compatible because VPN plugins were not using the tag and the previous behaviour does not change if the tag is not used. However, VPN plugins that want to properly support 2FA aunthentication will need to bump the NM version dependency because old daemons won't handle properly a hint with the new prefix tag. Finally, move the macro that defines the "x-vpn-message:" tag in a public header so it is more visible for users. It has been renamed and prefixed with the NM_ namespace so it shouldn't collide with macros defined in the VPN plugins. --- src/libnm-core-public/nm-dbus-interface.h | 14 +++++++++++++ src/libnmc-base/nm-secret-agent-simple.c | 24 +++++++++++++++++------ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/libnm-core-public/nm-dbus-interface.h b/src/libnm-core-public/nm-dbus-interface.h index 5acbf467f5..5467a99601 100644 --- a/src/libnm-core-public/nm-dbus-interface.h +++ b/src/libnm-core-public/nm-dbus-interface.h @@ -1415,4 +1415,18 @@ typedef enum /*< flags >*/ { NM_MPTCP_FLAGS_FULLMESH = 0x80, } NMMptcpFlags; +/* For secrets requests, hints starting with "x-vpn-message:" are a message to show, not + * a secret to request + */ +#define NM_SECRET_TAG_VPN_MSG "x-vpn-message:" + +/* For secrets requests, hints starting with "x-dynamic-challenge:" are dynamic + * 2FA challenges that are requested in a second authentication step, after the password + * (or whatever auth method is used) was already successfully validated. Because of + * that, the default secrets of the service mustn't be requested (again). + * + * Note: currently only implemented for VPN, but can be extended. + */ +#define NM_SECRET_TAG_DYNAMIC_CHALLENGE "x-dynamic-challenge:" + #endif /* __NM_DBUS_INTERFACE_H__ */ diff --git a/src/libnmc-base/nm-secret-agent-simple.c b/src/libnmc-base/nm-secret-agent-simple.c index 1b9aa57142..02f492bc7c 100644 --- a/src/libnmc-base/nm-secret-agent-simple.c +++ b/src/libnmc-base/nm-secret-agent-simple.c @@ -416,8 +416,6 @@ add_vpn_secret_helper(GPtrArray *secrets, } } -#define VPN_MSG_TAG "x-vpn-message:" - static gboolean add_vpn_secrets(RequestData *request, GPtrArray *secrets, char **msg) { @@ -425,19 +423,33 @@ add_vpn_secrets(RequestData *request, GPtrArray *secrets, char **msg) const NmcVpnPasswordName *p; const char *vpn_msg = NULL; char **iter; + char *secret_name; + bool is_challenge = FALSE; /* If hints are given, then always ask for what the hints require */ if (request->hints) { for (iter = request->hints; *iter; iter++) { - if (!vpn_msg && g_str_has_prefix(*iter, VPN_MSG_TAG)) - vpn_msg = &(*iter)[NM_STRLEN(VPN_MSG_TAG)]; - else - add_vpn_secret_helper(secrets, s_vpn, *iter, *iter); + if (!vpn_msg && NM_STR_HAS_PREFIX(*iter, NM_SECRET_TAG_VPN_MSG)) { + vpn_msg = &(*iter)[NM_STRLEN(NM_SECRET_TAG_VPN_MSG)]; + } else { + if (NM_STR_HAS_PREFIX(*iter, NM_SECRET_TAG_DYNAMIC_CHALLENGE)) { + secret_name = &(*iter)[NM_STRLEN(NM_SECRET_TAG_DYNAMIC_CHALLENGE)]; + is_challenge = TRUE; + } else { + secret_name = *iter; + } + + add_vpn_secret_helper(secrets, s_vpn, secret_name, secret_name); + } } } NM_SET_OUT(msg, g_strdup(vpn_msg)); + /* If we are in the 2nd step of a 2FA authentication, don't ask again for the default secrets */ + if (is_challenge) + return TRUE; + /* Now add what client thinks might be required, because hints may be empty or incomplete */ p = nm_vpn_get_secret_names(nm_setting_vpn_get_service_type(s_vpn)); while (p && p->name) { From 2ab56e82d42995ae851d374b96384bb27f8d8e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 22 Jan 2024 16:16:16 +0100 Subject: [PATCH 2/2] libnmc: allow user input in ECHO mode for 2FA challenges Depending on the type of challenge used in the 2FA authentication, the user input doesn't need to be hidden and sometimes it's even undesired (it makes more difficult to enter the text). Allow to VPN plugins to indicate that a secret that is being requested is a 2FA challenge with ECHO mode enabled: - When using auth dialog: accept a new option "ForceEcho" that can be set to TRUE to enable ECHO. - When using the fallback method: recognize the prefix "x-dynamic-challenge-echo". This indicate both that ECHO should be enabled and that this is a 2FA challenge (see previous commit). The correct way to enable echo mode from VPN plugins is doing both things: pass the hint prefixed with "x-dynamic-challenge-echo" and add the option "ForceEcho=true" for the auth dialog. An attempt to support ECHO mode from NM-openvpn was made by passing "IsSecret=false", but it didn't work because nm-secret-agent-simple ignores returned values for which "IsSecret=false". It's not a good idea to start accepting them because we could break other plugins, and anyway the challenge response is actually a secret, so it is better to keep it as such and add this new "ForceEcho" option. This is backwards compatible because existing plugins were not using the tag nor the auth dialog option. Withouth them, the previous behaviour is preserved. On the contrary, plugins that want to use this new feature will need to bump their NM version dependency because old daemons will not handle correctly the prefix tag. Secret agents will need to be updated to check secret->force_echo if they want to support this feature. Until they update, the only drawback is that ECHO mode will be ignored and the user's input will be hidden. Updated nmcli and nmtui to support ECHO mode. --- src/libnm-core-public/nm-dbus-interface.h | 7 ++-- src/libnmc-base/nm-secret-agent-simple.c | 41 +++++++++++++++++------ src/libnmc-base/nm-secret-agent-simple.h | 1 + src/nmcli/common.c | 2 +- src/nmtui/nmt-password-dialog.c | 2 +- 5 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/libnm-core-public/nm-dbus-interface.h b/src/libnm-core-public/nm-dbus-interface.h index 5467a99601..cc87c74474 100644 --- a/src/libnm-core-public/nm-dbus-interface.h +++ b/src/libnm-core-public/nm-dbus-interface.h @@ -1420,13 +1420,16 @@ typedef enum /*< flags >*/ { */ #define NM_SECRET_TAG_VPN_MSG "x-vpn-message:" -/* For secrets requests, hints starting with "x-dynamic-challenge:" are dynamic +/* For secrets requests, hints starting with "x-dynamic-challenge(-echo):" are dynamic * 2FA challenges that are requested in a second authentication step, after the password * (or whatever auth method is used) was already successfully validated. Because of * that, the default secrets of the service mustn't be requested (again). + * When using the "-echo" variant, the user input doesn't need to be hidden even + * without --show-secrets * * Note: currently only implemented for VPN, but can be extended. */ -#define NM_SECRET_TAG_DYNAMIC_CHALLENGE "x-dynamic-challenge:" +#define NM_SECRET_TAG_DYNAMIC_CHALLENGE "x-dynamic-challenge:" +#define NM_SECRET_TAG_DYNAMIC_CHALLENGE_ECHO "x-dynamic-challenge-echo:" #endif /* __NM_DBUS_INTERFACE_H__ */ diff --git a/src/libnmc-base/nm-secret-agent-simple.c b/src/libnmc-base/nm-secret-agent-simple.c index 02f492bc7c..4bb77c9802 100644 --- a/src/libnmc-base/nm-secret-agent-simple.c +++ b/src/libnmc-base/nm-secret-agent-simple.c @@ -170,6 +170,7 @@ _secret_real_new_plain(NMSecretAgentSecretType secret_type, .base.entry_id = g_strdup_printf("%s.%s", nm_setting_get_name(setting), property), .base.value = g_steal_pointer(&value), .base.is_secret = (secret_type != NM_SECRET_AGENT_SECRET_TYPE_PROPERTY), + .base.force_echo = FALSE, .setting = g_object_ref(setting), .property = g_strdup(property), }; @@ -180,7 +181,8 @@ static NMSecretAgentSimpleSecret * _secret_real_new_vpn_secret(const char *pretty_name, NMSetting *setting, const char *property, - const char *vpn_type) + const char *vpn_type, + gboolean force_echo) { SecretReal *real; const char *value; @@ -197,11 +199,12 @@ _secret_real_new_vpn_secret(const char *pretty_name, .base.pretty_name = g_strdup(pretty_name), .base.entry_id = g_strdup_printf("%s%s", NM_SECRET_AGENT_ENTRY_ID_PREFX_VPN_SECRETS, property), - .base.value = g_strdup(value), - .base.is_secret = TRUE, - .base.vpn_type = g_strdup(vpn_type), - .setting = g_object_ref(setting), - .property = g_strdup(property), + .base.value = g_strdup(value), + .base.is_secret = TRUE, + .base.force_echo = force_echo, + .base.vpn_type = g_strdup(vpn_type), + .setting = g_object_ref(setting), + .property = g_strdup(property), }; return &real->base; } @@ -227,6 +230,7 @@ _secret_real_new_wireguard_peer_psk(NMSettingWireGuard *s_wg, .base.value = g_strdup(preshared_key), .base.is_secret = TRUE, .base.no_prompt_entry_id = TRUE, + .base.force_echo = FALSE, .setting = NM_SETTING(g_object_ref(s_wg)), .property = g_strdup(public_key), }; @@ -388,7 +392,8 @@ static void add_vpn_secret_helper(GPtrArray *secrets, NMSettingVpn *s_vpn, const char *name, - const char *ui_name) + const char *ui_name, + gboolean force_echo) { NMSecretAgentSimpleSecret *secret; NMSettingSecretFlags flags; @@ -399,7 +404,8 @@ add_vpn_secret_helper(GPtrArray *secrets, secret = _secret_real_new_vpn_secret(ui_name, NM_SETTING(s_vpn), name, - nm_setting_vpn_get_service_type(s_vpn)); + nm_setting_vpn_get_service_type(s_vpn), + force_echo); /* Check for duplicates */ for (i = 0; i < secrets->len; i++) { @@ -408,6 +414,8 @@ add_vpn_secret_helper(GPtrArray *secrets, if (s->secret_type == secret->secret_type && nm_streq0(s->vpn_type, secret->vpn_type) && nm_streq0(s->entry_id, secret->entry_id)) { _secret_real_free(secret); + if (!force_echo) + s->force_echo = FALSE; return; } } @@ -425,6 +433,7 @@ add_vpn_secrets(RequestData *request, GPtrArray *secrets, char **msg) char **iter; char *secret_name; bool is_challenge = FALSE; + bool force_echo; /* If hints are given, then always ask for what the hints require */ if (request->hints) { @@ -435,11 +444,17 @@ add_vpn_secrets(RequestData *request, GPtrArray *secrets, char **msg) if (NM_STR_HAS_PREFIX(*iter, NM_SECRET_TAG_DYNAMIC_CHALLENGE)) { secret_name = &(*iter)[NM_STRLEN(NM_SECRET_TAG_DYNAMIC_CHALLENGE)]; is_challenge = TRUE; + force_echo = FALSE; + } else if (NM_STR_HAS_PREFIX(*iter, NM_SECRET_TAG_DYNAMIC_CHALLENGE_ECHO)) { + secret_name = &(*iter)[NM_STRLEN(NM_SECRET_TAG_DYNAMIC_CHALLENGE_ECHO)]; + is_challenge = TRUE; + force_echo = TRUE; } else { secret_name = *iter; + force_echo = FALSE; } - add_vpn_secret_helper(secrets, s_vpn, secret_name, secret_name); + add_vpn_secret_helper(secrets, s_vpn, secret_name, secret_name, force_echo); } } } @@ -453,7 +468,7 @@ add_vpn_secrets(RequestData *request, GPtrArray *secrets, char **msg) /* Now add what client thinks might be required, because hints may be empty or incomplete */ p = nm_vpn_get_secret_names(nm_setting_vpn_get_service_type(s_vpn)); while (p && p->name) { - add_vpn_secret_helper(secrets, s_vpn, p->name, _(p->ui_name)); + add_vpn_secret_helper(secrets, s_vpn, p->name, _(p->ui_name), FALSE); p++; } @@ -608,6 +623,7 @@ _auth_dialog_exited(GPid pid, int status, gpointer user_data) for (i = 1; groups[i]; i++) { gs_free char *pretty_name = NULL; + gboolean force_echo; if (!g_key_file_get_boolean(keyfile, groups[i], "IsSecret", NULL)) continue; @@ -615,11 +631,14 @@ _auth_dialog_exited(GPid pid, int status, gpointer user_data) continue; pretty_name = g_key_file_get_string(keyfile, groups[i], "Label", NULL); + force_echo = g_key_file_get_boolean(keyfile, groups[i], "ForceEcho", NULL); + g_ptr_array_add(secrets, _secret_real_new_vpn_secret(pretty_name, NM_SETTING(s_vpn), groups[i], - nm_setting_vpn_get_service_type(s_vpn))); + nm_setting_vpn_get_service_type(s_vpn), + force_echo)); } out: diff --git a/src/libnmc-base/nm-secret-agent-simple.h b/src/libnmc-base/nm-secret-agent-simple.h index a1d1588191..94197957a6 100644 --- a/src/libnmc-base/nm-secret-agent-simple.h +++ b/src/libnmc-base/nm-secret-agent-simple.h @@ -23,6 +23,7 @@ typedef struct { const char *vpn_type; bool is_secret : 1; bool no_prompt_entry_id : 1; + bool force_echo : 1; } NMSecretAgentSimpleSecret; #define NM_SECRET_AGENT_ENTRY_ID_PREFX_VPN_SECRETS "vpn.secrets." diff --git a/src/nmcli/common.c b/src/nmcli/common.c index 04d2ebf949..2f205e50cb 100644 --- a/src/nmcli/common.c +++ b/src/nmcli/common.c @@ -700,7 +700,7 @@ get_secrets_from_user(const NmcConfig *nmc_config, if (msg) nmc_print("%s\n", msg); - echo_on = secret->is_secret ? nmc_config->show_secrets : TRUE; + echo_on = secret->is_secret ? secret->force_echo || nmc_config->show_secrets : TRUE; if (secret->no_prompt_entry_id) pwd = nmc_readline_echo(nmc_config, echo_on, "%s: ", secret->pretty_name); diff --git a/src/nmtui/nmt-password-dialog.c b/src/nmtui/nmt-password-dialog.c index 75194d7b99..6f1a5f031f 100644 --- a/src/nmtui/nmt-password-dialog.c +++ b/src/nmtui/nmt-password-dialog.c @@ -139,7 +139,7 @@ nmt_password_dialog_constructed(GObject *object) nmt_newt_widget_set_padding(widget, 4, 0, 1, 0); flags = NMT_NEWT_ENTRY_NONEMPTY; - if (secret->is_secret) + if (secret->is_secret && !secret->force_echo) flags |= NMT_NEWT_ENTRY_PASSWORD; widget = nmt_newt_entry_new(30, flags); if (secret->value)