Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create: Static MAC optional #808

Merged
merged 10 commits into from
Jan 16, 2025
Merged

Conversation

tschettervictor
Copy link
Collaborator

@tschettervictor tschettervictor commented Jan 13, 2025

As per @michael-o I have now modified creating and cloning jails to retain old behavior. It is now required to use "-M|--static-mac" to have a statically assigned MAC address to your jail. MAC will be based on 2 things.

  1. The chosen interface's MAC prefix will be the first half.
  2. The combination of a hash of "interface+jailname" will be the second half.

If the jail has a statically assigned MAC, then cloning it will also assign a new static MAC to the cloned jail, otherwise, default behavior will persist.

To test this PR here are the steps both for a VNET and bridged-VNET jail.

VNET

  1. Create a jail and make sure there is no static MAC
  2. Create a jail and use -M to generate a static MAC for it and check jail.conf to ensure it is there and formatted properly
  3. Clone both of the above jails, making sure the first does not have a static MAC, and the second jails clone gets one that is different from the original jail

Do the above with VNET jails, then do it with bridged VNET jails. These are the only 2 scenarios that this PR touches.

@yaazkal
@JRGTH
@bmac2

@bmac2
Copy link
Collaborator

bmac2 commented Jan 14, 2025

tested at length today with @tschettervictor. with the -M it creates a static mac using the prefix, no -m the bridged do not have a static mac (ether)

Ready to merge based on @michael-o 's testing and feedback

@yaazkal @cedwards @tschettervictor

@michael-o
Copy link
Contributor

Testing...

Copy link
Contributor

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Previous behavior has been restored. Good! New behavior MAC generation needs improvement. See comment.

usr/local/share/bastille/common.sh Outdated Show resolved Hide resolved
@tschettervictor
Copy link
Collaborator Author

@michael-o The jib script does not respect the FreeBSD prefix. The MAC addresses it creates are just random it seems. Is that why you shared the link to the Bugzilla report?

usr/local/share/bastille/common.sh Show resolved Hide resolved
@@ -113,7 +113,7 @@ generate_static_mac() {
local jail_name="${1}"
local external_interface="${2}"
local external_interface_mac="$(ifconfig ${external_interface} | grep ether | awk '{print $2}')"
local macaddr_prefix="$(echo ${external_interface_mac} | cut -d':' -f1-3)"
local macaddr_prefix="58:9c:fc"
local macaddr_suffix="$(echo -n "${external_interface_mac}${jail_name}" | sed 's#:##g' | sha256 | cut -b -5 | sed 's/\([0-9a-fA-F][0-9a-fA-F]\)\([0-9a-fA-F][0-9a-fA-F]\)\([0-9a-fA-F]\)/\1:\2:\3/')"
if [ -z "${macaddr_prefix}" ] || [ -z "${macaddr_suffix}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... that seems to random for me. I need something that will be random, but the same for each if+jailname combo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look again tomorrow, As fas as I understand you want it to be reproducible, then you cannot you the time for that, of course.

Copy link
Contributor

@michael-o michael-o Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth looking at /usr/src/sys/net/ieee_oui.h:

75 #define OUI_FREEBSD_GENERATED_MASK  0x10ffff
76 #define OUI_FREEBSD_GENERATED_LOW   OUI_FREEBSD(0x100000)
77 #define OUI_FREEBSD_GENERATED_HIGH  OUI_FREEBSD(OUI_FREEBSD_GENERATED_MASK)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would look at /usr/src/sys/net/if_ethersubr.c: ether_gen_addr_byname(const char *nameunit, struct ether_addr *hwaddr). This is exactly what you are looking for.

@michael-o
Copy link
Contributor

@michael-o The jib script does not respect the FreeBSD prefix. The MAC addresses it creates are just random it seems. Is that why you shared the link to the Bugzilla report?

Correct, the jib script does generate a local MAC address, but does not use the FreeBSD prefix, hence my bug report.

@bmac2
Copy link
Collaborator

bmac2 commented Jan 14, 2025

So @michael-o are you saying that we should use the freebsd prefix all the time?? Just making sure I am clear on what you are saying.

@tschettervictor

@michael-o
Copy link
Contributor

So @michael-o are you saying that we should use the freebsd prefix all the time?? Just making sure I am clear on what you are saying.

@tschettervictor

Yes, pretty much like vm-bhyve does when a static MAC is requested.

@tschettervictor
Copy link
Collaborator Author

This is now done. FreeBSD prefix will be the prefix.

@yaazkal
Copy link
Collaborator

yaazkal commented Jan 15, 2025

great @tschettervictor thanks

So default behavior will be dynamic MAC as before and optional to have it static, all we agree on that? cc @bmac2 @JRGTH @cedwards

@bmac2
Copy link
Collaborator

bmac2 commented Jan 15, 2025

yes that was what everyone wanted.

@michael-o
Copy link
Contributor

great @tschettervictor thanks

So default behavior will be dynamic MAC as before and optional to have it static, all we agree on that? cc @bmac2 @JRGTH @cedwards

Yes, looking into the PR again.

@michael-o
Copy link
Contributor

@tschettervictor I think it would be wise to have a similiar logic (which we mostly have) to ether_gen_addr_byname() and using the denoted MAC address range of 20 bits. WDYT?

@tschettervictor
Copy link
Collaborator Author

tschettervictor commented Jan 15, 2025

Is that from vm-bhyve? Not finding that function.

EDIT: found it.

Since we are doing simple "sh" I'm not sure how we would implement that.

What we currently do so just sha265 the name, and use the last 5-6 digits.
Seems enough to me, unless you have some code that could do it better...

@bmac2
Copy link
Collaborator

bmac2 commented Jan 16, 2025

tested. Since @michael-o, myself, and @tschettervictor all tested this and Micheal is now good with the state of this, merging to return default behavior.

@yaazkal

@bmac2 bmac2 merged commit fb876b4 into BastilleBSD:master Jan 16, 2025
1 check passed
@tschettervictor tschettervictor deleted the patch-3 branch January 16, 2025 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants