feat: Rewrite MAC randomisation toggle in python (Task #1560)#1870
feat: Rewrite MAC randomisation toggle in python (Task #1560)#1870Meijuta wants to merge 61 commits intosecureblue:livefrom
Conversation
HastD
left a comment
There was a problem hiding this comment.
Thanks for writing this up. I do have a number of suggestions to bring this more in line with the other rewritten ujust scripts.
For most of the ujust toggle scripts, we've been switching over to ujust set-* [on|off|status] in place of ujust toggle-*; I think it would be good to follow that pattern here. See ujust set-unconfined-userns for a simple example of this. This also makes it easier to invoke the script from another script or from other Python code.
In this case, I think it would make sense to have "random" or "stable" as arguments that can be passed directly in place of just "on". That way, the script can be run non-interactively, e.g. as ujust set-mac-randomization stable or ujust set-mac-randomization off. A "status" argument is also useful and should print something indicating the current status, while "help" or "--help" should print usage info.
If the script is called without arguments, it should prompt the user which mode ("stable", "random", or "off") to select. The inquirer Python module is installed on secureblue and provides a nice interface for "select from these multiple options" prompts like this.
Additionally, for other similar ujust scripts, we're using the sandbox module (defined in files/system/usr/libexec/secureblue) to separate the unprivileged and privileged parts of ujust scripts, and run the privileged part with minimal permissions using systemd sandboxing. (The privileged script goes in the inner subdirectory.)
In this case, I think only writing or deleting the conf file needs to be handled by the privileged script, and it should just need write access to /etc/NetworkManager/conf.d; toggling the network connection can be done unprivileged, and all interactive prompts should be unprivileged too.
|
Other related improvements that are in progress:
None of these warrant blocking this PR, as the python rewrite alone is an improvement. Just mentioning for any who aren't aware. |
|
Everything should be done now! |
WavyEbuilder
left a comment
There was a problem hiding this comment.
Left a few nits. Looking good!
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class SystemdService: |
There was a problem hiding this comment.
was this copied from dns.py? if so it should be removed from dns.py and pointed to this shared util
There was a problem hiding this comment.
yes. i prefer not to mess with code i havent written more than absolutely necessary because i dont know the exact details of its use case, where it is used, or every one of its quirks. which is why i left it
There was a problem hiding this comment.
please do so, we shouldn't be copy pasting code.
|
|
||
| def run_disable_randomization() -> int: | ||
| """Runs sandboxed disable_randomization() function.""" | ||
| if Path(RAND_MAC_FILE).exists(): # may TOCTOU |
There was a problem hiding this comment.
is the TOCTOU comment unique to this toggle?
There was a problem hiding this comment.
i think so, this one uniquely deletes the file.
I dont remember what i was thinking when i wrote that, it might be like a 'if something is going wrong in these parts, its probably this' kind of comment.
| restart_success = run_restart_networkmanager() | ||
| if restart_success != 0: # 0 == success, not 0 == failure | ||
| print_wrapped( | ||
| "Failed to restart NetworkManager. " |
There was a problem hiding this comment.
there is a lot of copy/pasted boilerplate between these functions that could be refactored
why not have a single set_randomization_state() function that takes an enum with values DISABLED, STABLE, RANDOM?
There was a problem hiding this comment.
shouldnt i just reuse the Mode enums rather than seperately defining DISABLED, STABLE, RANDOM?
Co-authored-by: RoyalOughtness <[email protected]>
I've rewritten the MAC randomisation toggle in python with an emphasis on minimising permission usage.