+1 from me

*** Bug 733649 has been marked as a duplicate of this bug. ***

*** Bug 734161 has been marked as a duplicate of this bug. ***

review of jk/nmcli-secret-agent:

> clients: move secret agent to common directory

>-	NmtNewtEntryValidator validator;
>+#if 0
>+	NmcNewtEntryValidator validator;

I was going to say "you can't just remove that, nmtui needs it!", but then I saw that I never actually finished that. The idea was that NmtSecretAgent would provide a callback function that you could use to tell if the provided password was valid or not. [Eg, the right length for a WEP passphrase, or only hex for a hex WEP key.])

We probably do want that functionality at some point, but for now you can just remove it rather than #if 0'ing it. (And remove the existing #if 0 part too.)

>+ * nmc_secret_agent_new:
>+ * @name: tde identifier of secret agent

typo "tde"

>+	../common/nmc-secret-agent.c \
>+	../common/nmc-secret-agent.h \

does that work with srcdir != builddir? I think

    $(srcdir)/../common/nmc-secret-agent.c \

is safer.

Also, does that get disted correctly?



> clients: add real property name to NmcSecretAgentSecret

You could just move the existing "setting" and "property" fields from NmcSecretAgentSecretReal to NmcSecretAgentSecretBase.



> cli: use secret agent for getting passwords from user

>+	static gboolean first = TRUE;
>+
>+	if (first)
>+		g_print ("%s\n", msg);

Why do you only want to print the message once? Is it just to avoid redundancy if the password is incorrect and the user is asked a second time? I don't think printing the message again is bad in that case...

And it's possible to need two unrelated sets of secrets for a single activation (eg, if you have a Wi-Fi connection with a secondary VPN), and you definitely want both messages then.

>+	gboolean bb = FALSE;

no clue what "bb" stands for here...

>+		g_print ("%s\n", _("Warning: nmcli do not ask for password without '--ask' argument."));

"does not"



> cli: add 'passwords' parameter for 'nmcli connection up'

>+	              "  up [[id | uuid | path] <ID>] [ifname <ifname>] [ap <BSSID>] [nsp <name>] [passwords \"<pwd_spec>:<pwd> ...\"]\n\n"

It's not obvious that <pwd_spec> means the property name. Why not just say <property_name>?

>+	              "nsp    - specifies NSP to connect to (only valid for WiMAX)\n"
>+	              "passwords - password(s) required to activate the connection\n\n"));

you should probably realign the "-"s on the lines above this... it looks bad that they're all aligned except the last one.

>+/* E.g. "wifi.psk:tajneheslo 802-1x.password:krakonos" */

passwords can have spaces in them. Maybe it would be better to do

  password wifi.psk:tajneheslo password '802-1x.password:something with spaces'

?

>+				g_print ("%s\n",
>+				         _("Warning: nmcli do not ask for password without '--ask' argument."));

maybe mention the "passwords" argument too?

(In reply to comment #4)
> review of jk/nmcli-secret-agent:
> 
> > clients: move secret agent to common directory
> 
> >-	NmtNewtEntryValidator validator;
> >+#if 0
> >+	NmcNewtEntryValidator validator;
> 
> I was going to say "you can't just remove that, nmtui needs it!", but then I
> saw that I never actually finished that. The idea was that NmtSecretAgent would
> provide a callback function that you could use to tell if the provided password
> was valid or not. [Eg, the right length for a WEP passphrase, or only hex for a
> hex WEP key.])
> 
> We probably do want that functionality at some point, but for now you can just
> remove it rather than #if 0'ing it. (And remove the existing #if 0 part too.)
> 
'#if 0' code removed.

> >+ * nmc_secret_agent_new:
> >+ * @name: tde identifier of secret agent
> 
> typo "tde"
Fixed.

> 
> >+	../common/nmc-secret-agent.c \
> >+	../common/nmc-secret-agent.h \
> 
> does that work with srcdir != builddir? I think
> 
>     $(srcdir)/../common/nmc-secret-agent.c \
> 
> is safer.
> 
> Also, does that get disted correctly?
> 
It builds for me in a different directory. But I prepended "$(srcdir)/" as you suggest, it may be more robust. (I am weak in the autohell^H^H^H^Htools.)

> 
> 
> > clients: add real property name to NmcSecretAgentSecret
> 
> You could just move the existing "setting" and "property" fields from
> NmcSecretAgentSecretReal to NmcSecretAgentSecretBase.
> 
I left it as it is for now.
Is NmcSecretAgentSecretReal needed at all?

> 
> > cli: use secret agent for getting passwords from user
> 
> >+	static gboolean first = TRUE;
> >+
> >+	if (first)
> >+		g_print ("%s\n", msg);
> 
> Why do you only want to print the message once? Is it just to avoid redundancy
> if the password is incorrect and the user is asked a second time? I don't think
> printing the message again is bad in that case...
> 
> And it's possible to need two unrelated sets of secrets for a single activation
> (eg, if you have a Wi-Fi connection with a secondary VPN), and you definitely
> want both messages then.
> 
Yes, I preferred to print the message just once when a bad password is given. But you are right that it would have to be done per password. So I changed it to print the message each time for now.

> >+	gboolean bb = FALSE;
> 
> no clue what "bb" stands for here...
> 
Changed to success :)

> >+		g_print ("%s\n", _("Warning: nmcli do not ask for password without '--ask' argument."));
> 
> "does not"
> 
Fixed.

> 
> 
> > cli: add 'passwords' parameter for 'nmcli connection up'
> 
> >+	              "  up [[id | uuid | path] <ID>] [ifname <ifname>] [ap <BSSID>] [nsp <name>] [passwords \"<pwd_spec>:<pwd> ...\"]\n\n"
> 
> It's not obvious that <pwd_spec> means the property name. Why not just say
> <property_name>?
> 
Done.

> >+	              "nsp    - specifies NSP to connect to (only valid for WiMAX)\n"
> >+	              "passwords - password(s) required to activate the connection\n\n"));
> 
> you should probably realign the "-"s on the lines above this... it looks bad
> that they're all aligned except the last one.
> 
Done. I wonder if we should describe the passwords here in more detail. 

> >+/* E.g. "wifi.psk:tajneheslo 802-1x.password:krakonos" */
> 
> passwords can have spaces in them. Maybe it would be better to do
> 
>   password wifi.psk:tajneheslo password '802-1x.password:something with spaces'
> 
> ?
Yes, passwords can contain abritrary characters. So I thought of having user specify a delimiter not contained in passwords himself.
I have added a commit doing this via "delim:<char>" prefix to 'passwords' option.

> 
> >+				g_print ("%s\n",
> >+				         _("Warning: nmcli do not ask for password without '--ask' argument."));
> 
> maybe mention the "passwords" argument too?
I rephrased the message to:
g_printerr (_("Warning: password for '%s' not given via 'passwords' "
              "and nmcli cannot ask without '--ask' option.\n"), secret->prop_name);

Branch is rebased to master and re-pushed.

(In reply to comment #5)
> > Also, does that get disted correctly?

It looks like the answer to that is yes, but "make dist" fails anyway because you need to update po/POTFILES.in for the renamed file.

> > You could just move the existing "setting" and "property" fields from
> > NmcSecretAgentSecretReal to NmcSecretAgentSecretBase.
> > 
> I left it as it is for now.
> Is NmcSecretAgentSecretReal needed at all?

The idea behind the Real/Base split was that we might someday make this public API (since it's now shared by nmtui and nmcli, and it's based on the gnome-shell secret agent code), and the secret agent internals need to keep track of some information that the API users don't need to know about.

Keeping it as-is for now is fine.

> Yes, passwords can contain abritrary characters. So I thought of having user
> specify a delimiter not contained in passwords himself.
> I have added a commit doing this via "delim:<char>" prefix to 'passwords'
> option.

That seems like an awkward way to do it, and makes it difficult to use programmatically. (If you want to call nmcli and pass a user-provided password, you'd have to scan the password to find a character that it doesn't use to use as the delimiter character...)

Though I guess that since the password would be visible in "ps" for the entire time it takes the connection to activate, people are only going to use this option with passwords they don't really care about anyway, which means the password is probably going to be along the lines of "pass123", not "foo < $bar!&,"

(In reply to comment #6)
> (In reply to comment #5)
> > > Also, does that get disted correctly?
> 
> It looks like the answer to that is yes, but "make dist" fails anyway because
> you need to update po/POTFILES.in for the renamed file.
> 
Good catch! po/POTFILES.in updated now.

> > > You could just move the existing "setting" and "property" fields from
> > > NmcSecretAgentSecretReal to NmcSecretAgentSecretBase.
> > > 
> > I left it as it is for now.
> > Is NmcSecretAgentSecretReal needed at all?
> 
> The idea behind the Real/Base split was that we might someday make this public
> API (since it's now shared by nmtui and nmcli, and it's based on the
> gnome-shell secret agent code), and the secret agent internals need to keep
> track of some information that the API users don't need to know about.
> 
> Keeping it as-is for now is fine.
> 
Ok, I understand the split now.

> > Yes, passwords can contain abritrary characters. So I thought of having user
> > specify a delimiter not contained in passwords himself.
> > I have added a commit doing this via "delim:<char>" prefix to 'passwords'
> > option.
> 
> That seems like an awkward way to do it, and makes it difficult to use
> programmatically. (If you want to call nmcli and pass a user-provided password,
> you'd have to scan the password to find a character that it doesn't use to use
> as the delimiter character...)
> 
Putting passwords (strings with arbitrary special characters) on command line can be a problem anyway, because it may require various escape magic for shell processing. But, in general, offering a delimiter, allows users a way for providing more-or-less any password.

> Though I guess that since the password would be visible in "ps" for the entire
> time it takes the connection to activate, people are only going to use this
> option with passwords they don't really care about anyway, which means the
> password is probably going to be along the lines of "pass123", not "foo <
> $bar!&,"
I have added a note to the manual page that putting password on command line is not recommended for top secret passwords.

Anyway, we should enable a password storage (probably an integration with libsecret) for nmcli/nmtui in future which would make 'passwords' option unnecessary.

Branch rebased to latest master.

>> clients: move secret agent to common directory
    
> The agent code will be shared by both nmtui and nmcli.

What secrets a connection has is hard coded. Instead of libnm telling which properties are secrets, for example using NM_SETTING_PARAM_SECRET.
But well...


>> cli: use secret agent for getting passwords from user

+    if (nmc->ask) {
+         success = get_secrets_from_user (request_id, title, msg, secrets);
+    } else {
+         g_print ("%s\n", msg);
+         g_print ("%s\n", _("Warning: nmcli does not ask for password without '--ask' argument."));
+    }

I think nmcli should silently ignore password requests when called without --ask (instead of issuing a warning). Possibly, when called without --ask, nmcli should not even register a password agent.

Usually, you already have nm-applet running which gets the password from the keyring. Hence, the warning is not helpful (and wrong).


Maybe we could extend --ask to have --ask, --no-ask and --ask-tty. --ask works as it does now. --no-ask works as the current default. --ask-tty would work as --no-ask, except for passwords it would ask for them *only* when connected to a terminal.


>> cli: allow spaces in secrets inside 'passwords' option
>> cli: add 'passwords' parameter for 'nmcli connection up'

I don't think delim: is a good idea. As danw said, for a script it is very hard to escape a password. Why not escape everything with '\'? It makes it easy to escape from scripts?

But then again, combining different passwords in one command line argument makes it again hard for the script. And it also makes it very hard for bash-completion to do anything useful (especially, together with 'delim:').

I think it be much better to accept multiple password arguments separately:

   nmcli connection up ID password <SETTING.PROPERTY1> <PASSWD1> password <SETTING.PROPERTY2> <PASSWD2> ...

this way, you don't need any escaping. And bash completion can (relatively) easily complete to <SETTING.PROPERTY1>.




For always-ask passwords (two-factor-auth), providing the password on command line is indeed useful. But it is also terribly insecure. Thinking about it again: I think we should not encourage and support that.

I strongly vote for dropping this functionality entirely because many users are not aware that password can be seen with `ps` and that it ends up in the bash-history.

Passing the passwords via environment is a bit secure. At least only root and the user can read /proc/$$/environ. But even that is not great.

Why not reading it from a file descriptor instead?

(In reply to comment #8)
> >> clients: move secret agent to common directory
> 
> >> cli: use secret agent for getting passwords from user
> 
> +    if (nmc->ask) {
> +         success = get_secrets_from_user (request_id, title, msg, secrets);
> +    } else {
> +         g_print ("%s\n", msg);
> +         g_print ("%s\n", _("Warning: nmcli does not ask for password without
> '--ask' argument."));
> +    }
> 
> I think nmcli should silently ignore password requests when called without
> --ask (instead of issuing a warning). Possibly, when called without --ask,
> nmcli should not even register a password agent.
> 
> Usually, you already have nm-applet running which gets the password from the
> keyring. Hence, the warning is not helpful (and wrong).
> 

Well, the warning is not wrong. It says the user that he won't be asked without --ask. What was wrong was that next agent was not asked. It is fixed now.

> 
> 
> >> cli: allow spaces in secrets inside 'passwords' option
> >> cli: add 'passwords' parameter for 'nmcli connection up'
> 

I have changed that to use 'passwd-file <filename>' instead to provide passwords.

maybe mention in nmcli.1 that --ask controls password prompting too now?

Actually, maybe clarify that if NM needs a password, then (1) if you passed --ask, then nmcli will ask you for the password, but (2) if you didn't pass --ask, but are running a GUI secret agent (such as nm-applet or gnome-shell), then NM will ask for the password via that agent.

The code looks good to me.  I also took a second to figure out the --ask vs. GUI behavior, so would be good to document.  The rest is good!

I changed the "Nm" prefix to "NM" and put some clarificatios regarding --ask to the manual page. Thanks for reviewing.

Commits in master:
847e2a4 merge: use NM secret agent in nmcli when connecting to networks (bgo #705998)
252c8bf man: document/clarify --ask option in relation to password prompting
3c9b867 cli: add 'passwd-file' option for 'nmcli connection up' to provide passwords
de7f85b cli: use secret agent for getting passwords from user
b41cb60 clients: add real property name to NNSecretAgentSimpleSecret
801fc34 clients: move secret agent to common directory

Reopen for a fix, branch: jk/nmcli-ask-pwd-fix

When nmcli connection up eth-profile is called nmcli could ask for secrets for another profile, which might confuse user. Improve that by checking whether the request is for the activating connection.

Yeah. Although, nmtui should behave the same way, so maybe nm_secret_agent_simple_new() should take an optional NMConnection argument and do the filtering itself?

-         secrets_data->connection = connection;
+         secrets_data->connection = g_object_ref (connection);

(and unref likewise).

Actually, I wouldn't even pass on the NMConnection reference. Just g_strdup(nm_object_get_path()).




Re: I still strongly dislike the warning
  "Warning: password for '802-1x.identity' not given in 'passwd-file' and..." 

It happens now whenever NM needs secrets, although I have nm-applet running. Indeed I run `nmcli conn up` without --ask, because I want nm-applet to provide the secrets.

It is enough to document that interactive password input requires --ask.




Rest looks good.

(In reply to comment #14)
> Yeah. Although, nmtui should behave the same way, so maybe
> nm_secret_agent_simple_new() should take an optional NMConnection argument and
> do the filtering itself?

Done and pushed to jk/clients-secrets-ask-fix.

looks good

a928ce8 clients: only handle secret requests for connection being explicitly activated
pushed to master

I will yet look into the warning thing, leaving the bug open for it.

ok, let's close this. Seems everybody is fine with the warning.