interfaces: first version of the networkmanager interface

[morphis]

Dropping this for a first review round. There are still some things to figure out with the policy for other functionality parts but this gives us a working network-manager snap where connecting a plug and a slot with the networkmanager interface works.

[zyga]

whitelist this please

[niemeyer]

retest this please

[niemeyer]

@jdstrand @zyga can you please have a pass at this when you have a moment?

[jdstrand]

You repeated net_admin twice.

[morphis]

Fixed.

[jdstrand]

These are interesting @zyga-- nothing to change but take note the udev accesses for future interfaces.

[zyga]

@jdstrand phys_port_id and dev_id? Noted though I'm not quite sure what they mean yet.

[jdstrand]

Several accesses are part of the nameservice abstraction. Did you actively choose not to use the nameservice apparmor abstraction? If so, why?

[morphis]

Didn't know that there is one but will use it instead.

[jdstrand]

This is very wide open and lets network-manager ask for any properties for any unconfined service. Was this intentional? If so, why? Also, if so, you probably want path=/org/freedesktop/**

[morphis]

That is more or less a left over from the bluez apparmor profile which I used as template (see https://github.com/ubuntu-core/snappy/blob/master/interfaces/builtin/bluez.go#L56). Not sure if that was needed for bluez to access any registers profile provider for its properties but that wouldn't give a reason why its limited to /org/freedesktop/**

Let me drop this and see what breaks or if everything is still fine. However we should remember what we needed this for bluez and if it was just for one remote service lets limit it there.

[jdstrand]

This rules allows anything to connect to us. This is possibly not for this commit, but snappy is in a position to add 'peer=(label=snap...)' clauses on both the slots and plugs side, so this more open than it needs to be.

[jdstrand]

ditto

[jdstrand]

This is too open I think. Can we add method clauses to tighten this up?

[morphis]

Fixed.

[jdstrand]

These both need 'peer=(label=unconfined)' clauses.

[morphis]

Fixed

[jdstrand]

'peer=(label=snap..*)' where SLOTSIDE is substituted based on what the slots side name is. This was discussed in the bluez PR and irc last week. I gave @zyga the start of a patch for bluez, but not sure if he had a chance to look at it yet.

[morphis]

Will check with @zyga today what the state of this is. When its not ready yet are you fine with merging this in and add those things later?

[zyga]

I had a look at it but I didn't act on it yet. I have that as a TODO for today, to make it connection aware.

[jdstrand]

ditto

[jdstrand]

ditto

[jdstrand]

What is network-manager chowning things to? There are no per-snap UIDs at this time so we need to be careful here. Also, knowing what it chowns to will help us refine this rule when the seccomp arg filtering branch lands. Finally, I think you'll actually need this to avoid bugs on other architectures/future updates:

chown
chown32
fchown
fchown32
fchownat
lchown
lchown32

[morphis]

In our case NetworkManager will always chown to root:root but it has support to run chown on the connection settings files in $SNAP_DATA/conf/system-connections to other users but currently only uses it for testing purposes. I would like to go with this for now and limit it to root:root once the seccomp arg filtering branch lands. Is that ok?

Adding those other syscall abbreviations.

[jdstrand]

Yes, but please add this comment above it:

FIXME: adjust after seccomp argument filtering lands (LP: #1446748)

[jdstrand]

This is a complex bus policy. What is its origin? Is it appropriate for Ubuntu Core? For example, Core has no way to prompt with policykit and these vpns aren't available.

[morphis]

The policy file is directly from NetworkManager upstream. See https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/org.freedesktop.NetworkManager.conf

There are small additions to use the Ubuntu approach inside NetworkManager to deal with dnsmasq over dbus rather than restarting it.

I know that @tonyespy has a strong opinion on us not diverging too much from the policy file shipping with NetworkManager itself to not cause any security holes. However if we limit all the bits inside AppArmor there would be no need for us to carry all the deny/allow things here which aren't really necessary on a ubuntu-core device as those things are already secured by AppArmor.

@jdstrand What is your standpoint on this?

Also note that we will add VPN support in the future.

[jdstrand]

In general I think using the upstream's bus policy is fine here. However, there are a couple of places where the policy references PolicyKit, but PolicyKit is not on Ubuntu Core (it is on sdoc though, but the interaction with a network-manager snap on an sdoc system is currently undefined (ie, two network managers would attempt to run on the system bus which is certainly not what we want)). What will network-manager do if policykit is simply not available? Furthermore we don't have a strong concept of users on Ubuntu Core so the 'default' section is overly complicated. My preference would be to take the 'root' section from upstream and cull the allow rules from the 'default' section (leaving only deny rules), unless someone can come up with a use case for the non-root user to access network manager without policy kit.

@zyga and @niemeyer , this is another point we need to consider-- some snaps will simply not work or cause problems on an sdoc system.

[morphis]

Followed what @jdstrand suggested here and simplified the default section.

[jdstrand]

The new dbus policy looks ok.

[morphis]

@jdstrand @zyga @niemeyer Any further comments? Can we get this merged

[niemeyer]

Before merging this we need to consider the name of this interface. I'll have another look when I'm fully back tomorrow.

[tonyespy]

Shouldn't a snap that uses this plug be able to call the ObjectManager interface's GetManagedObjects method? If so doesn't this need to be "dbus (receive, send)"? Also not sure if the path needs adjustment to allow this too?

[tonyespy]

I assume snappy automatically prefixes <!DOCTYPE> when generating the final document?

[morphis]

Sure it does. It also puts the element around those parts in here.

[tonyespy]

Not sure if this policy file was updated using the 1.2 release? In the latest upstream version, the above comment actually reads:

<!-- These are there because some broken policies do                                                                                    
       <deny send_interface="..." /> (see dbus-daemon(8) for details).                                                                    
       This seems to override that for the known VPN plugins.                                                                             
   -->

[tonyespy]

Upstream also has the following additional elements in group="root":

  <allow send_destination="org.freedesktop.NetworkManager.l2tp"/>
  <allow send_destination="org.freedesktop.NetworkManager.libreswan"/>
  <allow send_destination="org.freedesktop.NetworkManager.fortisslvpn"/>
  <allow send_destination="org.freedesktop.NetworkManager.strongswan"/>
  <allow send_interface="org.freedesktop.NetworkManager.VPN.Plugin"/>

[tonyespy]

A couple of comments...

1. We should probably indicate somewhere in the interface which version {minium} of NM this policy corresponds to.
2. I've noted in a few places inline that the DBus policy as defined doesn't match up with the latest upstream version ( 1.2 ).
3. While I agree that we don't really have a case for non-root user scenarios, as I understand it, the default policy applies to root users too. Please review the dbus-daemon man page, it describes the order in which policies are applied. Thus we might want to keep the default policy as currently defined by upstream. That said, I haven't done any actual testing to verify this.
4. Regarding PolicyKit, the only references I see in the DBus configuration are comments, which state that extra authentication/authorization is performed by PolKit for some of the DBus interfaces. As core doesn't include PolicyKit, why don't we just build NM with it disabled ( eg. --enable-polkit=disabled )?
5. Upstream also defines two resource limits in the it's DBus configuration file:

<limit name="max_replies_per_connection">1024</limit>
<limit name="max_match_rules_per_connection">2048</limit>

[morphis]

@tonyespy

• synced policy config back with upstream 1.2 and brought all rules for context="default" back.
• the context="default" section is used for all users so you're right that its applied as first thing. In our rule set we explicitly disable all access with the first deny rules and then enable particular things allowed for every user again. The root user gets a wildcard for everything in on top of that. We have two options here:
  1. We take the policy as is from upstream with the consequence that a user other than "root" is allowed to call NetworkManager.
  2. We leave the deny-anything rules in so that no user can send to NetworkManager if no further rules are specified. We then leave the bits as they are in the section to give root access to everything needed.
• we already build with polkit support disabled. See https://git.launchpad.net/~morphis/snappy-hwe-snaps/+git/network-manager/tree/snapcraft.yaml?h=rework-for-snappy-2x#n110

[niemeyer]

@zyga Same problem of the bluez interface here.

[zyga]

Yep, I'll move the fix from bluez to a common function so that this branch can use it.

[zyga]

Any update on this?

[morphis]

Did not had time to fix this today. If you want feel free to do the rework. I will not until mid Monday next week.

[niemeyer]

Okay, this LGTM assuming three points:

1. network-manager rather than networkmanager
2. the snippets should hold the proper snap and app label rather than hardcoded snap.foo.*
3. this interface works on the desktop too

[morphis]

I've updated the implementation now to

1. Use 'network-manager' as interface name
2. Snippets are using the proper snap and app label by reusing the common code @zyga wrote in interfaces/builtin/utils.go
3. Minimized the apparmor policy to work the same way as the already implemented one for bluez does. Needs review from @jdstrand again to make sure we don't do anything wrong

I've tested this on a x86-64 KVM instance with the network-manager snap from https://launchpad.net/~morphis/+snap/network-manager/+build/836/+files/network-manager_1.1.94-1_amd64.snap

@niemeyer I would really like to exclude getting this interface working on desktop with considering this as just the first version we will improve over time. This is already sitting here for too long and we should add the desktop integration with a next step as this seems to open another set of open questions (how do we decide if an interface is implicit depending on the target environment? ...) and I don't want to delay a merge any longer as we have quite a few depending on this.

[zyga]

Why do we have peer=(label=unconfined) here but not on the two above?

[morphis]

The peer hostnamed runs unconfined in the OS snap that is why we have label=unconfined here.

[jdstrand]

We can use peer label rules here for the two rules zyga mentioned (and we should). If this is only for the service to talk to itself, you can use label=@{profile_name} (@{profile_name} is a special apparmor variable for the name of the profile of the current process) and if this is for connected client, perhaps this rule should be in connected slots instead of permanent slots.

Note, with 15.04 frameworks we said that dbus policy would be wide open and server policy would be wide-open and the framework-policy client policy would be the primary mediation point. However, with interfaces we have much more information to create precise rules for both server side and client side.

[morphis]

@jdstrand This is to allow the NetworkManager service to talk to the hostname service which is running as part of the ubuntu-core snap and through that it is unconfined and gets peer label "unconfined". Or is there anything I didn't see and we should change in this rule?

[jdstrand]

@morphis - let's not block on this rule. See my other comment.

[zyga]

Is this really the dbus object path used by wpa-supplicant? :-(

[morphis]

It is.

[zyga]

What does this allow exactly?

[morphis]

It allows sending to the network manager interface. Jamie implemented this approach once ago for bluez so I took the same to keep our interfaces consistent.

[zyga]

Does this dbus (receive) rule mean "Allow receiving messages from network manager"?

In case it does, why does it say peer=(label=unconfined)

[jdstrand]

It shouldn't, it should have the slot side names like what is happening in the new bluez rules.

[morphis]

Fixed.

[zyga]

I assume that PolicyKit is present on the desktop where n-m comes from the deb package. What about our build of n-m in a snap. Is policykit still looking at those?

[morphis]

PolicyKit doesn't look at this file at all. Support for this is implemented in network-manager itself if enabled.

[zyga]

I understand that much. My question was that if those writes are still secured with policykit in our n-m snap.

[morphis]

@zyga No, they are not. For now the network-manager interface means access to everything with no further level of secured access. If we want something like this we can work this out later. Also this interface is not meant for desktop yet.

[morphis]

Added desktop support.

[zyga]

I assume that we want to allow any tool running as root/service that has the network-manager plug to freely access those?

[morphis]

Yes, that is why those are allowed again for user="root" in the section above. Any other user is not allowed to call those.

[jdstrand]

Do these permanent slot rules rules work on classic desktop? I have a feeling they are missing a few things. If they are, can you organize the rules so that any additional classic desktop-only are in its own section (so we may better review how them).?

[morphis]

I didn't tested this interface on desktop yet and don't plan to do that for this MP. This interface should be considered only for Ubuntu Core for now and should not be implicitly exported by the desktop.

[morphis]

Added desktop support now.

[jdstrand]

This rule is not quite right. This says that a client can send anything to a connection name of org.freedesktop.NetworkManager that is unconfined. I suspect this is to cover the dbus daemon rules-- it would be best if the needed rules were enumerated more fully.

[morphis]

Fixed that and reduce the rules for a connected plug to a single one.

[jdstrand]

Note seccomp arg filtering is coming soon. What is nm chowning to?

[zyga]

Ohh, how soon? Before 16.04.1?

[jdstrand]

That is the plan. The branch is ready and needs a review from the security team (Tyler) which I believe he will look at soonish.

[morphis]

@jdstrand NetworkManager always calls chown root:root in newly created network settings files. However that code is only there to allow passing different user&group for test cases which we don't run in the snap all. Will add a comment in the code that we limit chown to root:root

[jdstrand]

@morphis perfect, thanks!

[morphis]

@zyga @jdstrand I've pushed a new version of this interface which addresses all comments. Please have another look.

[niemeyer]

Thanks a lot for the changes.

Can we please look at the desktop case? We shouldn't be introducing such fundamental interfaces without them working on the desktop, and we have zyga and jdstrand, as well as the desktop developers to help us. Let's just push that forward and get it in.

[morphis]

@niemeyer Added support for desktop now.

Also fixed all unit tests so that we should get a full passing CI run.

[morphis]

@zyga @jdstrand This is what I added to make it working on desktop installations. Can you please have a look?

[tonyespy]

@morphis

I re-reviewed again today, and it looks good to me. Other than the copyright issue we discussed earlier, I think my only suggestions are:

• As we already know NM 1.2.2 adds support for communicating with dnsmasq via DBus, we should go ahead and add the extra DBus policy and apparmor DBus rules now vs. trying to land them when we discover we need them.
• I think it would helpful to add some documentation-like comments to the NetworkManagerInterface public methods, giving a general description of the security semantics each implements.

[tonyespy]

Why is "DBus.**" used here ( and other places ) when there are no "/"s allowed in a DBus interface name?

[morphis]

Fixed.

[tonyespy]

Shouldn't this be "networkManagerPermanentSlotDBus"?

[morphis]

Fixed.

[morphis]

@tonyespy Fixed all comments.

Documentation of the security semantics of the public methods can be found in the source at interfaces/core.go. NetworkManager specifics should be documented the best with self explaining code.

[morphis]

@zyga @jdstrand If no last comments from your side this should be ready for integration now.

[zyga]

LGTM and iterate on issues as they araise

[jdstrand]

@morphis I think I'd like to see these broken out into separate send and receive rules so we can leverage peer=(), but let's not block on this alone. Can you provide your methodology for testing the DBus API so I may use it to refine these rules?

[morphis]

@jdstrand Fetch the network-manager snap from https://code.launchpad.net/~snappy-hwe-team/+snap/network-manager , install it then connection network-manager:service to network-manager:nmcli

Then use nmcli to configure your wifi / ethernet connection. Documentation can be found at https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Networking_Guide/sec-Using_the_NetworkManager_Command_Line_Tool_nmcli.html for example.

[jdstrand]

Noting for thoroughness, 16.04 network manager has <policy user="whoopsie">. Since whoopsie is not packaged as a snap and we don't have per-snap uids, this is not an issue. Otherwise, the bus policy looks fine.

[morphis]

We dropped the whoopsie statement as we want to ship the pure upstream dbus policy file without any debian/ubuntu additions. If we come to whoopsie we can add it again here.

[jdstrand]

Nice :)

[jdstrand]

I believe this should be conditional on if not release.OnClassic since classic already has a shipped bus policy as part of the deb (unless there are provisions for this that I'm not aware of). @zyga can you comment?

[zyga]

Yes though it doesn't matter much since the slot will be on the OS snap which has exactly 0 applications.

The related question is how to block network-manager as a snap when network-manager as a deb is installed?

[morphis]

First of all we will publish the network-manager snap only for Ubuntu Core in the store so that people can't install it on the desktop at all. If then still somebody does its his and not our problem I would say.

[jdstrand]

LGTM pending @zyga's response to my inline comment.

[zyga]

+1 because I don't see anything that needs fixing ASAP