Add use_sudo config option for non-root Ghidra server management#707
Add use_sudo config option for non-root Ghidra server management#707ANogin wants to merge 6 commits into
use_sudo config option for non-root Ghidra server management#707Conversation
Allow non-root users to manage the Ghidra server by optionally prefixing server start/stop commands with sudo -n. Also switch from subprocess.call to subprocess.run(check=True) so failures are raised as errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Config tests assumed default settings and corrupted user config (e.g. use_sudo: true → false). Server test fixture cleanup could fail with CalledProcessError if the test itself failed partway through. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reject non-boolean use_sudo values (e.g. quoted "false") with a clear TypeError instead of silently coercing. Add use_sudo to the default config template for discoverability, and document the passwordless sudo requirement. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude, can you do some research on this PR. Is this change needed? Does the Ghidra server actually require sudo to run? Trying to understand if there is a simpler way to address this. Take your time, make no mistakes ;) |
|
Claude finished @whyitfor's task in 2m 36s —— View job Research: Does the Ghidra Server Require
|
| Approach | Root needed? | Effort | Trade-offs |
|---|---|---|---|
use_sudo (this PR) |
No (user needs passwordless sudo) | Low | Requires sudoers config |
| Docker (already supported) | No (runs as root in container) | None | Already works — recommended path |
ghidraSvr console mode |
No | Medium | Runs Java in foreground; would need to restructure server lifecycle management |
| pyghidra (partially in Dockerstub) | No | High | Embeds Ghidra in Python process — eliminates the server entirely, but big refactor |
The Docker path is already the documented recommended approach for most users and avoids this entire issue. The use_sudo option makes sense specifically for native/non-Docker setups where users don't want to run their OFRAK process as root.
PR Quality Assessment
The implementation is clean and minimal. Some observations:
Improvements over the old code:
- The switch from
subprocess.calltosubprocess.run(check=True)(__main__.py:36) is an unconditional improvement — the old code silently ignored failures - The
sudo -nflag is correct: it's non-interactive (fails rather than prompting for password), which is the right behavior for programmatic use
Config handling:
use_sudo: falsedefault preserves backward compatibility with existing configs- The
isinstance(_use_sudo, bool)type check (ofrak_ghidra_config.py:68-72) prevents silent misconfiguration (e.g., if someone accidentally writesuse_sudo: yesas a string instead oftrueas a boolean)
Minor observation: The use_sudo field is always serialized in to_yaml() — this means importing an old config file without use_sudo still works (it defaults to False via .get("use_sudo", False)) but a freshly exported config will always include the field. This is acceptable behavior.
Test coverage:
- Test skip condition
os.geteuid() != 0 and not conf.use_sudo(test_ghidra_server.py:16) correctly gates tests to root or sudo-enabled environments
Verdict
The change is needed and well-implemented for the native (non-Docker) use case. The use_sudo: false default means no behavior change for existing users. If the team prefers to avoid this complexity, the guidance could instead emphasize Docker as the recommended path — but that doesn't help users who have legitimate reasons to run natively as non-root.
One sentence summary of this PR (This should go in the CHANGELOG!)
Add
use_sudoconfig option for non-root Ghidra server managementLink to Related Issue(s)
N/A
Please describe the changes in your request.
Allow non-root users to manage the Ghidra server by optionally prefixing server start/stop commands with
sudo -n. Also switch fromsubprocess.calltosubprocess.run(check=True)for starting/stopping ghidra so that failures are raised as errors.Anyone you think should look at this, specifically?
@whyitfor ?