1. 18 Apr, 2019 2 commits
    • Thomas Haller's avatar
      shared: move most of "shared/nm-utils" to "shared/nm-glib-aux" · 80db06f7
      Thomas Haller authored
      From the files under "shared/nm-utils" we build an internal library
      that provides glib-based helper utilities.
      
      Move the files of that basic library to a new subdirectory
      "shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
      to "libnm-glib-aux.la".
      
      Reasons:
      
       - the name "utils" is overused in our code-base. Everything's an
         "utils". Give this thing a more distinct name.
      
       - there were additional files under "shared/nm-utils", which are not
         part of this internal library "libnm-utils-base.la". All the files
         that are part of this library should be together in the same
         directory, but files that are not, should not be there.
      
       - the new name should better convey what this library is and what is isn't:
         it's a set of utilities and helper functions that extend glib with
         funcitonality that we commonly need.
      
      There are still some files left under "shared/nm-utils". They have less
      a unifying propose to be in their own directory, so I leave them there
      for now. But at least they are separate from "shared/nm-glib-aux",
      which has a very clear purpose.
      80db06f7
    • Thomas Haller's avatar
      shared: split C-only helper "shared/nm-std-aux" utils out of "shared/nm-utils" · b434b9ec
      Thomas Haller authored
      "shared/nm-utils" contains general purpose utility functions that only
      depend on glib (and extend glib with some helper functions).
      
      We will also add code that does not use glib, hence it would be good
      if the part of "shared/nm-utils" that does not depend on glib, could be
      used by these future projects.
      
      Also, we use the term "utils" everywhere. While that covers the purpose
      and content well, having everything called "nm-something-utils" is not
      great. Instead, call this "nm-std-aux", inspired by "c-util/c-stdaux".
      b434b9ec
  2. 10 Apr, 2019 5 commits
  3. 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.
      b1f6d53b
  4. 21 Feb, 2019 1 commit
  5. 20 Feb, 2019 2 commits
  6. 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), ...
      
        over
      
            g_string_append_printf (str, "%s%s", str->len ? " " : "", ...
      
      - use @addr_str buffer directly, instead of assigning to another
        temporary variable.
      1d0b07bc
    • 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 "0.0.0.0" 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 "0.0.0.0". 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
          "0.0.0.0".
      
          [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.
      
      https://github.com/systemd/systemd/commit/f8862395e8f802e4106a07ceaaf02b6a1faa5a6d
      39ac79c5
  7. 12 Feb, 2019 3 commits
  8. 06 Feb, 2019 2 commits
  9. 24 Jan, 2019 1 commit
  10. 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
      "/etc/dhcp".
      
      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:
      
          [connection-internal-dhcp-client-id-duid]
          match-device=dhcp-plugin:internal
          ipv4.dhcp-client-id=duid
      
      The reason to change from the previous default "duid" to "mac" are the
      following:
      
      - "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
      it.
      
      - 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
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1661165
      cfd696cc
    • 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.
      3bce451c
    • Thomas Haller's avatar
  11. 20 Dec, 2018 1 commit
  12. 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,
      really.
      
      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).
      3102b49f
    • 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.
      
      Changes:
      
      - 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).
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1634657
      9a6a3540
    • 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
      sd_dhcp_client_set_request_option().
      
      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
      added.
      
      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.
      2f2b489d
    • Thomas Haller's avatar
      dhcp: minor cleanup parsing default route for internal client · b05ebd54
      Thomas Haller authored
      Combine same code.
      b05ebd54
    • 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.
      3f99d01c
    • 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.
      4aa7285d
    • 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).
      d11572ac
    • 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
        sd_dhcp6_client_set_request_option().
      a057d8c3
    • 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:
      
        - SD_DHCP6_OPTION_DNS_SERVERS
        - SD_DHCP6_OPTION_DOMAIN_LIST
        - SD_DHCP6_OPTION_SNTP_SERVERS
        - SD_DHCP6_OPTION_NTP_SERVER
        - SD_DHCP6_OPTION_RAPID_COMMIT
      
      As such, SD_DHCP6_OPTION_CLIENTID is not accepted and requesting it
      was silently ignored.
      
      Fixes: d2dd3b2c
      fed16ff1
    • 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
      22e276a0
    • 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.
      a51c09dc
  13. 14 Dec, 2018 1 commit
  14. 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.
      f2f44a7a
  15. 19 Nov, 2018 1 commit
  16. 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.
      787f4b57
  17. 13 Nov, 2018 2 commits