From 907968ae848445e4ea6ba13209df3e9e36a5555f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Nov 2018 09:25:55 +0100 Subject: [PATCH 01/27] shared: use atomic operation for accessing global hash seed - fix thread-safety by adding a memory barrier (g_atomic_pointer_get()) to the double-checked locking pattern when initializing the hash key. - generate the random data outside the lock. Calling nm_utils_random_bytes() within the lock is ugly, because we don't want to assume that the function has no side effects which are prone to dead-lock. There is no problem attempting to generate the random data without lock, and only use it when the race is won. (cherry picked from commit 80220024cc8793759fedaaffd383958bb9db44c0) --- shared/nm-utils/nm-hash-utils.c | 53 ++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/shared/nm-utils/nm-hash-utils.c b/shared/nm-utils/nm-hash-utils.c index 4bc12b7c3a..9f164a119e 100644 --- a/shared/nm-utils/nm-hash-utils.c +++ b/shared/nm-utils/nm-hash-utils.c @@ -40,53 +40,66 @@ static const guint8 *volatile global_seed = NULL; static const guint8 * _get_hash_key_init (void) { + static gsize g_lock; /* the returned hash is aligned to guin64, hence, it is safe * to use it as guint* or guint64* pointer. */ static union { guint8 v8[HASH_KEY_SIZE]; } g_arr _nm_alignas (guint64); - static gsize g_lock; const guint8 *g; - CSipHash siph_state; - uint64_t h; - guint *p; + union { + guint8 v8[HASH_KEY_SIZE]; + guint vuint; + } t_arr; - g = global_seed; +again: + g = g_atomic_pointer_get (&global_seed); if (G_LIKELY (g != NULL)) { nm_assert (g == g_arr.v8); return g; } - if (g_once_init_enter (&g_lock)) { + { + CSipHash siph_state; + uint64_t h; - nm_utils_random_bytes (g_arr.v8, sizeof (g_arr.v8)); + /* initialize a random key in t_arr. */ + + nm_utils_random_bytes (&t_arr, sizeof (t_arr)); /* use siphash() of the key-size, to mangle the first guint. Otherwise, * the first guint has only the entropy that nm_utils_random_bytes() - * generated for the first 4 bytes and relies on a good random generator. */ - c_siphash_init (&siph_state, g_arr.v8); - c_siphash_append (&siph_state, g_arr.v8, sizeof (g_arr.v8)); + * generated for the first 4 bytes and relies on a good random generator. + * + * The first int is especially intersting for nm_hash_static() below, and we + * want to have it all the entropy of t_arr. */ + c_siphash_init (&siph_state, t_arr.v8); + c_siphash_append (&siph_state, (const guint8 *) &t_arr, sizeof (t_arr)); h = c_siphash_finalize (&siph_state); - p = (guint *) g_arr.v8; if (sizeof (guint) < sizeof (h)) - *p = *p ^ ((guint) (h & 0xFFFFFFFFu)) ^ ((guint) (h >> 32)); + t_arr.vuint = t_arr.vuint ^ ((guint) (h & 0xFFFFFFFFu)) ^ ((guint) (h >> 32)); else - *p = *p ^ ((guint) (h & 0xFFFFFFFFu)); - - g_atomic_pointer_compare_and_exchange (&global_seed, NULL, g_arr.v8); - g_once_init_leave (&g_lock, 1); + t_arr.vuint = t_arr.vuint ^ ((guint) (h & 0xFFFFFFFFu)); } - nm_assert (global_seed == g_arr.v8); - return g_arr.v8; + if (!g_once_init_enter (&g_lock)) { + /* lost a race. The random key is already initialized. */ + goto again; + } + + memcpy (g_arr.v8, t_arr.v8, HASH_KEY_SIZE); + g = g_arr.v8; + g_atomic_pointer_set (&global_seed, g); + g_once_init_leave (&g_lock, 1); + return g; } #define _get_hash_key() \ ({ \ const guint8 *_g; \ \ - _g = global_seed; \ - if (G_UNLIKELY (_g == NULL)) \ + _g = g_atomic_pointer_get (&global_seed); \ + if (G_UNLIKELY (!_g)) \ _g = _get_hash_key_init (); \ _g; \ }) From b4626ba2e7959ce1cc3fc491788ea14128cfb876 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 1 Nov 2018 11:37:57 +0100 Subject: [PATCH 02/27] shared: add nm_utils_checksum_get_digest*() helper The GChecksum API is cumbersome to use. For example, g_checksum_get_digest() requires a length input/output argument. At the same time, GChecksum does not allow you to query its checksum-type nor the desired digest-length. When you have a GChecksum at hand, you must always know the digest-length you are going to use. So, the length parameter is only good for asserting. Add a macro to make that more convenient. Benefits: it's less lines of code, and we always do all the asserts that are due. (cherry picked from commit 3746845204b77a80dacbd7a0d7272e7ca30d1e3c) --- shared/nm-utils/nm-shared-utils.h | 40 +++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 3ea887b468..03f52ef51c 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -379,6 +379,46 @@ char **_nm_utils_strv_cleanup (char **strv, /*****************************************************************************/ +#define NM_UTILS_CHECKSUM_LENGTH_MD5 16 +#define NM_UTILS_CHECKSUM_LENGTH_SHA1 20 +#define NM_UTILS_CHECKSUM_LENGTH_SHA256 32 + +#define nm_utils_checksum_get_digest(sum, arr) \ + G_STMT_START { \ + GChecksum *const _sum = (sum); \ + gsize _len; \ + \ + G_STATIC_ASSERT_EXPR ( sizeof (arr) == NM_UTILS_CHECKSUM_LENGTH_MD5 \ + || sizeof (arr) == NM_UTILS_CHECKSUM_LENGTH_SHA1 \ + || sizeof (arr) == NM_UTILS_CHECKSUM_LENGTH_SHA256); \ + G_STATIC_ASSERT_EXPR (sizeof (arr) == G_N_ELEMENTS (arr)); \ + \ + nm_assert (_sum); \ + \ + _len = G_N_ELEMENTS (arr); \ + \ + g_checksum_get_digest (_sum, (arr), &_len); \ + nm_assert (_len == G_N_ELEMENTS (arr)); \ + } G_STMT_END + +#define nm_utils_checksum_get_digest_len(sum, buf, len) \ + G_STMT_START { \ + GChecksum *const _sum = (sum); \ + const gsize _len0 = (len); \ + gsize _len; \ + \ + nm_assert (NM_IN_SET (_len0, NM_UTILS_CHECKSUM_LENGTH_MD5, \ + NM_UTILS_CHECKSUM_LENGTH_SHA1, \ + NM_UTILS_CHECKSUM_LENGTH_SHA256)); \ + nm_assert (_sum); \ + \ + _len = _len0; \ + g_checksum_get_digest (_sum, (buf), &_len); \ + nm_assert (_len == _len0); \ + } G_STMT_END + +/*****************************************************************************/ + guint32 _nm_utils_ip4_prefix_to_netmask (guint32 prefix); guint32 _nm_utils_ip4_get_default_prefix (guint32 ip); From d4c9401780f92b458053bb9d9006cc0c0db1f1a5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 1 Nov 2018 12:52:38 +0100 Subject: [PATCH 03/27] all: cleanup GChecksum handling - prefer nm_auto_free_checksum over explicit free. - use nm_utils_checksum_get_digest*(). - prefer defines for digest length. - assume g_checksum_new() cannot fail. (cherry picked from commit eb9f950a330212d4780971d269423b0d90c51c5c) --- libnm-core/nm-crypto.c | 15 +++----- libnm-core/nm-utils.c | 14 ++++---- src/dns/nm-dns-manager.c | 10 +++--- src/nm-core-utils.c | 51 ++++++--------------------- src/nm-ip4-config.c | 24 ++++--------- src/nm-ip6-config.c | 24 ++++--------- src/supplicant/nm-supplicant-config.c | 33 ++++++++--------- 7 files changed, 55 insertions(+), 116 deletions(-) diff --git a/libnm-core/nm-crypto.c b/libnm-core/nm-crypto.c index dc8f1c1538..1af2ea2840 100644 --- a/libnm-core/nm-crypto.c +++ b/libnm-core/nm-crypto.c @@ -879,13 +879,10 @@ nm_crypto_md5_hash (const guint8 *salt, gsize buflen) { nm_auto_free_checksum GChecksum *ctx = NULL; -#define MD5_DIGEST_LEN 16 - nm_auto_clear_static_secret_ptr const NMSecretPtr digest = NM_SECRET_PTR_STATIC (MD5_DIGEST_LEN); + nm_auto_clear_static_secret_ptr const NMSecretPtr digest = NM_SECRET_PTR_STATIC (NM_UTILS_CHECKSUM_LENGTH_MD5); gsize bufidx = 0; int i; - nm_assert (g_checksum_type_get_length (G_CHECKSUM_MD5) == MD5_DIGEST_LEN); - g_return_if_fail (password_len == 0 || password); g_return_if_fail (buffer); g_return_if_fail (buflen > 0); @@ -894,25 +891,21 @@ nm_crypto_md5_hash (const guint8 *salt, ctx = g_checksum_new (G_CHECKSUM_MD5); for (;;) { - gsize digest_len; - if (password_len > 0) g_checksum_update (ctx, (const guchar *) password, password_len); if (salt_len > 0) g_checksum_update (ctx, (const guchar *) salt, salt_len); - digest_len = MD5_DIGEST_LEN; - g_checksum_get_digest (ctx, digest.bin, &digest_len); - nm_assert (digest_len == MD5_DIGEST_LEN); + nm_utils_checksum_get_digest_len (ctx, digest.bin, NM_UTILS_CHECKSUM_LENGTH_MD5); - for (i = 0; i < MD5_DIGEST_LEN; i++) { + for (i = 0; i < NM_UTILS_CHECKSUM_LENGTH_MD5; i++) { if (bufidx >= buflen) return; buffer[bufidx++] = digest.bin[i]; } g_checksum_reset (ctx); - g_checksum_update (ctx, digest.ptr, MD5_DIGEST_LEN); + g_checksum_update (ctx, digest.ptr, NM_UTILS_CHECKSUM_LENGTH_MD5); } } diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 16f3d66366..3f8a322acf 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -2918,18 +2918,18 @@ nm_utils_uuid_generate_from_string_bin (NMUuid *uuid, const char *s, gssize slen sizeof (*uuid)); } else { nm_auto_free_checksum GChecksum *sum = NULL; - guint8 buf[20]; - gsize len; + union { + guint8 sha1[NM_UTILS_CHECKSUM_LENGTH_SHA1]; + NMUuid uuid; + } digest; sum = g_checksum_new (G_CHECKSUM_SHA1); g_checksum_update (sum, (guchar *) &ns_uuid, sizeof (ns_uuid)); g_checksum_update (sum, (guchar *) s, slen); - len = sizeof (buf); - g_checksum_get_digest (sum, buf, &len); - nm_assert (len == sizeof (buf)); + nm_utils_checksum_get_digest (sum, digest.sha1); - G_STATIC_ASSERT_EXPR (sizeof (*uuid) <= sizeof (buf)); - memcpy (uuid, buf, sizeof (*uuid)); + G_STATIC_ASSERT_EXPR (sizeof (digest.sha1) > sizeof (digest.uuid)); + *uuid = digest.uuid; } uuid->uuid[6] = (uuid->uuid[6] & 0x0F) | (uuid_type << 4); diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c index 5bffddb43d..a44ff3c5f8 100644 --- a/src/dns/nm-dns-manager.c +++ b/src/dns/nm-dns-manager.c @@ -55,7 +55,7 @@ #include "nm-dns-systemd-resolved.h" #include "nm-dns-unbound.h" -#define HASH_LEN 20 +#define HASH_LEN NM_UTILS_CHECKSUM_LENGTH_SHA1 #ifndef RESOLVCONF_PATH #define RESOLVCONF_PATH "/sbin/resolvconf" @@ -994,12 +994,11 @@ update_resolv_conf (NMDnsManager *self, static void compute_hash (NMDnsManager *self, const NMGlobalDnsConfig *global, guint8 buffer[HASH_LEN]) { - GChecksum *sum; - gsize len = HASH_LEN; + nm_auto_free_checksum GChecksum *sum = NULL; NMDnsIPConfigData *ip_data; sum = g_checksum_new (G_CHECKSUM_SHA1); - nm_assert (len == g_checksum_type_get_length (G_CHECKSUM_SHA1)); + nm_assert (HASH_LEN == g_checksum_type_get_length (G_CHECKSUM_SHA1)); if (global) nm_global_dns_config_update_checksum (global, sum); @@ -1013,8 +1012,7 @@ compute_hash (NMDnsManager *self, const NMGlobalDnsConfig *global, guint8 buffer nm_ip_config_hash (ip_data->ip_config, sum, TRUE); } - g_checksum_get_digest (sum, buffer, &len); - g_checksum_free (sum); + nm_utils_checksum_get_digest_len (sum, buffer, HASH_LEN); } static gboolean diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 2e25340bf9..4760ea3157 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2927,9 +2927,8 @@ nm_utils_stable_id_random (void) char * nm_utils_stable_id_generated_complete (const char *stable_id_generated) { - guint8 buf[20]; - GChecksum *sum; - gsize buf_size; + nm_auto_free_checksum GChecksum *sum = NULL; + guint8 buf[NM_UTILS_CHECKSUM_LENGTH_SHA1]; char *base64; /* for NM_UTILS_STABLE_TYPE_GENERATED we genererate a possibly long string @@ -2940,15 +2939,8 @@ nm_utils_stable_id_generated_complete (const char *stable_id_generated) g_return_val_if_fail (stable_id_generated, NULL); sum = g_checksum_new (G_CHECKSUM_SHA1); - nm_assert (sum); - g_checksum_update (sum, (guchar *) stable_id_generated, strlen (stable_id_generated)); - - buf_size = sizeof (buf); - g_checksum_get_digest (sum, buf, &buf_size); - nm_assert (buf_size == sizeof (buf)); - - g_checksum_free (sum); + nm_utils_checksum_get_digest (sum, buf); /* we don't care to use the sha1 sum in common hex representation. * Use instead base64, it's 27 chars (stripping the padding) vs. @@ -3132,22 +3124,14 @@ _set_stable_privacy (NMUtilsStableType stable_type, gsize key_len, GError **error) { - GChecksum *sum; - guint8 digest[32]; + nm_auto_free_checksum GChecksum *sum = NULL; + guint8 digest[NM_UTILS_CHECKSUM_LENGTH_SHA256]; guint32 tmp[2]; - gsize len = sizeof (digest); nm_assert (key_len); nm_assert (network_id); - /* Documentation suggests that this can fail. - * Maybe in case of a missing algorithm in crypto library? */ sum = g_checksum_new (G_CHECKSUM_SHA256); - if (!sum) { - g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, - "Can't create a SHA256 hash"); - return FALSE; - } key_len = MIN (key_len, G_MAXUINT32); @@ -3176,24 +3160,17 @@ _set_stable_privacy (NMUtilsStableType stable_type, tmp[1] = htonl (key_len); g_checksum_update (sum, (const guchar *) tmp, sizeof (tmp)); g_checksum_update (sum, (const guchar *) secret_key, key_len); - - g_checksum_get_digest (sum, digest, &len); - - nm_assert (len == sizeof (digest)); + nm_utils_checksum_get_digest (sum, digest); while (_is_reserved_ipv6_iid (digest)) { g_checksum_reset (sum); tmp[0] = htonl (++dad_counter); - g_checksum_update (sum, digest, len); + g_checksum_update (sum, digest, sizeof (digest)); g_checksum_update (sum, (const guchar *) &tmp[0], sizeof (tmp[0])); - g_checksum_get_digest (sum, digest, &len); - nm_assert (len == sizeof (digest)); + nm_utils_checksum_get_digest (sum, digest); } - g_checksum_free (sum); - memcpy (addr->s6_addr + 8, &digest[0], 8); - return TRUE; } @@ -3319,10 +3296,9 @@ _hw_addr_gen_stable_eth (NMUtilsStableType stable_type, const char *current_mac_address, const char *generate_mac_address_mask) { - GChecksum *sum; + nm_auto_free_checksum GChecksum *sum = NULL; guint32 tmp; - guint8 digest[32]; - gsize len = sizeof (digest); + guint8 digest[NM_UTILS_CHECKSUM_LENGTH_SHA256]; struct ether_addr bin_addr; guint8 stable_type_uint8; @@ -3330,8 +3306,6 @@ _hw_addr_gen_stable_eth (NMUtilsStableType stable_type, nm_assert (secret_key); sum = g_checksum_new (G_CHECKSUM_SHA256); - if (!sum) - return NULL; key_len = MIN (key_len, G_MAXUINT32); @@ -3345,10 +3319,7 @@ _hw_addr_gen_stable_eth (NMUtilsStableType stable_type, g_checksum_update (sum, (const guchar *) (ifname ?: ""), ifname ? (strlen (ifname) + 1) : 1); g_checksum_update (sum, (const guchar *) stable_id, strlen (stable_id) + 1); - g_checksum_get_digest (sum, digest, &len); - g_checksum_free (sum); - - g_return_val_if_fail (len == 32, NULL); + nm_utils_checksum_get_digest (sum, digest); memcpy (&bin_addr, digest, ETH_ALEN); _hw_addr_eth_complete (&bin_addr, current_mac_address, generate_mac_address_mask); diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c index 6604711c3d..ce7f7fc4c2 100644 --- a/src/nm-ip4-config.c +++ b/src/nm-ip4-config.c @@ -2996,29 +2996,19 @@ nm_ip4_config_hash (const NMIP4Config *self, GChecksum *sum, gboolean dns_only) gboolean nm_ip4_config_equal (const NMIP4Config *a, const NMIP4Config *b) { - GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); - GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); - guchar a_data[20], b_data[20]; - gsize a_len = sizeof (a_data); - gsize b_len = sizeof (b_data); - gboolean equal; + nm_auto_free_checksum GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); + nm_auto_free_checksum GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); + guint8 a_data[NM_UTILS_CHECKSUM_LENGTH_SHA1]; + guint8 b_data[NM_UTILS_CHECKSUM_LENGTH_SHA1]; if (a) nm_ip4_config_hash (a, a_checksum, FALSE); if (b) nm_ip4_config_hash (b, b_checksum, FALSE); - g_checksum_get_digest (a_checksum, a_data, &a_len); - g_checksum_get_digest (b_checksum, b_data, &b_len); - - nm_assert (a_len == sizeof (a_data)); - nm_assert (b_len == sizeof (b_data)); - equal = !memcmp (a_data, b_data, a_len); - - g_checksum_free (a_checksum); - g_checksum_free (b_checksum); - - return equal; + nm_utils_checksum_get_digest (a_checksum, a_data); + nm_utils_checksum_get_digest (b_checksum, b_data); + return !memcmp (a_data, b_data, sizeof (a_data)); } /*****************************************************************************/ diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c index a8c4aecdf2..42240e698f 100644 --- a/src/nm-ip6-config.c +++ b/src/nm-ip6-config.c @@ -2426,29 +2426,19 @@ nm_ip6_config_hash (const NMIP6Config *self, GChecksum *sum, gboolean dns_only) gboolean nm_ip6_config_equal (const NMIP6Config *a, const NMIP6Config *b) { - GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); - GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); - guchar a_data[20], b_data[20]; - gsize a_len = sizeof (a_data); - gsize b_len = sizeof (b_data); - gboolean equal; + nm_auto_free_checksum GChecksum *a_checksum = g_checksum_new (G_CHECKSUM_SHA1); + nm_auto_free_checksum GChecksum *b_checksum = g_checksum_new (G_CHECKSUM_SHA1); + guint8 a_data[NM_UTILS_CHECKSUM_LENGTH_SHA1]; + guint8 b_data[NM_UTILS_CHECKSUM_LENGTH_SHA1]; if (a) nm_ip6_config_hash (a, a_checksum, FALSE); if (b) nm_ip6_config_hash (b, b_checksum, FALSE); - g_checksum_get_digest (a_checksum, a_data, &a_len); - g_checksum_get_digest (b_checksum, b_data, &b_len); - - nm_assert (a_len == sizeof (a_data)); - nm_assert (b_len == sizeof (b_data)); - equal = !memcmp (a_data, b_data, a_len); - - g_checksum_free (a_checksum); - g_checksum_free (b_checksum); - - return equal; + nm_utils_checksum_get_digest (a_checksum, a_data); + nm_utils_checksum_get_digest (b_checksum, b_data); + return !memcmp (a_data, b_data, sizeof (a_data)); } /*****************************************************************************/ diff --git a/src/supplicant/nm-supplicant-config.c b/src/supplicant/nm-supplicant-config.c index 043b555021..bc2a9e52cd 100644 --- a/src/supplicant/nm-supplicant-config.c +++ b/src/supplicant/nm-supplicant-config.c @@ -648,31 +648,28 @@ add_string_val (NMSupplicantConfig *self, static void wep128_passphrase_hash (const char *input, - size_t input_len, - guint8 *out_digest, - size_t *out_digest_len) + gsize input_len, + guint8 *digest /* 13 bytes */) { - GChecksum *sum; + nm_auto_free_checksum GChecksum *sum = NULL; + guint8 md5[NM_UTILS_CHECKSUM_LENGTH_MD5]; guint8 data[64]; int i; - g_return_if_fail (out_digest != NULL); - g_return_if_fail (out_digest_len != NULL); - g_return_if_fail (*out_digest_len >= 16); + nm_assert (input); + nm_assert (input_len); + nm_assert (digest); /* Get at least 64 bytes by repeating the passphrase into the buffer */ for (i = 0; i < sizeof (data); i++) data[i] = input[i % input_len]; sum = g_checksum_new (G_CHECKSUM_MD5); - g_assert (sum); g_checksum_update (sum, data, sizeof (data)); - g_checksum_get_digest (sum, out_digest, out_digest_len); - g_checksum_free (sum); + nm_utils_checksum_get_digest (sum, md5); - g_assert (*out_digest_len == 16); /* WEP104 keys are 13 bytes in length (26 hex characters) */ - *out_digest_len = 13; + memcpy (digest, md5, 13); } static gboolean @@ -682,9 +679,10 @@ add_wep_key (NMSupplicantConfig *self, NMWepKeyType wep_type, GError **error) { - size_t key_len = key ? strlen (key) : 0; + gsize key_len; - if (!key || !key_len) + if ( !key + || (key_len = strlen (key)) == 0) return TRUE; if (wep_type == NM_WEP_KEY_TYPE_UNKNOWN) { @@ -723,11 +721,10 @@ add_wep_key (NMSupplicantConfig *self, return FALSE; } } else if (wep_type == NM_WEP_KEY_TYPE_PASSPHRASE) { - guint8 digest[16]; - size_t digest_len = sizeof (digest); + guint8 digest[13]; - wep128_passphrase_hash (key, key_len, digest, &digest_len); - if (!nm_supplicant_config_add_option (self, name, (const char *) digest, digest_len, "", error)) + wep128_passphrase_hash (key, key_len, digest); + if (!nm_supplicant_config_add_option (self, name, (const char *) digest, sizeof (digest), "", error)) return FALSE; } From 81024a9772cc8756546518f69c9cbf986074a88b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Nov 2018 19:01:54 +0100 Subject: [PATCH 04/27] core: minor cleanup of initializing nm_utils_get_testing() - add a commnt about thread-safety. - minor refactoring initializing the value in nm_utils_get_testing(). Instead of returning the flags we just set, go back to the begin and re-read the value (which must be initialized by now). No big difference, but feels a bit nicer to me. (cherry picked from commit e1413111a79da9554be32ea1cabc3f88c0893d8c) --- src/nm-core-utils.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 4760ea3157..9fc381655e 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -48,6 +48,17 @@ #include "nm-setting-wireless-security.h" G_STATIC_ASSERT (sizeof (NMUtilsTestFlags) <= sizeof (int)); + +/* we read _nm_utils_testing without memory barrier. This is thread-safe, + * because the static variable is initialized to zero, and only reset + * once to a non-zero value (via g_atomic_int_compare_and_exchange()). + * + * Since there is only one integer that contains the data, there is no + * caching problem reading this (atomic int) variable without + * synchronization/memory-barrier. Contrary to a double-checked locking, + * where one needs a memory barrier to read the variable and ensure + * that also the related data is coherent in cache. Here there is no + * related data. */ static int _nm_utils_testing = 0; gboolean @@ -66,6 +77,7 @@ nm_utils_get_testing () { NMUtilsTestFlags flags; +again: flags = (NMUtilsTestFlags) _nm_utils_testing; if (flags != NM_UTILS_TEST_NONE) { /* Flags already initialized. Return them. */ @@ -78,12 +90,11 @@ nm_utils_get_testing () if (g_test_initialized ()) flags |= _NM_UTILS_TEST_GENERAL; - if (g_atomic_int_compare_and_exchange (&_nm_utils_testing, 0, (int) flags)) { - /* Done. We set it. */ - return flags & NM_UTILS_TEST_ALL; - } - /* It changed in the meantime (??). Re-read the value. */ - return ((NMUtilsTestFlags) _nm_utils_testing) & NM_UTILS_TEST_ALL; + g_atomic_int_compare_and_exchange (&_nm_utils_testing, 0, (int) flags); + + /* regardless of whether we won the race of initializing _nm_utils_testing, + * go back and read the value again. It must be non-zero by now. */ + goto again; } void From 36ca7dd2c0968912c2c1432138b418e105c7c34a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 30 Oct 2018 14:07:11 +0100 Subject: [PATCH 05/27] core: refactor loading machine-id and cache it Previously, whenever we needed /etc/machine-id we would re-load it from file. The are 3 downsides of that: - the smallest downside is the runtime overhead of repeatedly reading the file and parse it. - as we read it multiple times, it may change anytime. Most code in NetworkManager does not expect or handle a change of the machine-id. Generally, the admin should make sure that the machine-id is properly initialized before NetworkManager starts, and not change it. As such, a change of the machine-id should never happen in practice. But if it would change, we would get odd behaviors. Note for example how generate_duid_from_machine_id() already cached the generated DUID and only read it once. It's better to pick the machine-id once, and rely to use the same one for the remainder of the program. If the admin wants to change the machine-id, NetworkManager must be restarted as well (in case the admin cares). Also, as we now only load it once, it makes sense to log an error (once) when we fail to read the machine-id. - previously, loading the machine-id could fail each time. And we have to somehow handle that error. It seems, the best thing what we anyway can do, is to log an error once and continue with a fake machine-id. Here we add a fake machine-id based on the secret-key or the boot-id. Now obtaining a machine-id can no longer fail and error handling is no longer necessary. Also, ensure that a machine-id of all zeros is not valid. Technically, a machine-id is not an RFC 4122 UUID. But it's the same size, so we also use NMUuid data structure for it. While at it, also refactor caching of the boot-id and the secret key. In particular, fix the thread-safety of the double-checked locking implementations. (cherry picked from commit 830831126430fbd24392dd8bebb6e2c2d50b7787) --- src/devices/nm-device-ethernet.c | 5 +- src/devices/nm-device.c | 10 +- src/dhcp/nm-dhcp-client.c | 1 - src/nm-core-utils.c | 261 ++++++++++++++++++++++--------- src/nm-core-utils.h | 12 +- src/systemd/nm-sd-utils.c | 16 ++ src/systemd/nm-sd-utils.h | 4 + src/tests/test-general.c | 50 ++++++ 8 files changed, 266 insertions(+), 93 deletions(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 0fb8fea42c..e7262683fd 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -1446,7 +1446,6 @@ new_default_connection (NMDevice *self) const char *uprop = "0"; gs_free char *defname = NULL; gs_free char *uuid = NULL; - gs_free char *machine_id = NULL; guint i, n_connections; if (nm_config_get_no_auto_default_for_device (nm_config_get (), self)) @@ -1470,12 +1469,10 @@ new_default_connection (NMDevice *self) if (!defname) return NULL; - machine_id = nm_utils_machine_id_read (); - /* Create a stable UUID. The UUID is also the Network_ID for stable-privacy addr-gen-mode, * thus when it changes we will also generate different IPv6 addresses. */ uuid = _nm_utils_uuid_generate_from_strings ("default-wired", - machine_id ?: "", + nm_utils_machine_id_str (), defname, perm_hw_addr, NULL); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index b4c0edac39..4888cc66e9 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -37,7 +37,6 @@ #include #include #include -#include #include "nm-utils/nm-dedup-multi.h" #include "nm-utils/nm-random-utils.h" @@ -8257,8 +8256,7 @@ generate_duid_uuid (guint8 *data, gsize data_len) static GBytes * generate_duid_from_machine_id (void) { - gs_free const char *machine_id_s = NULL; - uuid_t uuid; + const NMUuid *uuid; GChecksum *sum; guint8 sha256_digest[32]; gsize len = sizeof (sha256_digest); @@ -8267,13 +8265,11 @@ generate_duid_from_machine_id (void) if (global_duid) return g_bytes_ref (global_duid); - machine_id_s = nm_utils_machine_id_read (); - if (!nm_utils_machine_id_parse (machine_id_s, uuid)) - return NULL; + uuid = nm_utils_machine_id_bin (); /* Hash the machine ID so it's not leaked to the network */ sum = g_checksum_new (G_CHECKSUM_SHA256); - g_checksum_update (sum, (const guchar *) &uuid, sizeof (uuid)); + g_checksum_update (sum, (const guchar *) uuid, sizeof (*uuid)); g_checksum_get_digest (sum, sha256_digest, &len); g_checksum_free (sum); diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 143d261848..5299bdbf38 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -28,7 +28,6 @@ #include #include #include -#include #include #include "nm-utils/nm-dedup-multi.h" diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 9fc381655e..7f05b62cab 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2521,60 +2521,153 @@ nm_utils_is_specific_hostname (const char *name) /*****************************************************************************/ -gboolean -nm_utils_machine_id_parse (const char *id_str, /*uuid_t*/ guchar *out_uuid) +typedef struct { + NMUuid bin; + char _nul_sentinel; /* just for safety, if somebody accidentally uses the binary in a string context. */ + + /* depending on whether the string is packed or not (with/without hyphens), + * it's 32 or 36 characters long (plus the trailing NUL). + * + * The difference is that boot-id is a valid RFC 4211 UUID and represented + * as a 36 ascii string (with hyphens). The machine-id technically is not + * a UUID, but just a 32 byte sequence of hexchars. */ + char str[37]; + bool is_fake; +} UuidData; + +static UuidData * +_uuid_data_init (UuidData *uuid_data, + gboolean packed, + gboolean is_fake, + const NMUuid *uuid) { - int i; - guint8 v0, v1; + nm_assert (uuid_data); + nm_assert (uuid); - if (!id_str) - return FALSE; - - for (i = 0; i < 32; i++) { - if (!g_ascii_isxdigit (id_str[i])) - return FALSE; + uuid_data->bin = *uuid; + uuid_data->_nul_sentinel = '\0'; + uuid_data->is_fake = is_fake; + if (packed) { + G_STATIC_ASSERT_EXPR (sizeof (uuid_data->str) >= (sizeof (*uuid) * 2 + 1)); + _nm_utils_bin2hexstr_full (uuid, + sizeof (*uuid), + '\0', + FALSE, + uuid_data->str); + } else { + G_STATIC_ASSERT_EXPR (sizeof (uuid_data->str) >= 37); + _nm_utils_uuid_unparse (uuid, uuid_data->str); } - if (id_str[i] != '\0') - return FALSE; - - if (out_uuid) { - for (i = 0; i < 16; i++) { - v0 = g_ascii_xdigit_value (*(id_str++)); - v1 = g_ascii_xdigit_value (*(id_str++)); - out_uuid[i] = (v0 << 4) + v1; - } - } - return TRUE; + return uuid_data; } -char * -nm_utils_machine_id_read (void) +/*****************************************************************************/ + +static const UuidData * +_machine_id_get (void) { - gs_free char *contents = NULL; - int i; + static const UuidData *volatile p_uuid_data; + const UuidData *d; - /* Get the machine ID from /etc/machine-id; it's always in /etc no matter - * where our configured SYSCONFDIR is. Alternatively, it might be in - * LOCALSTATEDIR /lib/dbus/machine-id. - */ - if ( !g_file_get_contents ("/etc/machine-id", &contents, NULL, NULL) - && !g_file_get_contents (LOCALSTATEDIR "/lib/dbus/machine-id", &contents, NULL, NULL)) - return NULL; +again: + d = g_atomic_pointer_get (&p_uuid_data); + if (G_UNLIKELY (!d)) { + static gsize lock; + static UuidData uuid_data; + gs_free char *content = NULL; + gboolean is_fake = TRUE; + const char *fake_type = NULL; + NMUuid uuid; - contents = g_strstrip (contents); - - for (i = 0; i < 32; i++) { - if (!g_ascii_isxdigit (contents[i])) - return NULL; - if (contents[i] >= 'A' && contents[i] <= 'F') { - /* canonicalize to lower-case */ - contents[i] = 'a' + (contents[i] - 'A'); + /* Get the machine ID from /etc/machine-id; it's always in /etc no matter + * where our configured SYSCONFDIR is. Alternatively, it might be in + * LOCALSTATEDIR /lib/dbus/machine-id. + */ + if ( nm_utils_file_get_contents (-1, "/etc/machine-id", 100*1024, 0, &content, NULL, NULL) >= 0 + || nm_utils_file_get_contents (-1, LOCALSTATEDIR"/lib/dbus/machine-id", 100*1024, 0, &content, NULL, NULL) >= 0) { + g_strstrip (content); + if (_nm_utils_hexstr2bin_full (content, + FALSE, + FALSE, + NULL, + 16, + (guint8 *) &uuid, + sizeof (uuid), + NULL)) { + if (!nm_utils_uuid_is_null (&uuid)) { + /* an all-zero machine-id is not valid. */ + is_fake = FALSE; + } + } } - } - if (contents[i] != '\0') - return NULL; - return g_steal_pointer (&contents); + if (is_fake) { + const guint8 *seed_bin; + const char *hash_seed; + gsize seed_len; + + if (nm_utils_secret_key_get (&seed_bin, &seed_len)) { + /* we have no valid machine-id. Generate a fake one by hashing + * the secret-key. This key is commonly persisted, so it should be + * stable accross reboots (despite having a broken system without + * proper machine-id). */ + fake_type = "secret-key"; + hash_seed = "ab085f06-b629-46d1-a553-84eeba5683b6"; + } else { + /* the secret-key is not valid/persistent either. That happens when we fail + * to read/write the secret-key to disk. Fallback to boot-id. The boot-id + * itself may be fake and randomly generated ad-hoc, but that is as best + * as it gets. */ + seed_bin = (const guint8 *) nm_utils_get_boot_id_bin (); + seed_len = sizeof (NMUuid); + fake_type = "boot-id"; + hash_seed = "7ff0c8f5-5399-4901-ab63-61bf594abe8b"; + } + + /* the fake machine-id is based on secret-key/boot-id, but we hash it + * again, so that they are not literally the same. */ + nm_utils_uuid_generate_from_string_bin (&uuid, + (const char *) seed_bin, + seed_len, + NM_UTILS_UUID_TYPE_VERSION5, + (gpointer) hash_seed); + } + + if (!g_once_init_enter (&lock)) + goto again; + + d = _uuid_data_init (&uuid_data, TRUE, is_fake, &uuid); + g_atomic_pointer_set (&p_uuid_data, d); + g_once_init_leave (&lock, 1); + + if (is_fake) { + nm_log_err (LOGD_CORE, + "/etc/machine-id: no valid machine-id. Use fake one based on %s: %s", + fake_type, + d->str); + } else + nm_log_dbg (LOGD_CORE, "/etc/machine-id: %s", d->str); + } + + return d; +} + +const char * +nm_utils_machine_id_str (void) +{ + return _machine_id_get ()->str; +} + +const NMUuid * +nm_utils_machine_id_bin (void) +{ + return &_machine_id_get ()->bin; +} + +gboolean +nm_utils_machine_id_is_fake (void) +{ + return _machine_id_get ()->is_fake; } /*****************************************************************************/ @@ -2645,9 +2738,10 @@ gboolean nm_utils_secret_key_get (const guint8 **out_secret_key, gsize *out_key_len) { - static volatile const SecretKeyData *secret_key_static; + static const SecretKeyData *volatile secret_key_static; const SecretKeyData *secret_key; +again: secret_key = g_atomic_pointer_get (&secret_key_static); if (G_UNLIKELY (!secret_key)) { static gsize init_value = 0; @@ -2657,20 +2751,15 @@ nm_utils_secret_key_get (const guint8 **out_secret_key, gsize tmp_key_len; tmp_success = _secret_key_read (&tmp_secret_key, &tmp_key_len); - if (g_once_init_enter (&init_value)) { - secret_key_data.secret_key = tmp_secret_key; - secret_key_data.key_len = tmp_key_len; - secret_key_data.is_good = tmp_success; + if (!g_once_init_enter (&init_value)) + goto again; - if (g_atomic_pointer_compare_and_exchange (&secret_key_static, NULL, &secret_key_data)) { - g_steal_pointer (&tmp_secret_key); - secret_key = &secret_key_data; - } - - g_once_init_leave (&init_value, 1); - } - if (!secret_key) - secret_key = g_atomic_pointer_get (&secret_key_static); + secret_key_data.secret_key = g_steal_pointer (&tmp_secret_key); + secret_key_data.key_len = tmp_key_len; + secret_key_data.is_good = tmp_success; + secret_key = &secret_key_data; + g_atomic_pointer_set (&secret_key_static, secret_key); + g_once_init_leave (&init_value, 1); } *out_secret_key = secret_key->secret_key; @@ -2696,34 +2785,52 @@ nm_utils_secret_key_get_timestamp (void) /*****************************************************************************/ -const char * -nm_utils_get_boot_id (void) +static const UuidData * +_boot_id_get (void) { - static const char *boot_id; + static const UuidData *volatile p_boot_id; + const UuidData *d; - if (G_UNLIKELY (!boot_id)) { +again: + d = g_atomic_pointer_get (&p_boot_id); + if (G_UNLIKELY (!d)) { + static gsize lock; + static UuidData boot_id; gs_free char *contents = NULL; + NMUuid uuid; + gboolean is_fake = FALSE; nm_utils_file_get_contents (-1, "/proc/sys/kernel/random/boot_id", 0, NM_UTILS_FILE_GET_CONTENTS_FLAG_NONE, &contents, NULL, NULL); - if (contents) { - g_strstrip (contents); - if (contents[0]) { - /* clone @contents because we keep @boot_id until the program - * ends. - * nm_utils_file_get_contents() likely allocated a larger - * buffer chunk initially and (although using realloc to shrink - * the buffer) it might not be best to keep this memory - * around. */ - boot_id = g_strdup (contents); - } + if ( !contents + || !_nm_utils_uuid_parse (nm_strstrip (contents), &uuid)) { + /* generate a random UUID instead. */ + is_fake = TRUE; + _nm_utils_uuid_generate_random (&uuid); } - if (!boot_id) - boot_id = nm_utils_uuid_generate (); + + if (!g_once_init_enter (&lock)) + goto again; + + d = _uuid_data_init (&boot_id, FALSE, is_fake, &uuid); + g_atomic_pointer_set (&p_boot_id, d); + g_once_init_leave (&lock, 1); } - return boot_id; + return d; +} + +const char * +nm_utils_get_boot_id_str (void) +{ + return _boot_id_get ()->str; +} + +const NMUuid * +nm_utils_get_boot_id_bin (void) +{ + return &_boot_id_get ()->bin; } /*****************************************************************************/ @@ -3052,7 +3159,7 @@ nm_utils_stable_id_parse (const char *stable_id, if (CHECK_PREFIX ("${CONNECTION}")) _stable_id_append (str, uuid); else if (CHECK_PREFIX ("${BOOT}")) - _stable_id_append (str, bootid ?: nm_utils_get_boot_id ()); + _stable_id_append (str, bootid ?: nm_utils_get_boot_id_str ()); else if (CHECK_PREFIX ("${DEVICE}")) _stable_id_append (str, deviceid); else if (g_str_has_prefix (&stable_id[i], "${RANDOM}")) { diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 42f59cfcb5..2a92e58849 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -280,15 +280,19 @@ gboolean nm_utils_sysctl_ip_conf_is_path (int addr_family, const char *path, con gboolean nm_utils_is_specific_hostname (const char *name); -char *nm_utils_machine_id_read (void); -gboolean nm_utils_machine_id_parse (const char *id_str, /*uuid_t*/ guchar *out_uuid); +struct _NMUuid; + +const char *nm_utils_machine_id_str (void); +const struct _NMUuid *nm_utils_machine_id_bin (void); +gboolean nm_utils_machine_id_is_fake (void); + +const char *nm_utils_get_boot_id_str (void); +const struct _NMUuid *nm_utils_get_boot_id_bin (void); gboolean nm_utils_secret_key_get (const guint8 **out_secret_key, gsize *out_key_len); gint64 nm_utils_secret_key_get_timestamp (void); -const char *nm_utils_get_boot_id (void); - /* IPv6 Interface Identifier helpers */ /** diff --git a/src/systemd/nm-sd-utils.c b/src/systemd/nm-sd-utils.c index 914b4b448c..c6c4c12328 100644 --- a/src/systemd/nm-sd-utils.c +++ b/src/systemd/nm-sd-utils.c @@ -20,9 +20,12 @@ #include "nm-sd-utils.h" +#include "nm-core-internal.h" + #include "nm-sd-adapt.h" #include "path-util.h" +#include "sd-id128.h" /*****************************************************************************/ @@ -43,3 +46,16 @@ nm_sd_utils_path_startswith (const char *path, const char *prefix) { return path_startswith (path, prefix); } + +/*****************************************************************************/ + +NMUuid * +nm_sd_utils_id128_get_machine (NMUuid *out_uuid) +{ + g_assert (out_uuid); + + G_STATIC_ASSERT_EXPR (sizeof (*out_uuid) == sizeof (sd_id128_t)); + if (sd_id128_get_machine ((sd_id128_t *) out_uuid) < 0) + return NULL; + return out_uuid; +} diff --git a/src/systemd/nm-sd-utils.h b/src/systemd/nm-sd-utils.h index 8ca920e273..fb41f9fe57 100644 --- a/src/systemd/nm-sd-utils.h +++ b/src/systemd/nm-sd-utils.h @@ -29,5 +29,9 @@ const char *nm_sd_utils_path_startswith (const char *path, const char *prefix); /*****************************************************************************/ +struct _NMUuid; + +struct _NMUuid *nm_sd_utils_id128_get_machine (struct _NMUuid *out_uuid); + #endif /* __NM_SD_UTILS_H__ */ diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 91cbe09d84..f9c5975b7b 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -28,6 +28,8 @@ #include "NetworkManagerUtils.h" #include "nm-core-internal.h" +#include "nm-core-utils.h" +#include "systemd/nm-sd-utils.h" #include "dns/nm-dns-manager.h" @@ -1891,6 +1893,52 @@ test_dns_create_resolv_conf (void) /*****************************************************************************/ +static void +test_machine_id_read (void) +{ + NMUuid machine_id_sd; + const NMUuid *machine_id; + char machine_id_str[33]; + gpointer logstate; + + logstate = nmtst_logging_disable (FALSE); + /* If you run this test as root, without a valid /etc/machine-id, + * the code will try to get the secret-key (and possibly attempt + * to write it). + * + * That's especially ugly, if you run the test as root and it writes + * a new "/var/lib/NetworkManager/secret_key" file. Another reason + * not to run tests as root. */ + machine_id = nm_utils_machine_id_bin (); + nmtst_logging_reenable (logstate); + + g_assert (machine_id); + g_assert (_nm_utils_bin2hexstr_full (machine_id, + sizeof (NMUuid), + '\0', + FALSE, + machine_id_str) == machine_id_str); + g_assert (strlen (machine_id_str) == 32); + g_assert_cmpstr (machine_id_str, ==, nm_utils_machine_id_str ()); + + /* double check with systemd's implementation... */ + if (!nm_sd_utils_id128_get_machine (&machine_id_sd)) { + /* if systemd failed to read /etc/machine-id, the file likely + * is invalid. Our machine-id is fake, and we have nothing to + * compare against. */ + + /* NOTE: this test will fail, if you don't have /etc/machine-id, + * but a valid "LOCALSTATEDIR/lib/dbus/machine-id" file. + * Just don't do that. */ + g_assert (nm_utils_machine_id_is_fake ()); + } else { + g_assert (!nm_utils_machine_id_is_fake ()); + g_assert_cmpmem (&machine_id_sd, sizeof (NMUuid), machine_id, 16); + } +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -1937,6 +1985,8 @@ main (int argc, char **argv) g_test_add_func ("/general/stable-id/parse", test_stable_id_parse); g_test_add_func ("/general/stable-id/generated-complete", test_stable_id_generated_complete); + g_test_add_func ("/general/machine-id/read", test_machine_id_read); + g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf); return g_test_run (); From 0c1ee8c68eca68629a3cb48f50c579ffa4003170 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 13 Nov 2018 18:50:01 +0100 Subject: [PATCH 06/27] core: don't persist secret-key for tests Tests might access the secret-key. For CI builds we may very well build NM as root and also run unit tests. In such a situation it's bad to persist the secret key. For example, the SELinux label may be wrong, and subsequently starting NetworkManager may cause errors. Avoid persisting the secret key for tests. (cherry picked from commit 581e1c3269d420f09b9f29d2afb7269642dfb854) --- src/nm-core-utils.c | 6 ++++++ src/tests/test-general.c | 8 ++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 7f05b62cab..45fb4f455d 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -2713,6 +2713,12 @@ _secret_key_read (guint8 **out_secret_key, goto out; } + if (nm_utils_get_testing ()) { + /* for test code, we don't write the generated secret-key to disk. */ + success = FALSE; + goto out; + } + if (!nm_utils_file_set_contents (NMSTATEDIR "/secret_key", (char *) secret_key, key_len, 0077, &error)) { nm_log_warn (LOGD_CORE, "secret-key: failure to persist secret key in \"%s\" (%s) (use non-persistent key)", NMSTATEDIR "/secret_key", error->message); diff --git a/src/tests/test-general.c b/src/tests/test-general.c index f9c5975b7b..3eaf475a84 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1903,12 +1903,8 @@ test_machine_id_read (void) logstate = nmtst_logging_disable (FALSE); /* If you run this test as root, without a valid /etc/machine-id, - * the code will try to get the secret-key (and possibly attempt - * to write it). - * - * That's especially ugly, if you run the test as root and it writes - * a new "/var/lib/NetworkManager/secret_key" file. Another reason - * not to run tests as root. */ + * the code will try to get the secret-key. That is a bit ugly, + * but no real problem. */ machine_id = nm_utils_machine_id_bin (); nmtst_logging_reenable (logstate); From 10e280686e2ff5ba020717e8d3e1421ab44f0c52 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 12:39:02 +0100 Subject: [PATCH 07/27] core: pass boot-id to nm_utils_stable_id_parse() For testing purpose, it's bad to let nm_utils_stable_id_parse() directly access nm_utils_get_boot_id_str(). Instead, the function should have no side-effects. Since the boot-id is anyway cached, accessing it is cheap. Even if it likely won't be needed. (cherry picked from commit c51e63feb698447c5d91a06e0dcc3302845af452) --- src/devices/nm-device.c | 2 +- src/nm-core-utils.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 4888cc66e9..9c01ac8306 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1281,7 +1281,7 @@ _get_stable_id (NMDevice *self, stable_type = nm_utils_stable_id_parse (stable_id, nm_device_get_ip_iface (self), - NULL, + nm_utils_get_boot_id_str (), uuid, &generated); diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 45fb4f455d..37f1bc3211 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -3165,7 +3165,7 @@ nm_utils_stable_id_parse (const char *stable_id, if (CHECK_PREFIX ("${CONNECTION}")) _stable_id_append (str, uuid); else if (CHECK_PREFIX ("${BOOT}")) - _stable_id_append (str, bootid ?: nm_utils_get_boot_id_str ()); + _stable_id_append (str, bootid); else if (CHECK_PREFIX ("${DEVICE}")) _stable_id_append (str, deviceid); else if (g_str_has_prefix (&stable_id[i], "${RANDOM}")) { From b04c0330073b5c1fb3b30b93594bc07a186f4ea8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 31 Oct 2018 11:18:46 +0100 Subject: [PATCH 08/27] core: cleanup generating DUID in nm-device.c - use NMUuid type where appropriate. - no error handling for generate_duid_from_machine_id(). It cannot fail anymore. - add thread-safety to generate_duid_from_machine_id() with double-checked locking. - use unions for converting the sha256 digest to the target type. (cherry picked from commit 50121ee0286f00986406cd1b654c32dbed800946) --- src/devices/nm-device.c | 117 ++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 57 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9c01ac8306..779ad9fba7 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7533,11 +7533,10 @@ dhcp4_get_client_id (NMDevice *self, } if (nm_streq (client_id, "stable")) { + nm_auto_free_checksum GChecksum *sum = NULL; + guint8 digest[NM_UTILS_CHECKSUM_LENGTH_SHA1]; NMUtilsStableType stable_type; const char *stable_id; - GChecksum *sum; - guint8 buf[20]; - gsize buf_size; guint32 salted_header; const guint8 *secret_key; gsize secret_key_len; @@ -7551,20 +7550,14 @@ dhcp4_get_client_id (NMDevice *self, nm_utils_secret_key_get (&secret_key, &secret_key_len); sum = g_checksum_new (G_CHECKSUM_SHA1); - g_checksum_update (sum, (const guchar *) &salted_header, sizeof (salted_header)); g_checksum_update (sum, (const guchar *) stable_id, strlen (stable_id) + 1); g_checksum_update (sum, (const guchar *) secret_key, secret_key_len); - - buf_size = sizeof (buf); - g_checksum_get_digest (sum, buf, &buf_size); - nm_assert (buf_size == sizeof (buf)); - - g_checksum_free (sum); + nm_utils_checksum_get_digest (sum, digest); client_id_buf = g_malloc (1 + 15); client_id_buf[0] = 0; - memcpy (&client_id_buf[1], buf, 15); + memcpy (&client_id_buf[1], digest, 15); result = g_bytes_new_take (client_id_buf, 1 + 15); goto out_good; } @@ -8186,6 +8179,8 @@ dhcp6_prefix_delegated (NMDhcpClient *client, g_signal_emit (self, signals[IP6_PREFIX_DELEGATED], 0, prefix); } +/*****************************************************************************/ + /* RFC 3315 defines the epoch for the DUID-LLT time field on Jan 1st 2000. */ #define EPOCH_DATETIME_200001010000 946684800 @@ -8225,14 +8220,12 @@ generate_duid_ll (const guint8 *hwaddr /* ETH_ALEN bytes */) } static GBytes * -generate_duid_uuid (guint8 *data, gsize data_len) +generate_duid_uuid (const NMUuid *uuid) { - const guint16 duid_type = g_htons (4); - const int DUID_SIZE = 18; + const guint16 duid_type = htons (4); guint8 *duid_buffer; - nm_assert (data); - nm_assert (data_len >= 16); + nm_assert (uuid); /* Generate a DHCP Unique Identifier for DHCPv6 using the * DUID-UUID method (see RFC 6355 section 4). Format is: @@ -8240,41 +8233,47 @@ generate_duid_uuid (guint8 *data, gsize data_len) * u16: type (DUID-UUID = 4) * u8[16]: UUID bytes */ - duid_buffer = g_malloc (DUID_SIZE); - G_STATIC_ASSERT_EXPR (sizeof (duid_type) == 2); + G_STATIC_ASSERT_EXPR (sizeof (*uuid) == 16); + duid_buffer = g_malloc (18); memcpy (&duid_buffer[0], &duid_type, 2); - - /* UUID is 128 bits, we just take the first 128 bits - * (regardless of data size) as the DUID-UUID. - */ - memcpy (&duid_buffer[2], data, 16); - - return g_bytes_new_take (duid_buffer, DUID_SIZE); + memcpy (&duid_buffer[2], uuid, 16); + return g_bytes_new_take (duid_buffer, 18); } static GBytes * generate_duid_from_machine_id (void) { - const NMUuid *uuid; - GChecksum *sum; - guint8 sha256_digest[32]; - gsize len = sizeof (sha256_digest); - static GBytes *global_duid = NULL; + static GBytes *volatile global_duid = NULL; + GBytes *p; - if (global_duid) - return g_bytes_ref (global_duid); +again: + p = g_atomic_pointer_get (&global_duid); + if (G_UNLIKELY (!p)) { + nm_auto_free_checksum GChecksum *sum = NULL; + const NMUuid *machine_id; + union { + guint8 sha256[NM_UTILS_CHECKSUM_LENGTH_SHA256]; + NMUuid uuid; + } digest; - uuid = nm_utils_machine_id_bin (); + machine_id = nm_utils_machine_id_bin (); - /* Hash the machine ID so it's not leaked to the network */ - sum = g_checksum_new (G_CHECKSUM_SHA256); - g_checksum_update (sum, (const guchar *) uuid, sizeof (*uuid)); - g_checksum_get_digest (sum, sha256_digest, &len); - g_checksum_free (sum); + /* Hash the machine ID so it's not leaked to the network */ + sum = g_checksum_new (G_CHECKSUM_SHA256); + g_checksum_update (sum, (const guchar *) machine_id, sizeof (*machine_id)); + nm_utils_checksum_get_digest (sum, digest.sha256); - global_duid = generate_duid_uuid (sha256_digest, len); - return g_bytes_ref (global_duid); + G_STATIC_ASSERT_EXPR (sizeof (digest.sha256) > sizeof (digest.uuid)); + p = generate_duid_uuid (&digest.uuid); + + if (!g_atomic_pointer_compare_and_exchange (&global_duid, NULL, p)) { + g_bytes_unref (p); + goto again; + } + } + + return g_bytes_ref (p); } static GBytes * @@ -8285,8 +8284,6 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole gs_free char *duid_default = NULL; const char *duid_error; GBytes *duid_out; - guint8 sha256_digest[32]; - gsize len = sizeof (sha256_digest); gboolean duid_enforce = TRUE; gs_free char *logstr1 = NULL; @@ -8304,10 +8301,6 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole if (nm_streq (duid, "lease")) { duid_enforce = FALSE; duid_out = generate_duid_from_machine_id (); - if (!duid_out) { - duid_error = "failure to read machine-id"; - goto out_fail; - } goto out_good; } @@ -8347,12 +8340,21 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole } if (NM_IN_STRSET (duid, "stable-ll", "stable-llt", "stable-uuid")) { + nm_auto_free_checksum GChecksum *sum = NULL; NMUtilsStableType stable_type; const char *stable_id = NULL; guint32 salted_header; - GChecksum *sum; const guint8 *secret_key; gsize secret_key_len; + union { + guint8 sha256[NM_UTILS_CHECKSUM_LENGTH_SHA256]; + guint8 hwaddr[ETH_ALEN]; + NMUuid uuid; + struct _nm_packed { + guint8 hwaddr[ETH_ALEN]; + guint32 timestamp; + } llt; + } digest; stable_id = _get_stable_id (self, connection, &stable_type); if (!stable_id) @@ -8363,16 +8365,15 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole nm_utils_secret_key_get (&secret_key, &secret_key_len); sum = g_checksum_new (G_CHECKSUM_SHA256); - g_checksum_update (sum, (const guchar *) &salted_header, sizeof (salted_header)); g_checksum_update (sum, (const guchar *) stable_id, -1); g_checksum_update (sum, (const guchar *) secret_key, secret_key_len); + nm_utils_checksum_get_digest (sum, digest.sha256); - g_checksum_get_digest (sum, sha256_digest, &len); - g_checksum_free (sum); + G_STATIC_ASSERT_EXPR (sizeof (digest) == sizeof (digest.sha256)); if (nm_streq (duid, "stable-ll")) { - duid_out = generate_duid_ll (sha256_digest); + duid_out = generate_duid_ll (digest.hwaddr); } else if (nm_streq (duid, "stable-llt")) { gint64 time; @@ -8390,12 +8391,12 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole /* don't use too old timestamps. They cannot be expressed in DUID-LLT and * would all be truncated to zero. */ time = NM_MAX (time, EPOCH_DATETIME_200001010000 + EPOCH_DATETIME_THREE_YEARS); - time -= (unaligned_read_be32 (&sha256_digest[ETH_ALEN]) % EPOCH_DATETIME_THREE_YEARS); + time -= unaligned_read_be32 (&digest.llt.timestamp) % EPOCH_DATETIME_THREE_YEARS; - duid_out = generate_duid_llt (sha256_digest, time); + duid_out = generate_duid_llt (digest.llt.hwaddr, time); } else { nm_assert (nm_streq (duid, "stable-uuid")); - duid_out = generate_duid_uuid (sha256_digest, len); + duid_out = generate_duid_uuid (&digest.uuid); } goto out_good; @@ -8406,14 +8407,14 @@ dhcp6_get_duid (NMDevice *self, NMConnection *connection, GBytes *hwaddr, gboole out_fail: nm_assert (!duid_out && duid_error); { - guint8 uuid[16]; + NMUuid uuid; _LOGW (LOGD_IP6 | LOGD_DHCP6, "ipv6.dhcp-duid: failure to generate %s DUID: %s. Fallback to random DUID-UUID.", duid, duid_error); - nm_utils_random_bytes (uuid, sizeof (uuid)); - duid_out = generate_duid_uuid (uuid, sizeof (uuid)); + nm_utils_random_bytes (&uuid, sizeof (uuid)); + duid_out = generate_duid_uuid (&uuid); } out_good: @@ -8428,6 +8429,8 @@ out_good: return duid_out; } +/*****************************************************************************/ + static gboolean dhcp6_start_with_link_ready (NMDevice *self, NMConnection *connection) { From 13bf09fbd9ba7cc4f050f68b523e35258dc43ba4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 26 Oct 2018 17:32:59 +0200 Subject: [PATCH 09/27] dhcp: test systemd's default DHCP client identifier generation Internal DHCP client generates a default client ID. For one, we should ensure that this algorithm does not change without us noticing, for example, when upgrading systemd code. Add a test, that the generation algorithm works as we expect. Also note, that the generation algorithm uses siphash24(). That means, siphash24() implementation also must not change in the future, to ensure the client ID doesn't change. As we patch systemd sources to use shared/c-siphash, this is not obviously the case. Luckily c-siphash and systemd's siphash24 do agree, so all is good. The test is here to ensure that. Also, previously the generation algorithm is not exposed as a function, sd_dhcp_client will just generate a client-id when it needs it. However, later we want to know (and set) the client id before starting DHCP and not leave it unspecified to an implementation detail. This patch only adds a unit-test for the existing DHCP client ID generation to have something for comparison. In the next commit this will change further. (cherry picked from commit 187d356198182749c8d2f1ea240f505ccfcfe6da) --- src/systemd/nm-sd-utils.c | 46 ++++++++++++++++++ src/systemd/nm-sd-utils.h | 9 +++- src/tests/test-general.c | 98 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/src/systemd/nm-sd-utils.c b/src/systemd/nm-sd-utils.c index c6c4c12328..12741e680e 100644 --- a/src/systemd/nm-sd-utils.c +++ b/src/systemd/nm-sd-utils.c @@ -26,6 +26,7 @@ #include "path-util.h" #include "sd-id128.h" +#include "dhcp-identifier.h" /*****************************************************************************/ @@ -59,3 +60,48 @@ nm_sd_utils_id128_get_machine (NMUuid *out_uuid) return NULL; return out_uuid; } + +/*****************************************************************************/ + +/** + * nm_sd_utils_generate_default_dhcp_client_id: + * @ifindex: the interface ifindex + * @mac: the MAC address + * @mac_addr_len: the length of MAC address. + * + * Systemd's sd_dhcp_client generates a default client ID (type 255, node-specific, + * RFC 4361) if no explicit client-id is set. This function duplicates that + * implementation and exposes it as (internal) API. + * + * Returns: a %GBytes of generated client-id, or %NULL on failure. + */ +GBytes * +nm_sd_utils_generate_default_dhcp_client_id (int ifindex, + const guint8 *mac_addr, + gsize mac_addr_len) +{ + struct _nm_packed { + guint8 type; + guint32 iaid; + struct duid duid; + } client_id; + int r; + gsize duid_len; + + g_return_val_if_fail (ifindex > 0, NULL); + g_return_val_if_fail (mac_addr, NULL); + g_return_val_if_fail (mac_addr_len > 0, NULL); + + client_id.type = 255; + + r = dhcp_identifier_set_iaid (ifindex, (guint8 *) mac_addr, mac_addr_len, &client_id.iaid); + if (r < 0) + return NULL; + + r = dhcp_identifier_set_duid_en (&client_id.duid, &duid_len); + if (r < 0) + return NULL; + + return g_bytes_new (&client_id, + G_STRUCT_OFFSET (typeof (client_id), duid) + duid_len); +} diff --git a/src/systemd/nm-sd-utils.h b/src/systemd/nm-sd-utils.h index fb41f9fe57..55c5a590fc 100644 --- a/src/systemd/nm-sd-utils.h +++ b/src/systemd/nm-sd-utils.h @@ -33,5 +33,12 @@ struct _NMUuid; struct _NMUuid *nm_sd_utils_id128_get_machine (struct _NMUuid *out_uuid); -#endif /* __NM_SD_UTILS_H__ */ +/*****************************************************************************/ +GBytes *nm_sd_utils_generate_default_dhcp_client_id (int ifindex, + const guint8 *mac_addr, + gsize mac_addr_len); + +/*****************************************************************************/ + +#endif /* __NM_SD_UTILS_H__ */ diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 3eaf475a84..a3ba7c1184 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -22,6 +22,8 @@ #include #include +#include +#include /* need math.h for isinf() and INFINITY. No need to link with -lm */ #include @@ -1935,6 +1937,99 @@ test_machine_id_read (void) /*****************************************************************************/ +static void +test_nm_sd_utils_generate_default_dhcp_client_id (gconstpointer test_data) +{ + const guint8 HASH_KEY[16] = { 0x80, 0x11, 0x8c, 0xc2, 0xfe, 0x4a, 0x03, 0xee, 0x3e, 0xd6, 0x0c, 0x6f, 0x36, 0x39, 0x14, 0x09 }; + /* We run the test twice with two ifindexes. + * + * One is "1", which we expect to exist and having a name "lo". + * The other is a random number, which we expect not to exist. + * + * Regardless of whether the ifindex actually exists, the tests are + * supposed to pass. However, when our expectations are not met, we + * silently miss test cases. */ + const int IFINDEX = GPOINTER_TO_INT (test_data) + ? 1 + : (int) (nmtst_get_rand_int () % 10000); + const guint8 mac_addr[ETH_ALEN] = { 0x20, 0xaf, 0x51, 0x42, 0x29, 0x05 }; + const guint16 duid_type_en = htons (2); + const guint32 systemd_pen = htonl (43793); + guint32 iaid_mac; + guint32 iaid_ifname; + gs_unref_bytes GBytes *client_id = NULL; + char ifname_buf[IFNAMSIZ]; + const char *ifname; + gboolean has_ifindex; + gint64 u64; + const guint8 *cid; + const NMUuid *machine_id; + + /* see whether IFINDEX exists. */ + if (if_indextoname (IFINDEX, ifname_buf)) { + ifname = ifname_buf; + has_ifindex = TRUE; + } else { + ifname = "lo"; + has_ifindex = FALSE; + } + + /* generate the iaid based on the ifname and assert for expected + * values. + * + * We often expect that the interface name is "lo". Hence, assert + * for the expected hash values explicitly. + * + * Note that the iaid generated by dhcp_identifier_set_iaid() is + * in native endianness (https://github.com/systemd/systemd/pull/10614). */ + u64 = c_siphash_hash (HASH_KEY, (const guint8 *) ifname, strlen (ifname)); + if (nm_streq (ifname, "lo")) + g_assert_cmpint (u64, ==, 0x7297085c2b12c911llu); + iaid_ifname = bswap_32 ((u64 & 0xffffffffu) ^ (u64 >> 32)); + if (nm_streq (ifname, "lo")) + g_assert_cmpint (iaid_ifname, ==, 0x4dc18559u); + + /* generate the iaid based on the hard-coded MAC address */ + u64 = c_siphash_hash (HASH_KEY, mac_addr, sizeof (mac_addr)); + g_assert_cmpint (u64, ==, 0x1f3d1d8d15de49dcllu); + iaid_mac = bswap_32 ((u64 & 0xffffffffu) ^ (u64 >> 32)); + g_assert_cmpint (iaid_mac, ==, 0x5154e30au); + + /* as it is, nm_sd_utils_generate_default_dhcp_client_id() resolves the ifname (based on the + * ifindex) and loads /etc/machine-id. Maybe the code should be refactored, to accept + * such external input as arguments (to ease testing). + * + * Instead, we just duplicate the steps here, which are don't internally by the + * function. Hey, it's a test. Let's re-implement what the code does here. */ + client_id = nm_sd_utils_generate_default_dhcp_client_id (IFINDEX, mac_addr, sizeof (mac_addr)); + + if (!client_id) { + /* the only reason why this can fail, is because /etc/machine-id is invalid. */ + if (!g_file_test ("/etc/machine-id", G_FILE_TEST_EXISTS)) { + g_test_skip ("no /etc/machine-id"); + return; + } + g_assert_not_reached (); + } + + g_assert_cmpint (g_bytes_get_size (client_id), ==, 19); + cid = g_bytes_get_data (client_id, NULL); + + g_assert_cmpint (cid[0], ==, 255); + if (has_ifindex) + g_assert_cmpmem (&cid[1], 4, &iaid_ifname, sizeof (iaid_ifname)); + else + g_assert_cmpmem (&cid[1], 4, &iaid_mac, sizeof (iaid_mac)); + g_assert_cmpmem (&cid[5], 2, &duid_type_en, sizeof (duid_type_en)); + g_assert_cmpmem (&cid[7], 4, &systemd_pen, sizeof (systemd_pen)); + + machine_id = nm_utils_machine_id_bin (); + u64 = htole64 (c_siphash_hash (HASH_KEY, (const guint8 *) machine_id, sizeof (*machine_id))); + g_assert_cmpmem (&cid[11], 8, &u64, sizeof (u64)); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -1985,6 +2080,9 @@ main (int argc, char **argv) g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf); + g_test_add_data_func ("/general/nm_sd_utils_generate_default_dhcp_client_id/lo", GINT_TO_POINTER (TRUE), test_nm_sd_utils_generate_default_dhcp_client_id); + g_test_add_data_func ("/general/nm_sd_utils_generate_default_dhcp_client_id/rnd", GINT_TO_POINTER (FALSE), test_nm_sd_utils_generate_default_dhcp_client_id); + return g_test_run (); } From 3b8d8826584211b6ea0c1dc516717e506f6b880b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 30 Oct 2018 08:37:48 +0100 Subject: [PATCH 10/27] dhcp: reimplement node-specific DHCP client-id generation from systemd Our internal DHCP client (from systemd) defaults to a particular client ID. It is currently exposed as nm_sd_utils_generate_default_dhcp_client_id() and is based on the systemd implementation. One problem with that is, that it internally looks up the interface name with if_indextoname() and reads /etc/machine-id. Both makes it harder for testing. Another problem is, that this way of generating the client-id is currently limited to internal client. Why? If you use dhclient plugin, you may still want to use the same algorithm. Also, there is no explict "ipv4.dhcp-client-id" mode to select this client-id (so that it could be used in combination with "dhclient" plugin). As such, this code will be useful also aside systemd DHCP plugin. Hence, the function should not be obviously tied to systemd code. The implementation is simple enough, and since we already have a unit-test, refactor the code to our own implementation. (cherry picked from commit a55795772a051b261066a7799027c90363d3c1a6) --- src/nm-core-utils.c | 97 ++++++++++++++++++++++++++ src/nm-core-utils.h | 13 ++++ src/systemd/nm-sd-utils.c | 46 ------------- src/systemd/nm-sd-utils.h | 6 -- src/tests/test-general.c | 140 +++++++++++++++++--------------------- 5 files changed, 172 insertions(+), 130 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 37f1bc3211..18538e0939 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -39,6 +40,7 @@ #include "nm-utils/nm-random-utils.h" #include "nm-utils/nm-io-utils.h" +#include "nm-utils/unaligned.h" #include "nm-utils.h" #include "nm-core-internal.h" #include "nm-setting-connection.h" @@ -3487,6 +3489,101 @@ nm_utils_hw_addr_gen_stable_eth (NMUtilsStableType stable_type, /*****************************************************************************/ +/** + * nm_utils_dhcp_client_id_systemd_node_specific_full: + * @legacy_unstable_byteorder: historically, the code would generate a iaid + * dependent on host endianness. This is undesirable, if backward compatibility + * are not a concern, generate stable endianness. + * @interface_id: a binary identifer that is hashed into the DUID. + * Comonly this is the interface-name, but it may be the MAC address. + * @interface_id_len: the length of @interface_id. + * @machine_id: the binary identifier for the machine. It is hashed + * into the DUID. It commonly is /etc/machine-id (parsed in binary as NMUuid). + * @machine_id_len: the length of the @machine_id. + * + * Systemd's sd_dhcp_client generates a default client ID (type 255, node-specific, + * RFC 4361) if no explicit client-id is set. This function duplicates that + * implementation and exposes it as (internal) API. + * + * Returns: a %GBytes of generated client-id. This function cannot fail. + */ +GBytes * +nm_utils_dhcp_client_id_systemd_node_specific_full (gboolean legacy_unstable_byteorder, + const guint8 *interface_id, + gsize interface_id_len, + const guint8 *machine_id, + gsize machine_id_len) +{ + const guint8 HASH_KEY[16] = { 0x80, 0x11, 0x8c, 0xc2, 0xfe, 0x4a, 0x03, 0xee, 0x3e, 0xd6, 0x0c, 0x6f, 0x36, 0x39, 0x14, 0x09 }; + const guint16 DUID_TYPE_EN = 2; + const guint32 SYSTEMD_PEN = 43793; + struct _nm_packed { + guint8 type; + guint32 iaid; + struct _nm_packed { + guint16 type; + union { + struct _nm_packed { + /* DUID_TYPE_EN */ + guint32 pen; + uint8_t id[8]; + } en; + }; + } duid; + } *client_id; + guint64 u64; + guint32 u32; + + g_return_val_if_fail (interface_id, NULL); + g_return_val_if_fail (interface_id_len > 0, NULL); + g_return_val_if_fail (machine_id, NULL); + g_return_val_if_fail (machine_id_len > 0, NULL); + + client_id = g_malloc (sizeof (*client_id)); + + client_id->type = 255; + + u64 = c_siphash_hash (HASH_KEY, interface_id, interface_id_len); + u32 = (u64 & 0xffffffffu) ^ (u64 >> 32); + if (legacy_unstable_byteorder) { + /* original systemd code dhcp_identifier_set_iaid() generates the iaid + * in native endianness. Do that too, to preserve compatibility + * (https://github.com/systemd/systemd/pull/10614). */ + u32 = bswap_32 (u32); + } else { + /* generate fixed byteorder, in a way that on little endian systems + * the values agree. Meaning: legacy behavior is identical to this + * on little endian. */ + u32 = be32toh (u32); + } + unaligned_write_ne32 (&client_id->iaid, u32); + + unaligned_write_be16 (&client_id->duid.type, DUID_TYPE_EN); + + unaligned_write_be32 (&client_id->duid.en.pen, SYSTEMD_PEN); + + u64 = htole64 (c_siphash_hash (HASH_KEY, machine_id, machine_id_len)); + memcpy(client_id->duid.en.id, &u64, sizeof (client_id->duid.en.id)); + + G_STATIC_ASSERT_EXPR (sizeof (*client_id) == 19); + return g_bytes_new_take (client_id, 19); +} + +GBytes * +nm_utils_dhcp_client_id_systemd_node_specific (gboolean legacy_unstable_byteorder, + const char *ifname) +{ + g_return_val_if_fail (ifname && ifname[0], NULL); + + return nm_utils_dhcp_client_id_systemd_node_specific_full (legacy_unstable_byteorder, + (const guint8 *) ifname, + strlen (ifname), + (const guint8 *) nm_utils_machine_id_bin (), + sizeof (NMUuid)); +} + +/*****************************************************************************/ + /** * nm_utils_setpgid: * @unused: unused diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 2a92e58849..fb1effef36 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -384,6 +384,19 @@ char *nm_utils_hw_addr_gen_stable_eth (NMUtilsStableType stable_type, const char *current_mac_address, const char *generate_mac_address_mask); +/*****************************************************************************/ + +GBytes *nm_utils_dhcp_client_id_systemd_node_specific_full (gboolean legacy_unstable_byteorder, + const guint8 *interface_id, + gsize interface_id_len, + const guint8 *machine_id, + gsize machine_id_len); + +GBytes *nm_utils_dhcp_client_id_systemd_node_specific (gboolean legacy_unstable_byteorder, + const char *ifname); + +/*****************************************************************************/ + void nm_utils_array_remove_at_indexes (GArray *array, const guint *indexes_to_delete, gsize len); void nm_utils_setpgid (gpointer unused); diff --git a/src/systemd/nm-sd-utils.c b/src/systemd/nm-sd-utils.c index 12741e680e..c6c4c12328 100644 --- a/src/systemd/nm-sd-utils.c +++ b/src/systemd/nm-sd-utils.c @@ -26,7 +26,6 @@ #include "path-util.h" #include "sd-id128.h" -#include "dhcp-identifier.h" /*****************************************************************************/ @@ -60,48 +59,3 @@ nm_sd_utils_id128_get_machine (NMUuid *out_uuid) return NULL; return out_uuid; } - -/*****************************************************************************/ - -/** - * nm_sd_utils_generate_default_dhcp_client_id: - * @ifindex: the interface ifindex - * @mac: the MAC address - * @mac_addr_len: the length of MAC address. - * - * Systemd's sd_dhcp_client generates a default client ID (type 255, node-specific, - * RFC 4361) if no explicit client-id is set. This function duplicates that - * implementation and exposes it as (internal) API. - * - * Returns: a %GBytes of generated client-id, or %NULL on failure. - */ -GBytes * -nm_sd_utils_generate_default_dhcp_client_id (int ifindex, - const guint8 *mac_addr, - gsize mac_addr_len) -{ - struct _nm_packed { - guint8 type; - guint32 iaid; - struct duid duid; - } client_id; - int r; - gsize duid_len; - - g_return_val_if_fail (ifindex > 0, NULL); - g_return_val_if_fail (mac_addr, NULL); - g_return_val_if_fail (mac_addr_len > 0, NULL); - - client_id.type = 255; - - r = dhcp_identifier_set_iaid (ifindex, (guint8 *) mac_addr, mac_addr_len, &client_id.iaid); - if (r < 0) - return NULL; - - r = dhcp_identifier_set_duid_en (&client_id.duid, &duid_len); - if (r < 0) - return NULL; - - return g_bytes_new (&client_id, - G_STRUCT_OFFSET (typeof (client_id), duid) + duid_len); -} diff --git a/src/systemd/nm-sd-utils.h b/src/systemd/nm-sd-utils.h index 55c5a590fc..0af514eb84 100644 --- a/src/systemd/nm-sd-utils.h +++ b/src/systemd/nm-sd-utils.h @@ -35,10 +35,4 @@ struct _NMUuid *nm_sd_utils_id128_get_machine (struct _NMUuid *out_uuid); /*****************************************************************************/ -GBytes *nm_sd_utils_generate_default_dhcp_client_id (int ifindex, - const guint8 *mac_addr, - gsize mac_addr_len); - -/*****************************************************************************/ - #endif /* __NM_SD_UTILS_H__ */ diff --git a/src/tests/test-general.c b/src/tests/test-general.c index a3ba7c1184..773fc0bd4a 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1938,94 +1938,78 @@ test_machine_id_read (void) /*****************************************************************************/ static void -test_nm_sd_utils_generate_default_dhcp_client_id (gconstpointer test_data) +test_nm_utils_dhcp_client_id_systemd_node_specific (gconstpointer test_data) { + const int TEST_IDX = GPOINTER_TO_INT (test_data); const guint8 HASH_KEY[16] = { 0x80, 0x11, 0x8c, 0xc2, 0xfe, 0x4a, 0x03, 0xee, 0x3e, 0xd6, 0x0c, 0x6f, 0x36, 0x39, 0x14, 0x09 }; - /* We run the test twice with two ifindexes. - * - * One is "1", which we expect to exist and having a name "lo". - * The other is a random number, which we expect not to exist. - * - * Regardless of whether the ifindex actually exists, the tests are - * supposed to pass. However, when our expectations are not met, we - * silently miss test cases. */ - const int IFINDEX = GPOINTER_TO_INT (test_data) - ? 1 - : (int) (nmtst_get_rand_int () % 10000); - const guint8 mac_addr[ETH_ALEN] = { 0x20, 0xaf, 0x51, 0x42, 0x29, 0x05 }; const guint16 duid_type_en = htons (2); const guint32 systemd_pen = htonl (43793); - guint32 iaid_mac; - guint32 iaid_ifname; - gs_unref_bytes GBytes *client_id = NULL; - char ifname_buf[IFNAMSIZ]; - const char *ifname; - gboolean has_ifindex; + const struct { + NMUuid machine_id; + const char *ifname; + guint64 ifname_hash_1; + guint32 iaid_ifname; + guint64 duid_id; + } d_array[] = { + [0] = { + .machine_id = { 0xcb, 0xc2, 0x2e, 0x47, 0x41, 0x8e, 0x40, 0x2a, 0xa7, 0xb3, 0x0d, 0xea, 0x92, 0x83, 0x94, 0xef }, + .ifname = "lo", + .ifname_hash_1 = 0x7297085c2b12c911llu, + .iaid_ifname = htobe32 (0x5985c14du), + .duid_id = htobe64 (0x3d769bb2c14d29e1u), + }, + [1] = { + .machine_id = { 0x11, 0x4e, 0xb4, 0xda, 0xd3, 0x22, 0x4a, 0xff, 0x9f, 0xc3, 0x30, 0x83, 0x38, 0xa0, 0xeb, 0xb7 }, + .ifname = "eth0", + .ifname_hash_1 = 0x9e1cb083b54cd7b6llu, + .iaid_ifname = htobe32 (0x2b506735u), + .duid_id = htobe64 (0x551572e0f2a2a10fu), + }, + }; + int i; + typeof (d_array[0]) *d = &d_array[TEST_IDX]; gint64 u64; - const guint8 *cid; - const NMUuid *machine_id; + gint32 u32; - /* see whether IFINDEX exists. */ - if (if_indextoname (IFINDEX, ifname_buf)) { - ifname = ifname_buf; - has_ifindex = TRUE; - } else { - ifname = "lo"; - has_ifindex = FALSE; - } + /* the test already hard-codes the expected values iaid_ifname and duid_id + * above. Still, redo the steps to derive them from the ifname/machine-id + * and double check. */ + u64 = c_siphash_hash (HASH_KEY, (const guint8 *) d->ifname, strlen (d->ifname)); + g_assert_cmpint (u64, ==, d->ifname_hash_1); + u32 = be32toh ((u64 & 0xffffffffu) ^ (u64 >> 32)); + g_assert_cmpint (u32, ==, d->iaid_ifname); - /* generate the iaid based on the ifname and assert for expected - * values. - * - * We often expect that the interface name is "lo". Hence, assert - * for the expected hash values explicitly. - * - * Note that the iaid generated by dhcp_identifier_set_iaid() is - * in native endianness (https://github.com/systemd/systemd/pull/10614). */ - u64 = c_siphash_hash (HASH_KEY, (const guint8 *) ifname, strlen (ifname)); - if (nm_streq (ifname, "lo")) - g_assert_cmpint (u64, ==, 0x7297085c2b12c911llu); - iaid_ifname = bswap_32 ((u64 & 0xffffffffu) ^ (u64 >> 32)); - if (nm_streq (ifname, "lo")) - g_assert_cmpint (iaid_ifname, ==, 0x4dc18559u); + u64 = htole64 (c_siphash_hash (HASH_KEY, (const guint8 *) &d->machine_id, sizeof (d->machine_id))); + g_assert_cmpint (u64, ==, d->duid_id); - /* generate the iaid based on the hard-coded MAC address */ - u64 = c_siphash_hash (HASH_KEY, mac_addr, sizeof (mac_addr)); - g_assert_cmpint (u64, ==, 0x1f3d1d8d15de49dcllu); - iaid_mac = bswap_32 ((u64 & 0xffffffffu) ^ (u64 >> 32)); - g_assert_cmpint (iaid_mac, ==, 0x5154e30au); + for (i = 0; i < 2; i++) { + const gboolean legacy_unstable_byteorder = (i != 0); + gs_unref_bytes GBytes *client_id = NULL; + const guint8 *cid; + guint32 iaid = d->iaid_ifname; - /* as it is, nm_sd_utils_generate_default_dhcp_client_id() resolves the ifname (based on the - * ifindex) and loads /etc/machine-id. Maybe the code should be refactored, to accept - * such external input as arguments (to ease testing). - * - * Instead, we just duplicate the steps here, which are don't internally by the - * function. Hey, it's a test. Let's re-implement what the code does here. */ - client_id = nm_sd_utils_generate_default_dhcp_client_id (IFINDEX, mac_addr, sizeof (mac_addr)); + client_id = nm_utils_dhcp_client_id_systemd_node_specific_full (legacy_unstable_byteorder, + (const guint8 *) d->ifname, + strlen (d->ifname), + (const guint8 *) &d->machine_id, + sizeof (d->machine_id)); - if (!client_id) { - /* the only reason why this can fail, is because /etc/machine-id is invalid. */ - if (!g_file_test ("/etc/machine-id", G_FILE_TEST_EXISTS)) { - g_test_skip ("no /etc/machine-id"); - return; + g_assert (client_id); + g_assert_cmpint (g_bytes_get_size (client_id), ==, 19); + cid = g_bytes_get_data (client_id, NULL); + g_assert_cmpint (cid[0], ==, 255); +#if __BYTE_ORDER == __BIG_ENDIAN + if (legacy_unstable_byteorder) { + /* on non-little endian, the legacy behavior is to have the bytes + * swapped. */ + iaid = bswap_32 (iaid); } - g_assert_not_reached (); +#endif + g_assert_cmpmem (&cid[1], 4, &iaid, sizeof (iaid)); + g_assert_cmpmem (&cid[5], 2, &duid_type_en, sizeof (duid_type_en)); + g_assert_cmpmem (&cid[7], 4, &systemd_pen, sizeof (systemd_pen)); + g_assert_cmpmem (&cid[11], 8, &d->duid_id, sizeof (d->duid_id)); } - - g_assert_cmpint (g_bytes_get_size (client_id), ==, 19); - cid = g_bytes_get_data (client_id, NULL); - - g_assert_cmpint (cid[0], ==, 255); - if (has_ifindex) - g_assert_cmpmem (&cid[1], 4, &iaid_ifname, sizeof (iaid_ifname)); - else - g_assert_cmpmem (&cid[1], 4, &iaid_mac, sizeof (iaid_mac)); - g_assert_cmpmem (&cid[5], 2, &duid_type_en, sizeof (duid_type_en)); - g_assert_cmpmem (&cid[7], 4, &systemd_pen, sizeof (systemd_pen)); - - machine_id = nm_utils_machine_id_bin (); - u64 = htole64 (c_siphash_hash (HASH_KEY, (const guint8 *) machine_id, sizeof (*machine_id))); - g_assert_cmpmem (&cid[11], 8, &u64, sizeof (u64)); } /*****************************************************************************/ @@ -2080,8 +2064,8 @@ main (int argc, char **argv) g_test_add_func ("/general/test_dns_create_resolv_conf", test_dns_create_resolv_conf); - g_test_add_data_func ("/general/nm_sd_utils_generate_default_dhcp_client_id/lo", GINT_TO_POINTER (TRUE), test_nm_sd_utils_generate_default_dhcp_client_id); - g_test_add_data_func ("/general/nm_sd_utils_generate_default_dhcp_client_id/rnd", GINT_TO_POINTER (FALSE), test_nm_sd_utils_generate_default_dhcp_client_id); + g_test_add_data_func ("/general/nm_utils_dhcp_client_id_systemd_node_specific/0", GINT_TO_POINTER (0), test_nm_utils_dhcp_client_id_systemd_node_specific); + g_test_add_data_func ("/general/nm_utils_dhcp_client_id_systemd_node_specific/1", GINT_TO_POINTER (1), test_nm_utils_dhcp_client_id_systemd_node_specific); return g_test_run (); } From a6095fd0431acebee49ae3806ca9072b03607ca9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 15:16:35 +0200 Subject: [PATCH 11/27] dhcp: don't re-read DHCP client ID from configuration file for dhclient Why would we do this? The configuration file we are reading back was written by NetworkManager in the first place. Maybe when assuming a connection after restart, this information could be interesting. It however is not actually relevant. Note how nm_dhcp_client_get_client_id() has only very few callers. - nm_device_spawn_iface_helper() in 'nm-device.c'. In this case, we either should use the client-id which we used when starting DHCP, or none at all. - ip4_start() in 'nm-dhcp-dhclient.c', but this is before starting DHCP client and before it was re-read from configuration file. - in "src/dhcp/nm-dhcp-systemd.c", but this has no effect for the dhclient plugin. (cherry picked from commit 5411fb0cc68e3c3288f41c34976d4fd28b875682) --- src/dhcp/nm-dhcp-client.c | 4 ++-- src/dhcp/nm-dhcp-client.h | 6 ------ src/dhcp/nm-dhcp-dhclient.c | 19 ------------------- src/dhcp/nm-dhcp-dhcpcanon.c | 13 ------------- 4 files changed, 2 insertions(+), 40 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 5299bdbf38..2622179ce3 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -1066,7 +1066,7 @@ nm_dhcp_client_class_init (NMDhcpClientClass *client_class) g_signal_new (NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_FIRST, - G_STRUCT_OFFSET (NMDhcpClientClass, state_changed), + 0, NULL, NULL, NULL, G_TYPE_NONE, 4, G_TYPE_UINT, G_TYPE_OBJECT, G_TYPE_HASH_TABLE, G_TYPE_STRING); @@ -1074,7 +1074,7 @@ nm_dhcp_client_class_init (NMDhcpClientClass *client_class) g_signal_new (NM_DHCP_CLIENT_SIGNAL_PREFIX_DELEGATED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_FIRST, - G_STRUCT_OFFSET (NMDhcpClientClass, state_changed), + 0, NULL, NULL, NULL, G_TYPE_NONE, 1, G_TYPE_POINTER); } diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 86d60e3874..84821f8c23 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -103,12 +103,6 @@ typedef struct { * returned. */ GBytes *(*get_duid) (NMDhcpClient *self); - - /* Signals */ - void (*state_changed) (NMDhcpClient *self, - NMDhcpState state, - GObject *ip_config, - GHashTable *options); } NMDhcpClientClass; GType nm_dhcp_client_get_type (void); diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 33c2671234..bb8892f106 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -624,24 +624,6 @@ stop (NMDhcpClient *client, gboolean release, GBytes *duid) } } -static void -state_changed (NMDhcpClient *client, - NMDhcpState state, - GObject *ip_config, - GHashTable *options) -{ - NMDhcpDhclientPrivate *priv = NM_DHCP_DHCLIENT_GET_PRIVATE ((NMDhcpDhclient *) client); - gs_unref_bytes GBytes *client_id = NULL; - - if (nm_dhcp_client_get_client_id (client)) - return; - if (state != NM_DHCP_STATE_BOUND) - return; - - client_id = nm_dhcp_dhclient_get_client_id_from_config_file (priv->conf_file); - nm_dhcp_client_set_client_id (client, client_id); -} - static GBytes * get_duid (NMDhcpClient *client) { @@ -742,7 +724,6 @@ nm_dhcp_dhclient_class_init (NMDhcpDhclientClass *dhclient_class) client_class->ip6_start = ip6_start; client_class->stop = stop; client_class->get_duid = get_duid; - client_class->state_changed = state_changed; } const NMDhcpClientFactory _nm_dhcp_client_factory_dhclient = { diff --git a/src/dhcp/nm-dhcp-dhcpcanon.c b/src/dhcp/nm-dhcp-dhcpcanon.c index de40302062..acd3ce2d4c 100644 --- a/src/dhcp/nm-dhcp-dhcpcanon.c +++ b/src/dhcp/nm-dhcp-dhcpcanon.c @@ -216,18 +216,6 @@ stop (NMDhcpClient *client, gboolean release, GBytes *duid) } } -static void -state_changed (NMDhcpClient *client, - NMDhcpState state, - GObject *ip_config, - GHashTable *options) -{ - if (nm_dhcp_client_get_client_id (client)) - return; - if (state != NM_DHCP_STATE_BOUND) - return; -} - /*****************************************************************************/ static void @@ -270,7 +258,6 @@ nm_dhcp_dhcpcanon_class_init (NMDhcpDhcpcanonClass *dhcpcanon_class) client_class->ip4_start = ip4_start; client_class->ip6_start = ip6_start; client_class->stop = stop; - client_class->state_changed = state_changed; } const NMDhcpClientFactory _nm_dhcp_client_factory_dhcpcanon = { From 981a54024b344036ff10a6d1a91bb2822874bbc4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 15:23:24 +0200 Subject: [PATCH 12/27] dhcp: drop unused nm_dhcp_dhclient_get_client_id_from_config_file() Drop unused function. Aside from that, dhclient configuration files support a very complex syntax. The parser was very naive and insufficient in parsing such files. It's good we can just drop it. (cherry picked from commit 025157d59729332f468b4ed8d2dcc77bce6d6397) --- src/dhcp/nm-dhcp-dhclient-utils.c | 23 ----------------------- src/dhcp/nm-dhcp-dhclient-utils.h | 3 --- 2 files changed, 26 deletions(-) diff --git a/src/dhcp/nm-dhcp-dhclient-utils.c b/src/dhcp/nm-dhcp-dhclient-utils.c index a2c3bfb65d..eeb87e1887 100644 --- a/src/dhcp/nm-dhcp-dhclient-utils.c +++ b/src/dhcp/nm-dhcp-dhclient-utils.c @@ -233,29 +233,6 @@ read_client_id (const char *str) return nm_utils_hexstr2bin (s); } -GBytes * -nm_dhcp_dhclient_get_client_id_from_config_file (const char *path) -{ - gs_free char *contents = NULL; - gs_strfreev char **lines = NULL; - char **line; - - g_return_val_if_fail (path != NULL, NULL); - - if (!g_file_test (path, G_FILE_TEST_EXISTS)) - return NULL; - - if (!g_file_get_contents (path, &contents, NULL, NULL)) - return NULL; - - lines = g_strsplit_set (contents, "\n\r", 0); - for (line = lines; lines && *line; line++) { - if (!strncmp (*line, CLIENTID_TAG, NM_STRLEN (CLIENTID_TAG))) - return read_client_id (*line); - } - return NULL; -} - static gboolean read_interface (const char *line, char *interface, guint size) { diff --git a/src/dhcp/nm-dhcp-dhclient-utils.h b/src/dhcp/nm-dhcp-dhclient-utils.h index fab9196a3b..911fece488 100644 --- a/src/dhcp/nm-dhcp-dhclient-utils.h +++ b/src/dhcp/nm-dhcp-dhclient-utils.h @@ -43,7 +43,4 @@ gboolean nm_dhcp_dhclient_save_duid (const char *leasefile, const char *escaped_duid, GError **error); -GBytes *nm_dhcp_dhclient_get_client_id_from_config_file (const char *path); - #endif /* __NETWORKMANAGER_DHCP_DHCLIENT_UTILS_H__ */ - From d347c927cd35c8de6a581ad5395583f722998ad3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 15:28:45 +0200 Subject: [PATCH 13/27] dhcp: merge "duid" and "client_id" field in NMDhcpClient We only used "client_id" for IPv4 and "duid" for IPv6. Merge them. Another advantage is, that we can share the logging functionality of _set_client_id(). (cherry picked from commit 7e341b73e05ea3447e8ddf2f63fdeb8623ad65fb) --- src/dhcp/nm-dhcp-client.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index 2622179ce3..b9e3f743f0 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -68,7 +68,6 @@ typedef struct _NMDhcpClientPrivate { char * iface; GBytes * hwaddr; char * uuid; - GBytes * duid; GBytes * client_id; char * hostname; pid_t pid; @@ -138,14 +137,6 @@ nm_dhcp_client_get_uuid (NMDhcpClient *self) return NM_DHCP_CLIENT_GET_PRIVATE (self)->uuid; } -GBytes * -nm_dhcp_client_get_duid (NMDhcpClient *self) -{ - g_return_val_if_fail (NM_IS_DHCP_CLIENT (self), NULL); - - return NM_DHCP_CLIENT_GET_PRIVATE (self)->duid; -} - GBytes * nm_dhcp_client_get_hw_addr (NMDhcpClient *self) { @@ -233,7 +224,10 @@ _set_client_id (NMDhcpClient *self, GBytes *client_id, gboolean take) { gs_free char *s = NULL; - _LOGT ("client-id: %s", + _LOGT ("%s: set %s", + nm_dhcp_client_get_addr_family (self) == AF_INET6 + ? "duid" + : "client-id", priv->client_id ? (s = nm_dhcp_utils_duid_to_string (priv->client_id)) : "default"); @@ -565,6 +559,7 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, { NMDhcpClientPrivate *priv; gs_free char *str = NULL; + gs_unref_bytes GBytes *own_client_id = NULL; g_return_val_if_fail (NM_IS_DHCP_CLIENT (self), FALSE); @@ -573,16 +568,15 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, g_return_val_if_fail (priv->addr_family == AF_INET6, FALSE); g_return_val_if_fail (priv->uuid != NULL, FALSE); - nm_assert (!priv->duid); + nm_assert (!priv->client_id); nm_assert (client_id); if (!enforce_duid) - priv->duid = NM_DHCP_CLIENT_GET_CLASS (self)->get_duid (self); + own_client_id = NM_DHCP_CLIENT_GET_CLASS (self)->get_duid (self); - if (!priv->duid) - priv->duid = g_bytes_ref (client_id); - - _LOGD ("DUID is '%s'", (str = nm_dhcp_utils_duid_to_string (priv->duid))); + _set_client_id (self, + own_client_id ?: client_id, + FALSE); g_clear_pointer (&priv->hostname, g_free); priv->hostname = g_strdup (hostname); @@ -596,7 +590,7 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, dhcp_anycast_addr, ll_addr, privacy, - priv->duid, + priv->client_id, needed_prefixes, error); } @@ -663,7 +657,7 @@ nm_dhcp_client_stop (NMDhcpClient *self, gboolean release) /* Kill the DHCP client */ old_pid = priv->pid; - NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release, priv->duid); + NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release, priv->client_id); if (old_pid > 0) _LOGI ("canceled DHCP transaction, DHCP client pid %d", old_pid); else @@ -979,7 +973,6 @@ dispose (GObject *object) g_clear_pointer (&priv->uuid, g_free); g_clear_pointer (&priv->client_id, g_bytes_unref); g_clear_pointer (&priv->hwaddr, g_bytes_unref); - g_clear_pointer (&priv->duid, g_bytes_unref); G_OBJECT_CLASS (nm_dhcp_client_parent_class)->dispose (object); From 0e6c84e8064d95b30be4833ac62199bc3ba2e0e3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Oct 2018 09:50:39 +0100 Subject: [PATCH 14/27] dhcp: refactor nm_dhcp_dhclient_save_duid() to accept original DUID There should be lower layers that are concerned with writing and reading dhclient configuration files. It's wrong to have a nm_dhcp_dhclient_save_duid() function which requires the caller to pre-escape the string to write. The caller shouldn't be concerned with the file format, that's why the function is used in the first place. (cherry picked from commit cd9e418fbe3e092d1bd61667d7b33bd4cf5f4fd7) --- src/dhcp/nm-dhcp-dhclient-utils.c | 11 ++++- src/dhcp/nm-dhcp-dhclient-utils.h | 2 +- src/dhcp/nm-dhcp-dhclient.c | 5 +-- src/dhcp/tests/test-dhcp-dhclient.c | 69 ++++++++++++++++------------- 4 files changed, 49 insertions(+), 38 deletions(-) diff --git a/src/dhcp/nm-dhcp-dhclient-utils.c b/src/dhcp/nm-dhcp-dhclient-utils.c index eeb87e1887..716e15d06c 100644 --- a/src/dhcp/nm-dhcp-dhclient-utils.c +++ b/src/dhcp/nm-dhcp-dhclient-utils.c @@ -583,9 +583,10 @@ nm_dhcp_dhclient_read_duid (const char *leasefile, GError **error) gboolean nm_dhcp_dhclient_save_duid (const char *leasefile, - const char *escaped_duid, + GBytes *duid, GError **error) { + gs_free char *escaped_duid = NULL; gs_strfreev char **lines = NULL; char **iter, *l; GString *s; @@ -593,6 +594,14 @@ nm_dhcp_dhclient_save_duid (const char *leasefile, gsize len = 0; g_return_val_if_fail (leasefile != NULL, FALSE); + + if (!duid) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, + "missing duid"); + g_return_val_if_reached (FALSE); + } + + escaped_duid = nm_dhcp_dhclient_escape_duid (duid); g_return_val_if_fail (escaped_duid != NULL, FALSE); if (g_file_test (leasefile, G_FILE_TEST_EXISTS)) { diff --git a/src/dhcp/nm-dhcp-dhclient-utils.h b/src/dhcp/nm-dhcp-dhclient-utils.h index 911fece488..57a711db14 100644 --- a/src/dhcp/nm-dhcp-dhclient-utils.h +++ b/src/dhcp/nm-dhcp-dhclient-utils.h @@ -40,7 +40,7 @@ GBytes *nm_dhcp_dhclient_unescape_duid (const char *duid); GBytes *nm_dhcp_dhclient_read_duid (const char *leasefile, GError **error); gboolean nm_dhcp_dhclient_save_duid (const char *leasefile, - const char *escaped_duid, + GBytes *duid, GError **error); #endif /* __NETWORKMANAGER_DHCP_DHCLIENT_UTILS_H__ */ diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index bb8892f106..9e8dd6a5f3 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -418,10 +418,7 @@ dhclient_start (NMDhcpClient *client, /* Save the DUID to the leasefile dhclient will actually use */ if (addr_family == AF_INET6) { - gs_free char *escaped = NULL; - - escaped = nm_dhcp_dhclient_escape_duid (duid); - if (!nm_dhcp_dhclient_save_duid (priv->lease_file, escaped, &local)) { + if (!nm_dhcp_dhclient_save_duid (priv->lease_file, duid, &local)) { nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, "failed to save DUID to '%s': %s", diff --git a/src/dhcp/tests/test-dhcp-dhclient.c b/src/dhcp/tests/test-dhcp-dhclient.c index edac42572d..ab1f5551d4 100644 --- a/src/dhcp/tests/test-dhcp-dhclient.c +++ b/src/dhcp/tests/test-dhcp-dhclient.c @@ -760,62 +760,74 @@ test_read_commented_duid_from_leasefile (void) g_assert (duid == NULL); } +/*****************************************************************************/ + +static void +_save_duid (const char *path, + const guint8 *duid_bin, + gsize duid_len) +{ + gs_unref_bytes GBytes *duid = NULL; + GError *error = NULL; + gboolean success; + + g_assert (path); + g_assert (duid_bin); + g_assert (duid_len > 0); + + duid = g_bytes_new (duid_bin, duid_len); + success = nm_dhcp_dhclient_save_duid (path, duid, &error); + nmtst_assert_success (success, error); +} + static void test_write_duid (void) { - const char *duid = "\\000\\001\\000\\001\\027X\\350X\\000#\\025\\010~\\254"; + const guint8 duid[] = { 000, 001, 000, 001, 027, 'X', 0350, 'X', 0, '#', 025, 010, '~', 0254 }; const char *expected_contents = "default-duid \"\\000\\001\\000\\001\\027X\\350X\\000#\\025\\010~\\254\";\n"; GError *error = NULL; - char *contents = NULL; + gs_free char *contents = NULL; gboolean success; const char *path = "test-dhclient-write-duid.leases"; - success = nm_dhcp_dhclient_save_duid (path, duid, &error); - g_assert_no_error (error); - g_assert (success); + _save_duid (path, duid, G_N_ELEMENTS (duid)); success = g_file_get_contents (path, &contents, NULL, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); unlink (path); - g_assert_cmpstr (expected_contents, ==, contents); - g_free (contents); + g_assert_cmpstr (expected_contents, ==, contents); } static void test_write_existing_duid (void) { - const char *duid = "\\000\\001\\000\\001\\023o\\023n\\000\\\"\\372\\214\\326\\302"; + const guint8 duid[] = { 000, 001, 000, 001, 023, 'o', 023, 'n', 000, '\"', 0372, 0214, 0326, 0302 }; const char *original_contents = "default-duid \"\\000\\001\\000\\001\\027X\\350X\\000#\\025\\010~\\254\";\n"; const char *expected_contents = "default-duid \"\\000\\001\\000\\001\\023o\\023n\\000\\\"\\372\\214\\326\\302\";\n"; GError *error = NULL; - char *contents = NULL; + gs_free char *contents = NULL; gboolean success; const char *path = "test-dhclient-write-existing-duid.leases"; success = g_file_set_contents (path, original_contents, -1, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); /* Save other DUID; should be overwritten */ - success = nm_dhcp_dhclient_save_duid (path, duid, &error); - g_assert_no_error (error); - g_assert (success); + _save_duid (path, duid, G_N_ELEMENTS (duid)); /* reread original contents */ success = g_file_get_contents (path, &contents, NULL, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); unlink (path); g_assert_cmpstr (expected_contents, ==, contents); - - g_free (contents); } +static const guint8 DUID_BIN[] = { 000, 001, 000, 001, 023, 'o', 023, 'n', 000, '\"', 0372, 0214, 0326, 0302 }; #define DUID "\\000\\001\\000\\001\\023o\\023n\\000\\\"\\372\\214\\326\\302" + static void test_write_existing_commented_duid (void) { @@ -824,28 +836,22 @@ test_write_existing_commented_duid (void) "default-duid \"" DUID "\";\n" ORIG_CONTENTS; GError *error = NULL; - char *contents = NULL; + gs_free char *contents = NULL; gboolean success; const char *path = "test-dhclient-write-existing-commented-duid.leases"; success = g_file_set_contents (path, ORIG_CONTENTS, -1, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); /* Save other DUID; should be saved on top */ - success = nm_dhcp_dhclient_save_duid (path, DUID, &error); - g_assert_no_error (error); - g_assert (success); + _save_duid (path, DUID_BIN, G_N_ELEMENTS (DUID_BIN)); /* reread original contents */ success = g_file_get_contents (path, &contents, NULL, &error); - g_assert_no_error (error); - g_assert (success); + nmtst_assert_success (success, error); unlink (path); g_assert_cmpstr (expected_contents, ==, contents); - - g_free (contents); #undef ORIG_CONTENTS } @@ -865,8 +871,7 @@ test_write_existing_multiline_duid (void) success = g_file_set_contents (path, ORIG_CONTENTS, -1, &error); nmtst_assert_success (success, error); - success = nm_dhcp_dhclient_save_duid (path, DUID, &error); - nmtst_assert_success (success, error); + _save_duid (path, DUID_BIN, G_N_ELEMENTS (DUID_BIN)); success = g_file_get_contents (path, &contents, NULL, &error); nmtst_assert_success (success, error); From cc0c05688502bb11d2f4862bb03c6c9a8fd53288 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 15:37:55 +0200 Subject: [PATCH 15/27] dhcp: don't pass duid to client ip6_start() and stop() 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. (cherry picked from commit 7d55b1348bf6e0a5810e21723eca8678b02eb162) --- src/dhcp/nm-dhcp-client.c | 11 +++++------ src/dhcp/nm-dhcp-client.h | 4 +--- src/dhcp/nm-dhcp-dhclient.c | 13 +++++-------- src/dhcp/nm-dhcp-dhcpcanon.c | 6 +++--- src/dhcp/nm-dhcp-dhcpcd.c | 5 ++--- src/dhcp/nm-dhcp-systemd.c | 14 ++++++++------ 6 files changed, 24 insertions(+), 29 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index b9e3f743f0..e8bed46c58 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -364,7 +364,7 @@ nm_dhcp_client_stop_pid (pid_t pid, const char *iface) } static void -stop (NMDhcpClient *self, gboolean release, GBytes *duid) +stop (NMDhcpClient *self, gboolean release) { NMDhcpClientPrivate *priv; @@ -562,14 +562,14 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, gs_unref_bytes GBytes *own_client_id = NULL; g_return_val_if_fail (NM_IS_DHCP_CLIENT (self), FALSE); + g_return_val_if_fail (client_id, FALSE); priv = NM_DHCP_CLIENT_GET_PRIVATE (self); + g_return_val_if_fail (priv->pid == -1, FALSE); g_return_val_if_fail (priv->addr_family == AF_INET6, FALSE); g_return_val_if_fail (priv->uuid != NULL, FALSE); - - nm_assert (!priv->client_id); - nm_assert (client_id); + g_return_val_if_fail (!priv->client_id, FALSE); if (!enforce_duid) own_client_id = NM_DHCP_CLIENT_GET_CLASS (self)->get_duid (self); @@ -590,7 +590,6 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, dhcp_anycast_addr, ll_addr, privacy, - priv->client_id, needed_prefixes, error); } @@ -657,7 +656,7 @@ nm_dhcp_client_stop (NMDhcpClient *self, gboolean release) /* Kill the DHCP client */ old_pid = priv->pid; - NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release, priv->client_id); + NM_DHCP_CLIENT_GET_CLASS (self)->stop (self, release); if (old_pid > 0) _LOGI ("canceled DHCP transaction, DHCP client pid %d", old_pid); else diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index 84821f8c23..a349ddba73 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -85,13 +85,11 @@ typedef struct { const char *anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error); void (*stop) (NMDhcpClient *self, - gboolean release, - GBytes *duid); + gboolean release); /** * get_duid: diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 9e8dd6a5f3..468cbc8440 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -339,7 +339,6 @@ create_dhclient_config (NMDhcpDhclient *self, static gboolean dhclient_start (NMDhcpClient *client, const char *mode_opt, - GBytes *duid, gboolean release, pid_t *out_pid, int prefixes, @@ -418,7 +417,9 @@ dhclient_start (NMDhcpClient *client, /* Save the DUID to the leasefile dhclient will actually use */ if (addr_family == AF_INET6) { - if (!nm_dhcp_dhclient_save_duid (priv->lease_file, duid, &local)) { + if (!nm_dhcp_dhclient_save_duid (priv->lease_file, + nm_dhcp_client_get_client_id (client), + &local)) { nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, "failed to save DUID to '%s': %s", @@ -537,7 +538,6 @@ ip4_start (NMDhcpClient *client, nm_dhcp_client_set_client_id (client, new_client_id); } return dhclient_start (client, - NULL, NULL, FALSE, NULL, @@ -550,7 +550,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -578,7 +577,6 @@ ip6_start (NMDhcpClient *client, nm_dhcp_client_get_info_only (NM_DHCP_CLIENT (self)) ? "-S" : "-N", - duid, FALSE, NULL, needed_prefixes, @@ -586,12 +584,12 @@ ip6_start (NMDhcpClient *client, } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhclient *self = NM_DHCP_DHCLIENT (client); NMDhcpDhclientPrivate *priv = NM_DHCP_DHCLIENT_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhclient_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhclient_parent_class)->stop (client, release); if (priv->conf_file) if (remove (priv->conf_file) == -1) @@ -610,7 +608,6 @@ stop (NMDhcpClient *client, gboolean release, GBytes *duid) if (dhclient_start (client, NULL, - duid, TRUE, &rpid, 0, diff --git a/src/dhcp/nm-dhcp-dhcpcanon.c b/src/dhcp/nm-dhcp-dhcpcanon.c index acd3ce2d4c..0f033e2250 100644 --- a/src/dhcp/nm-dhcp-dhcpcanon.c +++ b/src/dhcp/nm-dhcp-dhcpcanon.c @@ -193,20 +193,20 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "dhcpcanon plugin does not support IPv6"); return FALSE; } + static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhcpcanon *self = NM_DHCP_DHCPCANON (client); NMDhcpDhcpcanonPrivate *priv = NM_DHCP_DHCPCANON_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcanon_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcanon_parent_class)->stop (client, release); if (priv->pid_file) { if (remove (priv->pid_file) == -1) diff --git a/src/dhcp/nm-dhcp-dhcpcd.c b/src/dhcp/nm-dhcp-dhcpcd.c index 98ab5342ce..e2a1354f2c 100644 --- a/src/dhcp/nm-dhcp-dhcpcd.c +++ b/src/dhcp/nm-dhcp-dhcpcd.c @@ -187,7 +187,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -196,12 +195,12 @@ ip6_start (NMDhcpClient *client, } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpDhcpcd *self = NM_DHCP_DHCPCD (client); NMDhcpDhcpcdPrivate *priv = NM_DHCP_DHCPCD_GET_PRIVATE (self); - NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcd_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_dhcpcd_parent_class)->stop (client, release); if (priv->pid_file) { if (remove (priv->pid_file) == -1) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 5b7b5fbeeb..aacfc13229 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -895,7 +895,6 @@ ip6_start (NMDhcpClient *client, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, NMSettingIP6ConfigPrivacy privacy, - GBytes *duid, guint needed_prefixes, GError **error) { @@ -907,10 +906,13 @@ ip6_start (NMDhcpClient *client, int r, i; const guint8 *duid_arr; gsize duid_len; + GBytes *duid; - g_assert (priv->client4 == NULL); - g_assert (priv->client6 == NULL); - g_return_val_if_fail (duid != NULL, FALSE); + g_return_val_if_fail (!priv->client4, FALSE); + g_return_val_if_fail (!priv->client6, FALSE); + + duid = nm_dhcp_client_get_client_id (client); + g_return_val_if_fail (duid, FALSE); duid_arr = g_bytes_get_data (duid, &duid_len); if (!duid_arr || duid_len < 2) @@ -1013,13 +1015,13 @@ errout: } static void -stop (NMDhcpClient *client, gboolean release, GBytes *duid) +stop (NMDhcpClient *client, gboolean release) { NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); int r = 0; - NM_DHCP_CLIENT_CLASS (nm_dhcp_systemd_parent_class)->stop (client, release, duid); + NM_DHCP_CLIENT_CLASS (nm_dhcp_systemd_parent_class)->stop (client, release); _LOGT ("dhcp-client%d: stop %p", priv->client4 ? '4' : '6', From 93eb69a0e9f9b9c00f607f3a4256503b14fb4392 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 16:19:18 +0200 Subject: [PATCH 16/27] dhcp: use cleanup attribute for get_dhclient_leasefile() (cherry picked from commit b833d68d685ebbbc9bd818c4b178b621fc546fa8) --- src/dhcp/nm-dhcp-dhclient.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 468cbc8440..e3d01e9b90 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -121,8 +121,8 @@ get_dhclient_leasefile (int addr_family, const char *uuid, char **out_preferred_path) { - char *rundir_path; - char *path; + gs_free char *rundir_path = NULL; + gs_free char *path = NULL; /* First, see if the lease file is in /run */ rundir_path = g_strdup_printf (NMRUNDIR "/dhclient%s-%s-%s.lease", @@ -132,7 +132,7 @@ get_dhclient_leasefile (int addr_family, if (g_file_test (rundir_path, G_FILE_TEST_EXISTS)) { NM_SET_OUT (out_preferred_path, g_strdup (rundir_path)); - return rundir_path; + return g_steal_pointer (&rundir_path); } /* /var/lib/NetworkManager is the preferred leasefile path */ @@ -142,18 +142,14 @@ get_dhclient_leasefile (int addr_family, iface); if (g_file_test (path, G_FILE_TEST_EXISTS)) { - g_free (rundir_path); NM_SET_OUT (out_preferred_path, g_strdup (path)); - return path; + return g_steal_pointer (&path); } - if (nm_config_get_configure_and_quit (nm_config_get ()) == NM_CONFIG_CONFIGURE_AND_QUIT_INITRD) { - g_free (path); - path = rundir_path; - } else { - g_free (rundir_path); - } - NM_SET_OUT (out_preferred_path, g_steal_pointer (&path)); + if (nm_config_get_configure_and_quit (nm_config_get ()) == NM_CONFIG_CONFIGURE_AND_QUIT_INITRD) + NM_SET_OUT (out_preferred_path, g_steal_pointer (&rundir_path)); + else + NM_SET_OUT (out_preferred_path, g_steal_pointer (&path)); /* If the leasefile we're looking for doesn't exist yet in the new location * (eg, /var/lib/NetworkManager) then look in old locations to maintain @@ -166,17 +162,16 @@ get_dhclient_leasefile (int addr_family, path = g_strdup_printf (LOCALSTATEDIR "/lib/dhcp/dhclient%s-%s-%s.lease", _addr_family_to_path_part (addr_family), uuid, iface); if (g_file_test (path, G_FILE_TEST_EXISTS)) - return path; + return g_steal_pointer (&path); /* Old Red Hat and Fedora location */ g_free (path); path = g_strdup_printf (LOCALSTATEDIR "/lib/dhclient/dhclient%s-%s-%s.lease", _addr_family_to_path_part (addr_family), uuid, iface); if (g_file_test (path, G_FILE_TEST_EXISTS)) - return path; + return g_steal_pointer (&path); /* Fail */ - g_free (path); return NULL; } From 1fecb5ec9dcdf9ca8c1c629777f2bb33e4e42321 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 Oct 2018 16:43:13 +0200 Subject: [PATCH 17/27] dhcp: minor refactoring return paths in NMDhcpDhclient.get_duid() (cherry picked from commit d6d2b7296fb1ddac06e167c3ec398e1c0ceff41c) --- src/dhcp/nm-dhcp-dhclient-utils.c | 1 + src/dhcp/nm-dhcp-dhclient.c | 24 +++++++++++------------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/dhcp/nm-dhcp-dhclient-utils.c b/src/dhcp/nm-dhcp-dhclient-utils.c index 716e15d06c..8128b5c8df 100644 --- a/src/dhcp/nm-dhcp-dhclient-utils.c +++ b/src/dhcp/nm-dhcp-dhclient-utils.c @@ -547,6 +547,7 @@ error: #define DUID_PREFIX "default-duid \"" +/* Beware: @error may be unset even if the function returns %NULL. */ GBytes * nm_dhcp_dhclient_read_duid (const char *leasefile, GError **error) { diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index e3d01e9b90..18aa6a7fed 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -619,7 +619,7 @@ get_duid (NMDhcpClient *client) NMDhcpDhclient *self = NM_DHCP_DHCLIENT (client); NMDhcpDhclientPrivate *priv = NM_DHCP_DHCLIENT_GET_PRIVATE (self); GBytes *duid = NULL; - char *leasefile; + gs_free char *leasefile = NULL; GError *error = NULL; /* Look in interface-specific leasefile first for backwards compat */ @@ -630,25 +630,23 @@ get_duid (NMDhcpClient *client) if (leasefile) { _LOGD ("looking for DUID in '%s'", leasefile); duid = nm_dhcp_dhclient_read_duid (leasefile, &error); - if (error) { _LOGW ("failed to read leasefile '%s': %s", leasefile, error->message); g_clear_error (&error); } - g_free (leasefile); + if (duid) + return duid; } - if (!duid) { - /* Otherwise read the default machine-wide DUID */ - _LOGD ("looking for default DUID in '%s'", priv->def_leasefile); - duid = nm_dhcp_dhclient_read_duid (priv->def_leasefile, &error); - if (error) { - _LOGW ("failed to read leasefile '%s': %s", - priv->def_leasefile, - error->message); - g_clear_error (&error); - } + /* Otherwise read the default machine-wide DUID */ + _LOGD ("looking for default DUID in '%s'", priv->def_leasefile); + duid = nm_dhcp_dhclient_read_duid (priv->def_leasefile, &error); + if (error) { + _LOGW ("failed to read leasefile '%s': %s", + priv->def_leasefile, + error->message); + g_clear_error (&error); } return duid; From 71e3db4f56f66ea8e46f2bed153162dcae17beee Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 25 Oct 2018 10:02:11 +0200 Subject: [PATCH 18/27] dhcp/trivial: wrap lines in calling client_start() 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. (cherry picked from commit ce1cfd7232273dcb577f9fc783b7694dd191ae86) --- src/dhcp/nm-dhcp-dhclient.c | 14 +++++++++-- src/dhcp/nm-dhcp-manager.c | 50 +++++++++++++++++++++++++++++++------ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/dhcp/nm-dhcp-dhclient.c b/src/dhcp/nm-dhcp-dhclient.c index 18aa6a7fed..0146c8b465 100644 --- a/src/dhcp/nm-dhcp-dhclient.c +++ b/src/dhcp/nm-dhcp-dhclient.c @@ -322,8 +322,18 @@ create_dhclient_config (NMDhcpDhclient *self, else _LOGD ("no existing dhclient configuration to merge"); - if (!merge_dhclient_config (self, addr_family, iface, new, client_id, dhcp_anycast_addr, - hostname, timeout, use_fqdn, orig, out_new_client_id, &error)) { + if (!merge_dhclient_config (self, + addr_family, + iface, + new, + client_id, + dhcp_anycast_addr, + hostname, + timeout, + use_fqdn, + orig, + out_new_client_id, + &error)) { _LOGW ("error creating dhclient configuration: %s", error->message); g_clear_error (&error); } diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 372f241d47..8e90f00bb8 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -311,10 +311,27 @@ nm_dhcp_manager_start_ip4 (NMDhcpManager *self, } } - return client_start (self, AF_INET, multi_idx, iface, ifindex, hwaddr, uuid, - route_table, route_metric, NULL, - dhcp_client_id, 0, timeout, dhcp_anycast_addr, hostname, - use_fqdn, FALSE, 0, last_ip_address, 0, error); + return client_start (self, + AF_INET, + multi_idx, + iface, + ifindex, + hwaddr, + uuid, + route_table, + route_metric, + NULL, + dhcp_client_id, + FALSE, + timeout, + dhcp_anycast_addr, + hostname, + use_fqdn, + FALSE, + 0, + last_ip_address, + 0, + error); } /* Caller owns a reference to the NMDhcpClient on return */ @@ -349,10 +366,27 @@ nm_dhcp_manager_start_ip6 (NMDhcpManager *self, /* Always prefer the explicit dhcp-hostname if given */ hostname = dhcp_hostname ?: priv->default_hostname; } - return client_start (self, AF_INET6, multi_idx, iface, ifindex, hwaddr, uuid, - route_table, route_metric, ll_addr, duid, enforce_duid, - timeout, dhcp_anycast_addr, hostname, TRUE, info_only, - privacy, NULL, needed_prefixes, error); + return client_start (self, + AF_INET6, + multi_idx, + iface, + ifindex, + hwaddr, + uuid, + route_table, + route_metric, + ll_addr, + duid, + enforce_duid, + timeout, + dhcp_anycast_addr, + hostname, + TRUE, + info_only, + privacy, + NULL, + needed_prefixes, + error); } void From d54b444cb439f8d90ff2af9f3fd490266267c0b2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Oct 2018 12:11:26 +0100 Subject: [PATCH 19/27] dhcp: cleanup initializing IPv4 client-id for internal DHCP - 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. (cherry picked from commit c3e7e6170dae14d553e650d9bd1d8b449e936f90) --- src/dhcp/nm-dhcp-client.c | 4 +- src/dhcp/nm-dhcp-manager.c | 1 + src/dhcp/nm-dhcp-systemd.c | 153 +++++++++++++++++++------------------ 3 files changed, 84 insertions(+), 74 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index e8bed46c58..dca5a6f086 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -900,11 +900,13 @@ set_property (GObject *object, guint prop_id, case PROP_IFACE: /* construct-only */ priv->iface = g_value_dup_string (value); + g_return_if_fail ( priv->iface + && nm_utils_is_valid_iface_name (priv->iface, NULL)); break; case PROP_IFINDEX: /* construct-only */ priv->ifindex = g_value_get_int (value); - g_warn_if_fail (priv->ifindex > 0); + g_return_if_fail (priv->ifindex > 0); break; case PROP_HWADDR: /* construct-only */ diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 8e90f00bb8..4bccee7534 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -181,6 +181,7 @@ client_start (NMDhcpManager *self, gsize hwaddr_len; g_return_val_if_fail (NM_IS_DHCP_MANAGER (self), NULL); + g_return_val_if_fail (iface, NULL); g_return_val_if_fail (ifindex > 0, NULL); g_return_val_if_fail (uuid != NULL, NULL); g_return_val_if_fail (!dhcp_client_id || g_bytes_get_size (dhcp_client_id) >= 2, NULL); diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index aacfc13229..f8b7ad4675 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -34,6 +34,7 @@ #include "nm-utils.h" #include "nm-config.h" #include "nm-dhcp-utils.h" +#include "nm-core-utils.h" #include "NetworkManagerUtils.h" #include "platform/nm-platform.h" #include "nm-dhcp-client-logging.h" @@ -482,22 +483,6 @@ get_leasefile_path (int addr_family, const char *iface, const char *uuid) /*****************************************************************************/ -static void -_save_client_id (NMDhcpSystemd *self, - uint8_t type, - const uint8_t *client_id, - size_t len) -{ - g_return_if_fail (self != NULL); - g_return_if_fail (client_id != NULL); - g_return_if_fail (len > 0); - - if (!nm_dhcp_client_get_client_id (NM_DHCP_CLIENT (self))) { - nm_dhcp_client_set_client_id_bin (NM_DHCP_CLIENT (self), - type, client_id, len); - } -} - static void bound4_handle (NMDhcpSystemd *self) { @@ -529,17 +514,9 @@ bound4_handle (NMDhcpSystemd *self) TRUE, &error); if (ip4_config) { - const uint8_t *client_id = NULL; - size_t client_id_len = 0; - uint8_t type = 0; - add_requests_to_options (options, dhcp4_requests); dhcp_lease_save (lease, priv->lease_file); - sd_dhcp_client_get_client_id (priv->client4, &type, &client_id, &client_id_len); - if (client_id) - _save_client_id (self, type, client_id, client_id_len); - nm_dhcp_client_set_state (NM_DHCP_CLIENT (self), NM_DHCP_STATE_BOUND, NM_IP_CONFIG_CAST (ip4_config), @@ -583,9 +560,9 @@ dhcp_event_cb (sd_dhcp_client *client, int event, gpointer user_data) } static guint16 -get_arp_type (GBytes *hwaddr) +get_arp_type (gsize hwaddr_len) { - switch (g_bytes_get_size (hwaddr)) { + switch (hwaddr_len) { case ETH_ALEN: return ARPHRD_ETHER; case INFINIBAND_ALEN: @@ -603,22 +580,27 @@ ip4_start (NMDhcpClient *client, { NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); - const char *iface = nm_dhcp_client_get_iface (client); GBytes *hwaddr; + const uint8_t *hwaddr_arr; + gsize hwaddr_len; + guint16 arptype; sd_dhcp_lease *lease = NULL; - GBytes *override_client_id; - const uint8_t *client_id = NULL; - size_t client_id_len = 0; + GBytes *client_id; + gs_unref_bytes GBytes *client_id_new = NULL; + const uint8_t *client_id_arr; + size_t client_id_len; struct in_addr last_addr = { 0 }; const char *hostname; int r, i; gboolean success = FALSE; - g_assert (priv->client4 == NULL); - g_assert (priv->client6 == NULL); + g_return_val_if_fail (!priv->client4, FALSE); + g_return_val_if_fail (!priv->client6, FALSE); g_free (priv->lease_file); - priv->lease_file = get_leasefile_path (AF_INET, iface, nm_dhcp_client_get_uuid (client)); + priv->lease_file = get_leasefile_path (AF_INET, + nm_dhcp_client_get_iface (client), + nm_dhcp_client_get_uuid (client)); r = sd_dhcp_client_new (&priv->client4, FALSE); if (r < 0) { @@ -635,22 +617,23 @@ ip4_start (NMDhcpClient *client, } hwaddr = nm_dhcp_client_get_hw_addr (client); - if (hwaddr) { - const uint8_t *data; - gsize len; - - data = g_bytes_get_data (hwaddr, &len); - r = sd_dhcp_client_set_mac (priv->client4, - data, - len, - get_arp_type (hwaddr)); - if (r < 0) { - nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); - goto errout; - } + if ( !hwaddr + || !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) + || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); + goto errout; + } + r = sd_dhcp_client_set_mac (priv->client4, + hwaddr_arr, + hwaddr_len, + arptype); + if (r < 0) { + nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); + goto errout; } - r = sd_dhcp_client_set_ifindex (priv->client4, nm_dhcp_client_get_ifindex (client)); + r = sd_dhcp_client_set_ifindex (priv->client4, + nm_dhcp_client_get_ifindex (client)); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set ifindex: %s"); goto errout; @@ -677,27 +660,44 @@ ip4_start (NMDhcpClient *client, } } - override_client_id = nm_dhcp_client_get_client_id (client); - if (override_client_id) { - client_id = g_bytes_get_data (override_client_id, &client_id_len); - nm_assert (client_id && client_id_len >= 2); - sd_dhcp_client_set_client_id (priv->client4, - client_id[0], - client_id + 1, - NM_MIN (client_id_len - 1, _NM_SD_MAX_CLIENT_ID_LEN)); - } else if (lease) { - r = sd_dhcp_lease_get_client_id (lease, (const void **) &client_id, &client_id_len); - if (r == 0 && client_id_len >= 2) { - sd_dhcp_client_set_client_id (priv->client4, - client_id[0], - client_id + 1, - client_id_len - 1); - _save_client_id (NM_DHCP_SYSTEMD (client), - client_id[0], - client_id + 1, - client_id_len - 1); + client_id = nm_dhcp_client_get_client_id (client); + if ( !client_id + && lease) { + r = sd_dhcp_lease_get_client_id (lease, + (const void **) &client_id_arr, + &client_id_len); + if ( r >= 0 + && client_id_len >= 2) { + client_id_new = g_bytes_new (client_id_arr, client_id_len); + client_id = client_id_new; } } + if (!client_id) { + client_id_new = nm_utils_dhcp_client_id_systemd_node_specific (TRUE, + nm_dhcp_client_get_iface (client)); + client_id = client_id_new; + } + + if ( !(client_id_arr = g_bytes_get_data (client_id, &client_id_len)) + || client_id_len < 2) { + + /* invalid client-ids are not expected. */ + nm_assert_not_reached (); + + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "no valid IPv4 client-id"); + goto errout; + } + + r = sd_dhcp_client_set_client_id (priv->client4, + client_id_arr[0], + client_id_arr + 1, + NM_MIN (client_id_len - 1, _NM_SD_MAX_CLIENT_ID_LEN)); + if (r < 0) { + nm_utils_error_set_errno (error, r, "failed to set IPv4 client-id: %s"); + goto errout; + } + + nm_dhcp_client_set_client_id (client, client_id); /* Add requested options */ for (i = 0; dhcp4_requests[i].name; i++) { @@ -954,21 +954,28 @@ ip6_start (NMDhcpClient *client, hwaddr = nm_dhcp_client_get_hw_addr (client); if (hwaddr) { - const uint8_t *data; - gsize len; + const uint8_t *hwaddr_arr; + gsize hwaddr_len; + guint16 arptype; + + if ( !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) + || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); + goto errout; + } - data = g_bytes_get_data (hwaddr, &len); r = sd_dhcp6_client_set_mac (priv->client6, - data, - len, - get_arp_type (hwaddr)); + hwaddr_arr, + hwaddr_len, + arptype); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); goto errout; } } - r = sd_dhcp6_client_set_ifindex (priv->client6, nm_dhcp_client_get_ifindex (client)); + r = sd_dhcp6_client_set_ifindex (priv->client6, + nm_dhcp_client_get_ifindex (client)); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set ifindex: %s"); goto errout; From b21652b8d03047c0a8a67849d36189c00765e024 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 29 Oct 2018 12:47:27 +0100 Subject: [PATCH 20/27] dhcp: don't load IPv4 client-id from lease file 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? (cherry picked from commit 5b9bc174d1f53c51a5afabd862fe859a4b4bbe62) --- clients/common/settings-docs.h.in | 2 +- libnm-core/nm-setting-ip4-config.c | 4 ++-- src/dhcp/nm-dhcp-systemd.c | 15 --------------- 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index f07769c059..fe4a4c5ef9 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -197,7 +197,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP_TUNNEL_TTL N_("The TTL to assign to tunneled packets. 0 is a special value meaning that packets inherit the TTL value.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ADDRESSES N_("Array of IP addresses.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DAD_TIMEOUT N_("Timeout in milliseconds used to check for the presence of duplicate IP addresses on the network. If an address conflict is detected, the activation will fail. A zero value means that no duplicate address detection is performed, -1 means the default value (either configuration ipvx.dad-timeout override or zero). A value greater than zero is a timeout in milliseconds. The property is currently implemented only for IPv4.") -#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If unset, a globally configured default is used. If still unset, the client-id from the last lease is reused.") +#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If unset, a globally configured default is used. If still unset, the default depends on the DHCP plugin.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_FQDN N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified FQDN will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-hostname\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_HOSTNAME N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified name will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-fqdn\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_SEND_HOSTNAME N_("If TRUE, a hostname is sent to the DHCP server when acquiring a lease. Some DHCP servers use this hostname to update DNS databases, essentially providing a static hostname for the computer. If the \"dhcp-hostname\" property is NULL and this property is TRUE, the current persistent hostname of the computer is sent.") diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c index 19f1cc8d40..686a50b044 100644 --- a/libnm-core/nm-setting-ip4-config.c +++ b/libnm-core/nm-setting-ip4-config.c @@ -724,8 +724,8 @@ nm_setting_ip4_config_class_init (NMSettingIP4ConfigClass *klass) * The special value "stable" is supported to generate a type 0 client identifier based * on the stable-id (see connection.stable-id) and a per-host key. * - * If unset, a globally configured default is used. If still unset, the - * client-id from the last lease is reused. + * If unset, a globally configured default is used. If still unset, the default + * depends on the DHCP plugin. **/ /* ---ifcfg-rh--- * property: dhcp-client-id diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index f8b7ad4675..655db15658 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -661,17 +661,6 @@ ip4_start (NMDhcpClient *client, } client_id = nm_dhcp_client_get_client_id (client); - if ( !client_id - && lease) { - r = sd_dhcp_lease_get_client_id (lease, - (const void **) &client_id_arr, - &client_id_len); - if ( r >= 0 - && client_id_len >= 2) { - client_id_new = g_bytes_new (client_id_arr, client_id_len); - client_id = client_id_new; - } - } if (!client_id) { client_id_new = nm_utils_dhcp_client_id_systemd_node_specific (TRUE, nm_dhcp_client_get_iface (client)); @@ -900,7 +889,6 @@ ip6_start (NMDhcpClient *client, { NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); - const char *iface = nm_dhcp_client_get_iface (client); GBytes *hwaddr; const char *hostname; int r, i; @@ -918,9 +906,6 @@ ip6_start (NMDhcpClient *client, if (!duid_arr || duid_len < 2) g_return_val_if_reached (FALSE); - g_free (priv->lease_file); - priv->lease_file = get_leasefile_path (AF_INET6, iface, nm_dhcp_client_get_uuid (client)); - r = sd_dhcp6_client_new (&priv->client6); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to create dhcp-client: %s"); From a31edf0b3e48d5f27d40787257b90c63170675fb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 10:18:23 +0100 Subject: [PATCH 21/27] dhcp: cleanup error handling in internal DHCP client's start - 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. (cherry picked from commit ab314065b8928c12347b40b25f200020c59fcbda) --- src/dhcp/nm-dhcp-systemd.c | 156 +++++++++++++++++++------------------ 1 file changed, 79 insertions(+), 77 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 655db15658..28aaf3908a 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -578,13 +578,14 @@ ip4_start (NMDhcpClient *client, const char *last_ip4_address, GError **error) { + nm_auto (sd_dhcp_client_unrefp) sd_dhcp_client *sd_client = NULL; NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); + gs_free char *lease_file = NULL; GBytes *hwaddr; const uint8_t *hwaddr_arr; gsize hwaddr_len; guint16 arptype; - sd_dhcp_lease *lease = NULL; GBytes *client_id; gs_unref_bytes GBytes *client_id_new = NULL; const uint8_t *client_id_arr; @@ -592,28 +593,22 @@ ip4_start (NMDhcpClient *client, struct in_addr last_addr = { 0 }; const char *hostname; int r, i; - gboolean success = FALSE; g_return_val_if_fail (!priv->client4, FALSE); g_return_val_if_fail (!priv->client6, FALSE); - g_free (priv->lease_file); - priv->lease_file = get_leasefile_path (AF_INET, - nm_dhcp_client_get_iface (client), - nm_dhcp_client_get_uuid (client)); - - r = sd_dhcp_client_new (&priv->client4, FALSE); + r = sd_dhcp_client_new (&sd_client, FALSE); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to create dhcp-client: %s"); return FALSE; } - _LOGT ("dhcp-client4: set %p", priv->client4); + _LOGT ("dhcp-client4: set %p", sd_client); - r = sd_dhcp_client_attach_event (priv->client4, NULL, 0); + r = sd_dhcp_client_attach_event (sd_client, NULL, 0); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to attach event: %s"); - goto errout; + return FALSE; } hwaddr = nm_dhcp_client_get_hw_addr (client); @@ -621,42 +616,43 @@ ip4_start (NMDhcpClient *client, || !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); - goto errout; + return FALSE; } - r = sd_dhcp_client_set_mac (priv->client4, + r = sd_dhcp_client_set_mac (sd_client, hwaddr_arr, hwaddr_len, arptype); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); - goto errout; + return FALSE; } - r = sd_dhcp_client_set_ifindex (priv->client4, + r = sd_dhcp_client_set_ifindex (sd_client, nm_dhcp_client_get_ifindex (client)); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set ifindex: %s"); - goto errout; + return FALSE; } - r = sd_dhcp_client_set_callback (priv->client4, dhcp_event_cb, client); - if (r < 0) { - nm_utils_error_set_errno (error, r, "failed to set callback: %s"); - goto errout; - } - - dhcp_lease_load (&lease, priv->lease_file); + lease_file = get_leasefile_path (AF_INET, + nm_dhcp_client_get_iface (client), + nm_dhcp_client_get_uuid (client)); if (last_ip4_address) inet_pton (AF_INET, last_ip4_address, &last_addr); - else if (lease) - sd_dhcp_lease_get_address (lease, &last_addr); + else { + nm_auto (sd_dhcp_lease_unrefp) sd_dhcp_lease *lease = NULL; + + dhcp_lease_load (&lease, lease_file); + if (lease) + sd_dhcp_lease_get_address (lease, &last_addr); + } if (last_addr.s_addr) { - r = sd_dhcp_client_set_request_address (priv->client4, &last_addr); + r = sd_dhcp_client_set_request_address (sd_client, &last_addr); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set last IPv4 address: %s"); - goto errout; + return FALSE; } } @@ -674,24 +670,22 @@ ip4_start (NMDhcpClient *client, nm_assert_not_reached (); nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "no valid IPv4 client-id"); - goto errout; + return FALSE; } - r = sd_dhcp_client_set_client_id (priv->client4, + r = sd_dhcp_client_set_client_id (sd_client, client_id_arr[0], client_id_arr + 1, NM_MIN (client_id_len - 1, _NM_SD_MAX_CLIENT_ID_LEN)); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set IPv4 client-id: %s"); - goto errout; + return FALSE; } - nm_dhcp_client_set_client_id (client, client_id); - /* Add requested options */ for (i = 0; dhcp4_requests[i].name; i++) { if (dhcp4_requests[i].include) - sd_dhcp_client_set_request_option (priv->client4, dhcp4_requests[i].num); + sd_dhcp_client_set_request_option (sd_client, dhcp4_requests[i].num); } hostname = nm_dhcp_client_get_hostname (client); @@ -700,28 +694,36 @@ ip4_start (NMDhcpClient *client, * only based on whether the hostname has a domain part or not. At the * moment there is no way to force one or another. */ - r = sd_dhcp_client_set_hostname (priv->client4, hostname); + r = sd_dhcp_client_set_hostname (sd_client, hostname); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set DHCP hostname: %s"); - goto errout; + return FALSE; } } + r = sd_dhcp_client_set_callback (sd_client, dhcp_event_cb, client); + if (r < 0) { + nm_utils_error_set_errno (error, r, "failed to set callback: %s"); + return FALSE; + } + + priv->client4 = g_steal_pointer (&sd_client); + + g_free (priv->lease_file); + priv->lease_file = g_steal_pointer (&lease_file); + + nm_dhcp_client_set_client_id (client, client_id); + r = sd_dhcp_client_start (priv->client4); if (r < 0) { + sd_dhcp_client_set_callback (priv->client4, NULL, NULL); + nm_clear_pointer (&priv->client4, sd_dhcp_client_unref); nm_utils_error_set_errno (error, r, "failed to start DHCP client: %s"); - goto errout; + return FALSE; } nm_dhcp_client_start_timeout (client); - - success = TRUE; - -errout: - sd_dhcp_lease_unref (lease); - if (!success) - sd_dhcp_client_unref (g_steal_pointer (&priv->client4)); - return success; + return TRUE; } static NMIP6Config * @@ -889,6 +891,7 @@ ip6_start (NMDhcpClient *client, { NMDhcpSystemd *self = NM_DHCP_SYSTEMD (client); NMDhcpSystemdPrivate *priv = NM_DHCP_SYSTEMD_GET_PRIVATE (self); + nm_auto (sd_dhcp6_client_unrefp) sd_dhcp6_client *sd_client = NULL; GBytes *hwaddr; const char *hostname; int r, i; @@ -899,14 +902,14 @@ ip6_start (NMDhcpClient *client, g_return_val_if_fail (!priv->client4, FALSE); g_return_val_if_fail (!priv->client6, FALSE); - duid = nm_dhcp_client_get_client_id (client); - g_return_val_if_fail (duid, FALSE); - - duid_arr = g_bytes_get_data (duid, &duid_len); - if (!duid_arr || duid_len < 2) + if ( !(duid = nm_dhcp_client_get_client_id (client)) + || !(duid_arr = g_bytes_get_data (duid, &duid_len)) + || duid_len < 2) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "missing DUID"); g_return_val_if_reached (FALSE); + } - r = sd_dhcp6_client_new (&priv->client6); + r = sd_dhcp6_client_new (&sd_client); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to create dhcp-client: %s"); return FALSE; @@ -917,12 +920,12 @@ ip6_start (NMDhcpClient *client, needed_prefixes); } - _LOGT ("dhcp-client6: set %p", priv->client6); + _LOGT ("dhcp-client6: set %p", sd_client); if (nm_dhcp_client_get_info_only (client)) - sd_dhcp6_client_set_information_request (priv->client6, 1); + sd_dhcp6_client_set_information_request (sd_client, 1); - r = sd_dhcp6_client_set_duid (priv->client6, + r = sd_dhcp6_client_set_duid (sd_client, unaligned_read_be16 (&duid_arr[0]), &duid_arr[2], duid_len - 2); @@ -931,10 +934,10 @@ ip6_start (NMDhcpClient *client, return FALSE; } - r = sd_dhcp6_client_attach_event (priv->client6, NULL, 0); + r = sd_dhcp6_client_attach_event (sd_client, NULL, 0); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to attach event: %s"); - goto errout; + return FALSE; } hwaddr = nm_dhcp_client_get_hw_addr (client); @@ -946,64 +949,63 @@ ip6_start (NMDhcpClient *client, if ( !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); - goto errout; + return FALSE; } - r = sd_dhcp6_client_set_mac (priv->client6, + r = sd_dhcp6_client_set_mac (sd_client, hwaddr_arr, hwaddr_len, arptype); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); - goto errout; + return FALSE; } } - r = sd_dhcp6_client_set_ifindex (priv->client6, + r = sd_dhcp6_client_set_ifindex (sd_client, nm_dhcp_client_get_ifindex (client)); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set ifindex: %s"); - goto errout; - } - - r = sd_dhcp6_client_set_callback (priv->client6, dhcp6_event_cb, client); - if (r < 0) { - nm_utils_error_set_errno (error, r, "failed to set callback: %s"); - goto errout; + return FALSE; } /* Add requested options */ for (i = 0; dhcp6_requests[i].name; i++) { if (dhcp6_requests[i].include) - sd_dhcp6_client_set_request_option (priv->client6, dhcp6_requests[i].num); + sd_dhcp6_client_set_request_option (sd_client, dhcp6_requests[i].num); } - r = sd_dhcp6_client_set_local_address (priv->client6, ll_addr); + r = sd_dhcp6_client_set_local_address (sd_client, ll_addr); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set local address: %s"); - goto errout; + return FALSE; } hostname = nm_dhcp_client_get_hostname (client); - r = sd_dhcp6_client_set_fqdn (priv->client6, hostname); + r = sd_dhcp6_client_set_fqdn (sd_client, hostname); if (r < 0) { nm_utils_error_set_errno (error, r, "failed to set DHCP hostname: %s"); - goto errout; + return FALSE; } + r = sd_dhcp6_client_set_callback (sd_client, dhcp6_event_cb, client); + if (r < 0) { + nm_utils_error_set_errno (error, r, "failed to set callback: %s"); + return FALSE; + } + + priv->client6 = g_steal_pointer (&sd_client); + r = sd_dhcp6_client_start (priv->client6); if (r < 0) { + sd_dhcp6_client_set_callback (priv->client6, NULL, NULL); + nm_clear_pointer (&priv->client6, sd_dhcp6_client_unref); nm_utils_error_set_errno (error, r, "failed to start client: %s"); - goto errout; + return FALSE; } nm_dhcp_client_start_timeout (client); - return TRUE; - -errout: - sd_dhcp6_client_unref (g_steal_pointer (&priv->client6)); - return FALSE; } static void From f70e762a4f53fe17240f0e9971d0da9b6635338b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 12:07:09 +0100 Subject: [PATCH 22/27] all: add "${MAC}" substituion for "connection.stable-id" We already had "${DEVICE}" which uses the interface name. In times of predictable interface naming, that works well. It allows the user to generate IDs per device which don't change when the hardware is replaced. "${MAC}" is similar, except that is uses the permanent MAC address of the device. The substitution results in the empty word, if the device has no permanent MAC address (like software devices). The per-device substitutions "${DEVICE}" and "${MAC}" are especially interesting with "connection.multi-connect=multiple". (cherry picked from commit 7ffbf7127603a935236785f6a59a104583194df4) --- clients/common/settings-docs.h.in | 2 +- libnm-core/nm-setting-connection.c | 4 ++-- src/devices/nm-device.c | 8 ++++++++ src/nm-core-utils.c | 3 +++ src/nm-core-utils.h | 1 + src/tests/test-general.c | 3 ++- 6 files changed, 17 insertions(+), 4 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index fe4a4c5ef9..122d50cd61 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -144,7 +144,7 @@ #define DESCRIBE_DOC_NM_SETTING_CONNECTION_READ_ONLY N_("FALSE if the connection can be modified using the provided settings service's D-Bus interface with the right privileges, or TRUE if the connection is read-only and cannot be modified.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_SECONDARIES N_("List of connection UUIDs that should be activated when the base connection itself is activated. Currently only VPN connections are supported.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_SLAVE_TYPE N_("Setting name of the device type of this slave's master connection (eg, \"bond\"), or NULL if this connection is not a slave.") -#define DESCRIBE_DOC_NM_SETTING_CONNECTION_STABLE_ID N_("This represents the identity of the connection used for various purposes. It allows to configure multiple profiles to share the identity. Also, the stable-id can contain placeholders that are substituted dynamically and deterministically depending on the context. The stable-id is used for generating IPv6 stable private addresses with ipv6.addr-gen-mode=stable-privacy. It is also used to seed the generated cloned MAC address for ethernet.cloned-mac-address=stable and wifi.cloned-mac-address=stable. It is also used as DHCP client identifier with ipv4.dhcp-client-id=stable and to derive the DHCP DUID with ipv6.dhcp-duid=stable-[llt,ll,uuid]. Note that depending on the context where it is used, other parameters are also seeded into the generation algorithm. For example, a per-host key is commonly also included, so that different systems end up generating different IDs. Or with ipv6.addr-gen-mode=stable-privacy, also the device's name is included, so that different interfaces yield different addresses. The '$' character is treated special to perform dynamic substitutions at runtime. Currently supported are \"${CONNECTION}\", \"${DEVICE}\", \"${BOOT}\", \"${RANDOM}\". These effectively create unique IDs per-connection, per-device, per-boot, or every time. Note that \"${DEVICE}\" corresponds the the interface name of the device. Any unrecognized patterns following '$' are treated verbatim, however are reserved for future use. You are thus advised to avoid '$' or escape it as \"$$\". For example, set it to \"${CONNECTION}-${BOOT}-${DEVICE}\" to create a unique id for this connection that changes with every reboot and differs depending on the interface where the profile activates. If the value is unset, a global connection default is consulted. If the value is still unset, the default is similar to \"${CONNECTION}\" and uses a unique, fixed ID for the connection.") +#define DESCRIBE_DOC_NM_SETTING_CONNECTION_STABLE_ID N_("This represents the identity of the connection used for various purposes. It allows to configure multiple profiles to share the identity. Also, the stable-id can contain placeholders that are substituted dynamically and deterministically depending on the context. The stable-id is used for generating IPv6 stable private addresses with ipv6.addr-gen-mode=stable-privacy. It is also used to seed the generated cloned MAC address for ethernet.cloned-mac-address=stable and wifi.cloned-mac-address=stable. It is also used as DHCP client identifier with ipv4.dhcp-client-id=stable and to derive the DHCP DUID with ipv6.dhcp-duid=stable-[llt,ll,uuid]. Note that depending on the context where it is used, other parameters are also seeded into the generation algorithm. For example, a per-host key is commonly also included, so that different systems end up generating different IDs. Or with ipv6.addr-gen-mode=stable-privacy, also the device's name is included, so that different interfaces yield different addresses. The '$' character is treated special to perform dynamic substitutions at runtime. Currently supported are \"${CONNECTION}\", \"${DEVICE}\", \"${MAC}\", \"${BOOT}\", \"${RANDOM}\". These effectively create unique IDs per-connection, per-device, per-boot, or every time. Note that \"${DEVICE}\" corresponds the the interface name of the device and \"${MAC}\" is the permanent MAC address of the device. Any unrecognized patterns following '$' are treated verbatim, however are reserved for future use. You are thus advised to avoid '$' or escape it as \"$$\". For example, set it to \"${CONNECTION}-${BOOT}-${DEVICE}\" to create a unique id for this connection that changes with every reboot and differs depending on the interface where the profile activates. If the value is unset, a global connection default is consulted. If the value is still unset, the default is similar to \"${CONNECTION}\" and uses a unique, fixed ID for the connection.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_TIMESTAMP N_("The time, in seconds since the Unix Epoch, that the connection was last _successfully_ fully activated. NetworkManager updates the connection timestamp periodically when the connection is active to ensure that an active connection has the latest timestamp. The property is only meant for reading (changes to this property will not be preserved).") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_TYPE N_("Base type of the connection. For hardware-dependent connections, should contain the setting name of the hardware-type specific setting (ie, \"802-3-ethernet\" or \"802-11-wireless\" or \"bluetooth\", etc), and for non-hardware dependent connections like VPN or otherwise, should contain the setting name of that setting type (ie, \"vpn\" or \"bridge\", etc).") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_UUID N_("A universally unique identifier for the connection, for example generated with libuuid. It should be assigned when the connection is created, and never changed as long as the connection still applies to the same network. For example, it should not be changed when the \"id\" property or NMSettingIP4Config changes, but might need to be re-created when the Wi-Fi SSID, mobile broadband network provider, or \"type\" property changes. The UUID must be in the format \"2815492f-7e56-435e-b2e9-246bd7cdc664\" (ie, contains only hexadecimal characters and \"-\").") diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 50240bc934..44021b9c72 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1626,11 +1626,11 @@ nm_setting_connection_class_init (NMSettingConnectionClass *klass) * name is included, so that different interfaces yield different addresses. * * The '$' character is treated special to perform dynamic substitutions - * at runtime. Currently supported are "${CONNECTION}", "${DEVICE}", + * at runtime. Currently supported are "${CONNECTION}", "${DEVICE}", "${MAC}", * "${BOOT}", "${RANDOM}". * These effectively create unique IDs per-connection, per-device, per-boot, * or every time. Note that "${DEVICE}" corresponds the the interface name of the - * device. + * device and "${MAC}" is the permanent MAC address of the device. * Any unrecognized patterns following '$' are treated verbatim, however * are reserved for future use. You are thus advised to avoid '$' or * escape it as "$$". diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 779ad9fba7..d0a4a0555f 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1263,6 +1263,8 @@ _get_stable_id (NMDevice *self, gs_free char *generated = NULL; NMUtilsStableType stable_type; NMSettingConnection *s_con; + gboolean hwaddr_is_fake; + const char *hwaddr; const char *stable_id; const char *uuid; @@ -1279,8 +1281,14 @@ _get_stable_id (NMDevice *self, uuid = nm_connection_get_uuid (connection); + /* the cloned-mac-address may be generated based on the stable-id. + * Thus, at this point, we can only use the permanant MAC address + * as seed. */ + hwaddr = nm_device_get_permanent_hw_address_full (self, TRUE, &hwaddr_is_fake); + stable_type = nm_utils_stable_id_parse (stable_id, nm_device_get_ip_iface (self), + !hwaddr_is_fake ? hwaddr : NULL, nm_utils_get_boot_id_str (), uuid, &generated); diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 18538e0939..ec5dbe635d 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -3092,6 +3092,7 @@ _stable_id_append (GString *str, NMUtilsStableType nm_utils_stable_id_parse (const char *stable_id, const char *deviceid, + const char *hwaddr, const char *bootid, const char *uuid, char **out_generated) @@ -3170,6 +3171,8 @@ nm_utils_stable_id_parse (const char *stable_id, _stable_id_append (str, bootid); else if (CHECK_PREFIX ("${DEVICE}")) _stable_id_append (str, deviceid); + else if (CHECK_PREFIX ("${MAC}")) + _stable_id_append (str, hwaddr); else if (g_str_has_prefix (&stable_id[i], "${RANDOM}")) { /* RANDOM makes not so much sense for cloned-mac-address * as the result is simmilar to specyifing "cloned-mac-address=random". diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index fb1effef36..ab9dade210 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -346,6 +346,7 @@ typedef enum { NMUtilsStableType nm_utils_stable_id_parse (const char *stable_id, const char *deviceid, + const char *hwaddr, const char *bootid, const char *uuid, char **out_generated); diff --git a/src/tests/test-general.c b/src/tests/test-general.c index 773fc0bd4a..372fca91d2 100644 --- a/src/tests/test-general.c +++ b/src/tests/test-general.c @@ -1739,7 +1739,7 @@ do_test_stable_id_parse (const char *stable_id, else g_assert (stable_id); - stable_type = nm_utils_stable_id_parse (stable_id, "_DEVICE", "_BOOT", "_CONNECTION", &generated); + stable_type = nm_utils_stable_id_parse (stable_id, "_DEVICE", "_MAC", "_BOOT", "_CONNECTION", &generated); g_assert_cmpint (expected_stable_type, ==, stable_type); @@ -1778,6 +1778,7 @@ test_stable_id_parse (void) _parse_generated ("x${BOOT}", "x${BOOT}=5{_BOOT}"); _parse_generated ("x${BOOT}${CONNECTION}", "x${BOOT}=5{_BOOT}${CONNECTION}=11{_CONNECTION}"); _parse_generated ("xX${BOOT}yY${CONNECTION}zZ", "xX${BOOT}=5{_BOOT}yY${CONNECTION}=11{_CONNECTION}zZ"); + _parse_generated ("${MAC}x", "${MAC}=4{_MAC}x"); _parse_random ("${RANDOM}"); _parse_random (" ${RANDOM}"); _parse_random ("${BOOT}${RANDOM}"); From 0983f14be65236fe173d832ac4a9317a4c92bc03 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 11:44:05 +0100 Subject: [PATCH 23/27] doc: add hint about ipv4.dhcp-client-id=stable (cherry picked from commit 5ef93c3323ae311e0a8be542fa9a8a12eb490307) --- clients/common/settings-docs.h.in | 4 ++-- libnm-core/nm-setting-ip4-config.c | 4 +++- libnm-core/nm-setting-ip6-config.c | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index 122d50cd61..4add809ade 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -197,7 +197,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP_TUNNEL_TTL N_("The TTL to assign to tunneled packets. 0 is a special value meaning that packets inherit the TTL value.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ADDRESSES N_("Array of IP addresses.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DAD_TIMEOUT N_("Timeout in milliseconds used to check for the presence of duplicate IP addresses on the network. If an address conflict is detected, the activation will fail. A zero value means that no duplicate address detection is performed, -1 means the default value (either configuration ipvx.dad-timeout override or zero). A value greater than zero is a timeout in milliseconds. The property is currently implemented only for IPv4.") -#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If unset, a globally configured default is used. If still unset, the default depends on the DHCP plugin.") +#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If you set the stable-id, you may want to include the \"${DEVICE}\" or \"{$MAC}\" specifier to get a per-device key. If unset, a globally configured default is used. If still unset, the default depends on the DHCP plugin.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_FQDN N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified FQDN will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-hostname\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_HOSTNAME N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified name will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-fqdn\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_SEND_HOSTNAME N_("If TRUE, a hostname is sent to the DHCP server when acquiring a lease. Some DHCP servers use this hostname to update DNS databases, essentially providing a static hostname for the computer. If the \"dhcp-hostname\" property is NULL and this property is TRUE, the current persistent hostname of the computer is sent.") @@ -218,7 +218,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE N_("Configure method for creating the address for use with RFC4862 IPv6 Stateless Address Autoconfiguration. The permitted values are: NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_EUI64 (0) or NM_SETTING_IP6_CONFIG_ADDR_GEN_MODE_STABLE_PRIVACY (1). If the property is set to EUI64, the addresses will be generated using the interface tokens derived from hardware address. This makes the host part of the address to stay constant, making it possible to track host's presence when it changes networks. The address changes when the interface hardware is replaced. The value of stable-privacy enables use of cryptographically secure hash of a secret host-specific key along with the connection's stable-id and the network address as specified by RFC7217. This makes it impossible to use the address track host's presence, and makes the address stable when the network interface hardware is replaced. On D-Bus, the absence of an addr-gen-mode setting equals enabling stable-privacy. For keyfile plugin, the absence of the setting on disk means EUI64 so that the property doesn't change on upgrade from older versions. Note that this setting is distinct from the Privacy Extensions as configured by \"ip6-privacy\" property and it does not affect the temporary addresses configured with this option.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_ADDRESSES N_("Array of IP addresses.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DAD_TIMEOUT N_("Timeout in milliseconds used to check for the presence of duplicate IP addresses on the network. If an address conflict is detected, the activation will fail. A zero value means that no duplicate address detection is performed, -1 means the default value (either configuration ipvx.dad-timeout override or zero). A value greater than zero is a timeout in milliseconds. The property is currently implemented only for IPv4.") -#define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DHCP_DUID N_("A string containing the DHCPv6 Unique Identifier (DUID) used by the dhcp client to identify itself to DHCPv6 servers (RFC 3315). The DUID is carried in the Client Identifier option. If the property is a hex string ('aa:bb:cc') it is interpreted as a binary DUID and filled as an opaque value in the Client Identifier option. The special value \"lease\" will retrieve the DUID previously used from the lease file belonging to the connection. If no DUID is found and \"dhclient\" is the configured dhcp client, the DUID is searched in the system-wide dhclient lease file. If still no DUID is found, or another dhcp client is used, a global and permanent DUID-UUID (RFC 6355) will be generated based on the machine-id. The special values \"llt\" and \"ll\" will generate a DUID of type LLT or LL (see RFC 3315) based on the current MAC address of the device. In order to try providing a stable DUID-LLT, the time field will contain a constant timestamp that is used globally (for all profiles) and persisted to disk. The special values \"stable-llt\", \"stable-ll\" and \"stable-uuid\" will generate a DUID of the corresponding type, derived from the connection's stable-id and a per-host unique key. So, the link-layer address of \"stable-ll\" and \"stable-llt\" will be a generated address derived from the stable id. The DUID-LLT time value in the \"stable-llt\" option will be picked among a static timespan of three years (the upper bound of the interval is the same constant timestamp used in \"llt\"). When the property is unset, the global value provided for \"ipv6.dhcp-duid\" is used. If no global value is provided, the default \"lease\" value is assumed.") +#define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DHCP_DUID N_("A string containing the DHCPv6 Unique Identifier (DUID) used by the dhcp client to identify itself to DHCPv6 servers (RFC 3315). The DUID is carried in the Client Identifier option. If the property is a hex string ('aa:bb:cc') it is interpreted as a binary DUID and filled as an opaque value in the Client Identifier option. The special value \"lease\" will retrieve the DUID previously used from the lease file belonging to the connection. If no DUID is found and \"dhclient\" is the configured dhcp client, the DUID is searched in the system-wide dhclient lease file. If still no DUID is found, or another dhcp client is used, a global and permanent DUID-UUID (RFC 6355) will be generated based on the machine-id. The special values \"llt\" and \"ll\" will generate a DUID of type LLT or LL (see RFC 3315) based on the current MAC address of the device. In order to try providing a stable DUID-LLT, the time field will contain a constant timestamp that is used globally (for all profiles) and persisted to disk. The special values \"stable-llt\", \"stable-ll\" and \"stable-uuid\" will generate a DUID of the corresponding type, derived from the connection's stable-id and a per-host unique key. You may want to include the \"${DEVICE}\" or \"${MAC}\" specifier in the stable-id, in case this profile gets activated on multiple devices. So, the link-layer address of \"stable-ll\" and \"stable-llt\" will be a generated address derived from the stable id. The DUID-LLT time value in the \"stable-llt\" option will be picked among a static timespan of three years (the upper bound of the interval is the same constant timestamp used in \"llt\"). When the property is unset, the global value provided for \"ipv6.dhcp-duid\" is used. If no global value is provided, the default \"lease\" value is assumed.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DHCP_HOSTNAME N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified name will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-fqdn\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DHCP_SEND_HOSTNAME N_("If TRUE, a hostname is sent to the DHCP server when acquiring a lease. Some DHCP servers use this hostname to update DNS databases, essentially providing a static hostname for the computer. If the \"dhcp-hostname\" property is NULL and this property is TRUE, the current persistent hostname of the computer is sent.") #define DESCRIBE_DOC_NM_SETTING_IP6_CONFIG_DHCP_TIMEOUT N_("A timeout for a DHCP transaction in seconds.") diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c index 686a50b044..008b0f16cf 100644 --- a/libnm-core/nm-setting-ip4-config.c +++ b/libnm-core/nm-setting-ip4-config.c @@ -722,7 +722,9 @@ nm_setting_ip4_config_class_init (NMSettingIP4ConfigClass *klass) * type of links. * * The special value "stable" is supported to generate a type 0 client identifier based - * on the stable-id (see connection.stable-id) and a per-host key. + * on the stable-id (see connection.stable-id) and a per-host key. If you set the + * stable-id, you may want to include the "${DEVICE}" or "{$MAC}" specifier to get a + * per-device key. * * If unset, a globally configured default is used. If still unset, the default * depends on the DHCP plugin. diff --git a/libnm-core/nm-setting-ip6-config.c b/libnm-core/nm-setting-ip6-config.c index a55fd80195..fa65be910a 100644 --- a/libnm-core/nm-setting-ip6-config.c +++ b/libnm-core/nm-setting-ip6-config.c @@ -852,7 +852,8 @@ nm_setting_ip6_config_class_init (NMSettingIP6ConfigClass *klass) * * The special values "stable-llt", "stable-ll" and "stable-uuid" will generate * a DUID of the corresponding type, derived from the connection's stable-id and - * a per-host unique key. + * a per-host unique key. You may want to include the "${DEVICE}" or "${MAC}" specifier + * in the stable-id, in case this profile gets activated on multiple devices. * So, the link-layer address of "stable-ll" and "stable-llt" will be a generated * address derived from the stable id. The DUID-LLT time value in the "stable-llt" * option will be picked among a static timespan of three years (the upper bound From cb50647144e77a5933289d2f92282e59056a2c5c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 15:49:07 +0100 Subject: [PATCH 24/27] dhcp: always require hwaddr in internal DHCP clint ip6_start() Note how client_start() in NMDhcpManager already asserts that we have a MAC address. It's always present, like for the IPv6 case. (cherry picked from commit dfdd4e3bd36057860460578cd8a677b35d15a101) --- src/dhcp/nm-dhcp-systemd.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/dhcp/nm-dhcp-systemd.c b/src/dhcp/nm-dhcp-systemd.c index 28aaf3908a..c33de3accd 100644 --- a/src/dhcp/nm-dhcp-systemd.c +++ b/src/dhcp/nm-dhcp-systemd.c @@ -898,6 +898,9 @@ ip6_start (NMDhcpClient *client, const guint8 *duid_arr; gsize duid_len; GBytes *duid; + const uint8_t *hwaddr_arr; + gsize hwaddr_len; + guint16 arptype; g_return_val_if_fail (!priv->client4, FALSE); g_return_val_if_fail (!priv->client6, FALSE); @@ -941,25 +944,19 @@ ip6_start (NMDhcpClient *client, } hwaddr = nm_dhcp_client_get_hw_addr (client); - if (hwaddr) { - const uint8_t *hwaddr_arr; - gsize hwaddr_len; - guint16 arptype; - - if ( !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) - || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { - nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); - return FALSE; - } - - r = sd_dhcp6_client_set_mac (sd_client, - hwaddr_arr, - hwaddr_len, - arptype); - if (r < 0) { - nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); - return FALSE; - } + if ( !hwaddr + || !(hwaddr_arr = g_bytes_get_data (hwaddr, &hwaddr_len)) + || (arptype = get_arp_type (hwaddr_len)) == ARPHRD_NONE) { + nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, "invalid MAC address"); + return FALSE; + } + r = sd_dhcp6_client_set_mac (sd_client, + hwaddr_arr, + hwaddr_len, + arptype); + if (r < 0) { + nm_utils_error_set_errno (error, r, "failed to set MAC address: %s"); + return FALSE; } r = sd_dhcp6_client_set_ifindex (sd_client, From 525830a4a05117a2ecd682b104eb61d47bb4a105 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 17:05:03 +0100 Subject: [PATCH 25/27] dhcp: add "ipv4.dhcp-client-id=duid" setting Add a new mode for the DHCPv4 client identifier. "duid" is what the internal (systemd) DHCP client already does by default. It is also the same as used by systemd-networkd's "ClientIdentifier=duid" setting. What we still lack (compared to networkd) are a way to overwrite IAID and the DUID. Previously, this mode was used by the internal DHCP plugin by default. However, it could not be explicitly configured. In general, our default values should also be explicitly selectable. Now the "duid" client identifier can also be used with the "dhclient" plugin. (cherry picked from commit 8861ac2976795e05aa6a5f5e6c822977bf950ed3) --- clients/common/settings-docs.h.in | 2 +- libnm-core/nm-setting-ip4-config.c | 3 +++ src/devices/nm-device.c | 6 ++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index 4add809ade..6ecdf3100b 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -197,7 +197,7 @@ #define DESCRIBE_DOC_NM_SETTING_IP_TUNNEL_TTL N_("The TTL to assign to tunneled packets. 0 is a special value meaning that packets inherit the TTL value.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ADDRESSES N_("Array of IP addresses.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DAD_TIMEOUT N_("Timeout in milliseconds used to check for the presence of duplicate IP addresses on the network. If an address conflict is detected, the activation will fail. A zero value means that no duplicate address detection is performed, -1 means the default value (either configuration ipvx.dad-timeout override or zero). A value greater than zero is a timeout in milliseconds. The property is currently implemented only for IPv4.") -#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If you set the stable-id, you may want to include the \"${DEVICE}\" or \"{$MAC}\" specifier to get a per-device key. If unset, a globally configured default is used. If still unset, the default depends on the DHCP plugin.") +#define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_CLIENT_ID N_("A string sent to the DHCP server to identify the local machine which the DHCP server may use to customize the DHCP lease and options. When the property is a hex string ('aa:bb:cc') it is interpreted as a binary client ID, in which case the first byte is assumed to be the 'type' field as per RFC 2132 section 9.14 and the remaining bytes may be an hardware address (e.g. '01:xx:xx:xx:xx:xx:xx' where 1 is the Ethernet ARP type and the rest is a MAC address). If the property is not a hex string it is considered as a non-hardware-address client ID and the 'type' field is set to 0. The special values \"mac\" and \"perm-mac\" are supported, which use the current or permanent MAC address of the device to generate a client identifier with type ethernet (01). Currently, these options only work for ethernet type of links. The special value \"duid\" generates a RFC4361-compliant client identifier based on a hash of the interface name as IAID and /etc/machine-id. The special value \"stable\" is supported to generate a type 0 client identifier based on the stable-id (see connection.stable-id) and a per-host key. If you set the stable-id, you may want to include the \"${DEVICE}\" or \"{$MAC}\" specifier to get a per-device key. If unset, a globally configured default is used. If still unset, the default depends on the DHCP plugin.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_FQDN N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified FQDN will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-hostname\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_HOSTNAME N_("If the \"dhcp-send-hostname\" property is TRUE, then the specified name will be sent to the DHCP server when acquiring a lease. This property and \"dhcp-fqdn\" are mutually exclusive and cannot be set at the same time.") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_DHCP_SEND_HOSTNAME N_("If TRUE, a hostname is sent to the DHCP server when acquiring a lease. Some DHCP servers use this hostname to update DNS databases, essentially providing a static hostname for the computer. If the \"dhcp-hostname\" property is NULL and this property is TRUE, the current persistent hostname of the computer is sent.") diff --git a/libnm-core/nm-setting-ip4-config.c b/libnm-core/nm-setting-ip4-config.c index 008b0f16cf..38056abc94 100644 --- a/libnm-core/nm-setting-ip4-config.c +++ b/libnm-core/nm-setting-ip4-config.c @@ -721,6 +721,9 @@ nm_setting_ip4_config_class_init (NMSettingIP4ConfigClass *klass) * with type ethernet (01). Currently, these options only work for ethernet * type of links. * + * The special value "duid" generates a RFC4361-compliant client identifier based + * on a hash of the interface name as IAID and /etc/machine-id. + * * The special value "stable" is supported to generate a type 0 client identifier based * on the stable-id (see connection.stable-id) and a per-host key. If you set the * stable-id, you may want to include the "${DEVICE}" or "{$MAC}" specifier to get a diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index d0a4a0555f..30d7ae3681 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7540,6 +7540,12 @@ dhcp4_get_client_id (NMDevice *self, goto out_good; } + if (nm_streq (client_id, "duid")) { + result = nm_utils_dhcp_client_id_systemd_node_specific (TRUE, + nm_device_get_ip_iface (self)); + goto out_good; + } + if (nm_streq (client_id, "stable")) { nm_auto_free_checksum GChecksum *sum = NULL; guint8 digest[NM_UTILS_CHECKSUM_LENGTH_SHA1]; From 22ffd02f87116420755a85a8a50309187fcdb757 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Nov 2018 17:48:26 +0100 Subject: [PATCH 26/27] core/trivial: add code comment about DHCP client-id/DUID (cherry picked from commit e9630e7d5795ff3171b04b817acc0c8d9eb97776) --- src/dhcp/nm-dhcp-manager.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 4bccee7534..878336178e 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -234,6 +234,32 @@ client_start (NMDhcpManager *self, c_list_link_tail (&priv->dhcp_client_lst_head, &client->dhcp_client_lst); g_signal_connect (client, NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED, G_CALLBACK (client_state_changed), self); + /* unfortunately, our implementations work differently per address-family regarding client-id/DUID. + * + * - for IPv4, the calling code may determine a client-id (from NM's connection profile). + * If present, it is taken. If not present, the DHCP plugin uses a plugin specific default. + * - for "internal" plugin, the default is just "duid". + * - for "dhclient", we try to get the configuration from dhclient's /etc/dhcp or fallback + * to whatever dhclient uses by default. + * We do it this way, because for dhclient the user may configure a default + * outside of NM, and we want to honor that. Worse, dhclient could be a wapper + * script where the wrapper script overwrites the client-id. We need to distinguish + * between: force a particular client-id and leave it unspecified to whatever dhclient + * wants. + * + * - for IPv6, the calling code always determines a client-id. It also specifies @enforce_duid, + * to determine whether the given client-id must be used. + * - for "internal" plugin @enforce_duid doesn't matter and the given client-id is + * always used. + * - for "dhclient", @enforce_duid FALSE means to first try to load the DUID from the + * lease file, and only otherwise fallback to the given client-id. + * - other plugins don't support DHCPv6. + * It's done this way, so that existing dhclient setups don't change behavior on upgrade. + * + * This difference is cumbersome and only exists because of "dhclient" which supports hacking the + * default outside of NetworkManager API. + */ + if (addr_family == AF_INET) { success = nm_dhcp_client_start_ip4 (client, dhcp_client_id, From 62a5ff605b4403f4dbda09eb124774ccb4988eca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Nov 2018 07:44:07 +0100 Subject: [PATCH 27/27] dhcp: initialize hostname as construct-property 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. (cherry picked from commit 787f4b57cda7ba03fde412bb7693baf8f5f67ef5) --- src/dhcp/nm-dhcp-client.c | 22 ++++++++++++++-------- src/dhcp/nm-dhcp-client.h | 3 +-- src/dhcp/nm-dhcp-manager.c | 3 +-- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/dhcp/nm-dhcp-client.c b/src/dhcp/nm-dhcp-client.c index dca5a6f086..54615f1b2a 100644 --- a/src/dhcp/nm-dhcp-client.c +++ b/src/dhcp/nm-dhcp-client.c @@ -61,6 +61,7 @@ NM_GOBJECT_PROPERTIES_DEFINE (NMDhcpClient, PROP_ROUTE_TABLE, PROP_TIMEOUT, PROP_UUID, + PROP_HOSTNAME, ); typedef struct _NMDhcpClientPrivate { @@ -511,7 +512,6 @@ gboolean nm_dhcp_client_start_ip4 (NMDhcpClient *self, GBytes *client_id, const char *dhcp_anycast_addr, - const char *hostname, const char *last_ip4_address, GError **error) { @@ -531,9 +531,6 @@ nm_dhcp_client_start_ip4 (NMDhcpClient *self, nm_dhcp_client_set_client_id (self, client_id); - g_clear_pointer (&priv->hostname, g_free); - priv->hostname = g_strdup (hostname); - return NM_DHCP_CLIENT_GET_CLASS (self)->ip4_start (self, dhcp_anycast_addr, last_ip4_address, @@ -552,7 +549,6 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, gboolean enforce_duid, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, - const char *hostname, NMSettingIP6ConfigPrivacy privacy, guint needed_prefixes, GError **error) @@ -578,9 +574,6 @@ nm_dhcp_client_start_ip6 (NMDhcpClient *self, own_client_id ?: client_id, FALSE); - g_clear_pointer (&priv->hostname, g_free); - priv->hostname = g_strdup (hostname); - if (priv->timeout == NM_DHCP_TIMEOUT_INFINITY) _LOGI ("activation: beginning transaction (no timeout)"); else @@ -860,6 +853,9 @@ get_property (GObject *object, guint prop_id, case PROP_UUID: g_value_set_string (value, priv->uuid); break; + case PROP_HOSTNAME: + g_value_set_string (value, priv->hostname); + break; case PROP_ROUTE_METRIC: g_value_set_uint (value, priv->route_metric); break; @@ -922,6 +918,10 @@ set_property (GObject *object, guint prop_id, /* construct-only */ priv->uuid = g_value_dup_string (value); break; + case PROP_HOSTNAME: + /* construct-only */ + priv->hostname = g_value_dup_string (value); + break; case PROP_ROUTE_TABLE: priv->route_table = g_value_get_uint (value); break; @@ -1030,6 +1030,12 @@ nm_dhcp_client_class_init (NMDhcpClientClass *client_class) G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS); + obj_properties[PROP_HOSTNAME] = + g_param_spec_string (NM_DHCP_CLIENT_HOSTNAME, "", "", + NULL, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | + G_PARAM_STATIC_STRINGS); + obj_properties[PROP_ROUTE_TABLE] = g_param_spec_uint (NM_DHCP_CLIENT_ROUTE_TABLE, "", "", 0, G_MAXUINT32, RT_TABLE_MAIN, diff --git a/src/dhcp/nm-dhcp-client.h b/src/dhcp/nm-dhcp-client.h index a349ddba73..8be5071717 100644 --- a/src/dhcp/nm-dhcp-client.h +++ b/src/dhcp/nm-dhcp-client.h @@ -41,6 +41,7 @@ #define NM_DHCP_CLIENT_IFINDEX "ifindex" #define NM_DHCP_CLIENT_INTERFACE "iface" #define NM_DHCP_CLIENT_MULTI_IDX "multi-idx" +#define NM_DHCP_CLIENT_HOSTNAME "hostname" #define NM_DHCP_CLIENT_ROUTE_METRIC "route-metric" #define NM_DHCP_CLIENT_ROUTE_TABLE "route-table" #define NM_DHCP_CLIENT_TIMEOUT "timeout" @@ -142,7 +143,6 @@ gboolean nm_dhcp_client_get_use_fqdn (NMDhcpClient *self); gboolean nm_dhcp_client_start_ip4 (NMDhcpClient *self, GBytes *client_id, const char *dhcp_anycast_addr, - const char *hostname, const char *last_ip4_address, GError **error); @@ -151,7 +151,6 @@ gboolean nm_dhcp_client_start_ip6 (NMDhcpClient *self, gboolean enforce_duid, const char *dhcp_anycast_addr, const struct in6_addr *ll_addr, - const char *hostname, NMSettingIP6ConfigPrivacy privacy, guint needed_prefixes, GError **error); diff --git a/src/dhcp/nm-dhcp-manager.c b/src/dhcp/nm-dhcp-manager.c index 878336178e..a51c6e3847 100644 --- a/src/dhcp/nm-dhcp-manager.c +++ b/src/dhcp/nm-dhcp-manager.c @@ -222,6 +222,7 @@ client_start (NMDhcpManager *self, NM_DHCP_CLIENT_IFINDEX, ifindex, NM_DHCP_CLIENT_HWADDR, hwaddr, NM_DHCP_CLIENT_UUID, uuid, + NM_DHCP_CLIENT_HOSTNAME, hostname, NM_DHCP_CLIENT_ROUTE_TABLE, (guint) route_table, NM_DHCP_CLIENT_ROUTE_METRIC, (guint) route_metric, NM_DHCP_CLIENT_TIMEOUT, (guint) timeout, @@ -264,7 +265,6 @@ client_start (NMDhcpManager *self, success = nm_dhcp_client_start_ip4 (client, dhcp_client_id, dhcp_anycast_addr, - hostname, last_ip4_address, error); } else { @@ -273,7 +273,6 @@ client_start (NMDhcpManager *self, enforce_duid, dhcp_anycast_addr, ipv6_ll_addr, - hostname, privacy, needed_prefixes, error);