dubious code for random mac address determination

[Bischoff]

System Information

Terraform version

commit 9e02168780b83e158c050cbe1f32102304861e58 October 11, 2020

Setup

Seen on source code. problem was introduced by:

commit 625e9420e1a89d65f910551f5057c72a0a6a12eb
Author: root <root@zimbra-mta.univ-brest.fr>
Date:   Tue Feb 5 11:18:26 2019 +0100

The issue

This function appends 3 randoms bytes after 52:54:00: to build a random MAC address:

// randomMACAddress returns a randomized MAC address
// with libvirt prefix
func randomMACAddress() (string, error) {
        buf := make([]byte, 3)
        rand.Seed(time.Now().UnixNano())
        _, err := rand.Read(buf)
        if err != nil {
                return "", err
        }

        // set local bit and unicast
        buf[0] = (buf[0] | 2) & 0xfe
        // Set the local bit
        buf[0] |= 2

        // avoid libvirt-reserved addresses
        if buf[0] == 0xfe {
                buf[0] = 0xee
        }

        return fmt.Sprintf("52:54:00:%02x:%02x:%02x",
                buf[0], buf[1], buf[2]), nil
}

There is some processing to set "local" bit and "unicast" bit on first byte, as well as to avoid reserved value 0xFE for that first byte.

The problem is: the first byte in the array is the fourth byte in the MAC address, and the fourth byte in the MAC address does not have those constraints. The first byte in the MAC address is always 0x52 and is already respecting these constraints.

Reference:

https://upload.wikimedia.org/wikipedia/commons/9/94/MAC-48_Address.svg

Suggested fix

Either:

• let all 6 bytes be picked at random, as it was before this commit, or
• remove unnecessary lines 28-37

Note: setting the prefix to 52:54:00 also has the undesirable effect of reducing the entropy (and therefore augmenting the chance of collisions).

Hope that helps.

[MalloZup]

thx @Bischoff I think we might need to spend more time on this.

If you feel , you can still send a PR to help us a bit. thx anyways for issue!

[Bischoff]

I am lacking information here.

I would suggest to "fix" it by simply reverting the commit 625e9420e1a89d65f910551f5057c72a0a6a12eb from root@zimbra-mta.univ-brest.fr .

But I don't know what motivated this commit. Maybe there is a good reason behing this commit that I ignore, and then it would be wrong to revert.

Pull request #548 from @Isma399 does not state any motivation for that change.

[Isma399]

Sorry for the bug. I believe that KVM guest must have the first 3 pairs in the MAC address of the Ethernet interface be of the sequence ’52:54:00’.

[Bischoff]

No worries. It's not really a "bug", it's more like leftover useless instructions...

52:54:00 is not linked to KVM, but to QEMU. You can specify any MAC address on the qemu command line (--device ...,mac=...), but I suppose that 52:54:00 is the default when you don't do that.

What purpose are you trying to achieve by using 52:54:00? Are there problems to use something else?

[Isma399]

Yes, it's QEMU. In our network, we have many KVMs and all guests MAC Address start like that. So, I wanted to keep that, because we use this prefix to detect if a VM is on a KVM or other hypervisor (ESX mainly).

[Bischoff]

OK, makes sense.

Then I will do a pull request that removes the unnecessary lines.

Thanks for your feedback.

[Bischoff]

Pull request #799 is ready, closing this issue.