1. 23 Jul, 2019 3 commits
  2. 22 Jul, 2019 2 commits
  3. 18 Jul, 2019 1 commit
  4. 17 Jul, 2019 7 commits
    • Thomas Haller's avatar
      settings: add more trace logging for auto-default (default wired) connections · 2870037b
      Thomas Haller authored
      Automatically creating profiles is suprising. Add more logging to understand
      what's happening.
      2870037b
    • Thomas Haller's avatar
      device: move check for config "no-auto-default" to NMDevice's new_default_connection() · 7644b184
      Thomas Haller authored
      Only NMDeviceEthernet implements new_default_connection(). Anyway, it
      makes only sense to do this precheck by the caller first, and not by
      each implementation.
      7644b184
    • Thomas Haller's avatar
      settings: write tombstones when deleting connection with duplicate files on disk · da72f276
      Thomas Haller authored
      Create such duplicate files:
      
        UUID=0df1bac3-1131-42d4-8893-4492d5424d11
        rm -f /etc/NetworkManager/system-connections/x[12]
        rm -f /{etc,run}/NetworkManager/system-connections/"$UUID".nmmeta
        printf -v C "[connection]\nuuid=$UUID\ntype=ethernet\nautoconnect=false"
        echo "$C" > /etc/NetworkManager/system-connections/x1
        echo "$C" > /etc/NetworkManager/system-connections/x2
        chmod 600 /etc/NetworkManager/system-connections/x[12]
        touch /etc/NetworkManager/system-connections/x2
        ls -l --full-time /etc/NetworkManager/system-connections/x[12] /{etc,run}/NetworkManager/system-connections/"$UUID".nmmeta 2>/dev/null
        nmcli connection reload
        nmcli -f all connection show | grep $UUID
      
      Now, we have x2 file loaded, and x1 is shadowed. When we delete x2,
      we probably don't want to delete the hidden x1 file.
      
      What previously happend was that when calling
      
        nmcli connection delete $UUID
      
      the command would hang because the profile wasn't really deleted:
      
        <trace> [1563355597.3671] keyfile: commit: deleted "/etc/NetworkManager/system-connections/x2", profile 0df1bac3-1131-42d4-8893-4492d5424d11 (deleted from disk)
        <trace> [1563355597.3672] settings: storage[0df1bac3-1131-42d4-8893-4492d5424d11,91e13003dd84928f/keyfile]: change event for dropping profile (file "/etc/NetworkManager/system-connections/x2")
        <trace> [1563355597.3672] settings: update[0df1bac3-1131-42d4-8893-4492d5424d11]: updating connection "x1" (2b798d30d43b0daf/keyfile)
        <debug> [1563355597.3674] ++ connection 'update connection' (0x55a167693ee0/NMSimpleConnection/"802-3-ethernet" < 0x55a16762e580/NMSimpleConnection/"802-3-ethernet") [/org/freedesktop/NetworkManager/Settings/41]:
        <debug> [1563355597.3675] ++ connection                [ 0x55a16782a400 < 0x55a16762c350 ]
        <debug> [1563355597.3675] ++ connection.id             = 'x1' < 'x2'
        <info>  [1563355597.3680] audit: op="connection-delete" uuid="0df1bac3-1131-42d4-8893-4492d5424d11" name="x1" pid=32077 uid=0 result="success"
      
      instead, we need to write a tombstone:
      
        <trace> [1563359300.2910] keyfile: commit: deleted "/etc/NetworkManager/system-connections/x2", profile 0df1bac3-1131-42d4-8893-4492d5424d11 (deleted from disk)
        <trace> [1563359300.2911] settings: storage[0df1bac3-1131-42d4-8893-4492d5424d11,0c12620295ac7f83/keyfile]: change event for dropping profile (file "/etc/NetworkManager/system-connections/>
        <trace> [1563359300.2912] keyfile: commit: writing nmmeta symlink "/etc/NetworkManager/system-connections/0df1bac3-1131-42d4-8893-4492d5424d11.nmmeta" (pointing to "/dev/null") succeeded
        <trace> [1563359300.2912] settings: storage[0df1bac3-1131-42d4-8893-4492d5424d11,02a430e6ee52358d/keyfile]: change event for hiding profile (file "/etc/NetworkManager/system-connections/0d>
        <trace> [1563359300.2912] settings: update[0df1bac3-1131-42d4-8893-4492d5424d11]: delete connection "x2" (02a430e6ee52358d/keyfile)
        <debug> [1563359300.2914] Deleting secrets for connection /org/freedesktop/NetworkManager/Settings (x2)
        <trace> [1563359300.2915] dbus-object[13d79ec95177f9eb]: unexport: "/org/freedesktop/NetworkManager/Settings/54"
        <trace> [1563359300.2916] settings-connection[13d79ec95177f9eb,0df1bac3-1131-42d4-8893-4492d5424d11]: update settings-connection flags to none (was visible)
        <info>  [1563359300.2917] audit: op="connection-delete" uuid="0df1bac3-1131-42d4-8893-4492d5424d11" name="x2" pid=22572 uid=0 result="success"
        <debug> [1563359300.2918] settings-connection[13d79ec95177f9eb,0df1bac3-1131-42d4-8893-4492d5424d11]: disposing
      
      and of course after a `nmcli connection reload` the profile stays hidden:
      
        <trace> [1563359412.0355] settings: storage[0df1bac3-1131-42d4-8893-4492d5424d11,e45535721abb092a/keyfile]: change event with connection "x1" (file "/etc/NetworkManager/system-connections/x1")
        <trace> [1563359412.0355] settings: storage[0df1bac3-1131-42d4-8893-4492d5424d11,02a430e6ee52358d/keyfile]: change event for hiding profile (file "/etc/NetworkManager/system-connections/0df1bac3-1131-42d4-8893-4492d5424d11.nmmeta")
      da72f276
    • Thomas Haller's avatar
      settings: add nm_settings_plugin_cmp_by_priority() function · 1735a0a8
      Thomas Haller authored
      Initially I thought I would use this somewhere else. Didn't do so far,
      but this seems a useful function to have on its own because also
      NMSettings is concerned about the relative priority of plugins.
      1735a0a8
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      settings: fix wrong assertion in keyfiles _storages_consolidate() · bc29389e
      Thomas Haller authored
      The storage may also contain a tombstone, and have no connection to steal.
      bc29389e
    • Thomas Haller's avatar
  5. 16 Jul, 2019 14 commits
    • Thomas Haller's avatar
      settings: rework tracking settings connections and settings plugins · d35d3c46
      Thomas Haller authored
      Completely rework how settings plugin handle connections and how
      NMSettings tracks the list of connections.
      
      Previously, settings plugins would return objects of (a subtype of) type
      NMSettingsConnection. The NMSettingsConnection was tightly coupled with
      the settings plugin. That has a lot of downsides.
      
      Change that. When changing this basic relation how settings connections
      are tracked, everything falls appart. That's why this is a huge change.
      Also, since I have to largely rewrite the settings plugins, I also
      added support for multiple keyfile directories, handle in-memory
      connections only by keyfile plugin and (partly) use copy-on-write NMConnection
      instances. I don't want to spend effort rewriting large parts while
      preserving the old way, that anyway should change. E.g. while rewriting ifcfg-rh,
      I don't want to let it handle in-memory connections because that's not right
      long-term.
      
      --
      
      If the settings plugins themself create subtypes of NMSettingsConnection
      instances, then a lot of knowledge about tracking connections moves
      to the plugins.
      Just try to follow the code what happend during nm_settings_add_connection().
      Note how the logic is spread out:
       - nm_settings_add_connection() calls plugin's add_connection()
       - add_connection() creates a NMSettingsConnection subtype
       - the plugin has to know that it's called during add-connection and
         not emit NM_SETTINGS_PLUGIN_CONNECTION_ADDED signal
       - NMSettings calls claim_connection() which hocks up the new
         NMSettingsConnection instance and configures the instance
         (like calling nm_settings_connection_added()).
      This summary does not sound like a lot, but try to follow that code. The logic
      is all over the place.
      
      Instead, settings plugins should have a very simple API for adding, modifying,
      deleting, loading and reloading connections. All the plugin does is to return a
      NMSettingsStorage handle. The storage instance is a handle to identify a profile
      in storage (e.g. a particular file). The settings plugin is free to subtype
      NMSettingsStorage, but it's not necessary.
      There are no more events raised, and the settings plugin implements the small
      API in a straightforward manner.
      NMSettings now drives all of this. Even NMSettingsConnection has now
      very little concern about how it's tracked and delegates only to NMSettings.
      
      This should make settings plugins simpler. Currently settings plugins
      are so cumbersome to implement, that we avoid having them. It should not be
      like that and it should be easy, beneficial and lightweight to create a new
      settings plugin.
      
      Note also how the settings plugins no longer care about duplicate UUIDs.
      Duplicated UUIDs are a fact of life and NMSettings must handle them. No
      need to overly concern settings plugins with that.
      
      --
      
      NMSettingsConnection is exposed directly on D-Bus (being a subtype of
      NMDBusObject) but it was also a GObject type provided by the settings
      plugin. Hence, it was not possible to migrate a profile from one plugin to
      another.
      However that would be useful when one profile does not support a
      connection type (like ifcfg-rh not supporting VPN). Currently such
      migration is not implemented except for migrating them to/from keyfile's
      run directory. The problem is that migrating profiles in general is
      complicated but in some cases it is important to do.
      
      For example checkpoint rollback should recreate the profile in the right
      settings plugin, not just add it to persistent storage. This is not yet
      properly implemented.
      
      --
      
      Previously, both keyfile and ifcfg-rh plugin implemented in-memory (unsaved)
      profiles, while ifupdown plugin cannot handle them. That meant duplication of code
      and a ifupdown profile could not be modified or made unsaved.
      This is now unified and only keyfile plugin handles in-memory profiles (bgo #744711).
      Also, NMSettings is aware of such profiles and treats them specially.
      In particular, NMSettings drives the migration between persistent and non-persistent
      storage.
      
      Note that a settings plugins may create truly generated, in-memory profiles.
      The settings plugin is free to generate and persist the profiles in any way it
      wishes. But the concept of "unsaved" profiles is now something explicitly handled
      by keyfile plugin. Also, these "unsaved" keyfile profiles are persisted to file system
      too, to the /run directory. This is great for two reasons: first of all, all
      profiles from keyfile storage in fact have a backing file -- even the
      unsaved ones. It also means you can create "unsaved" profiles in /run
      and load them with `nmcli connection load`, meaning there is a file
      based API for creating unsaved profiles.
      The other advantage is that these profiles now survive restarting
      NetworkManager. It's paramount that restarting the daemon is as
      non-disruptive as possible. Persisting unsaved files to /run improves
      here significantly.
      
      --
      
      In the past, NMSettingsConnection also implemented NMConnection interface.
      That was already changed a while ago and instead users call now
      nm_settings_connection_get_connection() to delegate to a
      NMSimpleConnection. What however still happened was that the NMConnection
      instance gets never swapped but instead the instance was modified with
      nm_connection_replace_settings_from_connection(), clear-secrets, etc.
      Change that and treat the NMConnection instance immutable. Instead of modifying
      it, reference/clone a new instance. This changes that previously when somebody
      wanted to keep a reference to an NMConnection, then the profile would be cloned.
      Now, it is supposed to be safe to reference the instance directly and everybody
      must ensure not to modify the instance. nmtst_connection_assert_unchanging()
      should help with that.
      The point is that the settings plugins may keep references to the
      NMConnection instance, and so does the NMSettingsConnection. We want
      to avoid cloning the instances as long as they are the same.
      Likewise, the device's applied connection can now also be referenced
      instead of cloning it. This is not yet done, and possibly there are
      further improvements possible.
      
      --
      
      Also implement multiple keyfile directores /usr/lib, /etc, /run (rh #1674545,
      bgo #772414).
      
      It was always the case that multiple files could provide the same UUID
      (both in case of keyfile and ifcfg-rh). For keyfile plugin, if a profile in
      read-only storage in /usr/lib gets modified, then it gets actually stored in
      /etc (or /run, if the profile is unsaved).
      
      --
      
      While at it, make /etc/network/interfaces profiles for ifupdown plugin reloadable.
      
      --
      
      https://bugzilla.gnome.org/show_bug.cgi?id=772414
      https://bugzilla.gnome.org/show_bug.cgi?id=744711
      https://bugzilla.redhat.com/show_bug.cgi?id=1674545
      d35d3c46
    • Thomas Haller's avatar
      settings/trivial: rename nm_keyfile_loaded_uuid_*() API to nm_keyfile_nmmeta_*() · 0631129c
      Thomas Haller authored
      The file got a wider scope to contain generic meta data about profiles.
      Rename the internal API to reflect that (and be consistend with the
      naming of the files).
      0631129c
    • Thomas Haller's avatar
      settings: change filename for per-connection metadata (previously UUID nm-loaded symlinks) · 5ce589a7
      Thomas Haller authored
      We may want to store meta-data for a profile to disk. The immediate
      need are "tombstones": markers that the particular UUID is shadowed
      and the profile does not exist (despite being in read-only location).
      
      Change the filename of these symlinks from
      
        ".loaded-${UUID}.nmconnection"
      
      to
      
        "${UUID}.nmmeta"
      
      The leading dot is not desirable as tools tend to hide such files.
      Use a different scheme for the filename that does not have the leading dot.
      Note that nm_keyfile_utils_ignore_filename() would also ignore ".nmmeta"
      as not a valid keyfile. This is just what we want, and influences the
      choice of this file suffix.
      
      Also, "nmmeta" is a better name, because this name alludes that there is
      a wider use for the file: namely to have addtional per-profile metadata.
      That is regardless that the upcoming first use will be only to store symlinks
      to "/dev/null" to indicate the tombstones.
      
      Note that per-profile metadata is not new. Currently we write the files
      
        /var/lib/NetworkManager/{seen-bssids,timestamps}
      
      that have a similar purpose. Maybe the content from these files could one
      day be migrated to the ".nmmeta" file. The naming scheme would make it
      suitable.
      5ce589a7
    • Thomas Haller's avatar
      settings/keyfile: output "struct stat" from nms_keyfile_loaded_uuid_read() · 050f6151
      Thomas Haller authored
      We already stat() the file, so optionally return the stat result to the
      caller.
      050f6151
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      core: reapply changes to profile to all devices · b52b51e3
      Thomas Haller authored
      Profiles can now be "connection.multi-connect" multiple, so we should
      look at all devices.
      b52b51e3
    • Thomas Haller's avatar
      libnm: accept %NULL argument in nmtst_connection_assert_unchanging() · a33e602a
      Thomas Haller authored
      It's just more convenient, as it saves us the %NULL check.
      a33e602a
    • Thomas Haller's avatar
      355390fa
    • Thomas Haller's avatar
      shared: suppress -Werror=stringop-overflow= warning in nm_strndup_a() · d5ad315f
      Thomas Haller authored
      nm_strndup_a() uses strncpy() because we want the behavior of clearing out
      the memory after the first NUL byte. But that can cause a compiler warning:
      
          CC       src/settings/plugins/keyfile/libNetworkManager_la-nms-keyfile-utils.lo
        In file included from ../../shared/nm-default.h:279,
                         from ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:20:
        In function ‘_nm_strndup_a_step’,
            inlined from ‘nms_keyfile_loaded_uuid_is_filename’ at ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:65:9:
        ../../shared/nm-glib-aux/nm-macros-internal.h:1661:3: error: ‘strncpy’ specified bound depends on the length of the source argument [-Werror=stringop-overflow=]
         1661 |   strncpy (s, str, len);
              |   ^~~~~~~~~~~~~~~~~~~~~
        ../../src/settings/plugins/keyfile/nms-keyfile-utils.c: In function ‘nms_keyfile_loaded_uuid_is_filename’:
        ../../src/settings/plugins/keyfile/nms-keyfile-utils.c:48:8: note: length computed here
           48 |  len = strlen (filename);
              |        ^~~~~~~~~~~~~~~~~
      
      It's true that the len argument of _nm_strndup_a_step() depends on the
      string length of the source string. But in this case it's safe, because
      we checked that the destination buffer is exactly the right size too.
      By that reasoning we should use memcpy() or strcpy(), but both are
      unsuitable. That is because we want nm_strndup_a() to behave like
      strndup(), which means we need to handle cases where the len argument
      is larger than the string length of the source string. That is, we want
      always to return a buffer of size len+1, but we want to copy only the
      characters up to the first NUL byte, and clear out the rest. That's what
      strncpy() does for us.
      
      Silence the warning.
      d5ad315f
    • Thomas Haller's avatar
      device: fix reapplying changes to connection ID and UUID · adb51c2a
      Thomas Haller authored
      4 properties are not really relevant for an already activated connection
      or it makes not sense to change them. These are connection.id, connection.uuid,
      connection.autoconnect and connection.stable-id.
      
      For convenience, we allow to reapply these. This way, one can take
      a different setting (e.g. with a different connection.id or
      connection.uuid) and reapply them, but such changes are silently
      ignored.
      
      However this was done wrongly. Instead of reverting the change to the new
      applied connection, we would change the input connection.
      
      This is bad, for example with
      
        nmcli connection up uuid cb922f18-e99a-49c6-b200-1678b5070a82
        nmcli connection modify cb922f18-e99a-49c6-b200-1678b5070a82 con-name "bogus"
        nmcli device reapply eth0
      
      the last re-apply would reset the settings-connection's connection ID to
      what was before, while accepting the new name on the applied-connection
      (while it should have been rejected).
      
      Fixes: bf3b3d44 ('device: avoid changing immutable properties during reapply')
      adb51c2a
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      policy-routing: take ownership of externally configured rules · 15b13044
      Thomas Haller authored
      IP addresses, routes, TC and QDiscs are all tied to a certain interface.
      So when NetworkManager manages an interface, it can be confident that
      all related entires should be managed, deleted and modified by NetworkManager.
      
      Routing policy rules are global. For that we have NMPRulesManager which
      keeps track of whether NetworkManager owns a rule. This allows multiple
      connection profiles to specify the same rule, and NMPRulesManager can
      consolidate this information to know whether to add or remove the rule.
      
      NMPRulesManager would also support to explicitly block a rule by
      tracking it with negative priority. However that is still unused at
      the moment. All that devices do is to add rules (track with positive
      priority) and remove them (untrack) once the profile gets deactivated.
      
      As rules are not exclusively owned by NetworkManager, NetworkManager
      tries not to interfere with rules that it knows nothing about. That
      means in particular, when NetworkManager starts it will "weakly track"
      all rules that are present. "weakly track" is mostly interesting for two
      cases:
      
        - when NMPRulesManager had the same rule explicitly tracked (added) by a
          device, then deactivating the device will leave the rule in place.
      
        - when NMPRulesManager had the same rule explicitly blocked (tracked
          with negative priority), then it would restore the rule when that
          block gets removed (as said, currently nobody actually does this).
      
      Note that when restarting NetworkManager, then the device may stay and
      the rules kept. However after restart, NetworkManager no longer knows
      that it previously added this route, so it would weakly track it and
      never remove them again.
      
      That is a problem. Avoid that, by whenever explicitly tracking a rule we
      also make sure to no longer weakly track it. Most likely this rule was
      indeed previously managed by NetworkManager. If this was really a rule
      added by externally, then the user really should choose distinct
      rule priorities to avoid such conflicts altogether.
      15b13044
    • Thomas Haller's avatar
      libnm,core: add support for "suppress_prefixlength" rule attribute · 6ea56bc0
      Thomas Haller authored
      WireGuard's wq-quick configures such rules to avoid routing loops.
      While we currently don't have an automatic solution for this, at least
      we should support it via explicit user configuration.
      
      One problem is that suppress_prefixlength is relatively new and kernel
      might not support this attribute. That can lead to odd results, because
      the NetworkManager is valid but it cannot be configured on the current
      kernel. But this is a general problem, and we would require a general
      solution. The solution cannot be to only support rule attributes that
      are supported by the oldest possible kernel. It's not clear how much of
      a problem there really is, or which general solution is required (if
      any).
      6ea56bc0
    • Thomas Haller's avatar
      libnm: accept special table names for policy-routing · 70b23c79
      Thomas Haller authored
      The tables "main", "local", and "default" have well known names.
      Accept them as aliases when parsing the string representation of
      the rule.
      
      Note that iproute2 also considers /etc/iproute2/rt_tables for table
      names. In particular, that allows a user to re-map the well-known names
      like "main" to a different table. We never honor that file, and "main"
      always means table 254.
      
      Note that this only affects how we parse the string representation for
      rules. As the representation is neither unique nor enforced to be normalized,
      being more graceful here is no problem.
      
      The point is of course that the user possibly has existing iproute2
      scripts that use such keyword. This makes it simpler to copy & paste
      the rule.
      70b23c79
  6. 15 Jul, 2019 5 commits
  7. 12 Jul, 2019 2 commits
  8. 11 Jul, 2019 1 commit
  9. 10 Jul, 2019 5 commits