Commit graph

73 commits

Author SHA1 Message Date
Thomas Haller
186c5b7a65 core: use nm_connection_get_setting_ip_config() helper
(cherry picked from commit 2be022ad68)
2019-03-05 12:23:59 +01:00
Thomas Haller
6f35efe6fe policy: treat WireGuard devices as VPN for DNS
WireGuard devices are (will be) regular NMDevice implementations,
but NMDnsManager should treat them like VPN.

For that, reuse the device's type and nm_device_get_route_metric_default().
2019-02-14 08:00:29 +01:00
Thomas Haller
a3370af3a8 all: drop unnecessary includes of <errno.h> and <string.h>
"nm-macros-interal.h" already includes <errno.h> and <string.h>.
No need to include it everywhere else too.
2019-02-12 08:50:28 +01:00
Beniamino Galvani
797eab2f6b vpn: add route to vpn gw when parent has a default device route
When the parent device has a device default route (i.e. without
gateway) and we establish a VPN on top of it, 'ip route get' for the
VPN gateway returns a device route, which is the same result we get
for an unreachable VPN gateway. However it is necessary to add the
route to the gateway or otherwise it will possibly become unreachable
once the VPN gets activated.

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/93
2019-02-04 11:07:02 +01:00
Thomas Haller
617bdbd8c2 all/trivial: rename NM_UTILS_LOOKUP_STR() to have "_A" suffix
NM_UTILS_LOOKUP_STR() uses alloca(). Partly to avoid the overhead of
malloc(), but more important because it's convenient to use. It does
not require to declare a varible to manage the lifetime of the heap
allocation.

It's quite safe, because the stack allocation is of a fixed size of only
a few bytes. Overall, I think the convenience that we get (resulting in
simpler code) outweighs the danger of stack allocation in this case. It's
still worth it.
However, as it uses alloca(), it still must not be used inside a (unbound)
loop and it is obviously a macro.

Rename the macros to have a _A() suffix. This should make the
peculiarities more apparent.
2019-01-15 09:52:01 +01:00
Thomas Haller
d18f40320d platform: merge NMPlatformError with nm-error
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from <errno.h>, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from <errno.h> or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.

This is also what systemd does, except that systemd only returns
(negative) integers from <errno.h>, while we merge our own error codes
into the range of <errno.h>.

The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from <errno.h>, but it did not entail error codes from netlink.
2018-12-27 21:33:59 +01:00
Thomas Haller
a51c09dc12 all: don't use static buffer for nm_utils_inet*_ntop()
While nm_utils_inet*_ntop() accepts a %NULL buffer to fallback
to a static buffer, don't do that.

I find the possibility of using a static buffer here error prone
and something that should be avoided. There is of course the downside,
that in some cases it requires an additional line of code to allocate
the buffer on the stack as auto-variable.
2018-12-19 09:23:08 +01:00
Thomas Haller
b635b4d419 core: improve and fix keeping connection active based on "connection.permissions"
By setting "connection.permissions", a profile is restricted to a
particular user.
That means for example, that another user cannot see, modify, delete,
activate or deactivate the profile. It also means, that the profile
will only autoconnect when the user is logged in (has a session).

Note that root is always able to activate the profile. Likewise, the
user is also allowed to manually activate the own profile, even if no
session currently exists (which can easily happen with `sudo`).

When the user logs out (the session goes away), we want do disconnect
the profile, however there are conflicting goals here:

1) if the profile was activate by root user, then logging out the user
   should not disconnect the profile. The patch fixes that by not
   binding the activation to the connection, if the activation is done
   by the root user.

2) if the profile was activated by the owner when it had no session,
   then it should stay alive until the user logs in (once) and logs
   out again. This is already handled by the previous commit.

   Yes, this point is odd. If you first do

      $ sudo -u $OTHER_USER nmcli connection up $PROFILE

   the profile activates despite not having a session. If you then

      $ ssh guest@localhost nmcli device

   you'll still see the profile active. However, the moment the SSH session
   ends, a session closes and the profile disconnects. It's unclear, how to
   solve that any better. I think, a user who cares about this, should not
   activate the profile without having a session in the first place.

There are quite some special cases, in particular with internal
activations. In those cases we need to decide whether to bind the
activation to the profile's visibility.

Also, expose the "bind" setting in the D-Bus API. Note, that in the future
this flag may be modified via D-Bus API. Like we may also add related API
that allows to tweak the lifetime of the activation.

Also, I think we broke handling of connection visiblity with 37e8c53eee
"core: Introduce helper class to track connection keep alive". This
should be fixed now too, with improved behavior.

Fixes: 37e8c53eee

https://bugzilla.redhat.com/show_bug.cgi?id=1530977
2018-12-09 14:47:32 +01:00
Beniamino Galvani
446e5b27d6 core: add checks on connection default properties
Add a new CON_DEFAULT() macro that places a property name into a
special section used at runtime to check whether it is a supported
connection default.

Unfortunately, this mechanism doesn't work for plugins so we have to
enumerate the connection defaults from plugins in the daemon using
another CON_DEFAULT_NOP() macro.
2018-12-01 15:16:48 +01:00
Thomas Haller
f71f9b54a8 vpn: disconnect signal handlers from proxy in NMVpnConnection::dispose()
We cannot be sure who holds a reference to the proxy, and
who is gonna call us back after the VPN connection instance
is destroyed.

(cherry picked from commit 6ebb9091d2)
2018-09-14 15:24:31 +02:00
Thomas Haller
011dd919fa vpn: fix assertion during "SecretsRequired" in unexpected state
Got this assertion:

    NetworkManager[12939]: <debug> [1536917977.4868] active-connection[0x563d8fd34540]: set state deactivated (was deactivating)
    ...
    NetworkManager[12939]: nm-openvpn[1106] <info>  openvpn[1132]: send SIGTERM
    NetworkManager[12939]: nm-openvpn[1106] <info>  wait for 1 openvpn processes to terminate...
    NetworkManager[12939]: nm-openvpn[1106] <warn>  openvpn[1132] exited with error code 1
    NetworkManager[12939]: <info>  [1536917977.5035] vpn-connection[0x563d8fd34540,2fdeaea3-975f-4325-8305-83ebca5eaa26,"my-openvpn-Red-Hat",0]: VPN plugin: requested secrets; state disconnected (9)
    NetworkManager[12939]: plugin_interactive_secrets_required: assertion 'priv->vpn_state == STATE_CONNECT || priv->vpn_state == STATE_NEED_AUTH' failed

Meaning. We should either ensure that secrets_required_cb() signal callback
is disconnected from proxy's signal, or we gracefully handle callbacks at
unexpected moments. Do the latter.

(cherry picked from commit 92344dd084)
2018-09-14 15:24:31 +02:00
Beniamino Galvani
bc7efc750a core: add support for connection.llmnr 2018-09-06 09:07:41 +02:00
Beniamino Galvani
53d9050b36 core: add nm_config_data_get_connection_default_int64() 2018-09-06 09:07:41 +02:00
Thomas Haller
38273a8871 settings: use delegation instead of inheritance for NMSettingsConnection and NMConnection
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).

NMSettingsConnection does a lot of things already:

  1) it "is-a" NMDBusObject and exports the API of a connection profile
     on D-Bus
  2) it interacts with NMSettings and contains functionality
     for tracking the profiles.
  3) it is the base-class of types like NMSKeyfileConnection and
     NMIfcfgConnection. These handle how the profile is persisted
     on disk.
  4) it implements NMConnection interface, to itself track the
     settings of the profile.

3) and 4) would be better implemented via delegation than inheritance.

Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.

Advantages:

  - by delegating, there is a clearer separation of what
    NMSettingsConnection does. For example, in C we often required
    casts from NMSettingsConnection to NMConnection. NMConnection
    is a very trivial object with very little logic. When we have
    a NMConnection instance at hand, it's good to know that it is
    *only* that simple instead of also being an entire
    NMSettingsConnection instance.

    The main purpose of this patch is to simplify the code by separating
    the NMConnection from the NMSettingsConnection. We should generally
    be aware whether we handle a NMSettingsConnection or a trivial
    NMConnection instance. Now, because NMSettingsConnection no longer
    "is-a" NMConnection, this distinction is apparent.

  - NMConnection is implemented as an interface and we create
    NMSimpleConnection instances whenever we need a real instance.
    In GLib, interfaces have a performance overhead, that we needlessly
    pay all the time. With this change, we no longer require
    NMConnection to be an interface. Thus, in the future we could compile
    a version of libnm-core for the daemon, where NMConnection is not an
    interface but a GObject implementation akin to NMSimpleConnection.

  - In the previous implementation, we cannot treat NMConnection immutable
    and copy-on-write.
    For example, when NMDevice needs a snapshot of the activated
    profile as applied-connection, all it can do is clone the entire
    NMSettingsConnection as a NMSimpleConnection.
    Likewise, when we get a NMConnection instance and want to keep
    a reference to it, we cannot do that, because we never know
    who also references and modifies the instance.
    By separating NMSettingsConnection we could in the future have
    NMConnection immutable and copy-on-write, to avoid all unnecessary
    clones.
2018-08-28 22:27:55 +02:00
Thomas Haller
e1c7a2b5d0 all: don't use gchar/gshort/gint/glong but C types
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.

    $ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
    587
    $ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
    21114

One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during

  g_object_set (obj, PROPERTY, (gint) value, NULL);

However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.

Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).

A simple style guide is instead: don't use these typedefs.

No manual actions, I only ran the bash script:

  FILES=($(git ls-files '*.[hc]'))
  sed -i \
      -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
      -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>  /\1   /g' \
      -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
      "${FILES[@]}"
2018-07-11 12:02:06 +02:00
Lubomir Rintel
e69d386975 all: use the elvis operator wherever possible
Coccinelle:

  @@
  expression a, b;
  @@
  -a ? a : b
  +a ?: b

Applied with:

  spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .

With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.

Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
2018-05-10 14:36:58 +02:00
Beniamino Galvani
1b5925ce88 all: remove consecutive empty lines
Normalize coding style by removing consecutive empty lines from C
sources and headers.

https://github.com/NetworkManager/NetworkManager/pull/108
2018-04-30 16:24:52 +02:00
Thomas Haller
5c4a6e9b6d manager: ensure valid specific_object path is passed to _new_active_connection()
From the D-Bus layer, no specific-object is represented by "/". We
should early on normalize such values to NULL, and not expect or
handle them later (like during _new_active_connection()).
2018-04-18 07:55:15 +02:00
Beniamino Galvani
43a0f47ea2 core: specify an activation reason for active connections
Specify a reason when creating active connections. The reason will be
used in the next commit to tell whether slaves must be reconnected or
not if a master has autoconnect-slaves=yes.
2018-04-08 09:40:14 +02:00
Thomas Haller
297d4985ab core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API
Previously, we used the generated GDBusInterfaceSkeleton types and glued
them via the NMExportedObject base class to our NM types. We also used
GDBusObjectManagerServer.

Don't do that anymore. The resulting code was more complicated despite (or
because?) using generated classes. It was hard to understand, complex, had
ordering-issues, and had a runtime and memory overhead.

This patch refactors this entirely and uses the lower layer API GDBusConnection
directly. It replaces the generated code, GDBusInterfaceSkeleton, and
GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
and static descriptor instances of type GDBusInterfaceInfo.

This adds a net plus of more then 1300 lines of hand written code. I claim
that this implementation is easier to understand. Note that previously we
also required extensive and complex glue code to bind our objects to the
generated skeleton objects. Instead, now glue our objects directly to
GDBusConnection. The result is more immediate and gets rid of layers of
code in between.
Now that the D-Bus glue us more under our control, we can address issus and
bottlenecks better, instead of adding code to bend the generated skeletons
to our needs.

Note that the current implementation now only supports one D-Bus connection.
That was effectively the case already, although there were places (and still are)
where the code pretends it could also support connections from a private socket.
We dropped private socket support mainly because it was unused, untested and
buggy, but also because GDBusObjectManagerServer could not export the same
objects on multiple connections. Now, it would be rather straight forward to
fix that and re-introduce ObjectManager on each private connection. But this
commit doesn't do that yet, and the new code intentionally supports only one
D-Bus connection.
Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
for the moment. It could be easily extended later, for example with polling whether the
system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
supported either -- just like before.

Note how NMDBusManager now implements the ObjectManager D-Bus interface
directly.

Also, this fixes race issues in the server, by no longer delaying
PropertiesChanged signals. NMExportedObject would collect changed
properties and send the signal out in idle_emit_properties_changed()
on idle. This messes up the ordering of change events w.r.t. other
signals and events on the bus. Note that not only NMExportedObject
messed up the ordering. Also the generated code would hook into
notify() and process change events in and idle handle, exhibiting the
same ordering issue too.
No longer do that. PropertiesChanged signals will be sent right away
by hooking into dispatch_properties_changed(). This means, changing
a property in quick succession will no longer be combined and is
guaranteed to emit signals for each individual state. Quite possibly
we emit now more PropertiesChanged signals then before.
However, we are now able to group a set of changes by using standard
g_object_freeze_notify()/g_object_thaw_notify(). We probably should
make more use of that.

Also, now that our signals are all handled in the right order, we
might find places where we still emit them in the wrong order. But that
is then due to the order in which our GObjects emit signals, not due
to an ill behavior of the D-Bus glue. Possibly we need to identify
such ordering issues and fix them.

Numbers (for contrib/rpm --without debug on x86_64):

- the patch changes the code size of NetworkManager by
  - 2809360 bytes
  + 2537528 bytes (-9.7%)

- Runtime measurements are harder because there is a large variance
  during testing. In other words, the numbers are not reproducible.
  Currently, the implementation performs no caching of GVariants at all,
  but it would be rather simple to add it, if that turns out to be
  useful.
  Anyway, without strong claim, it seems that the new form tends to
  perform slightly better. That would be no surprise.

  $ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .;  done)
  - real    1m39.355s
  + real    1m37.432s

  $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
  - real    0m26.843s
  + real    0m25.281s

- Regarding RSS size, just looking at the processes in similar
  conditions, doesn't give a large difference. On my system they
  consume about 19MB RSS. It seems that the new version has a
  slightly smaller RSS size.
  - 19356 RSS
  + 18660 RSS
2018-03-12 18:37:08 +01:00
Thomas Haller
c03a534963 core: implement setting MDNS setting for systemd
The connection.mdns setting is a per-connection setting,
so one might expect that one activated device can only have
one MDNS setting at a time.

However, with certain VPN plugins (those that don't have their
own IP interface, like libreswan), the VPN configuration is merged
into the configuration of the device. So, in this case, there
might be multiple settings for one device that must be merged.

We already have a mechanism for that. It's NMIP4Config. Let NMIP4Config
track this piece of information. Although, stricitly speaking this
is not tied to IPv4, the alternative would be to introduce a new
object to track such data, which would be a tremendous effort
and more complicated then this.

Luckily, NMDnsManager and NMDnsPlugin are already equipped to
handle multiple NMIPConfig instances per device (IPv4 vs. IPv6,
and Device vs. VPN).

Also make "connection.mdns" configurable via global defaults in
NetworkManager.conf.
2018-01-09 14:24:54 +01:00
Lubomir Rintel
6672c5e92e all: get rid of a handful of unused-but-set variables 2017-12-18 13:29:32 +01:00
Thomas Haller
616976d6a8 core: refactor NMSettingsConnectionCallId typedef not to be a pointer to struct
Typedefs to structs are fine, but a typedef for a pointer seems confusing to
me. Let's avoid it.
2017-11-27 14:04:11 +01:00
Thomas Haller
f76dbfc1a6 core/vpn: mark secret hints as const 2017-11-23 14:44:25 +01:00
Beniamino Galvani
8f677a7772 vpn: avoid adding unneeded routes when ipvx.ignore-auto-routes=yes
Instead of adding routes and then let nm_ipx_config_merge_setting()
remove them, don't add them in the first place when
ipvx.ignore-auto-routes=yes.
2017-11-17 13:36:31 +01:00
Thomas Haller
6df212ccb1 vpn: avoid coverity warning in print_vpn_config()
coverity thinks that @address4 might be NULL. Maybe it can. We
have an nm_assert(), but to be sure, check the value.
2017-11-15 18:31:25 +01:00
Beniamino Galvani
b16c853bef vpn: consider the never-default connection property
After commit 5c299454b4 ("core: rework tracking of
gateway/default-route in ip-config") NM set a default route for VPNs
only based on the "never-default" option reported by the plugin. It
should also consider the connection setting.

Fixes: 5c299454b4

https://bugzilla.redhat.com/show_bug.cgi?id=1505886
2017-10-25 09:01:25 +02:00
Thomas Haller
3434261811 core,clients: use our own string hashing function nm_str_hash()
Replace the usage of g_str_hash() with our own nm_str_hash().

GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.

Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.

This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.

At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
2017-10-18 13:05:00 +02:00
Thomas Haller
5c299454b4 core: rework tracking of gateway/default-route in ip-config
Instead of having 3 properties @gateway, @never_default and @has_gateway
on NMIP4Config/NMIP6Config that determine the default-route, track the
default-route as a regular route.

The gateway setting is the configuration knob for the default-route.
Since an NMIP4Config/NMIP6Config instance only has one gateway property,
it cannot track more then one default-routes (see related bug rh#1445417).
Especially with policy routing, it might be interesting to configure a
default-route in multiple tables.

Also, later it might be interesting to allow adding default-routes as
regular static routes in a connection, so that the user can configure additional
route parameters for the default-route or add default-routes in multiple tables.

With this patch, default-routes now have a rt_source property according to their
origin.

Also, the previous commits of this branch broke handling of the
default-route :) . That should be working now again.
2017-10-10 08:46:47 +02:00
Thomas Haller
9003dae6cd core: don't track route MSS in ip-config
The MSS is only set for VPN connections (by merging it in the respective
NMIP4Config/NMIP6Config).

It is also only used when setting the MSS of the default route.

Don't track that property in NMIP4Config/NMIP6Config, instead, set the
mss of the route directly in nm_vpn_connection_ip4_config_get() and
nm_vpn_connection_ip6_config_get().

There is a potential change in behavior here: NMDevice also consisdered
the MSS for the default route, but that would only be set if the MSS
gets merged from an vpn-ip-config. Which at most is the case for
iterface-less VPN types (libreswan). But even in that case, it doesn't
seem right to me to use the VPN's MSS for the device's default-route.
2017-10-09 22:06:25 +02:00
Thomas Haller
2e14614870 core: use ipv6.route-table setting for other IPv6 routes
Including device-routes, default-route, SLAAC.
2017-10-09 22:06:25 +02:00
Thomas Haller
01930c96b8 core: use ipv4.route-table setting for other IPv4 routes
Including device-routes, default-route, DHCPv4, IPv4LL.
2017-10-09 22:06:25 +02:00
Thomas Haller
cc1ee1d286 all: rework configuring route table support by adding "route-table" setting
We added "ipv4.route-table-sync" and "ipv6.route-table-sync" to not change
behavior for users that configured policy routing outside of NetworkManager,
for example, via a dispatcher script. Users had to explicitly opt-in
for NetworkManager to fully manage all routing tables.

These settings were awkward. Replace them with new settings "ipv4.route-table"
and "ipv6.route-table". Note that this commit breaks API/ABI on the unstable
development branch by removing recently added API.

As before, a connection will have no route-table set by default. This
has the meaning that policy-routing is not enabled and only the main table
will be fully synced. Once the user sets a table, we recognize that and
NetworkManager manages all routing tables.

The new route-table setting has other important uses: analog to
"ipv4.route-metric", it is the default that applies to all routes.
Currently it only works for static routes, not DHCP, SLAAC,
default-route, etc. That will be implemented later.

For static routes, each route still can explicitly set a table, and
overwrite the per-connection setting in "ipv4.route-table" and
"ipv6.route-table".
2017-10-09 22:05:36 +02:00
Thomas Haller
3dd60d0ef0 device/trivial: rename nm_device_get_ip_route_metric() to nm_device_get_route_metric()
Brevity!
2017-10-06 11:13:43 +02:00
Thomas Haller
4804fb778a device: remove wrappers for nm_device_get_ip_route_metric() 2017-10-06 11:13:43 +02:00
Thomas Haller
5b0f895e19 libnm,core: add TABLE attribute for routes settings
https://bugzilla.redhat.com/show_bug.cgi?id=1436531
2017-09-26 19:39:36 +02:00
Thomas Haller
03e1cc96a5 core: fix handling IPv6 device-route and use correct route metric
Before commit 6698bf58bb, we would rely on
kernel to add the device-route for manual IPv6 routes. We broke that and now
kernel would still add the device-route, however nm_platform_ip_route_sync()
would delete it immediately after.
That is because previously nm_platform_ip_route_sync() would ignore routes
with rtm_protocol RTPRO_KERNEL. Now, it will sync and delete those too.

Fix that by adding the device-route like we do it for IPv4. This also
fixes an actual issue where the automatically added route always had
route-metric 256. Instead, we now use the metric from ipv6.route-metric
setting.

Fixes: 6698bf58bb
2017-09-19 11:49:29 +02:00
Thomas Haller
2cc1813340 core: workaround configuring IPv6 routes with "src" (RTA_PREFSRC)
Kernel does not allow to add IPv6 routes with "src", as long as the
corresponding address is still tentative (related bug rh#1457196).

The workaround for this is cumbersome. First, when we fail to add such a
route with "pref_src", we guess that it happend due to this issue. In
that case, nm_ip6_config_commit() returns the list of routes that could
not be added for the moment (but hopefully can be added later).

We track this list in NMDevice, and keep trying to merge the routes
back into ip6_config. In order to not try indefinitely, keep track of a
timestamp when we tried to add this route for the first time.

Another uglyness is that pending tentative routes don't explicitly block
activation. In practice they may do, because for these routes we also have
an IPv6 address that is still doing DAD, so the IP configuration is
still pending due to that.

https://bugzilla.redhat.com/show_bug.cgi?id=1452684
2017-09-15 17:28:48 +02:00
Thomas Haller
9a3117f1d3 core: track IPv4 device routes in NMIP4Config
For IPv6, we create device routes when processing the RA and add it to
NMIP6Config like any other route. For IPv4 we didn't do that. Instead
we created the list of device routes during nm_ip4_config_commit() and
passed it to nm_platform_ip_route_sync().
2017-09-13 08:17:31 +02:00
Thomas Haller
77ec302714 core: rework handling of default-routes and drop NMDefaultRouteManager
Remove NMDefaultRouteManager. Instead, add the default-route to the
NMIP4Config/NMIP6Config instance.

This basically reverts commit e8824f6a52.
We added NMDefaultRouteManager because we used the corresponding to `ip
route replace` when configuring routes. That would replace default-routes
on other interfaces so we needed a central manager to coordinate routes.
Now, we use the corresponding of `ip route append` to configure routes,
and each interface can configure routes indepdentently.

In NMDevice, when creating the default-route, ignore @auto_method for
external devices. We shall not touch these devices.

Especially the code in NMPolicy regarding selection of the best-device
seems wrong. It probably needs further adjustments in the future.
Especially get_best_ip_config() should be replaced, because this
distinction VPN vs. devices seems wrong to me.
Thereby, remove the @ignore_never_default argument. It was added by
commit bb75026004, I don't think it's
needed anymore.

This brings another change. Now that we track default-routes in
NMIP4Config/NMIP6Config, they are also exposed on D-Bus like regular
routes. I think that makes sense, but it is a change in behavior, as
previously such routes were not exposed there.
2017-09-08 11:11:21 +02:00
Thomas Haller
96f1358eef core: return new route from _nm_ip_config_add_obj()
Later we will need the exact instance that we just added (or the previously
existing one, if the new route is already tracked).
2017-09-08 11:05:05 +02:00
Thomas Haller
10fc74819c vpn: only rely on ip route get to resolve route to external VPN gateway
Until recently, we would only consier the IP config of the parent device
to determine the route to the external VPN gateway. We changed that, to
additionally improve the guess by letting kernel resolve the route.

Now, drop checking the parent's config entirely. The only thing that
matters is the here and now runtime configuraion on the parent device.
And for that we ask kernel to resolve the route.
2017-09-07 12:00:36 +02:00
Thomas Haller
f6dabee068 vpn: improve resolving route to external VPN gateway by restricting oif
Previously, we would try to resolve the route in general (unrestricted
to a certain ifindex), and reject it the result wasn't on the parent
device.

Now, use the oif argument, and resolve the route only on the parent device.

The problem is that kernel would pretend that the destination is onlink, if
there is no route to it. Hence, hack around that by only accepting an onlink
route, if the VPN gateway itself is site-local. Yes, there are scenarios where
this will still lead to a wrong guess. See related bug rh#1489343 for kernel.
2017-09-07 11:57:44 +02:00
Thomas Haller
7046ce5332 platform: add oif argument to nm_platform_ip_route_get()
Analog to `ip route get $DST oif $IFACE`.
2017-09-07 11:14:27 +02:00
Thomas Haller
cac10198f6 vpn: apply parent config in nm_vpn_connection_apply_config() first
In practice, it shouldn't matter much, because NM may frequently
reapply the IP config. Hence, it anyway must cope with the fact that
IP config from a previous iteration is already applied on the VPN device,
before applying it to the parent device.

Anyway, it makes a bit more sense to apply it first the the parent device.
2017-09-07 11:14:27 +02:00
Thomas Haller
c8e6f3e5fb vpn: fix ifindex of parent IP config in apply_parent_device_config()
When creating the NMIP4Config/NMIP6Config instance, we must always use the right
ifindex. That is the ifindex, on which we want to apply the config. It also means,
that for device-based VPNs (those with priv->ip_ifindex set, like OpenVPN), the
parent's config must have the ip-ifindex of the parent device. Not of the
VPN's device.

One effect of this bug is that in add_ip4_vpn_gateway_route() we resolve
the route to the external gateway and only accept it if it's on the
parent device. But since the ifindex of the config was wrong, we would accept
route on the wrong interface.

https://bugzilla.gnome.org/show_bug.cgi?id=787370
2017-09-07 11:14:27 +02:00
Thomas Haller
fa7fafe8fd vpn: resolve route to external VPN gateway from kernel
When activating a route, we commonly need to add a route to the
external VPN gateway, so that the (encrypted) data is not sent over
the VPN itself.

Currently, our VPN connections are rather strongly tied to their
parent device. Maybe the shouldn't be, as VPN may happily support
changing the route from one device/IP address to another.

Anyway, our previous way to determine the gateway for the route
was not great. Instead, ask kernel how to reach the gateway via
(something like) `ip route get`. If kernel would route the packets
to the gateway via our parent device, we take that gateway.

If not, we keep our previous heuristic (which is probably wrong
in this case).
2017-08-24 10:48:03 +02:00
Thomas Haller
f0de7d347f platform: add non-exclusive routes and drop route-manager
Previously, we would add exclusive routes via netlink message flags
NLM_F_CREATE | NLM_F_REPLACE for RTM_NEWROUTE. Similar to `ip route replace`.
Using that form of RTM_NEWROUTE message, we could only add a certain
route with a certain network/plen,metric triple once. That was already
hugely inconvenient, because

 - when configuring routes, multiple (managed) interfaces may get
   conflicting routes (multihoming). Only one of the routes can be actually
   configured using `ip route replace`, so we need to track routes that are
   currently shadowed.

 - when configuring routes, we might replace externally configured
   routes on unmanaged interfaces. We should not interfere with such
   routes.

That was worked around by having NMRouteManager (and NMDefaultRouteManager).
NMRouteManager would keep a list of the routes which NetworkManager would like
to configure, even if momentarily being unable to do so due to conflicting routes.
This worked mostly well but was complicated. It involved bumping metrics to
avoid conflicts for device routes, as we might require them for gateway routes.

Drop that now. Instead, use the corresponding of `ip route append` to configure
routes. This allows NetworkManager to confiure (almost) all routes that we care.
Especially, it can configure all routes on a managed interface, without
replacing/interfering with routes on other interfaces. Hence, NMRouteManager
becomes obsolete.

It practice it is a bit more complicated because:

 - when adding an IPv4 address, kernel will automatically create a device route
   for the subnet. We should avoid that by using the IFA_F_NOPREFIXROUTE flag for
   IPv4 addresses (still to-do). But as kernel may not support that flag for IPv4
   addresses yet (and we don't require such a kernel yet), we still need functionality
   similar to nm_route_manager_ip4_route_register_device_route_purge_list().
   This functionality is now handled via nm_platform_ip4_dev_route_blacklist_set().

 - trying to configure an IPv6 route with a source address will be rejected
   by kernel as long as the address is tentative (see related bug rh#1457196).
   Preferably, NMDevice would keep the list of routes which should be configured,
   while kernel would have the list of what actually is configured. There is a
   feed-back loop where both affect each other (for example, when externally deleting
   a route, NMDevice must forget about it too). Previously, NMRouteManager would have
   the task of remembering all routes which we currently want to configure, but cannot
   due to conflicting routes.
   We get rid of that, because now we configure non-exclusive routes. We however still
   will need to remember IPv6 routes with a source address, that currently cannot be
   configured yet. Hence, we will need to keep track of routes that
   currently cannot be configured, but later may be.
   That is still not done yet, as NMRouteManager didn't handle this
   correctly either.
2017-08-24 10:48:03 +02:00
Thomas Haller
990a050aff platform: cleanup and renaming of nm_platform_address_flush() function
Rename to nm_platform_ip_address_flush(), it's more consistent with naming
for other platform functions.

Also, pass an address family argument. Sometimes I feel an option makes it clearer
what the function does. Otherwise, from the name it's not clear which address
families are affected. As an API, it feels more correct to me.

We soon also get a nm_platform_ip_route_flush() function, which will
look similar.
2017-08-23 18:37:22 +02:00
Thomas Haller
5f99512366 core: prevent invalid routes in NMIP4Config/NMIP6Config
Kernel requires that the host part of a route (based on network/plen)
is zero. Routes with non-zero host part don't really exist.

In settings (NMIPRoute), we don't enforce that. Hence we must ensure
that we don't let such invalid routes into NMIP4Config/NMIP6Config.

Also at other places where we obtain routes from untrusted sources,
we must sanitize them first.

Also add an assertion to catch such bugs.
2017-07-25 06:44:13 +02:00