They all look good to me.

All look good except:

> cli: Only construct an agent for a connection if there is a connection

The point of the code is that we only want to answer secret requests if they're for the connection that we're activating.

But that gets complicated here, because we won't know the connection path until the AddAndActivate() call returns, but if we wait until that point to register the secret agent, then the daemon might get the registration too late... So I guess, we need to add nm_secret_agent_simple_set_connection_path() so we can first create the agent without a path, and then set the path later once we know it. (Although, we also need to make it so it doesn't respond to any requests in between... I guess you could just pass a dummy path rather than no path when creating it.)

Created attachment 290799 [details]
[PATCH] cli: Only construct an agent for a connection if there is a connection

(In reply to comment #2)
> All look good except:
> 
> > cli: Only construct an agent for a connection if there is a connection
> 
> The point of the code is that we only want to answer secret requests if they're
> for the connection that we're activating.
> 
> But that gets complicated here, because we won't know the connection path until
> the AddAndActivate() call returns, but if we wait until that point to register
> the secret agent, then the daemon might get the registration too late... So I
> guess, we need to add nm_secret_agent_simple_set_connection_path() so we can
> first create the agent without a path, and then set the path later once we know
> it. (Although, we also need to make it so it doesn't respond to any requests in
> between... I guess you could just pass a dummy path rather than no path when
> creating it.)

You mean, like this? I think (not tested yet) it's still racy, as the secret might be requested before we set the path?

I'm wondering if we shouldn't use the path of the device instead?

(In reply to comment #3)
> You mean, like this? I think (not tested yet) it's still racy, as the secret
> might be requested before we set the path?

Hm, yes, I was thinking it was safe because we know you'll get the AddAndActivateConnection reply before you could get a secrets request, but we don't know for sure that all of the async NMObject property loading stuff will finish in time.

> I'm wondering if we shouldn't use the path of the device instead?

The secret agent doesn't know the device (and can't necessarily figure it out from the connection yet, because of the same race condition).

Oh... but we can just use the connection UUID rather than the path. That should work.

(In reply to comment #4)
> (In reply to comment #3)
> > You mean, like this? I think (not tested yet) it's still racy, as the secret
> > might be requested before we set the path?
> 
> Hm, yes, I was thinking it was safe because we know you'll get the
> AddAndActivateConnection reply before you could get a secrets request, but we
> don't know for sure that all of the async NMObject property loading stuff will
> finish in time.
> 
> > I'm wondering if we shouldn't use the path of the device instead?
> 
> The secret agent doesn't know the device (and can't necessarily figure it out
> from the connection yet, because of the same race condition).
> 
> Oh... but we can just use the connection UUID rather than the path. That should
> work.

Consulted with Jirka -- turned out that we need to deal with the "activate an existing connection, but let server negotiate it" case, so generating uuid in client is not a viable option.

I've updated the branch. I'm now deferring the secret request processing to until we learn the actual path.

It works for me, I'm not particularly happy about the abuse of "/" literal for "we'll learn the real connection path later"; ideas about how to make it nicer are welcome.

(In reply to comment #5)
> It works for me, I'm not particularly happy about the abuse of "/" literal for
> "we'll learn the real connection path later"; ideas about how to make it nicer
> are welcome.

Hm, yeah.

You could separate out the path-setting and stop/go into separate properties. So instead of adding a "path" arg to nm_secret_agent_simple_new(), add a "running" arg. Then, in the "nmcli agent" case, you'd do:

  agent = nm_secret_agent_simple_new ("nmcli", TRUE);

And in the activate case:

  agent = nm_secret_agent_simple_new ("nmcli", TRUE);
  nm_secret_agent_simple_set_path (agent, connection_path);

And in the add-and-activate case:

  agent = nm_secret_agent_simple_new ("nmcli", FALSE);

  /* time passes */

  nm_secret_agent_simple_set_path (agent, connection_path);
  nm_secret_agent_simple_set_running (agent, TRUE);


>+	if (!g_str_has_prefix (request->request_id, priv->path)) {
>+		request_secrets_from_ui (request);

The "!" shouldn't be there should it?

>>+	if (!g_str_has_prefix (request->request_id, priv->path)) {
>>+		request_secrets_from_ui (request);

> The "!" shouldn't be there should it?

Right, we should handle matching path.

> »·······»·······»·······error = g_error_new (NM_SECRET_AGENT_ERROR,  NM_SECRET_AGENT_ERROR_FAILED,
> »·······»·······»·······»·······»·······     "Request for %s secrets doesn't match path %s",
> »·······»·······»·······»·······»·······     request->request_id, priv->path);

indentation (two extra tabs)

I have published a branch jk/nmcli-dev-connect-secrets that adds secret agent support for 'nmcli device connect' too (on top of Lubomir stuff).

(In reply to comment #6)
> (In reply to comment #5)
> > It works for me, I'm not particularly happy about the abuse of "/" literal for
> > "we'll learn the real connection path later"; ideas about how to make it nicer
> > are welcome.
> 
> Hm, yeah.
> 
> You could separate out the path-setting and stop/go into separate properties.
> So instead of adding a "path" arg to nm_secret_agent_simple_new(), add a
> "running" arg. Then, in the "nmcli agent" case, you'd do:
> 
>   agent = nm_secret_agent_simple_new ("nmcli", TRUE);
> 
> And in the activate case:
> 
>   agent = nm_secret_agent_simple_new ("nmcli", TRUE);
>   nm_secret_agent_simple_set_path (agent, connection_path);
> 
> And in the add-and-activate case:
> 
>   agent = nm_secret_agent_simple_new ("nmcli", FALSE);
> 
>   /* time passes */
> 
>   nm_secret_agent_simple_set_path (agent, connection_path);
>   nm_secret_agent_simple_set_running (agent, TRUE);

Done.

I'm doing nm_secret_agent_simple_enable_running() which unconditionally sets
running to true instead of nm_secret_agent_simple_set_running() as it does not
really make sense to disable the agent once it's running and it would make the
implementation more difficult (we'd need to keep track of requests that were
presented to the user already).

(In reply to comment #7)
> >>+	if (!g_str_has_prefix (request->request_id, priv->path)) {
> >>+		request_secrets_from_ui (request);
> 
> > The "!" shouldn't be there should it?
> 
> Right, we should handle matching path.

Fixed.

> > »·······»·······»·······error = g_error_new (NM_SECRET_AGENT_ERROR,  NM_SECRET_AGENT_ERROR_FAILED,
> > »·······»·······»·······»·······»·······     "Request for %s secrets doesn't match path %s",
> > »·······»·······»·······»·······»·······     request->request_id, priv->path);
> 
> indentation (two extra tabs)

Fixed.

>+	nmc->secret_agent = nm_secret_agent_simple_new ("nmcli-connect", FALSE);
>+	if (nmc->secret_agent) {
> 		g_signal_connect (nmc->secret_agent, "request-secrets", G_CALLBACK (secrets_requested), nmc);
>+		if (connection) {
>+			nm_secret_agent_simple_set_connection_path (nmc->secret_agent,
>+			                                            nm_object_get_path (NM_OBJECT (connection)));
>+			nm_secret_agent_simple_enable_running (nmc->secret_agent);

There's no need to pass FALSE initially and then set enable_running; there's no way a request could be received in between creating it and setting the path, so you can just pass TRUE. (Likewise in the nmtui patch.)

>+nm_secret_agent_simple_enable_running (NMSecretAgent *agent)

"enable_running" sounds weird. "_enable()", "_run()", "_start()", "_set_running()"

> 	agent = g_initable_new (NM_TYPE_SECRET_AGENT_SIMPLE, NULL, NULL,
> 	                        NM_SECRET_AGENT_IDENTIFIER, name,
> 	                        NULL);
>-	NM_SECRET_AGENT_SIMPLE_GET_PRIVATE (agent)->path = g_strdup (path);
>+	NM_SECRET_AGENT_SIMPLE_GET_PRIVATE (agent)->running = running;

This is already broken in master but I'm just noticing it now; g_initable_new() will return NULL here if NM isn't running (or some other error occurs while trying to register the agent), so you need to check that agent is non-NULL before setting path/running.

(Unless you disagree with any of this stuff I don't think another round of review is needed. Just fix it up and commit.)

I added a fix and support for secrets in 'nmcli device connect'

jk/nmcli-dev-connect-secrets rebased on master.

Commits in master:
224acba cli,vpn: merge branch 'lr/nm-1-0-fixes'
991df80 cli: Process secret agent request for a connection only if we know its path
d4240ad cli: Abort when given name of a non-existent connection for nmcli up
75f767c libnm: Don't expect VPN connections to attach to the device
282d9b0 vpn: Propagate daemon exec error correctly
5197419 cli: Only escape VPN banner if it's present

4b799db cli: add support for secret agent to 'nmcli dev connect' too
a1f16d2 clients: fix processing a secret agent request