1. 15 Aug, 2019 3 commits
  2. 13 Aug, 2019 10 commits
    • Thomas Haller's avatar
      cli: don't require "ifname" when adding connection · 02e5a8d1
      Thomas Haller authored
        $ nmcli connection add type ethernet con-name t autoconnect no
        Error: ifname argument is required.
      This reverts commit a91eafdf ('cli: 'con add': make ifname mandatory
      (except bond,bridge,vlan) (bgo #698113)'). Apparently ifname argument was
      required to avoid confusion (unexpected behavior). But I don't agree
      that is an issue, it's just annoying. Often you really have just one
      ethernet or Wi-Fi device, so this does not seem helpful.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      all: allow configuring default-routes as manual, static routes · c167e014
      Thomas Haller authored
      Up until now, a default-route (with prefix length zero) could not
      be configured directly. The user could only set ipv4.gateway,
      ipv4.never-default, ipv4.route-metric and ipv4.route-table to influence
      the setting of the default-route (respectively for IPv6).
      That is a problematic limitation. For one, whether a route has prefix
      length zero or non-zero does not make a fundamental difference. Also,
      it makes it impossible to configure all the routing attributes that one can
      configure otherwise for static routes. For example, the default-route could
      not be configured as "onlink", could not have a special MTU, nor could it be
      placed in a dedicated routing table.
      Fix that by lifting the restriction. Note that "ipv4.never-default" does
      not apply to /0 manual routes. Likewise, the previous manners of
      configuring default-routes ("ipv4.gateway") don't conflict with manual
      Server-side this all the pieces are already in place to accept a default-route
      as static routes. This was done by earlier commits like 5c299454
      ('core: rework tracking of gateway/default-route in ip-config').
      A long time ago, NMIPRoute would assert that the prefix length is
      positive. That was relaxed by commit a2e93f2d ('libnm: allow zero
      prefix length for NMIPRoute'), already before 1.0.0. Using libnm from
      before 1.0.0 would result in assertion failures.
      Note that the default-route-metric-penalty based on connectivity
      checking applies to all /0 routes, even these static routes. Be they
      added due to DHCP, "ipv4.gateway", "ipv4.routes" or "wireguard.peer-routes".
      I wonder whether doing that unconditionally is desirable, and maybe
      there should be a way to opt-out/opt-in for the entire profile or even
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      libnm: set errno in nm_key_file_get_boolean() to distinguish between missing key and error · cc7b2cde
      Thomas Haller authored
      This is also what nm_keyfile_plugin_kf_get_int64() does. It's useful to
      know whether a value was missing or invalid.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      dhcp: minor refactoring to switch default IPv4 DHCP plugin to "nettools" with one-line change · 75503c85
      Thomas Haller authored
      Minor refactoring so that there is only a one-line change necessary to
      flip the implementation of the "internal" DHCP plugin for IPv4 from
      "systemd" to "nettools".
      We don't do that yet, because there are still some issues (e.g. the
      lease is not persisted for nettools plugin). Eventually we want to
      switch, so prepare the code to be almost there.
    • Thomas Haller's avatar
      dhcp: make "systemd" DHCP plugin configurable · b53e2614
      Thomas Haller authored
      We have the "internal" DHCP plugin. That's our preferred plugin,
      and eventually we may drop all other plugins.
      Currently, the "internal" plugin is based on code from systemd-networkd
      and implemented in "src/dhcp/nm-dhcp-systemd.c". As this code is forked
      we eventually want to switch to nettools' n-dhcp4 library (for IPv4).
      For that reason we already have "src/dhcp/nm-dhcp-nettools.c".
      Note that "nettools" can be configured as a DHCP plugin, but this configuration
      is only experimental and for testing. There is never supposed to be a
      "nettools" plugin, but eventually the "internal" plugin will switch
      We don't want to replace systemd-based implementation right away. Not until
      we are sure that nettools works well. For that reason we keep them
      both in parallel for a while.
      This commit makes "systemd" DHCP plugin explicitly configurable
      in NetworkManager.conf. Like "nettools" this is an undocumented option,
      only for testing.
      If you choose "internal" (the default), you get one of the
      implementations (currently the "systemd" one). But by selecting
      "systemd" or "nettools" explicitly, you can select the exact plugin.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      dhcp: cleanup selecting GType from DHCP client factory · b32cf718
      Thomas Haller authored
      Instead of returning a client-factory, return the GType right
  3. 12 Aug, 2019 9 commits
  4. 10 Aug, 2019 2 commits
  5. 09 Aug, 2019 1 commit
  6. 08 Aug, 2019 14 commits
    • Thomas Haller's avatar
      settings: return errno from nms_keyfile_nmmeta_write() for better logging · 4e36521d
      Thomas Haller authored
      I encountered a failure in the log
          <trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" failed
          <trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" simulated
      I think that was due to SELinux (rh #1738010).
      Let nms_keyfile_nmmeta_write() return an errno code so we can log
      more information about the failure.
    • Thomas Haller's avatar
      shared,all: return boolean success from nm_utils_file_get_contents() · b216abb0
      Thomas Haller authored
      ... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
      Don't mix negative errno return value with a GError output. Instead,
      return a boolean result indicating success or failure.
      Also, optionally
        - output GError
        - set out_errsv to the positive errno (or 0 on success)
      Obviously, the return value and the output arguments (contents, length,
      out_errsv, error) must all agree in their success/failure result.
      That means, you may check any of the return value, out_errsv, error, and
      contents to reliably detect failure or success.
      Also note that out_errsv gives the positive(!) errno. But you probably
      shouldn't care about the distinction and use nm_errno_native() either
      way to normalize the value.
    • Thomas Haller's avatar
      shared: let nm_utils_file_set_contents() return a errno error code · 1bad3506
      Thomas Haller authored
      nm_utils_file_set_contents() is a re-implementation of g_file_set_contents(),
      as such it returned merely a boolean success value.
      It's sometimes interesting to get the native error code. Let the function
      deviate from glib's original g_file_set_contents() and return the error code
      (as negative value) instead.
      This requires all callers to change. Also, it's potentially a dangerous
      change, as this is easy to miss.
      Note that nm_utils_file_get_contents() also returns an errno, and
      already deviates from g_file_get_contents() in the same way. This patch
      resolves at least the inconsistency with nm_utils_file_get_contents().
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      secret-agent: rework secret-agent to better handle service shutdown · f6624659
      Thomas Haller authored
      The secret-agent D-Bus API knows 4 methods: GetSecrets, SaveSecrets,
      DeleteSecrets and CancelGetSecrets. When we cancel a GetSecrets
      request, we must issue another CancelGetSecrets to tell the agent
      that the request was aborted. This is also true during shutdown.
      Well, technically, during shutdown we anyway drop off the bus and
      it woudn't matter. In practice, I think we should get this right and
      always cancel properly.
      To better handle shutdown change the following:
      - each request now takes a reference on NMSecretAgent. That means,
        as long as there are pending requests, the instance stays alive.
        The way to get this right during shutdown, is that NMSecretAgent
        registers itself via nm_shutdown_wait_obj_register() and
        NetworkManager is supposed to keep running as long as requests
        are keeping the instance alive.
      - now, the 3 regular methods are cancellable (which means: we are
        no longer interested in the result). CancelGetSecrets is not
        cancellable, but it has a short timeout NM_SHUTDOWN_TIMEOUT_MS
        to handle this. We anyway don't really care about the result,
        aside logging and to be sure that the request fully completed.
      - this means, a request (NMSecretAgentCallId) can now immediately
        be cancelled and destroyed, both when the request returns and
        when the caller cancels it. The exception is GetSecrets which
        keeps the request alive while waiting for CancelGetSecrets. But
        this is easily handled by unlinking the call-id and pass it on
        to the CancelGetSecrets callback.
        Previously, the NMSecretAgentCallId was only destroyed when
        the D-Bus call returns, even if it was cancelled earlier. That's
        unnecessary complicated.
      - previously, D-Bus requests SaveSecrets and DeleteSecrets were not cancellable.
        That is a problem. We need to be able to cancel them in order to shutdown in
      - use GDBusConnection instead of GDBusProxy. As most of the time, GDBusProxy
        provides features we don't use.
      - again, don't log direct pointer values, but obfuscate the indentifiers.
    • Thomas Haller's avatar
      secret-agent: use NMCListElem to track permissions in NMSecretAgent · 52f9c8ec
      Thomas Haller authored
      I don't like GSList.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      secret-agent: avoid log plain pointer values · a010484c
      Thomas Haller authored
      This defeats ASLR. Obfuscate the pointers.
    • Thomas Haller's avatar
      dbus-manager: drop unused private-socket functions from "nm-dbus-manager.c" · 0dbb870f
      Thomas Haller authored
      These functions are now unused. Drop them.
      Also, if we ever reintroduce private unix socket, we sure won't use
      GDBusProxy. Good riddance.
    • Thomas Haller's avatar
      secret-agent: drop unused private-socket code from secret-agent · 8a347dbd
      Thomas Haller authored
      In the past, we had a private unix socket. That is long gone.
      Drop the remains in "nm-secret-agent.c". The request here really
      always comes from the main D-Bus connection.
      Maybe the private unix socket makes sense and we might resurrect it one
      day. But at that point it would be an entire rewrite and the existing
      code is probably not useful either way. Drop it.
    • Thomas Haller's avatar
      secret-agent: enable trace log messages · 58e5e55f
      Thomas Haller authored
      They seem useful for debugging. Don't only enable them --with-more-logging.
    • Thomas Haller's avatar
      shared: add nm_c_list_elem_find_first() helper macro · dda32892
      Thomas Haller authored
      - add nm_c_list_elem_find_first() macro that takes a predicate
        and returns the first match.
        This macro has a non-function-like behavior, which we often try to
        avoid because macros should behave like functions. In this case it's
        however convenient, so let's do it.
        Also, despite being non-function-like, it should be pretty hard to
        use wrongly.
      - rename nm_c_list_elem_find_first() to nm_c_list_elem_find_first_ptr().
    • Thomas Haller's avatar
      n-dhcp4: allocate memory of right size in n_dhcp4_client_probe_option_new() · b80b2505
      Thomas Haller authored
      Non-critical, as the allocated memory was larger than needed.
  7. 07 Aug, 2019 1 commit
    • Thomas Haller's avatar
      firewall: refactor "nm-firewall-manager.c" to not use GDBusProxy · 1b59d752
      Thomas Haller authored
      - Don't use GDBusProxy but plain GDBusConnection. NMFirewallManager
        is very simple, it doesn't use any of the features that GDBusProxy
      - make NMFirewallManagerCallId typedef a pointer to the opaque call-id
        struct, instead of the struct itself. It's confusing to have a
        variable that does not look like a pointer and assigning %NULL to
      - internally drop the CBInfo typename and name the call-id variable
        constsistantly as "call_id".
      - no need to keep the call-id struct alive after cancelling it. That
        simplifies the lifetime managment of the pending call because the
        completion callback is always invoked shortly before destroying
        the call-id.
      - note that the caller is no longer allowed to cancel a call-id from
        inside the completion callback. That just complicates the
        implementation and is not necessary. Assert against that.