Skip to content

Commit

Permalink
more random mac
Browse files Browse the repository at this point in the history
  • Loading branch information
tschettervictor authored Dec 30, 2024
1 parent 31ccecb commit d3fd055
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion usr/local/share/bastille/common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,12 @@ generate_static_mac() {
local jail_name="${1}"
local external_interface="${2}"
local macaddr_prefix="$(ifconfig ${external_interface} | grep ether | awk '{print $2}' | cut -d':' -f1-3)"
local macaddr_suffix="$(echo -n ${jail_name} | 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/')"
local macaddr_suffix="$(echo -n "${external_interface}${jail_name}" | 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
error_notify "Failed to generate MAC address."
fi
macaddr="${macaddr_prefix}:${macaddr_suffix}"
export macaddr
}

generate_vnet_jail_netblock() {
Expand Down

12 comments on commit d3fd055

@JRGTH
Copy link
Collaborator

@JRGTH JRGTH commented on d3fd055 Dec 31, 2024

Choose a reason for hiding this comment

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

Hello @tschettervictor happy holidays, This change will not make the MAC more random, it will just change the HASH result still remaining static.

If you meant to change/randomize a new MAC address for a jail with the same name even after re-creation for added security, you need to also randomize either a prefix/postfix in between the jail name in order to always get a fresh MAC address if the user want that behavior, hence this should be an user selection option, for example look at the mere basic example below:

root@mserver: ~# echo -n em0name | 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/'
2b:e8:c
root@mserver: ~# echo -n em0name | 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/'
2b:e8:c
root@mserver: ~# echo -n em0name | 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/'
2b:e8:c
root@mserver: ~# echo -n `mktemp`name | 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/'
fb:84:9
root@mserver: ~# echo -n `mktemp`name | 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/'
32:6e:8
root@mserver: ~# echo -n `mktemp`name | 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/'
8a:81:e

Look at the above pattern on how the MAC address does remains the same on the first 3 attempts, so the prefix for the "${external_interface}" is really not needed, but rather a random number generation in conjunction with the jail name.

Be aware that the above example is just a quick hack to show the wanted behavior and that mktemp creates a temporary file, so this cannot be used without code to remove this file after MAC generation, another/better approach is to generate a random number based on the hardware then using such variable for said purpose among other ways.

Regards!

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for that.

My goal is ultimately to have a MAC that will always be the same for a jail with the same name. The issue I had was that when interfaces were created by the host named "bridge0 bridge1 bridge2" etc.. the prefix for these would always be the same because the system creates MAC addresses for bridge interfaces where only on number is different (see below). I've also recently added PR #783 that allows you to add interfaces to jails. With this change, a jail linked to bridge0 would have the same mac on two interfaces if bridge1 is added.

But with the implementation I currently have, the MAC of the host interface is hashed for the first six digits, then the jail name for the last six. I am not so concerned with randomization as I am with the "static" part. Meaning a jail with the same name on the same interface should always have the same MAC.

Notice that in the following, only the last 2 digits are different. I think hashing the full MAC of the host interface will give the best balance of "static" "security and "randomization".

EDIT: I've also now changed so the section you commented on above is not what it currently looks like. See the updated version...

bridge0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
        ether 58:9c:fc:10:ff:d4
        inet 192.168.1.157 netmask 0xfffffe00 broadcast 192.168.1.255
        id 00:00:00:00:00:00 priority 32768 hellotime 2 fwddelay 15
        maxage 20 holdcnt 6 proto rstp maxaddr 2000 timeout 1200
        root id 00:00:00:00:00:00 priority 32768 ifcost 0 port 0
        groups: bridge
        nd6 options=9<PERFORMNUD,IFDISABLED>
bridge1: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
        ether 58:9c:fc:10:ff:c9
        id 00:00:00:00:00:00 priority 32768 hellotime 2 fwddelay 15
        maxage 20 holdcnt 6 proto rstp maxaddr 2000 timeout 1200
        root id 00:00:00:00:00:00 priority 0 ifcost 0 port 0
        member: e4a_test1 flags=143<LEARNING,DISCOVER,AUTOEDGE,AUTOPTP>
                ifmaxaddr 0 port 12 priority 128 path cost 2000
        member: e6a_test2 flags=143<LEARNING,DISCOVER,AUTOEDGE,AUTOPTP>
                ifmaxaddr 0 port 13 priority 128 path cost 2000
        member: e1a_kristy flags=143<LEARNING,DISCOVER,AUTOEDGE,AUTOPTP>
                ifmaxaddr 0 port 6 priority 128 path cost 2000
        groups: bridge
        nd6 options=9<PERFORMNUD,IFDISABLED>
bridge2: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
        ether 58:9c:fc:10:ff:fc
        id 00:00:00:00:00:00 priority 32768 hellotime 2 fwddelay 15
        maxage 20 holdcnt 6 proto rstp maxaddr 2000 timeout 1200
        root id 00:00:00:00:00:00 priority 0 ifcost 0 port 0
        groups: bridge
        nd6 options=9<PERFORMNUD,IFDISABLED>

@JRGTH
Copy link
Collaborator

@JRGTH JRGTH commented on d3fd055 Jan 1, 2025

Choose a reason for hiding this comment

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

Hi @tschettervictor, I understand now your case scenario on multiple Jails with the same name now and I think we talked about this before while I was busy, since one of the bastille goals is keep an easy to use yet highly secure/customizable Jail manager, this should be an user selectable option i.e: --static-mac when user creates a Jail requesting for a static MAC for ease of management while keep MAC random(the default) for the average user for added security, really sorry if this is already an user option.

Sorry I've been busy to stay up to date on all bastille changes but I will get a break asap in order to revise the code on the ZFS stuff and overall consistency/compatibility/cleanup.

Happy new year!
Regards!

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will have to discuss it some more.
We could have a -m switch for it.

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could very easily be achieved, but it would require an additional two blocks inside the “generate_vnet_block” function. Easily managed I’m sure, I’m just not convinced as to the need of it. But for security, you are right that I would be better.

I’ll try to implement this change once my last few PRs are merged.

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have this implemented in my fork now. Simply add -M|--static-mac to options when creating to have a statically assigned Mac based on the IF name and jail name.

Will clean up the code and request another PR shortly.

@JRGTH
Copy link
Collaborator

@JRGTH JRGTH commented on d3fd055 Jan 2, 2025

Choose a reason for hiding this comment

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

Great, also fyi handling/combining of multiple --long-options can be easily implemented with something like the below example:

# echo "--static-mac --vnet --some-option --thick --thin" | xargs | tr ' ' '\n'
--static-mac
--vnet
--some-option
--thick
--thin

Another example with unnecessary white spaces in between the options handled by xargs:

# echo "  ---static-mac --vnet --some-option    --thick --thin    " | xargs | tr ' ' '\n'
---static-mac
--vnet
--some-option
--thick
--thin

Then the error handling code takes care of misspelled options and/or for options who can't live together.
Regards!

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whitespace isn’t really an issue. I just tried and everything worked as expected. My biggest goal was to allow for “-fsm” etc… (bunching short options together)

@JRGTH
Copy link
Collaborator

@JRGTH JRGTH commented on d3fd055 Jan 2, 2025

Choose a reason for hiding this comment

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

Right only handling multiple short options is enough and keep the case/esac code smaller/cleaner, the user can always explicitly select --long-option for special/advanced commands without short/single character option.

Regards!

@tschettervictor
Copy link
Collaborator Author

@tschettervictor tschettervictor commented on d3fd055 Jan 2, 2025

Choose a reason for hiding this comment

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

One more question before I let you go on this one.
Should we keep it as is like the following...
Or shorten it like the second block?
The second is shorter, but seems harder to follow.

# Handle options.
BRIDGE_VNET_JAIL=0
FORCE=0
STATIC_MAC=0
START=0
VNET_JAIL=0
while [ "$#" -gt 0 ]; do
    case "${1}" in
        -h|--help|help)
            usage
            ;;
        -b|-B|--bridge)
            BRIDGE_VNET_JAIL=1
            shift
            ;;
        -f|--force)
            FORCE=1
            shift
            ;;
        -m|-M|--static-mac)
            STATIC_MAC=1
            shift
            ;;
        -s|--start)
            START=1
            shift
            ;;
        -v|-V|--vnet)
            VNET_JAIL=1
            shift
            ;;
        -*)
            for _o in $(echo ${1} | sed 's/-//g' | fold -w1); do
                case ${_o} in
                    b|B) BRIDGE_VNET_JAIL=1 ;;
                    f) FORCE=1 ;;
                    m|M) STATIC_MAC=1 ;;
                    s) START=1 ;;
                    v|V) VNET_JAIL=1 ;;
                    *) error_exit "Unknown Option: \"${1}\"" ;; 
                esac
            done
            shift
            ;;
        *)
            break
            ;;
    esac
done
# Handle options.
BRIDGE_VNET_JAIL=0
FORCE=0
STATIC_MAC=0
START=0
VNET_JAIL=0
while [ "$#" -gt 0 ]; do
    case "${1}" in
        -h|--help|help) usage ;;
        -b|-B|--bridge) BRIDGE_VNET_JAIL=1 shift ;;
        -f|--force) FORCE=1 shift ;;
        -m|-M|--static-mac) STATIC_MAC=1 shift ;;
        -s|--start) START=1 shift ;;
        -v|-V|--vnet) VNET_JAIL=1 shift ;;
        -*) for _o in $(echo ${1} | sed 's/-//g' | fold -w1); do
                case ${_o} in
                    b|B) BRIDGE_VNET_JAIL=1 ;;
                    f) FORCE=1 ;;
                    m|M) STATIC_MAC=1 ;;
                    s) START=1 ;;
                    v|V) VNET_JAIL=1 ;;
                    *) error_exit "Unknown Option: \"${1}\"" ;; 
                esac
            done shift ;;
        *) break ;;
    esac
done

@JRGTH
Copy link
Collaborator

@JRGTH JRGTH commented on d3fd055 Jan 3, 2025

Choose a reason for hiding this comment

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

Hi, the first block while a little larger, its ident is more readable and easier to follow as you said, also the too short variable _o should be renamed to something more meaningful like _opt as well.

Regards!

@tschettervictor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted. Thanks.

Please sign in to comment.