1. 23 Feb, 2019 1 commit
    • Thomas Haller's avatar
      build/meson: increase timeouts for some tests · b1f6d53b
      Thomas Haller authored
      The defaults for test timeouts in meson is 30 seconds. That is not long
      enough when running
        $ NMTST_USE_VALGRIND=1 ninja -C build test
      Note that meson supports --timeout-multiplier, and automatically
      increases the timeout when running under valgrind. However, meson
      does not understand that we are running tests under valgrind via
      NMTST_USE_VALGRIND=1 environment variable.
      Timeouts are really not expected to be reached and are a mean of last
      resort. Hence, increasing the timeout to a large value is likely to
      have no effect or to fix test failures where the timeout was too rigid.
      It's unlikely that the test indeed hangs and the increase of timeout
      causes a unnecessary increase of waittime before aborting.
  2. 21 Feb, 2019 1 commit
  3. 20 Feb, 2019 2 commits
  4. 19 Feb, 2019 2 commits
    • Thomas Haller's avatar
      dhcp/internal: cleanup logging and failure handling in lease_to_ip4_config() · 1d0b07bc
      Thomas Haller authored
      ... and lease_to_ip6_config().
      - Handle reasons that render the lease invalid first, before logging
        anything. This way, upon invalid lease we don't have partially logged
        about the lease.
      - prefer logging one line for options that contain multiple values, for
        example for search domains.
      - reorder statements to consistently log first before calling add_option().
      - prefer
            g_string_append (nm_gstring_add_space_delimiter (str), ...
            g_string_append_printf (str, "%s%s", str->len ? " " : "", ...
      - use @addr_str buffer directly, instead of assigning to another
        temporary variable.
    • Thomas Haller's avatar
      systemd: dhcp: handle multiple addresses for "Router" (option 3) in DHCP library · 39ac79c5
      Thomas Haller authored
      Imported from systemd:
          The Router DHCP option may contain a list of one or more
          routers ([1]). Extend the API of sd_dhcp_lease to return a
          list instead of only the first.
          Note that networkd still only uses the first router (if present).
          Aside from extending the internal API of the DHCP client, there
          is almost no change in behavior. The only visible difference in
          behavior is that the "ROUTER" variable in the lease file is now a
          list of addresses.
          Note how RFC 2132 does not define certain IP addresses as invalid for the
          router option. Still, previously sd_dhcp_lease_get_router() would never
          return a "" address. In fact, the previous API could not
          differenciate whether no router option was present, whether it
          was invalid, or whether its first router was "". No longer let
          the DHCP client library impose additional restrictions that are not
          part of RFC. Instead, the caller should handle this. The patch does
          that, and networkd only consideres the first router entry if it is not
          [1] https://tools.ietf.org/html/rfc2132#section-3.5
      This also required adjusting "src/dhcp/nm-dhcp-systemd.c" due to the
      changed internal API.
  5. 12 Feb, 2019 3 commits
  6. 06 Feb, 2019 2 commits
  7. 24 Jan, 2019 1 commit
  8. 07 Jan, 2019 3 commits
    • Thomas Haller's avatar
      dhcp: default ipv4.dhcp-client-id of internal plugin to "mac" · cfd696cc
      Thomas Haller authored
      The "ipv4.dhcp-client-id" is configurable per-profile and the default
      value can be overwritten via connection defaults in NetworkManager.conf.
      For "dhclient" DHCP plugin, the ultimate default for "ipv4.dhcp-client-id"
      is determined by dhclient itself, or possibly by user configuration from
      For the "internal" DHCP plugin, the default must be decided by
      NetworkManager. Also, the default here is important, as we preferably
      won't change it anymore. That is because a changing the client-id
      will result in different IP addresses after upgrade of NetworkManager
      version. That should be avoided.
      Still, change it now. If a downstream does not want this, it either needs
      to patch the sources or add a configuration snippet like:
      The reason to change from the previous default "duid" to "mac" are the
      - "duid" is an RFC 4361 compatible client-id ([1]) and "mac" is defined
      in RFC 2132.
      - "duid" cannot (easily) be predicated a-priori as it is a hash of the
      interface-name and "/etc/machine-id". In particular in cloud and server
      environments, admins often prefer "mac" as they do know the MAC address
      and pre-configure the DHCP server accordingly.
      - with "dhclient" plugin, the default is decided by dhclient package or
      user configuration in "/etc/dhcp". However, in fact the default is often
      "client-identifier hardware" (for example on RHEL/CentOS).
      - for RHEL/CentOS we require a way to select "mac" as default. That was
      done by installing a configuration snippet via the NetworkManager-config-server
      package. It's confusing to have the default depend on a package. Avoid
      that. Also, users required "mac" in certain scenarios, but no users
      explicitly asked for "duid" as default.
      - our "duid" implementation generates a 32 bit IAID based on a hash of the
      interface-name, and only 8 bytes entropy that contains a hash
      of "/etc/machine-id". The point is, that is not a lot of entropy to
      avoid conflicting client-ids. Another point is, that the choosen algorithm
      for "duid" is suitable for RFC 4361, but it's only one of many possibly
      implementations. Granted, each possibility has up and downsides but selecting
      one of them as default seems wrong (given that it has obvious downsides
      already). For "mac" there is only one straight-forward way to implement
      - RFC 7844 (Anonymity Profiles for DHCP Clients) is not yet supported by
      NetworkManager. But we should not select a default client-id which
      counteracts anonymit. Choosing "mac" does not reveal information which
      is not already exposed.
      [1] https://tools.ietf.org/html/rfc4361#section-4
    • Thomas Haller's avatar
      core/trivial: rename nm_utils_detect_arp_type_from_addrlen() to... · 3bce451c
      Thomas Haller authored
      core/trivial: rename nm_utils_detect_arp_type_from_addrlen() to nm_utils_arp_type_detect_from_hwaddrlen()
      Rename the function so that the function name's prefix is
      the topic what this is about: arp-type.
    • Thomas Haller's avatar
  9. 20 Dec, 2018 1 commit
  10. 19 Dec, 2018 11 commits
    • Thomas Haller's avatar
      core: allow addresses with zero prefix length · 3102b49f
      Thomas Haller authored
      There is really no problem here, allow it.
      Previously we would assert against a non-zero prefix length.
      But I am not sure that all callers really ensured that this
      couldn't happen. Anyway, there is no problem we such addresses,
      Only we need to make sure that nm_ip4_config_add_dependent_routes()
      and nm_ip6_config_add_dependent_routes() don't add prefix routes for
      such addresses (which is the case now).
    • Thomas Haller's avatar
      dhcp: fix static-route handling for intenal client and support multiple default routes · 9a6a3540
      Thomas Haller authored
      Preface: RFC 3442 (The Classless Static Route Option for Dynamic Host
      Configuration Protocol (DHCP) version 4) states:
         If the DHCP server returns both a Classless Static Routes option and
         a Router option, the DHCP client MUST ignore the Router option.
         Similarly, if the DHCP server returns both a Classless Static Routes
         option and a Static Routes option, the DHCP client MUST ignore the
         Static Routes option.
      - sd_dhcp_lease_get_routes() returns the combination of both option 33
      (static routes) and 121 (classless static routes). If classless static
      routes are provided, the state routes must be ignored.
      - we collect the options hash that we expose on D-Bus. For that purpose,
      we must not merge both option types as classless static routes. Instead,
      we want to expose the values like we received them originally: as two
      different options.
      - we continue our deviation from RFC 3442, when receiving classless static
      routes with option 3 (Router), we only ignore the router if we didn't
      already receive a default route via classless static routes.
      - in the past, NetworkManager treated the default route specially, and
      one device could only have one default route. That limitation was
      already (partly) lifted by commit 5c299454
      (core: rework tracking of gateway/default-route in ip-config). However,
      from DHCP we still would only accept one default route. Fix that for
      internal client. Installing multiple default routes might make sense, as
      kernel apparently can skip unreachable routers (as it notes via ICMP
      messages) (rh#1634657).
    • Thomas Haller's avatar
      dhcp: request classless-static-route option first according to RFC 3442 · 2f2b489d
      Thomas Haller authored
      In ip4_start(), we iterate over @dhcp4_requests array and add the
      options that are to be included. We do so, by calling
      Note that sd_dhcp_client_set_request_option() only appends the options
      to a list, not taking special care about the order in which options are
      RFC 3442 (The Classless Static Route Option for Dynamic Host Configuration
      Protocol (DHCP) version 4) says:
         DHCP clients that support this option and send a parameter request
         list MAY also request the Static Routes option, for compatibility
         with older servers that don't support Classless Static Routes.  The
         Classless Static Routes option code MUST appear in the parameter
         request list prior to both the Router option code and the Static
         Routes option code, if present.
      Compare to RFC 2132 (DHCP Options and BOOTP Vendor Extensions) which says
      about the parameter request list:
         The client MAY list the options in order of preference.
      Note, with RFC 7844 (Anonymity Profiles for DHCP Clients), the order
      should be randomized. But since we don't follow RFC 7844 (yet), let's follow
      at least RFC 3442.
    • Thomas Haller's avatar
      dhcp: minor cleanup parsing default route for internal client · b05ebd54
      Thomas Haller authored
      Combine same code.
    • Thomas Haller's avatar
      dhcp: cleanup parsing of DHCP lease for internal client · 3f99d01c
      Thomas Haller authored
      - check errors when accessing the lease. Some errors, like a failure
        from sd_dhcp_lease_get_address() are fatal.
      - while parsing the individual options, don't incrementally build the
        NMPlatformIP4Address instance (and similar). Instead, parse the
        options to individual variales, and only package them as platform
        structure at the point where they are needed. It makes the code simpler,
        because all individual bits (like "r_plen" variable) are only
        initialized and used at one place. That is clearer than incrementally
        building a platform structure, where you have to follow the code to
        see how the structure mutates.
      - drop redundant comments that only serve as a visual separator
        for structuring the code. Instead, structure the code.
    • Thomas Haller's avatar
      dhcp: let lease_to_ip4_config() allocate option hash · 4aa7285d
      Thomas Haller authored
      lease_to_ip4_config() can fail, if the lease is broken. As such, a function
      that fails should not modifiy an in/out parameter. Avoid that, by not
      having the caller pre-allocate the options hash, but instead allocate it
      by the lease_to_ip*_config() functions, and return it only on success.
    • Thomas Haller's avatar
      dhcp: fix signedness of loop variable in lease_to_ip4_config() · d11572ac
      Thomas Haller authored
      The loop variable should have the same type as the variable
      that holds the number of elements ("num", in this case).
    • Thomas Haller's avatar
      dhcp: cleanup static option list for internal client · a057d8c3
      Thomas Haller authored
      - use proper data types "guint16" and "bool" in static
        option list. It saves a few bytes, but also it's the appropriate
        type. Well, at least, it's the appropriate type for DHCPv6,
        not for DHCPv4 (which is guint8).
      - assert against failure of sd_dhcp_client_set_request_option() and
    • Thomas Haller's avatar
      dhcp: don't request DHCP6 client-id option with internal client · fed16ff1
      Thomas Haller authored
      sd_dhcp6_client_set_request_option() only accepts a white-listed
      set of options. Unexpected options are rejected with -EINVAL.
      Currently supported are only:
      As such, SD_DHCP6_OPTION_CLIENTID is not accepted and requesting it
      was silently ignored.
      Fixes: d2dd3b2c
    • Thomas Haller's avatar
      dhcp: cleanup error paths in bound4_handle() and bound6_handle() · 22e276a0
      Thomas Haller authored
      - return-early on error
      - use cleanup attribute
    • Thomas Haller's avatar
      all: don't use static buffer for nm_utils_inet*_ntop() · a51c09dc
      Thomas Haller authored
      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.
  11. 14 Dec, 2018 1 commit
  12. 29 Nov, 2018 1 commit
    • Thomas Haller's avatar
      dhcp: always explicitly set IAID of internal DHCPv6 client · f2f44a7a
      Thomas Haller authored
      During start, sd_dhcp6_client would call client_ensure_iaid(),
      to initialize the IAID.
      First of all, the IAID is important to us, we should not leave the
      used IAID up to an implementation detail. In the future, we possibly
      want to make this configurable.
      The problem is that client_ensure_iaid() calls dhcp_identifier_set_iaid()
      which looks up the interface name via if_indextoname() (in our fork).
      The problem is that dhcp_identifier_set_iaid() looks up information by
      quering the system, which makes it hard for testing and also makes it
      unpredictable. It's better to use our implementation nm_utils_create_dhcp_iaid(),
      which is solely based on the interface-name, which is given as a well
      defined parameter to the DHCP client instance.
  13. 19 Nov, 2018 1 commit
  14. 14 Nov, 2018 1 commit
    • Thomas Haller's avatar
      dhcp: initialize hostname as construct-property · 787f4b57
      Thomas Haller authored
      The hostname property is only initialized once, early on during
      start. Move the initialization even earlier during object constructions.
      This effectively makes the hostname an immutable property.
      This also makes sense, because the hostname is used by IPv4 and
      IPv6 DHCP instances alike.
  15. 13 Nov, 2018 9 commits
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      dhcp: always require hwaddr in internal DHCP clint ip6_start() · dfdd4e3b
      Thomas Haller authored
      Note how client_start() in NMDhcpManager already asserts
      that we have a MAC address. It's always present, like
      for the IPv6 case.
    • Thomas Haller's avatar
      dhcp: cleanup error handling in internal DHCP client's start · ab314065
      Thomas Haller authored
      - use nm_auto to return early when something goes wrong
      - don't modify NMDhcpClient's state until the end, when it looks
        like we are (almost) started successfully.
      - for IPv4, only attempt to load the lease if we actually are
        interested in the address. Also, reduce the scope of the lease
        variable, to the one place where we need it.
    • Thomas Haller's avatar
      dhcp: don't load IPv4 client-id from lease file · 5b9bc174
      Thomas Haller authored
      The client-id is something that we want to determine top-down.
      Meaning, if the user specifies it via ipv4.dhcp-client-id, then it
      should be used. If the user leaves it unspecified, we choose a
      default stable client-id. For the internal DHCP plugin, this is
      a node specific client-id based on
        - the predictable interface name
        - and /etc/machine-id
      It's not clear, why we should allow specifying the client-id in
      the lease file as a third source of configuration. It really pushes
      the configuration first down (when we do DHCP without lease file),
      to store an additional bit of configuration for future DHCP attempts.
      If the machine-id or the interface-name changes, then so does the
      default client-id. In this case, also "ipv4.dhcp-client-id=stable"
      changes. It's fair to require that the user keeps the machine-id
      stable, if the machine identity doesn't change.
      Also, the lease files are stored in /var/lib/NetworkManager, which
      is more volatile than /etc/machine-id. So, if we think that machine-id
      and interface-name is not stable, why would we assume that we have
      a suitable lease file?
      Also, if you do:
         nmcli connection add con-name "$PROFILE" ... ipv4.dhcp-client-id ''
         nmcli connection up $PROFILE
         nmcli connection modify "$PROFILE" ipv4.dhcp-client-id mac
         nmcli connection up $PROFILE
         nmcli connection modify "$PROFILE" ipv4.dhcp-client-id ''
         nmcli connection up $PROFILE
      wouldn't you expect that the original (default) client-id is used again?
      Also, this works badly with global connection defaults in
      NetworkManager.conf. If you configure a connection default, previously
      already this would always force the client-id and overrule the lease.
      That is reasonable, but in which case would you ever want to use
      the client-id from the lease?
    • Thomas Haller's avatar
      dhcp: cleanup initializing IPv4 client-id for internal DHCP · c3e7e617
      Thomas Haller authored
      - if we leave the client-id of sd_dhcp_client unset, it will
        anyway generate a node-specific client-id (and may fail if
        "/etc/machine-id" is invalid).
        Anticipate that, and don't let the client-id unset. In case
        we have no client-id from configuration or lease, just generate
        the id ourself (using the same algorithm). The advantage is,
        that we know it upfront and can store the client-id in the
        NMDhcpClient instance. We no longer need to peel it out from
        the lease later.
      - to generate the IPv4 client-id, we need a valid MAC address. Also,
        sd_dhcp_client needs a MAC address for dhcp_network_bind_raw_socket()
        as well. Just require that a MAC address is always needed. Likewise,
        we need a valid ifindex and ifname set.
      - likewise for IPv6 and IPv4, cleanup detecting the arptype and
        checking MAC address length. sd_dhcp_client_set_mac() is overly
        strict at asserting input arguments, so we must validate them anyway.
      - also, now that we always initialize the client-id when starting
        the DHCP client, there is no need to retroactively extract it
        again when we receive the first lease.
    • Thomas Haller's avatar
      dhcp/trivial: wrap lines in calling client_start() · ce1cfd72
      Thomas Haller authored
      A possible issue is that client_start() has about 136 arguments.
      It doesn't get simpler by saving lines of code and writing them
      all in the same line.
      Wrap the lines.
      While at it, use "FALSE" for "enforce_duid" argument, instead of "0".
      It's a boolean.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      dhcp: don't pass duid to client ip6_start() and stop() · 7d55b134
      Thomas Haller authored
      We don't do that for ip4_start() either. The duid/client-id
      is stored inside the NMDhcpClient instance, and the function can
      access it from there.
      Maybe, it is often preferable to have stateless objects and not
      relying on ip4_start() to obtain the client ID from the client's
      state. However, the purpose of the NMDhcpClient object is to
      hold state about DHCP. To simplify the complexity of objects that
      inherrently have state, we should be careful about mutating the state.
      It adds little additional complexity of only reading the state when
      needed anyway. In fact, it adds complexity, because previously
      it wasn't enough to check all callers of nm_dhcp_client_get_client_id()
      to see where the client-id is used. Instead, one would also need to
      follow the @duid argument several layers of the call stack.