The .name file for the plugin already supports aliases.

E.g. nm-libreswan has:

[VPN Connection]
name=libreswan
service=org.freedesktop.NetworkManager.libreswan
aliases=org.freedesktop.NetworkManager.openswan


so, when editing an existing connection that works already correctly, because we have a NMConnection with vpn.service-type set.
Then ce_page_vpn_new() calls vpn_get_plugin_by_service() which finds the plugin by considering the alias.
Later, the plugin also will have access to the connection, so it knows whether it handles a libreswan or openswan connection.



in case of the nm-connection-editor "Add" functionality, it is handled here:

https://git.gnome.org/browse/network-manager-applet/tree/src/connection-editor/connection-helpers.c?id=0c197cce44a01ce8f1628e157768d8ea94ec4ed2#n261

For each plugin, it gets NM_VPN_EDITOR_PLUGIN_NAME and creates one entry.

We could extend this to iterate over each aliases too and call a new virtual function

  char *nm_vpn_editor_plugin_get_name_for_alias (plugin, alias_name);

if the plugin returns a string for this function, set_up_connection_type_combo() could add additional entires for each alias.




How about that?

I think that works; yes. Thanks!

So even without the 'Add VPN' support in the GUI, this ought to work for connections added with 'nmcli con add':
https://git.gnome.org/browse/network-manager-openconnect/commit/?id=34051bb

It doesn't. Both gnopme-shell and nm-applet (in Fedora 24) fail to spawn the auth-dialog. 

nm-applet says:
(nm-applet:109932): nm-applet-WARNING **: Could not find the authentication dialog for VPN connection type 'org.freedesktop.NetworkManager.openconnect.nc'

gnome-shell says:
Gjs-Message: JS LOG: Invalid VPN service type (cannot find authentication binary)

Untested branch for review: th/vpn-service-info-bgo767197


NM: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/vpn-service-info-bgo767197

nm-a: https://git.gnome.org/browse/network-manager-applet/log/?h=th/vpn-service-info-bgo767197

I've fixed gnome-shell to spawn the auth-dialog correctly (for gnome-3-20 and master). Would be good if we could backport the fix (bug 765329) for nm-applet.

For KDE I've filed https://bugs.kde.org/show_bug.cgi?id=363917

Created attachment 329112 [details] [review]
Make nm_vpn_get_plugin_names() honour aliases too


(In reply to Thomas Haller from comment #4)
> Untested branch for review: th/vpn-service-info-bgo767197

I still see this:

# nmcli con add type vpn con-name "Juniper VPN" ifname "*" vpn-type openconnect.nc -- vpn.data "gateway=vpn.example.com"
Warning: 'vpn-type': openconnect.nc not known.
Connection 'Juniper VPN' (d2b07011-3073-45fd-9885-48c652701d75) successfully added.

It's going to want something like this patch... which I was relatively happy with right up to the last minute when I tested it and realised I had to strip the 'org.freedesktop.NetworkManager.' from the beginning of every alias...

Created attachment 329113 [details] [review]
Make nm_vpn_get_plugin_names() honour aliases too (v2)

Or perhaps with a little less debugging cruft...

(In reply to David Woodhouse from comment #6)
> Created attachment 329112 [details] [review] [review]
> Make nm_vpn_get_plugin_names() honour aliases too
> 
> 
> (In reply to Thomas Haller from comment #4)
> > Untested branch for review: th/vpn-service-info-bgo767197
> 
> I still see this:
> 
> # nmcli con add type vpn con-name "Juniper VPN" ifname "*" vpn-type
> openconnect.nc -- vpn.data "gateway=vpn.example.com"
> Warning: 'vpn-type': openconnect.nc not known.
> Connection 'Juniper VPN' (d2b07011-3073-45fd-9885-48c652701d75) successfully
> added.
> 
> It's going to want something like this patch... which I was relatively happy
> with right up to the last minute when I tested it and realised I had to
> strip the 'org.freedesktop.NetworkManager.' from the beginning of every
> alias...


currently nmcli's "vpn-type" setting is kinda the name of the plugin and it is used to set the service-type.

But with multiple service-types (aliases), thus we need multiple names.
But "vpn-type" should rather be a short-name for the service-type, not the name for the plugin.

th/vpn-service-info-bgo767197 already provides NM_VPN_EDITOR_PLUGIN_NAME, NM_VPN_EDITOR_PLUGIN_DESCRIPTION, NM_VPN_EDITOR_PLUGIN_SERVICE per-service-type. We just lack the short-name.

(In reply to David Woodhouse from comment #7)
> Created attachment 329113 [details] [review] [review]
> Make nm_vpn_get_plugin_names() honour aliases too (v2)
> 
> Or perhaps with a little less debugging cruft...

I fixed it now differently. Previously, the distinction between short-name, service-name, and vpn-name was very unclear.

Now, libnm now provides a function nm_vpn_plugin_info_list_find_service_type() to find a full-qualified service-type (or alias) based on something like a short-name.

Repushed: th/vpn-service-info-bgo767197

Ok, now the entire ensemble. See patches th/vpn-service-info-bgo767197 for NetworkManager, nm-applet, and nm-librewean:


NM: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/vpn-service-info-bgo767197

nm-a: https://git.gnome.org/browse/network-manager-applet/log/?h=th/vpn-service-info-bgo767197

nm-libreswan: https://git.gnome.org/browse/network-manager-libreswan/log/?h=th/vpn-service-info-bgo767197

If you were to do that last one for nm-openconnect it could actually be committed...

nm-openconnect: https://git.gnome.org/browse/network-manager-openconnect/log/?h=th/vpn-service-info-bgo767197

(I didn't bother with the names and descriptions because I am not familiar with this VPN type, please suggest better names/aliases as they are unsuitable).



David, how do you like this all?

Still moderately unconvinced by the generic call stuff — I do wonder if we should either go the whole hog and make it a GObject feature and try to integrate it with existing introspection and marshalling, or just KISS. Our *main* problem there is that libnm is abstracting away something that it doesn't know about; we're not just letting the *caller* call into the new methods (indicated by feature bits) in the editor-plugin, but it has to go through an accessor function in libnm.

But other than that, yes that looks great; thanks!

In fact openconnect.anyconnect is redundant with openconnect (anyconnect is the default protocol), so I probably wouldn't set the CAN_ADD flag for that one, and the strings can be basically identical (which they almost are, so that looks sane). It's only supported because it would take a special-case to *not* support it.

openconnect.nc is:

  "openconnect.nc"
  "Juniper Network Connect (openconnect)"
  "Compatible with Juniper Network Connect / Pulse Secure SSL VPN"

Do I want to know about the whole NM_OPENCONNECT_OLD thing and the reason we have two copies of the same code being built, and only one of them gets this new addition?

(In reply to David Woodhouse from comment #13)

> In fact openconnect.anyconnect is redundant with openconnect (anyconnect is
> the default protocol), so I probably wouldn't set the CAN_ADD flag for that
> one, and the strings can be basically identical (which they almost are, so
> that looks sane). It's only supported because it would take a special-case
> to *not* support it.
> 
> openconnect.nc is:
> 
>   "openconnect.nc"
>   "Juniper Network Connect (openconnect)"
>   "Compatible with Juniper Network Connect / Pulse Secure SSL VPN"


The service-type/aliases are here basically hacked to be the protocol option. I don't like that so much, because for me different service-types should be for fundamentally different types, where we would show a completely different UI.
Instead, you could just add a NM_OPENCONNECT_KEY_PROTOCOL option instead. Then it would also be natural, that the GUI shows a combobox to choose the protocol.

Ok, you do get a few benefits from what you did:

  (1) the aliases are statically define in the .name file, so nm-c-e only shows
    the known entries "anyconnect" and "nc". However, to add a new protocol 
    you can simple to edit the .name file, instead of patching the plugin.
    This could be achieved instead, by passing the NMVpnPluginInfo (which 
    basically is the .name file) from libnm down to the plugin. It seams 
    anyway useful for the plugin to be able to look into it's configuration,
    precisely to allow manual patching of certain options.

  (2) it allows to generate multiple entries in the add-connection-list of the 
    UI. Also could can be achieved differently as well, by letting the plugin 
    announce that it wishes multiple add-entries. I think that makes a lot of 
    sense (note that I always dream of more functionality in th plugins. Hence, 
    the plugins must tell more about themselves).
    
I implemented (2) in the UI. The previous change is still there, it allows  for each service-type/alias to create separate Add entries. The new change, allows for one service-type to create multiple Add entries.





(this discussion is independent as to whether implementing the new VPN calls generically, that topic shall be evaluated as well)

(In reply to Thomas Haller from comment #14)
> Instead, you could just add a NM_OPENCONNECT_KEY_PROTOCOL option instead.
> Then it would also be natural, that the GUI shows a combobox to choose the
> protocol.

I was keen to avoid that because it isn't natural for the *user*. The user wants to configure a VPN... they know it's an AnyConnect VPN, or they know it's a Juniper Network Connect VPN. They really want to see those two as top-level options for adding a new VPN.

They really *don't* want to have to choose 'openconnect' which is the open-source software they've never heard of which happens to support those protocols on this platform, and then select the specific protocol after that.

I'm happy to implement it as an internal NM_OPENCONNECT_KEY_PROTOCOL which is *automatically* set when the appropriate add-vpn is invoked, and use the same service type for everything. But I *really* want the multiple add-vpn options.

(In reply to David Woodhouse from comment #15)
> (In reply to Thomas Haller from comment #14)
> > Instead, you could just add a NM_OPENCONNECT_KEY_PROTOCOL option instead.
> > Then it would also be natural, that the GUI shows a combobox to choose the
> > protocol.
> 
> I was keen to avoid that because it isn't natural for the *user*. The user
> wants to configure a VPN... they know it's an AnyConnect VPN, or they know
> it's a Juniper Network Connect VPN. They really want to see those two as
> top-level options for adding a new VPN.
> 
> They really *don't* want to have to choose 'openconnect' which is the
> open-source software they've never heard of which happens to support those
> protocols on this platform, and then select the specific protocol after that.
> 
> I'm happy to implement it as an internal NM_OPENCONNECT_KEY_PROTOCOL which
> is *automatically* set when the appropriate add-vpn is invoked, and use the
> same service type for everything. But I *really* want the multiple add-vpn
> options.

Yes, I agree that makes sense. With what I have in mind, that would be possible too (from user's point-of-view, there should be no difference).

Let me implement it for nm-openconnect, and then we'll see how that is...

Implemented now for nm-openconnect.

How about that?

That looks really good; thanks. I can revert the hackery that I added in main() to accept other values for vpn_service. And for NM 1.2 maybe I *will* add that visible 'protocol' option that I so disliked, but I can live with it as long as we're doing it nicely in future.

I'm *still* unconvinced by the generic "call" stuff... more and more so, the more I see all that marshalling code inserted for each VPN plugin, and each new call, that needs it.

I like to see proper structures and datatypes... repeatedly opencoding this kind of thing is just *designing* for bugs, I suspect:

	g_value_set_string (args_out[0], _("Juniper Network Connect"));
	g_value_set_string (args_out[1], _("Compatible with Juniper…"));
	g_value_set_string (args_out[2], NM_OPENCONNECT_KEY_PROTOCOL);
	g_value_set_uint (args_out[3], 0);

FWIW I *might* be less unhappy with the generic call stuff if it used GVariant, and the response generation looked something like

...
#define NM_VPN_PLUGIN_CALL_GET_SERVICE_DETAIL_RESPONSE "(sssu)"
...

 args_out = g_variant_new(NM_VPN_PLUGIN_CALL_GET_SERVICE_DETAIL_RESPONSE,
           _("Juniper Network Connect"), _("Compatible with Juniper…"),
           NM_OPENCONNECT_KEY_PROTOCOL, 0);

That might resolve a lot of my concern about type-checking and marshalling...

(In reply to David Woodhouse from comment #18)
> That looks really good; thanks. I can revert the hackery that I added in
> main() to accept other values for vpn_service. And for NM 1.2 maybe I *will*
> add that visible 'protocol' option that I so disliked, but I can live with
> it as long as we're doing it nicely in future.
> 
> I'm *still* unconvinced by the generic "call" stuff... more and more so, the
> more I see all that marshalling code inserted for each VPN plugin, and each
> new call, that needs it.
> 
> I like to see proper structures and datatypes... repeatedly opencoding this
> kind of thing is just *designing* for bugs, I suspect:
> 
> 	g_value_set_string (args_out[0], _("Juniper Network Connect"));
> 	g_value_set_string (args_out[1], _("Compatible with Juniper…"));
> 	g_value_set_string (args_out[2], NM_OPENCONNECT_KEY_PROTOCOL);
> 	g_value_set_uint (args_out[3], 0);

I am suggesting this because:

  - in the future, I'd like to make more use of the VPN plugins. Although 
    infrequently, I think we will add new functions. We didn't add new
    functionality in the past, but also now the plugins can be loaded from nmcli
    and nmtui. So there scope grew.
  - we want to do major releases of NM more frequently, with less backporting
    of new API to older stable branches.
  - while I'd like to see more major releases of NM, it'd like to see less major
    releases of the plugins and applet. E.g. if a new functionality doesn't 
    require a new libnm version, there is no reason why we need to bump the 
    VPN/applet version. I could imagine, in a year NM/libnm already being
    at 1.8.0, while the VPN plugin is still at 1.4.z. As it makes no use of the
    new API, 1.4.z will work with NM 1.4.0 and newer.
    The version of the VPN plugin should not get bumped unless 
    it requires new libnm API (and via this generic thing, you can avoid
    requiring a new API) .
    Thus, in a year, we could still have the VPN and applet at version 
    1.4.z, while NM is already at 1.8.0. Then adding a new function to
    the VPN plugin and use it from applet would not require the user to
    fully upgrade to 1.8.0.



Hold on, I try to make implementation more straight forward.

IMO, the caller site is now pretty neat, I can just grab shared/nm-vpn-editor-plugin-call.h, which implements the access correctly and types-afe. I added now additional runtime checks, by specifying the type during the call:

»···if (!nm_vpn_editor_plugin_call (plugin,
»···                                NM_VPN_EDITOR_PLUGIN_CALL_GET_SERVICE_INFO,
»···                                NULL,
»···                                G_TYPE_STRING, &service_type,
»···                                G_TYPE_INVALID,
»···                                G_TYPE_STRING, &short_name_local,
»···                                G_TYPE_STRING, &pretty_name_local,
»···                                G_TYPE_STRING, &description_local,
»···                                G_TYPE_UINT,   out_flags ?: &flags_local,
»···                                G_TYPE_INVALID))


For the VPN plugin site, I don't see how to make it more slick. Arguably, it's ugly. Maybe not too unsafe, because if you mix up types, you quickly get assertion failures (g_critical).


so do people still think it should not be done this way?

FWIW it's the VPN plugin sites I'm most concerned about. The callers are generally worked on by people like you, who are paying attention. Then we get to the plethora of sporadically-maintained VPN plugins which mostly cargo-cult the NM integration bits without even understanding them too well (or maybe that's just me? :)

If you want to do this I'd *really* like to see you *completely* hide it from the VPN plugins themselves, by using glib-genmarshal or *something*, so that they only have to provide the function as normal C code, with real datatypes.

Yeah, nm_vpn_editor_plugin_call() is slick, but it's just a way of making well-defined link-time operations happen at call-time.

What are we trying to avoid here?

Why would we do something like this for the VPN plugins, but not for other stuff like libnm?  It just seems like we're re-implementing calls and marshalling and basically making our own internal procedure call and working around API/ABI.

RE comment 18 and comment 19, I can't see the previous branch history.  But why does the service itself need to be queried for the protocol?  Couldn't it just be put into the VPN plugin .service file, and if the .service file says that the plugin supports a certain alias/protocol, the plugin must do so or the connection fails.

It seems (though perhaps I didn't read completely) that the goal is to support one plugin presenting multiple VPN types in the UI.

I don't think it would be too hard to just add some stuff to the .service file that's a list of alias service types the plugin supports.  This would then be matched against NM_SETTING_VPN_SERVICE_TYPE when trying to find the plugin to run.  The service types should be unique to each plugin; eg we shouldn't have common 'ipsec' service types that both libreswan and openswan support, since that just leads to confusion about which plugins support which options and what the UIs look like.

But on the actual UI side, where we do want to show human-readable names rather than "libreswan", and where we now want to support a single VPN plugin showing multiple entries in the VPN menu, that's currently done through loading the VPN editor plugin and reading NM_VPN_EDITOR_PLUGIN_NAME.

Here, we could add another property to editor plugins called NM_VPN_EDITOR_PLUGIN_ALIASES or something like that.  It returns a GHashTable<string:string> or whatever, where one string is the alias/service-type from the .service file, and the other is a human-readable string to put into the dropdown list.

When the user selects one of these "alias" options from the list, the new VPN connection gets created with service-type=alias.

The VPN service itself gets the alias/serivce/whatever through the NMSettingVPN and can do what it wants with it.

Would that be a solution to adding Juniper support in the UI for openconnect?

(In reply to Dan Williams from comment #23)
> I don't think it would be too hard to just add some stuff to the .service
> file that's a list of alias service types the plugin supports.  This would
> then be matched against NM_SETTING_VPN_SERVICE_TYPE when trying to find the
> plugin to run. 

Hm, we already have that in libreswan to support the 'openswan' alias, and I've made use of precisely that method in the *existing* NM-openconnect git HEAD:
https://git.gnome.org/browse/network-manager-openconnect/commit/?id=34051bb91c
https://git.gnome.org/browse/network-manager-openconnect/commit/?id=c69a180f1c

Thomas was working on a solution which let me use only a single service-type (org.freedesktop.NetworkManager.openconnect) and automatically set a 'protocol' config option according to which top-level "Add VPN" option (AnyConnect/Juniper) was invoked. Which is all I *really* need, and might be a little simpler.

I'm ambivalent about how it gets done, really. My only real concern, as discussed in comment 15, is that I *don't* want people to have to add a VPN of type "OpenConnect", then select "AnyConnect" or "Juniper" as a sub-option of that.

We really do want "Cisco AnyConnect" amd "Juniper Network Connect" to be the first thing the user sees in the 'Add VPN' dialog.

(In reply to Dan Williams from comment #23)
> Yeah, nm_vpn_editor_plugin_call() is slick, but it's just a way of making
> well-defined link-time operations happen at call-time.
> 
> What are we trying to avoid here?

I tried to explain that in comment 20.

After releasing 1.4.0, I don't want that we bump the major version numbers of VPN plugins again. The plugin should just be backward compatible with anything >= 1.4.0.


> Why would we do something like this for the VPN plugins, but not for other
> stuff like libnm?

I don't see how that would be useful for libnm.

> It just seems like we're re-implementing calls and
> marshalling and basically making our own internal procedure call and working
> around API/ABI.

The client shall only call the VPN plugin through API defined in libnm, which requires a new libnm version when new API to the plugin is added.

with this generic mechanism, we can introduce/backport functionality to nm-applet (1.4.0) and nn-openconnect (1.4.0) without requiring an update/backport to libnm.





>>> The discussion above is completely independent from below.




> RE comment 18 and comment 19, I can't see the previous branch history.  But
> why does the service itself need to be queried for the protocol?  Couldn't
> it just be put into the VPN plugin .service file, and if the .service file
> says that the plugin supports a certain alias/protocol, the plugin must do
> so or the connection fails.

Sorry, can you re-state your question. I think there is some misunderstanding here.


> It seems (though perhaps I didn't read completely) that the goal is to
> support one plugin presenting multiple VPN types in the UI.

The goal is to support multiple openconnect protocols, +

quote:
 My only real concern, as discussed in comment 15, is that I *don't* want people to have to add a VPN of type "OpenConnect", then select "AnyConnect" or "Juniper" as a sub-option of that.



> I don't think it would be too hard to just add some stuff to the .service
> file that's a list of alias service types the plugin supports.  This would
> then be matched against NM_SETTING_VPN_SERVICE_TYPE when trying to find the
> plugin to run.  The service types should be unique to each plugin; eg we
> shouldn't have common 'ipsec' service types that both libreswan and openswan
> support, since that just leads to confusion about which plugins support
> which options and what the UIs look like.

isn't that exactly what is happening? the services are read from the .name file as:
  
  [VPN Connection]
  service=org.freedesktop.NetworkManager.libreswan
  aliases=org.freedesktop.NetworkManager.openswan



> But on the actual UI side, where we do want to show human-readable names
> rather than "libreswan", and where we now want to support a single VPN
> plugin showing multiple entries in the VPN menu, that's currently done
> through loading the VPN editor plugin and reading NM_VPN_EDITOR_PLUGIN_NAME.

> Here, we could add another property to editor plugins called
> NM_VPN_EDITOR_PLUGIN_ALIASES or something like that.  It returns a
> GHashTable<string:string> or whatever, where one string is the
> alias/service-type from the .service file, and the other is a human-readable
> string to put into the dropdown list.

This is exactly what happens.
you suggest:

  g_object_get (plugin,
                NM_VPN_EDITOR_PLUGIN_ALIASES_NAME,
                &pretty_names,
                NM_VPN_EDITOR_PLUGIN_ALIASES_DESCRIPTION,
                &descriptions,
                NM_VPN_EDITOR_PLUGIN_ALIASES_FLAGS,
                &all_service_flags,
                NULL);
  pretty_name = g_hash_table_lookup (pretty_names, service_type);
  descriptions = g_hash_table_lookup (descriptions, service_type);
  service_flags = GPOINTER_TO_UINT (g_hash_table_lookup (all_service_flags, 
                                                         service_type));

and I added instead:

  nm_vpn_editor_plugin_get_service_info (plugin,
                                         service_type,
                                         &short_name,
                                         &pretty_name,
                                         &description,
                                         &service_flags);

(that is no real difference).


All done in commit "c-e: allow one VPN plugin to create multiple "Add" entires per provided service-type"


> When the user selects one of these "alias" options from the list, the new
> VPN connection gets created with service-type=alias.
> 
> The VPN service itself gets the alias/serivce/whatever through the
> NMSettingVPN and can do what it wants with it.
> 
> Would that be a solution to adding Juniper support in the UI for openconnect?

this is exactly what was added in earlier versions. nm-openconnect had multiple service-types, and it worked like that. In fact, all the code to make this happen is still there! See "c-e: allow one VPN plugin to create multiple "Add" entires per prov..."




Later, I suggested to solve this differently, and not use different service-types to express protocols in nm-openconnect. There isn't a hugely good reason for that, but to me a different service type is something more distinctive. Instead, it seems to me the protocol should be just a property of the connection for openconnect.
So, I extended nm-c-e, to allow multiple add-entries for one service-type. See "c-e: support multiple "Add VPN connection" entires per service-type"

Ok, completely reworked the way how to call generic functions (that is functions, that libnm isn't aware of). Now done via a function table.

I think that is much better now.

All repushed as th/vpn-service-info-bgo767197.
(previous version for reference was th/vpn-service-info-bgo767197-1).

I'm happier with that version; thanks for the continued effort!

Seems ok to me up to "libnm/vpn: pass NMVpnPluginInfo to the
NMVpnEditorPlugin instance". The last commit (vtable) probably
is independent from the rest, and I don't have an opinion about
it yet.

The nm-applet and openconnect parts work fine here.

lgtm.


Just a couple of tiny imperfections in branch th/vpn-service-info-bgo767197 (that could be safely left as they are):
1) in the commit on top, a typo in a comment (missing space):
 + /* we make use of ___otherinternal___ header files

2) in the same commit, file POTFILES.skip:
 vpn-daemons/vpnc
 contrib/fedora/rpm/
 # https://bugs.launchpad.net/intltool/+bug/1117944
+shared/nm-vpn-editor-plugin-call.h
 sub/policy/org.freedesktop.NetworkManager.policy.in

inserted line splits comment from the line it refers to

(In reply to Francesco Giudici from comment #30)
> lgtm.

thanks



all merged to master:


NM: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b6b84d0442809d1b127c4bb36f09445ece2ac262

nm-applet: https://git.gnome.org/browse/network-manager-applet/commit/?id=e884f255dd8fd13d914a337de4281c243d037c29

nm-openconnect: https://git.gnome.org/browse/network-manager-openconnect/commit/?id=4215786f5b92dd461fc3c82c3bd7bb975f84c892