hd-zero / hdzero-goggle

MIT License
261 stars 76 forks source link

Add option to disable random MAC address in client mode #403

Open yenon opened 7 months ago

yenon commented 7 months ago

To allow for easier access via WIFI, the changing mac address of the goggle will result in different IP's every boot, thanks to the dhcp on the network not recognizing the device by it's MAC. This PR introduces a (semi) permanent MAC that can be used as a fixed address, if so desired. The first address of the device is generated randomly from system time and saved to the settings_ini. Changing of the address is also supported by placing a file named macaddr.txt in the SD-card root. Validation is in place, and only valid mac's will be accepted.

Please validate this a bit more cautiously, my C skills are rusty.

SumolX commented 7 months ago

Nice job! I would recommend not keeping both clientid and the option for random MAC address. Remove the clientid setting attribute and associated references, use the persistent Mac attribute instead. Simplify the code to always use the generate MAC address instead. I don't see a benefit for having the user switch between random and static MAC address and having extra UI options. While the file on the sd card is good for debugging and manually setting the Mac once you have verified this works is it necessary to keep the logic for loading a Mac from sd card? I would imagine most people won't know about this and probably just rely on the auto generation since you have solved the underlying dhcp ip reuse issue.

yenon commented 7 months ago

Before removing features, I'd like to know nobody actually uses them. Secondly, what does clientid actually do? I cannot find a manpage for udhcpc that has the x flag, no idea what it does. Removing random MAC completely might be bad for privacy in open networks, thanks to fingerprinting. (We are talking about a video goggle though.)

I see the point of not needing the MAC to be changed via SD card. Mostly just me wanting to set the default to what it used to be on my first try. Maybe in case of a settings file update? I foresee no real changes to the way MAC addresses work in the wild, so the code would essentially remain untouched for a long time, so why not have it? How it works is actually described in the note at the bottom of the settings page.

SumolX commented 7 months ago

clientid - https://techhub.hpe.com/eginfolib/networking/docs/switches/5130ei/5200-3942_l3-ip-svcs_cg/content/483572383.htm

So if you are going to keep random Mac for privacy reasons.... then the client id should be made random as well or it should be updated to use the MAC address as well regardless what mode the privacy is set to.

My argument for simplifying the MAC address handling was to minimize confusion and clutter within the UI and having an external text file used for overriding. The approach should be too keep it simple since at the end of the day this is not a full blown PC envirnoment. I know I don't use the googles to access the web for surfing.... ;)

Anyway those are my recommendations.... you can choose how to move forward.

yenon commented 7 months ago

Keeping the MAC static seems sane for me, especially with our usecase. The clientid can be the static mac address, just as a fallback in case of weird behavioured dhcp's. I'll drop the menu entry for changing it, that will already simplify a lot. I'd like to keep the mac changing by file, since we do not adhere to any reasonable standard for it's assignment and duplicates are unlikely, but not impossible. Sound good?

yenon commented 7 months ago

Should be alright now.

nerdCopter commented 2 months ago

April 26 comment makes 100% sense to me as someone in the IT industry. Maybe rebase on main and mark ready for review.

yenon commented 2 months ago

It would make sense, if the used wifi driver actually supported it properly. The current driver seems to offer better performance, but lacks any means of changing the mac address, if I remember correctly. Feel free to pull and build this, but it will not work.

SumolX commented 5 days ago

I would close this PR if a new WiFi driver is not included since there is a driver limitation that prevents this PR from being successfully integrated.