This basically comes down to "NetworkManager should read the user's mind". Preserving the changed MAC address after the connection goes down is as likely to be the wrong thing as it is to be the right thing.

There is already a supported way to do this in NM: create a new connection profile, with a cloned MAC address, and connect to that when you want to use the alternate MAC address. If you want to be able to do it with macchanger instead, then don't use NetworkManager.

(In reply to comment #1)
> This basically comes down to "NetworkManager should read the user's mind".

Nope. It comes down to "NetworkManager should know about external MAC address changes and should not revert such changes." That sounds a bit more valid than the former interpretation.

I don't say whether using macchanger is good or not. Basicaly, in many other cases NetworkManager is now following external changes and acting on them. There's no apparent reason not to do that for MAC addresses if the use case describe in this bug report is accepted.

It's not "reverting" it, it's resetting the device back to its base state when the connection is disconnected, and then setting it up again with the on-disk connection state when reconnecting. The changes in pavlix/runtime would make us notice that the MAC address had changed while the connection is up, but it wouldn't (automatically) save the changes to disk, and so you wouldn't expect the MAC change to be reapplied when reconnecting.

(In reply to comment #3)
> It's not "reverting" it, it's resetting

Not that I see much difference...

> the device back to its base state

How exactly is the base state determined?

> when
> the connection is disconnected, and then setting it up again with the on-disk
> connection state when reconnecting.

> The changes in pavlix/runtime would make us
> notice that the MAC address had changed while the connection is up, but it
> wouldn't (automatically) save the changes to disk,

Correct.

> and so you wouldn't expect the MAC change to be reapplied when reconnecting.

That's true.

As you said, NetworkManager tries to reset the device back to its "base state". I think the only thing we disagree on is what is this "base state" means.

If I understood correctly, according to syslog and to the original 1b49f94 commit, which introduced the clone Mac addresses, this "base state" was the "permanent hardware address". The hardcoded MAC of the device.

But in fact, and as you seems to mention in your comments, NetworkManager resets it to the "initial hardware address". Which is the MAC address the device had when NetworkManager started. Our initial analysis of the problem was indeed wrong on this point, and mislead by the intention of the original patch and the log message.

We believe the proper "base state" should be the hardware address as NetworkManager found it before making the connection that has just been disconnected. In other words, before establishing a connection, the "initial hardware address" should be updated with the hardware address in use before a connection is established. Otherwise I fail to understand the practical difference between the "permanent" and "initial" hardware address, both handled internally by NetworkManager.

I don't think this implies reading the user's mind. We are not talking about changes that could have been made to the MAC address *while* the connection was up, but rolling back to the state the device was *before* the connection.

And, regarding the user experience and scenario that we proposed, we already know there is a way of working around that through the GUI as you said. But we said this adds more complexity because, at least in GNOME Shell 3.4, the window for editing network connections is not available anymore directly from the notification area as it used to be with GNOME 2. You have to go to Shell → Applications → System Tools → Network Connections. I personally thought it had disappears until someone else told me it was there.

So I still think this bug introduces a change that is both dangerous and invisible at first sight for people who have been trained to protect themselves using tools as macchanger when NetworkManager was not capable of spoofing MAC addresses, and they might now be tracked while thinking they are safe.

(In reply to comment #5)
> We believe the proper "base state" should be the hardware address as
> NetworkManager found it before making the connection that has just been
> disconnected.

That's not what you describe above; you were saying there that when the user changes the MAC address while a connection is up, and then the connection goes down and comes back up again, that NM should NOT reset the MAC to what it was before the connection was established.

We never meant to say that. Could you quote which part of our bug report made you believe that? There is probably some misunderstanding somewhere.

We always said that a MAC address that was changed *before* establishing a connection, was not reset to the same state when NM leaves this connection.

Here is a summary of the scenario we are considering:

1. Permanent MAC address is address [A].
2. NM starts with no connection established;
   address [A] is considered to be the "initial" MAC address by NM.
3. The user changes the MAC address to [B] using macchanger.
4. NM establishes a connection with no cloned MAC address, it uses MAC address [B].
5. The connection is lost due to a weak signal.
   NM resets the MAC address to the "initial" address [A].
7. NM connects back to the same network using address [A] instead of [B].

Result: address [A] is leaked to this network even if the user explicitly spoofed it to address [B] before establishing that connection.

Desired result: when disconnecting in step 5, NM rollbacks to address [B] = the one had before establishing the connection in step 4.

We believe this can be achieved by updating the "initial" address kept by NM to the current MAC before establishing a connection.

ah, indeed, I thought in your example the user was changing the MAC address after enabling the connection

(Another Tails developer here.)

I'm glad we eventually understood each other :)

Then, we can now fast-forward to what seems to be the only blocker left: do we agree that updating the "initial" address remembered by NM to the current MAC, before establishing a connection, would make sense and be a desirable behavior?

Ping? I'd like to start looking for someone to implement this, but it would be good to make sure we agree on what should be done first.

*** Bug 721193 has been marked as a duplicate of this bug. ***

Is it known why NM actually ever changes the MAC address?
It seems like there should be no reason for this action for NM at all.
It should read the new MAC if it has been changed externally, but resetting it is too much.

I think Yuri is right. NM shouldn't touch any mac address ever unless the user specifically asked to do so through the user interface or configuration files. It should indeed not try to read the users mind by making counter-intuitive and dangerous choices. Why did you assume in the first place that it is appropriate to change mac addresses (well worse, reset them to hardware) without the user requesting this?

As of version 1.0.8 I see the following behavior:

I use a udev rule to call macchanger on an ethernet device, when NM later uses this device to create a "Wired Connection 1", it will reset the MAC address to the hardware value. 

Removing all references of MAC addresses from the GUI doesn't change this. 
Removing "/etc/NetworkManager/system-connections/Wired connection 1" + rebooting doesn't change this behavior neither.

I don't see this behavior on wifi devices.

All that being said maybe network devices can even leak the real mac address before getting changed on udev add. I haven't come round to testing that, but I see that Tails blacklists all network kernel modules before spoofing the address. For that reason it's a dubious feature for NM anyways to give users the impression that they can spoof a MAC address from the GUI because potentially they have already leaked their MAC address by the time NM runs. 

For that reason I would remove the feature from NM all together, keeping it's essential functionality of facilitating easy basic configuration and connection without having to use the command line. 

When I decided to spoof my MAC address I never even considered that NM might be the tool for this. I didn't realize it messed with MAC until I noticed it sabotaging my attempts of never leaking the hardware MAC.

work in progress: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/device-inital-mac-addr-bgo708820

*** Bug 767420 has been marked as a duplicate of this bug. ***

as Thomas points out, bug 767420 is due to the same cause.  I'd recommend retitling this bug report:

  "NetworkManager resets MAC address to the permanent one upon (re)connection"

since it affects systems where the MAC address was already changed before network-manager ever got to handle the device.

thanks for the WIP branch, Thomas!  Looking at it, it appears to leave the default value for cloned-mac-address as "permanent".  I think the default should be "leave", in line with the arguments made by Naja in Comment 13.

As a side note, i'm not sure that the term "cloned-mac-address" matches this config parameter any more: there are many different (and useful) options available here besides cloning.  Maybe "assigned-mac-address" makes more sense?

It's also unclear to me how `ethernet.cloned-mac-address` interacts with `wifi.cloned-mac-address` (or is a wifi device simply not considered an ethernet device?), or how either of these settings interacts with wifi.mac-address-randomization.

Is there documentation cleanup needed, or deprecation of some config options?

(In reply to Daniel Kahn Gillmor from comment #17)
> thanks for the WIP branch, Thomas!  Looking at it, it appears to leave the
> default value for cloned-mac-address as "permanent".  I think the default
> should be "leave", in line with the arguments made by Naja in Comment 13.

yes, the last fallback default is still "permanent", that was the behavior since we added the cloned-mac-address property 2 years ago. I'd rather not change behavior there now (although we changed behavior before).

Anyway, no strong opinion about this. Note that you will be able to overwrite that default with a file like /etc/NetworkManager/conf.d/10-no-cloned-mac-address.conf

[connection.no-cloned]
ethernet.cloned-mac-address=leave
wifi.cloned-mac-address.leave
 

> As a side note, i'm not sure that the term "cloned-mac-address" matches this
> config parameter any more: there are many different (and useful) options
> available here besides cloning.  Maybe "assigned-mac-address" makes more
> sense?

You are right. But renaming an existing property and deprecating the old one is painful too and adds implementation and documentation overhead. I think it's actually worse then live with than ill name. 


> It's also unclear to me how `ethernet.cloned-mac-address` interacts with
> `wifi.cloned-mac-address` (or is a wifi device simply not considered an
> ethernet device?),

They don't interact, because a [ethernet] section is only applicable to certain device types (ethernet, bridge, bond, team), while [wifi] only applies to wireless devices.

> or how either of these settings interacts with
> wifi.mac-address-randomization.

Yes, there is some overlap. In hindsight, wifi.mac-address-randomization should not have been added as a separate property. This still needs to be evaluated. Currently wifi.mac-address-randomization works by telling wpa-supplicant to randomize the address. It's not clear to me yet whether we want supplicant to set the MAC address (for association -- not during scanning!). Still needs to be solved...


> Is there documentation cleanup needed, or deprecation of some config options?

Yes.

(In reply to Thomas Haller from comment #14)
> work in progress:
> https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/device-
> inital-mac-addr-bgo708820

In general looks good to me; pushed a trivial fixup.


> device: refactor nm_device_get_applied_setting()

 NMConnection *
 nm_device_get_applied_connection (NMDevice *self)
 {
        NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);

+       g_return_val_if_fail (NM_IS_DEVICE (self), NULL);
+
+       priv = NM_DEVICE_GET_PRIVATE (self);

Remove the initial assignment of @priv.


> device: support random cloned MAC address

+               /* FIXME: handle special cloned-mac-addresses */
		if (nm_setting_wired_get_cloned_mac_address (s_wired))
			return;

Probably the update should be avoided only for cloned-address == "permanent" (or empty).

(In reply to Thomas Haller from comment #18)
> yes, the last fallback default is still "permanent", that was the behavior
> since we added the cloned-mac-address property 2 years ago. I'd rather not
> change behavior there now (although we changed behavior before).

I'd much rather revert to the behavior from before 2 years ago than carry on with the new default.  If a user hasn't instructed n-m to do something, it should not do something :)

> Anyway, no strong opinion about this. Note that you will be able to
> overwrite that default with a file like
> /etc/NetworkManager/conf.d/10-no-cloned-mac-address.conf
> 
> [connection.no-cloned]
> ethernet.cloned-mac-address=leave
> wifi.cloned-mac-address.leave

Yes, but this is additional configuration shouldn't in general be necessary to get N-M to not do something that wasn't asked for, and it also forces admins to understand the logic behind connection stanza searching in NetworkManager.conf(5), which is fairly complex.

Please consider having the default be "leave" !


> > As a side note, i'm not sure that the term "cloned-mac-address" matches this
> > config parameter any more: there are many different (and useful) options
> > available here besides cloning.  Maybe "assigned-mac-address" makes more
> > sense?
> 
> You are right. But renaming an existing property and deprecating the old one
> is painful too and adds implementation and documentation overhead. I think
> it's actually worse then live with than ill name. 

I totally agree that renaming an existing property and deprecating the old one is painful in the short term.  In the long term, i suspect your users will be less confused, and the support load will be lower with a well-named option.  So this is a classic short-term/long-term tradeoff, entirely yours to make.

> Yes, there is some overlap. In hindsight, wifi.mac-address-randomization
> should not have been added as a separate property. This still needs to be
> evaluated. Currently wifi.mac-address-randomization works by telling
> wpa-supplicant to randomize the address. 

Sounds to me like we should deprecate wifi.mac-address-randomization so that there's just one option to set in n-m.  Then that option could e 

> It's not clear to me yet whether we
> want supplicant to set the MAC address (for association -- not during
> scanning!). Still needs to be solved...

How does all of this interact with scanning?  That is a clear wifi-specific feature, and it would be great to be able to control that separately.  Is there a separate bug documenting that issue, or should i start one?

Thanks for your work on this!

(In reply to Daniel Kahn Gillmor from comment #20)

> Sounds to me like we should deprecate wifi.mac-address-randomization so that
> there's just one option to set in n-m.  Then that option could e 

bah, sorry, you can drop this last sentence.  overall i think a single option controlling MAC addresses for active connections is the right way to go, though i think that controlling scanning behavior might be best as a separate (orthogonal) option.

> core/utils: convert MAC address in nm_match_spec_hwaddr() from string once

Instead of doing the G_UNLIKELY dance, why not put this right before the for() loop?  We only need to convert once, so it doesn't need to happen in the for() loop at all.

if (!specs) {
    return NM_MATCH_SPEC_NO_MATCH;
}

hwaddr_len = _nm_utils_hwaddr_length (hwaddr);
if (!hwaddr_len)
    g_return_val_if_reached (NM_MATCH_SPEC_NO_MATCH);
if (!nm_utils_hwaddr_aton (hwaddr, hwaddr_bin, hwaddr_len))
    nm_assert_not_reached ();

for (...) {


> core: use nm_utils_read_urandom() in nm_utils_secret_key_read()

Should probably have a commit comment or a code comment about why what NM was doing before with read() was a problem.


> device: refactor setting HW address via nm_device_set_hw_addr()

do_set doesn't seem the like the right name, since both TRUE & FALSE set the MAC address to something.  I'd actually just move the priv->XXXX logic into hw_addr_set()/hw_addr_reset().

reset() will always use priv->initial_hw_addr.

set() will always do "new_addr = passed_in_addr ? passed_in_addr : priv->perm_hw_addr".

Seems clearer to just put these two cases in the calling functions since they will never overlap.


> device: re-read the current MAC address when the link changes
> device: re-read initial hw-address before activating connection

update_initial_hw_address() now gets called from update_hw_address() and is only called by itself one other place: _set_state_full() in PREPARE state.  Should we just fold update_initial() into update_hw_address() and change the PREPARE state call to update_hw_address()?


> device: only set permanent hardware address once

Per above for "refactor setting HW address via nm_device_set_hw_addr()" since TRUE is only passed in this commit in one place (_hw_addr_set()) we don't really need to add another parameter to nm_device_get_permanent_hw_address().  Instead, the new check can just be in nm_device_hw_addr_set().


> device: let device specs match on permanent MAC address

I think this will break some stuff from the BT angle.  The BT address should never change since if it does it would be a completely new BT device.  So yes we can/should set the PERM_HW_ADDR, but we shouljd also set the HW_ADDR to the same value.  PERM/CUR should never be different.

I haven't tested, but I think this will cause BT devices to no longer have a CUR_HW address, right?  Since that's only updated in nm_device_update_hw_address(), and BT devices aren't netdevices and so nm_device_update_hw_address() will get an error from the platform.


> device: extend MAC address handling including randomization for ethernet and wifi

I would actually just drop the -bia suffix ones for now.  If we find that's actually needed later, great we can add it.  Even wpa_supplicant doesn't support BIA at this time, and if they don't support it, then I can't see why we'd need to yet.

Next up, why can't we handle the new strings within the existing D-Bus interface?  A bytestring can clearly contain any characters we want, and we can memcmp() instead of strcmp() or whatever we need to do.  We're already doing this kind of thing with the 802.1x "scheme" stuff.


> wifi: implement MAC address randomization in NetworkManager instead of supplicant

This is still icky because we need to coordinate scanning and stuff.  But I'd honestly just let the supplicant do the pre-assoc scan randomization still anyway, there's no reason it can't do that, and it's less code for us.  NM could still do the association-time stuff.  We still get to remove all the post-assoc randomization code.

Looks generally fine to me; just a couple of nit picks.

I'd like to see this well tested too; specifically to make sure the devices with buggy drivers that initially have all zero hardware address still can be activated when they gt an address and devices that have no permanent MAC address still work well (e.g. veths restore their initial address when deactivated).

> 84ea19d wifi: move static lookup-array for is_manf_default_ssid()
> efd0482 libnm: fix comparing NMSettingIPConfig for address and route properties
> c0a0bec core: make global variable _nm_utils_testing static
> 35ebca6 core/utils: convert MAC address in nm_match_spec_hwaddr() from string once

Ok.

> 7c9c857 ifcfg-rh: explicitly clear HWADDR setting and others in write_wired_for_virtual()

Perhaps you want to mention you fixed MTU handling too.

> 6c8fe00 core: add utils for file handling
> 8ba5a8e core: use nm_utils_read_urandom() in nm_utils_secret_key_read()

Nak. Why?

> 19dfab0 libnm: add internal util function _nm_utils_hwaddr_length()

+ * Returns: if @asc is a valid representation of a hardware address
+ *   (of any length up to NM_UTILS_HWADDR_LEN_MAX), it returns the binary
+ *   length. Otherwise, 0 is returned.

Perhaps it would be a good idea to avoid the funny indentation by keeping the
Return tag concise and short and describe the logic in a paragraph above instead?

> 0c45382 libnm: don't serialize empty hardware address on D-Bus
> e123e93 device: refactor nm_device_get_applied_setting()
> 3e8cca0 device: clear initial_hw_addr in nm_device_update_initial_hw_address()
> 352c915 device: always set "cloned-mac-address" even with missing NMSettingWired
> 4a86e5b device: refactor setting HW address via nm_device_set_hw_addr()

Ok.

> 55ac30a device: don't use g_warning for differing hw-addr-len after reading permanent address

Hopefully ok.

> 213cf74 device: initialize NMDevice's hw_addr at end of object construction

>   Also, ensure that @hw_addr_len is zero iff @hw_addr is unset.
A typo in the commit message:              ^^^

> 4feb617 device: split nm_device_update_permanent_hw_address() out of nm_device_update_initial_hw_address()
> ae14339 device: cleanup logging for setting MAC address

Ok.

> 57f98d5 device: re-read the current MAC address when the link changes

>   Also, the MAC address of NMDeviceEthernet is exposed on D-Bus. That
>   property should show the currently configured MAC address,
Did we previously expose the permanent MAC address on D-Bus?

> 95cb04f device: re-read initial hw-address before activating connection
> fbafcec device/trivial: rename hw-addr related fields in NMDevicePrivate
> dfd6653 device: only set permanent hardware address once
> b7ddff5 device: implememnt "perm-hw-address" property in NMDevice
> ddf2f88 device: don't clear the current MAC address

Ok.

> 10c3e44 device: let device specs match on permanent MAC address
> 3f292ed core: fix comparing nm_setting_wired_get_mac_address() with permanent MAC address
> 2ee5887 device: fix matching MAC address for VLAN and MACVLAN devices
> 1262d3e device: let infiniband prefer permanent MAC address for infiniband.mac-address setting

These do not look good to me; it could easily break someone's configuration?
Suppose someone replaced a NIC and used an udev rule to set the MAC address to
match the old one so that the configuration with hardwired addresses keeps functioning.
Not sure if we can change it.

> 02b6d73 device: use permanent MAC address for creating default wired connection

Why? If someone/something would change the MAC address prior to starting
NetworkManager they would get a connection that won't match the device?

> adaf63e device: extend MAC address handling including randomization for ethernet and wifi

Oh; the mac-address address on the D-Bus is now of a different type than the
assigned-mac-address. Not nice, but I guess not much can be done about that
anymore?

The "-bia" and ":<TOKEN>" semantics seems like an extra complexity with little
or zero usefullness and difficult to implement by clients. I'd vote to remove
those.

Also, can we really change the default from "leave" to "permanent"?
What about the scenario with changing MAC outside NM mentioned above?

A nitpick: maybe "preserve" is cleaner than "leave"?

> 2ee958f device: fail activation on failure to set cloned MAC address

Is this because it could be a privacy problem? Perhaps you could elaborate a
bit in the commit message.

> 575d7d9 libnm: deprecated wireless.mac-address-randomization property for wireless.cloned-mac-address
> b5ad743 wifi: implement MAC address randomization in NetworkManager instead of supplicant

Ok.

(In reply to Dan Williams from comment #22)
> > core/utils: convert MAC address in nm_match_spec_hwaddr() from string once
> 
> Instead of doing the G_UNLIKELY dance, why not put this right before the
> for() loop?  We only need to convert once, so it doesn't need to happen in
> the for() loop at all.

we only need to convert zero or one times (depending on whether @specs contains a "mac:" tag or an unqualified item). E.g. for "interface-name"eth0" it doesn't have to be evaluated, by moving before the loop, it has to.


> > core: use nm_utils_read_urandom() in nm_utils_secret_key_read()
> 
> Should probably have a commit comment or a code comment about why what NM
> was doing before with read() was a problem.

done.

> > device: refactor setting HW address via nm_device_set_hw_addr()
> 
> do_set doesn't seem the like the right name, since both TRUE & FALSE set the
> MAC address to something.  I'd actually just move the priv->XXXX logic into
> hw_addr_set()/hw_addr_reset().
>
> reset() will always use priv->initial_hw_addr.
> 
> set() will always do "new_addr = passed_in_addr ? passed_in_addr :
> priv->perm_hw_addr".
> 
> Seems clearer to just put these two cases in the calling functions since
> they will never overlap.

Changed.



> > device: re-read the current MAC address when the link changes
> > device: re-read initial hw-address before activating connection
> 
> update_initial_hw_address() now gets called from update_hw_address() and is
> only called by itself one other place: _set_state_full() in PREPARE state. 
> Should we just fold update_initial() into update_hw_address() and change the
> PREPARE state call to update_hw_address()?

In the last commit, that is refined further.


> > device: only set permanent hardware address once
> 
> Per above for "refactor setting HW address via nm_device_set_hw_addr()"
> since TRUE is only passed in this commit in one place (_hw_addr_set()) we
> don't really need to add another parameter to
> nm_device_get_permanent_hw_address().  Instead, the new check can just be in
> nm_device_hw_addr_set().

Changed too.


> > device: let device specs match on permanent MAC address
> 
> I think this will break some stuff from the BT angle.  The BT address should
> never change since if it does it would be a completely new BT device.  So
> yes we can/should set the PERM_HW_ADDR, but we shouljd also set the HW_ADDR
> to the same value.  PERM/CUR should never be different.
>
>
> I haven't tested, but I think this will cause BT devices to no longer have a
> CUR_HW address, right?  Since that's only updated in
> nm_device_update_hw_address(), and BT devices aren't netdevices and so
> nm_device_update_hw_address() will get an error from the platform.

Pushed a fixup, that should fix it right?

(btw, treating ifnames that are not kernel interfaces as such is wrong. E.g. when somebody would add an interface named like the bdaddr of a bluez device, havoc will ensue).


> > device: extend MAC address handling including randomization for ethernet and wifi
> 
> I would actually just drop the -bia suffix ones for now.  If we find that's
> actually needed later, great we can add it.  Even wpa_supplicant doesn't
> support BIA at this time, and if they don't support it, then I can't see why
> we'd need to yet.

macchanger supports it, and it's trivial to do, so why not?

What supplicant+macchanger supports is randomizing only the non-OUI part. I don't find that useful and didn't add it, because I wonder what should be taken as OUI on a failure to read the permanent MAC address.



> Next up, why can't we handle the new strings within the existing D-Bus
> interface?  A bytestring can clearly contain any characters we want, and we
> can memcmp() instead of strcmp() or whatever we need to do.  We're already
> doing this kind of thing with the 802.1x "scheme" stuff.

That needs some kind of special encoding. E.g. "random" (sans '\0') is 72:61:6e:64:6f:6d. So, you surely need to encode the '\0' to get not ETH_ALEN bytes.

Possible. But pretty ugly. Is that less ugly then this?


> > wifi: implement MAC address randomization in NetworkManager instead of supplicant
> 
> This is still icky because we need to coordinate scanning and stuff.  But
> I'd honestly just let the supplicant do the pre-assoc scan randomization
> still anyway, there's no reason it can't do that, and it's less code for us.
> NM could still do the association-time stuff.  We still get to remove all
> the post-assoc randomization code.

As explained in the commit message, setting pre-assoc in supplicant will set 'wpa_s->mac_addr_changed'. Later, when supplicant associates, it will reset the address to the permanent MAC address. That conflicts with us setting a cloned MAC address. And it already breaks cloned-mac-address on master.
Also, say, you fix supplicant not to touch the MAC address on association.
How do you guarantee that after NM sets the cloned MAC address, supplicant will not do one more scan and reset a random one?
This certainly needs fixes to supplicant.
But why is that better? What's the problem with this solution?



Thanks, Repushed.

(In reply to Lubomir Rintel from comment #23)

> > 6c8fe00 core: add utils for file handling
> > 8ba5a8e core: use nm_utils_read_urandom() in nm_utils_secret_key_read()
> 
> Nak. Why?

Documentation hints that reads can be interrupted (EINTR) and partial reads can happen. Systemd thinks so too.

Are you suggesting both cannot happen when reading /dev/urandom? Do you have any reliable source on that? Note that I didn't google for exact proof that this failure scenario can happen, because what matters is proof that this cannot happen.

A few weeks ago you ACK'd  a patch that does something very similar:
https://git.gnome.org/browse/network-manager-libreswan/commit/?id=f48cec9f5c6ce76ec1922a34414e469c4aa82762
Why is this now different?


> > 213cf74 device: initialize NMDevice's hw_addr at end of object construction
> 
> >   Also, ensure that @hw_addr_len is zero iff @hw_addr is unset.
> A typo in the commit message:              ^^^

iff stands for "if and only if"
https://en.wikipedia.org/wiki/If_and_only_if


> > 57f98d5 device: re-read the current MAC address when the link changes
> 
> >   Also, the MAC address of NMDeviceEthernet is exposed on D-Bus. That
> >   property should show the currently configured MAC address,
> Did we previously expose the permanent MAC address on D-Bus?

No, the current MAC address that was read at some point. But when changing MAC address it was no updated.


> > 10c3e44 device: let device specs match on permanent MAC address
> > 3f292ed core: fix comparing nm_setting_wired_get_mac_address() with permanent MAC address
> > 2ee5887 device: fix matching MAC address for VLAN and MACVLAN devices
> > 1262d3e device: let infiniband prefer permanent MAC address for infiniband.mac-address setting
> 
> These do not look good to me; it could easily break someone's configuration?
> Suppose someone replaced a NIC and used an udev rule to set the MAC address
> to
> match the old one so that the configuration with hardwired addresses keeps
> functioning.
> Not sure if we can change it.

How does it make sense to match a device against the non-permanent MAC address?

For example, you activate a connection with ethernet.mac-address and ethernet.cloned-mac-address set. After activation, the MAC address differs. You restart NetworkManager. The same connection no longer matches the device
(I am not even talking about connection-assumption, even if you are not assuming connections, the connection would no longer match the device). That seems wrong.

Arguably, systemd's Match.MACAddress also seems to use the current MAC address. But I don't see how that makes sense.


Parts of the documentation (on master) already imply that this is the permanent MAC address. Comparing with the current MAC address was a bug that gets fixed.

»··· * NMSettingWired:mac-address:
»··· *
»··· * If specified, this connection will only apply to the Ethernet device
»··· * whose permanent MAC address matches. This property does not change the
»··· * MAC address of the device (i.e. MAC spoofing).


> > 02b6d73 device: use permanent MAC address for creating default wired connection
> 
> Why? If someone/something would change the MAC address prior to starting
> NetworkManager they would get a connection that won't match the device?

Why would it not match? With the changes above, it still would match because we compare against the permanent MAC address, and likewise set ethernet.mac-address to the permanent.


> > adaf63e device: extend MAC address handling including randomization for ethernet and wifi
> 
> Oh; the mac-address address on the D-Bus is now of a different type than the
> assigned-mac-address. Not nice, but I guess not much can be done about that
> anymore?

dcbw suggests to hack the existing binary field to encode the strings. That would be possible, but preferable?


> The "-bia" and ":<TOKEN>" semantics seems like an extra complexity with
> little
> or zero usefullness and difficult to implement by clients. I'd vote to remove
> those.

The ":<TOKEN>" I find useful. It's the only way to create two connections that use the same generated MAC address, without assigning it explicitly.
Say, you create two connections for the same network, say they differ by IP addressing.
Then the MAC address is spoofed but stable. And both connections get the same MAC address. Ok, that isn't much better then assigning explict MAC addresses to those connections.
But what you can do with ":<TOKEN>", is to deploy these connections on multiple machines, where each machine generates different stable addresses. You cannot achieve that any other way.

Regarding -bia: bug 705545 requests for support for macchanger, which has such a --bia setting. I guess it can make ~some~ sense and be useful to somebody? At least, macchanger thinks so.
The implementation overhead for this additional feature seems acceptable to me.



> Also, can we really change the default from "leave" to "permanent"?
> What about the scenario with changing MAC outside NM mentioned above?

The default is already permanent since commit 1b49f941a69af910b0e68530be7339e8053068e5. Actually, this bug report requests to change that default back to what was before. For now, I didn't do that precisely not to change behavior (again). But maybe we should?


> A nitpick: maybe "preserve" is cleaner than "leave"?

I like "preserve" better. Changed.


> > 2ee958f device: fail activation on failure to set cloned MAC address
> 
> Is this because it could be a privacy problem? Perhaps you could elaborate a
> bit in the commit message.

done.




Repushed, with fixups.

(In reply to Thomas Haller from comment #25)
> > The "-bia" and ":<TOKEN>" semantics seems like an extra complexity with
> > little
> > or zero usefullness and difficult to implement by clients. I'd vote to remove
> > those.
> 
> The ":<TOKEN>" I find useful. It's the only way to create two connections
> that use the same generated MAC address, without assigning it explicitly.
> Say, you create two connections for the same network, say they differ by IP
> addressing.
> Then the MAC address is spoofed but stable. And both connections get the
> same MAC address. Ok, that isn't much better then assigning explict MAC
> addresses to those connections.
> But what you can do with ":<TOKEN>", is to deploy these connections on
> multiple machines, where each machine generates different stable addresses.
> You cannot achieve that any other way.
 
> Regarding -bia: bug 705545 requests for support for macchanger, which has
> such a --bia setting. I guess it can make ~some~ sense and be useful to
> somebody? At least, macchanger thinks so.
> The implementation overhead for this additional feature seems acceptable to
> me.

I personally would keep only permanent, preserve and random. Other options seems
not justified by real needs at the moment and we can add them later if someone
requests them with a strong use case.

> libnm: add NMSettingConnection:stable-id property

+       g_object_class_install_property
+               (object_class, PROP_STABLE_ID,
+                g_param_spec_string (NM_SETTING_CONNECTION_STABLE_ID, "", "",
+                                     NULL,
+                                     G_PARAM_READWRITE |
+                                     NM_SETTING_PARAM_FUZZY_IGNORE |
+                                     NM_SETTING_PARAM_INFERRABLE |

Since the stable-id is not populated when a assumed connection is
generated I think the NM_SETTING_PARAM_INFERRABLE must be removed,
otherwise the matching will fail.


> core/utils: convert MAC address in nm_match_spec_hwaddr() from string once

This doesn't compile as _nm_utils_hwaddr_length() is added in a later commit.


> all: make MAC address randomization algorithm configurable

libnm-core/nm-setting-wireless.c :

+       g_prefix_error (error, "%s.%s: ", NM_SETTING_WIRED_SETTING_NAME, NM_SETTING_WIRED_GENERATE_MAC_ADDRESS_MASK);
+       g_param_spec_string (NM_SETTING_WIRED_GENERATE_MAC_ADDRESS_MASK, "", "",

s/WIRED/WIRELESS/g


The rest LGTM.

(In reply to Beniamino Galvani from comment #27)

thanks. Addressed all and repushed.

LGTM

merged branch as https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9a354cdc906a8d04d6541f1275e80540b7c3d567



I think this implements everything this bug requests, except that a default of ethernet.cloned-mac-address=(NULL) still ultimately falls bad to "permanent" (and not "preserve" as requested).

That may be still changed, but changing existing behavior is bad (although one my argue that it was changed years ago, and now the previous one would be restored).


A user can configure the default via:

# cat /etc/NetworkManager/conf.d/00-cloned-mac-address-preserve.conf
[connection-mac-randomization]
ethernet.cloned-mac-address=preserve
wifi.cloned-mac-address=preserve



Closing as FIXED. If something is missing/not-working please open a new bug to keep the discussion focused. Thanks.