netatalk: add unprivileged single-user mode - #3277
Conversation
a473a06 to
e221d8d
Compare
|
@andylemin something that's been on my mind for a few years, now finally possible! this involved quite a lot of scaffolding so hardly a plug'n'play thing for beginners to run locally, but perhaps it could be the foundation of a variant of the containerized netatalk that runs non-privileged? |
andylemin
left a comment
andylemin
left a comment
There was a problem hiding this comment.
Thanks for this — the restriction set is well chosen and validate_unprivileged_config() is thorough. A few things I'd like to talk through, mostly around the setuid-root afppasswd path and the config-parse/daemonize() reordering. Comments inline.
Two I think need resolving before this lands:
- the new non-root
afppasswd -cpath reachingopen(O_CREAT|O_TRUNC)with euid 0 on a caller-supplied-ptarget afp_config_parse()moving ahead ofdaemonize(), which silently drops file logging for the default root service
One more that didn't fall on a changed line, so I couldn't anchor it: update_srp_passwd() dereferences getpass() without a NULL check (lines 562, 617, 646), which segfaults when there's no controlling terminal. That's pre-existing, but the manual now shows a script-shaped afppasswd -c -p <path>, so it's newly easy to reach — worth a cheap guard while we're here?
| return -1; | ||
| } | ||
|
|
||
| if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { |
There was a problem hiding this comment.
afppasswd is installed rwsr-xr-x (bin/afppasswd/meson.build:28) and nothing in the program drops euid, so this open() runs as root against whatever -p supplies — no O_EXCL, no O_NOFOLLOW, no path restriction.
How do we feel about afppasswd -c -f -w x -p /etc/shadow from an unprivileged account? It truncates the target and fchmods it 0600. Non-root -c was refused outright before, so this is the first path that gets there.
Would dropping to the real uid before creating, or requiring the path to resolve under the caller's own home plus O_EXCL|O_NOFOLLOW, be enough to close it?
There was a problem hiding this comment.
Thanks for the get_password() wrapper — that's the getpass() NULL path cleanly closed across all seven call sites.
This one still stands, though. create_srp_file_for_user() opens the caller-supplied -p path with O_CREAT | O_TRUNC while euid is still 0: the binary installs rwsr-xr-x (bin/afppasswd/meson.build:28) and the only credential call in the file is getuid() at line 1064, so nothing drops privilege first. afppasswd -c -f -p /etc/shadow from any account truncates it as root.
Cheapest shape is probably to drop to the real uid before touching anything a caller named — the non-root create path needs no root at all, since it only writes a file the user owns. Alternatively refuse -p outright when geteuid() != getuid(). Happy to look at either.
| return -1; | ||
| } | ||
|
|
||
| if (!i && ((flags & OPT_FORCE) == 0)) { |
There was a problem hiding this comment.
With -p omitted, path is still _PATH_AFPDSRPPWFILE, so a plain afppasswd -c -f from a regular user lands on the shared system verifier and create_srp_file_for_user() truncates it — everyone else's entries included.
Should non-root -c require an explicit -p, and refuse a target it doesn't already own?
There was a problem hiding this comment.
Still open as well, and it compounds the setuid issue above: with -p omitted, path is set to _PATH_AFPDSRPPWFILE (line ~1128) before the new non-root branch runs, so a plain afppasswd -c -f from a regular account calls create_srp_file_for_user() on the shared system verifier and replaces it with one entry for the calling user.
Requiring an explicit -p in the non-root create path would close it, or defaulting that path to something under $HOME when the caller is not root. Combined with the privilege drop, the blast radius goes away entirely.
|
|
||
| /* Verify old password for non-root users */ | ||
| if ((flags & OPT_ISROOT) == 0) { | ||
| if ((flags & OPT_ISROOT) == 0 && !(flags & OPT_INITIAL)) { |
There was a problem hiding this comment.
OPT_INITIAL is only ever set immediately after create_srp_file_for_user(), so skipping the old-password proof is self-consistent in isolation. But the file it just wrote over may have held a real verifier, so combined with the default -p above, does this let someone set a new AFP credential without knowing the current one?
Would deriving this from "we created this file in this run and it was genuinely new" be tighter than a caller-supplied flag?
There was a problem hiding this comment.
OPT_INITIAL itself reads fine now that it is only ever set immediately after the create — thanks for tightening that.
The residual is inherited from the default-path thread rather than from this flag: because a non-root -c -f can still land on the shared verifier, OPT_INITIAL then skips the old-password proof on an entry the caller never owned. Fixing the path and privilege side closes this without any change here.
| p++; | ||
|
|
||
| if (!(flags & OPT_ISROOT) && (*p == PASSWD_ILLEGAL)) { | ||
| if (!(flags & OPT_ISROOT) && !(flags & OPT_INITIAL) |
There was a problem hiding this comment.
Same thought on the * marker: OPT_INITIAL skips it, and the truncate upstream means an admin-disabled entry is gone by the time we reach here anyway. Is re-enabling a disabled account meant to be reachable without root?
There was a problem hiding this comment.
Same relationship as the old-password thread: the * skip is reasonable for a genuinely fresh file, and only becomes a re-enable-without-root path while a non-root -c can still target the shared verifier. No change needed here once that is fixed.
| return -1; | ||
| } | ||
|
|
||
| return update_srp_passwd(path, pwd->pw_name, flags | OPT_INITIAL, pass); |
There was a problem hiding this comment.
By this point create_srp_file_for_user() has already truncated the file and written the * placeholder, so a password mismatch, a cracklib rejection, or Ctrl-C leaves the previous verifier destroyed and the account disabled.
Recovery looks awkward too: -c without -f is refused above, and a plain update takes the non-OPT_INITIAL branch and prints "Your password is disabled. Please see your administrator." — in a mode whose premise is that there's no administrator.
Could we collect and validate the password first, then create and write once?
There was a problem hiding this comment.
Still create-then-prompt: create_srp_file_for_user() writes the placeholder record before update_srp_passwd() asks for the password, so a typo at the confirmation prompt or a Ctrl-C leaves a disabled verifier behind — and on an existing file, the previous contents are already gone.
Collecting and validating the password first and writing once would fix it. If you would rather keep the two functions separate, writing to a temp file in the same directory and rename()ing on success gives the same atomicity.
| exit(EXITERR_CONF); | ||
| } | ||
|
|
||
| volumes_loaded = (load_afp_conf_vols(&obj, LV_ALL) == 0); |
There was a problem hiding this comment.
One side effect of parsing before check_lockfile(): a second netatalk invocation now loads all volumes first, so get_vol_uuid() can generate and append a UUID to afp_voluuid.conf and every volume path gets EA-probed, before we notice another instance is already running. Previously a duplicate start exited before touching anything.
Could the lockfile check move back ahead of this?
There was a problem hiding this comment.
Order is still parse (1056) then load_afp_conf_vols() (1060) then check_lockfile() (1066), so a second netatalk invocation loads every volume — appending UUIDs to afp_voluuid.conf, running the EA probes — before it discovers the lock and exits.
Moving check_lockfile() above the volume load keeps the duplicate-instance case free of side effects.
| return count == 1; | ||
| } | ||
|
|
||
| static bool dbpath_parent_is_writable(const char *path) |
There was a problem hiding this comment.
The trailing-slash strip and strrchr parent derivation here is identical to pidfile_path_is_private() just below — the two differ only in what they check afterwards. Since both feed security decisions, would a single shared helper be safer than keeping two copies in sync? I couldn't find a dirname-style primitive in libatalk to reuse, so a small static one here might be the way.
There was a problem hiding this comment.
The trailing-slash strip and strrchr parent derivation is still duplicated between the two helpers. Since both are security checks, a single helper taking the path plus a "needs write" flag would keep them from drifting apart later.
| static void show_usage(void) | ||
| { | ||
| fprintf(stderr, "Usage:\tafpd [-d] [-F configfile]\n"); | ||
| fprintf(stderr, "Usage:\tafpd [-du] [-F configfile]\n"); |
There was a problem hiding this comment.
doc/manpages/man8/afpd.8.md still lists only -d, -v, -V, -h and -F — should -u be documented there to match this? (netatalk.8.md covers -u and -P nicely.)
There was a problem hiding this comment.
doc/manpages/man8/afpd.8.md still documents only -d, -v, -V, -h and -F, so -u remains accepted and advertised in the usage string but absent from the man page. The netatalk.8.md wording you added would carry over nicely.
| char *unprivileged_args[] = { "afpd", "-u" }; | ||
| TEST(afp_options_parse_cmdline(&unprivileged_obj, 2, unprivileged_args), | ||
| "parse afpd rootless command-line option"); | ||
| TEST_expr(reti = 0, |
There was a problem hiding this comment.
This asserts the flag bit gets set, which would stay green even if the pw_uid != getuid() check in auth.c were removed — and that's the only runtime enforcement point for the whole mode.
Worth adding a case asserting a uid mismatch is rejected and a match accepted, plus something over validate_unprivileged_config()? It carries most of the security contract and currently has no coverage.
There was a problem hiding this comment.
The new case is a good start — parsing -u and asserting the flag lands is worth having.
It does still pass if the behaviour behind the flag is removed: deleting the pw_uid != getuid() check in auth.c keeps this green, because nothing here exercises the refusal. A case that logs in with a non-matching uid and expects rejection would pin the part that matters.
|
@andylemin just wanted to leave a note to say that I have this on my radar -- a lot of great feedback here that I want to work through properly do you think it's okay to push this into the 4.6.0 release when all the feedback is addressed, or better to postpone? |
We are getting close to the release date now, but at the same time, this is an "it either works or it doesn't change", so I don't mind. |
Allow a non-root user to run the controller with a safe, explicitly configured single-user AFP service. Restrict the mode to a user-owned SRP verifier and static SQLite volumes with explicit UUIDs and signatures, pass the mode through to afpd, and authenticate only the invoking account. Require a private absolute PID path, reject privilege-dependent features, and disable configuration reloads. Keep the SQLite CNID database, WAL, and shared-memory files private to the serving user. Add non-root SRP verifier initialization and robust password-input error handling in afppasswd, update the manuals, and cover command-line parsing.
e221d8d to
5444e7c
Compare
|
@andylemin I basically concur with all feedback given :) can you please take a closer look at the SQLite hacks to make the database files only owner accessible in unprivileged mode? |
|
📊 Performance DashboardCommit: 🔥 Spectest (AFP 3.4) - FlameGraphNetatalk Code-time: 3.6% · Runtime: 60s · Stacks: 846 🔥 Click the preview to open the interactive flamegraph (zoom + search). 🔝 Top 10 leaf functions
📈 Speedtest (AFP 3.4) - PerfGraphPeak Read: 7929 MB/s (+6.9% vs hist avg 7419.1 MB/s; min 6342 / max 9526 over 30 PRs) 🔝 Throughputs per operation (vs. historical average)
⏱️ Lantest (AFP 3.4) - LatencyGraphAvg total runtime: 3997 ms (+1.0% vs hist avg 3957.7 ms; min 2122 / max 5085 over 30 PRs) 🐢 All operations (avg runtime, in test order, vs. historical average)
Run baseline: median op-test delta -1.8%, MAD 6.3%. Adj Δ% shifts each delta by the median; standouts ≥5% in bold. A large MAD means the run did not move uniformly — read the adjusted column with caution. |
|
@rdmark rather than calling this unprivileged mode, I think we should call this single user mode. Unprivileged makes this sound "less than", while it is actually "more than" as no one but the user can access anything, not even the CNID at rest. One could argue this is a perfect design for home directories, where each user/home has its own single user process, and the CNID is kept separately under the homes path in a hidden location or something. The SQLite in-process library-based design is perfect for single user mode, as it allows for many concurrent single user databases (being part of afpd they never collide). And you only need MySQL for multiuser mode (keeping the advanced machinery only where needed). I see lots of value and potential in having both "private"/"single user" shares and standard multi user shares coexisting on the same server. The more I think about it, the more elegant it appears; Thought some more; Whilst not your original separate daemon/port intention I know, but the piece we might be missing is a per-volume This class of change (separate DB for private vols) would also help to solve the Xapian/LocalSearch privacy issues (Xapian/LocalSearch would need separate daemons, but private SQLite CNID becomes a trivial solve for home directories).. |
|
Went back through the threads against Genuinely closed, and I have left those resolved:
The SQLite work you asked me to look at is right for the afpd path; the only gap is that it keys on the process's own command line, so another tool run by the same user undoes it. Details on that thread. Of what is left, two are the ones I would treat as merge blockers, both unchanged so far: the setuid-root Worth flagging one thing that is not in any thread: the manual's own example cannot start a working server yet, because the Happy to re-review as soon as you want another pass, and equally happy to take any of the mechanical ones off your hands if that helps get it in. |
|
@andylemin I see your point about the naming -- "unprivileged" is a description of the mechanics rather than the utility for the end user. if we lean into the CNID database isolation in the documentation, I might favor and yes, if you are up for it I'm happy for you to take over this PR and take care of the finishing touches! I'm feeling under the weather this week so at a limited mental capacity. if you like you can cherry-pick the current commit into a fresh feature branch. |
|
@rdmark Hope you feel better soon. Take care |
|
@rdmark Happy to take this over. Let me have a think about how to approach it. But I need to find the correct compromise so it works with the standard multi-user architecture (where the parent process federates users). It needs to work in such a way that the standard model gains the same benefits; individual users can access both private vols/CNID databases and shared vols/CNID databases. I believe this is the more impactful and common use-case which can work by default for home shares, but needs the child afpd process to maintain multiple CNIDs. SQLite fits the use case beautifully as its biggest limitation is multiple user contention. And its biggest strength is being in-process library. So an enterprise setup would have a child proc with a MySQL connection for the master CNID holding all the shared vols, and then as many private in-proc SQLite objects as needed. So requires some form of CNID backend array. This keeps the existing code structurally the same (where the master CNID can be different backends), and the new private vol array (which can only be SQLite). These private backends will need to be stored inline within the same private vols for both scalability, security and compartmentalisation. |
|
@rdmark playing devils advocate; Given the above described functionality which offers a containerised-like vol access (with the private CNID inside the target vol), what is the remaining benefit of this PR as it stands? The remaining feature it would offer would be a single-user parent. I think that would only benefit very specific use cases or locked down systems where an operator has no admin access (so would need manual start etc)? What do you think? |
|
This one is definitely for after the upcoming release |
|
@andylemin the original use case I had in mind was a non-sysadmin user who have a regular account and want to share their home directory casually it could be someone who installs Netatalk on macOS via Homebrew, for instance, where the default assumption/expectation is that an installed app runs as a non-root user I've also toyed with the idea of a macOS GUI app that emulates old-school "personal file sharing" by launching Netatalk -- which obviously would run as a non-privileged user! another use case would be for testing: maybe the spectest could by default launch its own netatalk process with a temporary file system |
|
Working on this now when I can |
2 participants
Allow a non-root user to run the controller with a safe, explicitly configured single-user AFP service. Restrict the mode to a user-owned SRP verifier and static SQLite volumes with explicit UUIDs and signatures, pass the mode through to afpd, and authenticate only the invoking account.
Require a private absolute PID path, reject privilege-dependent features, and disable configuration reloads. Keep the SQLite CNID database, WAL, and shared-memory files private to the serving user.
Add non-root SRP verifier initialization and robust password-input error handling in afppasswd, update the manuals, and cover command-line parsing.