feat(amuleapi): one credential store, shared by amule, amuled and amuleapi - #665
Merged
Merged
Conversation
got3nks
force-pushed
the
feat/amuleapi-credentials
branch
4 times, most recently
from
July 28, 2026 09:00
03c754e to
2827b40
Compare
…leapi
amuleapi's admin and guest passwords were kept in two places: unsalted MD5
digests in amule.conf, and an amuleapi-passwords file. amule.conf always won
at load and was never written back, so a password set from amuleapi lost
silently and permanently to a stale one from the preferences dialog.
Collapse them onto amuleapi-passwords as the only store, and give every
frontend the same ability to manage it.
The format and the hashing move to src/libwebcommon/Credentials.{h,cpp},
shared by the amuleapi binary and the aMule core so the processes writing
the file cannot drift apart on it. Records become PHC-style
pbkdf2-sha256$<iterations>$<salt>$<hash> instead of a bare digest a rainbow
table resolves instantly. The KDF input stays the MD5 the preferences dialog
already produces, so the EC wire is unchanged; a bare-MD5 record from a
development config still verifies and is rewritten at the current cost on the
next login, without moving the file's mtime.
amuleapi re-reads the file on every login attempt, so a password set from
anywhere takes effect with no restart. A token whose iat predates the file's
mtime is rejected, so rotating a leaked password ends the sessions it opened;
PATCH /auth/passwords re-issues the caller's own token so the operator making
the change is not signed out by it.
The core keeps the two password preferences as transient write-only requests
(Cfg_Transient), never persisted: the dialog field opens empty, empty means
"keep the current password", and the panel says whether one is set, because a
stretched record cannot be shown. Unticking the new guest checkbox clears the
stored guest password, which is what turns guest access off. Over EC both
tags become a container with an optional hash child, carrying state from the
daemon and a request from the client; the guest toggle goes through
ApplyBoolean so a sparse PATCH /preferences cannot wipe it. The stale
/AmuleApi/Password and /AmuleApi/GuestPassword keys are deleted from
amule.conf on load.
amuleapi_password is no longer settable through PATCH /preferences: it would
travel over EC to whichever aMule this amuleapi is attached to and land in
that host's config directory, and it had neither re-auth nor rate limiting.
New: GET and PATCH /api/v0/auth/passwords, both admin-only, with the PATCH
requiring the current password against the login rate limiter.
got3nks
force-pushed
the
feat/amuleapi-credentials
branch
from
July 28, 2026 09:06
2827b40 to
61d403b
Compare
Author
|
Manual testing on Ubuntu ARM64, branch tip Monolithic aMule — local dialog
amulegui → amuled over EC — no automated coverage
Cross-cutting
Three defects found and fixed during this pass, none reachable by CI as it stood:
CI green on all 13 checks, including both mingw-w64 legs. |
This was referenced Jul 28, 2026
Merged
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
Mechanical rebase to resolve po/ conflicts against master's amule-org#665 -- no source changes here.
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
…-only effect got3nks's review on amule-org#662, both real issues: 1. The tooltip had it backwards. A sparse file reserves *no* physical disk space up front -- it's created at full logical length but grows physically as data lands, the opposite of what the old wording said ("reserves its full size... immediately"). Rewritten with the exact corrected text from the review. 2. The setting is a no-op outside Windows: PlatformSpecific.cpp and PartFile.cpp both call CFile::Create(name, true) on POSIX regardless of this preference, and part files end up sparse anyway once chunks land at offsets. Documented in the tooltip and a code comment rather than gating the checkbox behind __WINDOWS__ -- a remote amuleGUI has no EC capability tag to know what platform the core it's driving runs on, so a compile-time gate would be wrong in both directions. The ID collision with amule-org#665 (IDC_AMULEAPI_GUEST_ENABLED etc. took 10489-10491, the same range IDC_CREATEFILESSPARSE was using) was already resolved during the rebase -- moved to 10492. po/ regenerated last, after the tooltip fix, so the corrected string (not the wrong one) is what lands in the catalogs. Verified: built and ran both amule and amuleGUI, full unit test suite passing (28/28, including the new CredentialsTest from amule-org#665).
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
…-only effect got3nks's review on amule-org#662, both real issues: 1. The tooltip had it backwards. A sparse file reserves *no* physical disk space up front -- it's created at full logical length but grows physically as data lands, the opposite of what the old wording said ("reserves its full size... immediately"). Rewritten with the exact corrected text from the review. 2. The setting is a no-op outside Windows: PlatformSpecific.cpp and PartFile.cpp both call CFile::Create(name, true) on POSIX regardless of this preference, and part files end up sparse anyway once chunks land at offsets. Documented in the tooltip and a code comment rather than gating the checkbox behind __WINDOWS__ -- a remote amuleGUI has no EC capability tag to know what platform the core it's driving runs on, so a compile-time gate would be wrong in both directions. The ID collision with amule-org#665 (IDC_AMULEAPI_GUEST_ENABLED etc. took 10489-10491, the same range IDC_CREATEFILESSPARSE was using) was already resolved during the rebase -- moved to 10492. po/ regenerated last, after the tooltip fix, so the corrected string (not the wrong one) is what lands in the catalogs. Verified: built and ran both amule and amuleGUI, full unit test suite passing (28/28, including the new CredentialsTest from amule-org#665).
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
Mechanical rebase to resolve po/ conflicts against master's amule-org#665/amule-org#667 -- no source changes here.
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
Mechanical rebase to resolve conflicts against master's amule-org#642/amule-org#643 -- no source changes here. Resolved a real conflict in amuleDlg.cpp's event table: amule-org#642 added the Alt+<letter> EVT_MENU block right where this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept amule-org#642's block, dropped the connect-button line as intended. Regenerated again via scripts/update-po.sh after later rebasing onto master's amule-org#671 (Preferences sidebar) and amule-org#665/amule-org#662 (amuleapi credentials, sparse part-file setting), which had added strings this branch's catalogs didn't have yet.
LSalami
added a commit
to LSalami/amule
that referenced
this pull request
Jul 28, 2026
Mechanical rebase to resolve conflicts against master's amule-org#642/amule-org#643 -- no source changes here. Resolved a real conflict in amuleDlg.cpp's event table: amule-org#642 added the Alt+<letter> EVT_MENU block right where this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept amule-org#642's block, dropped the connect-button line as intended. Regenerated again via scripts/update-po.sh after later rebasing onto master's amule-org#671 (Preferences sidebar) and amule-org#665/amule-org#662 (amuleapi credentials, sparse part-file setting), which had added strings this branch's catalogs didn't have yet.
got3nks
pushed a commit
that referenced
this pull request
Jul 28, 2026
* feat(gui): remove the global Connect/Disconnect toolbar button Closes #402. Per-network controls already exist (Kad pane's Start/Stop, Servers pane's ED2K connect/disconnect), the connection state is already shown in the status bar, and after discussing relocating it into the Networks view the agreed call (got3nks + ngosang) was simply to remove it -- mirrors what #585 already did on the Web UI side. Removed the toolbar tool, its three skin icons (Toolbar_Connect/ Disconnect/Connecting) and their bitmap wiring, the button-update block in ShowConnectionState(), and the two EnableTool(ID_BUTTONCONNECT, ...) call sites. CamuleDlg::OnBnConnect() itself stays -- CMuleTrayIcon::DoConnectDisconnect() still calls it for the tray icon's own connect/disconnect action, which is unaffected by this change. ShowConnectionState()'s skinChanged parameter was only ever read by the now-removed block, so it's gone too, along with the one call site that passed true and the forceUpdate plumbing that reached it through GuiEvents.cpp's ShowConnState() free function. Left ID_BUTTONCONNECT and muuli_wdr.cpp's muleToolbar() (which still calls it) alone -- that function predates Apply_Toolbar_Skin, is never called anywhere, and touching pre-existing dead code is out of scope for this fix. Verified: built and ran both amule and amulegui cleanly, full unit test suite passing (26/26). Confirmed via the po/ diff that "Connect"/ "Disconnect"/"Cancel" remain in the catalog (still used by other per-network buttons) -- only their now-orphaned toolbar-specific tooltip strings were dropped. Could not get a real on-screen visual confirmation of the toolbar layout -- same macOS Accessibility automation limitations as #180 got in the way of screenshotting the actual app window. * i18n: regenerate po/ catalogs after rebasing onto current master Mechanical rebase to resolve conflicts against master's #642/#643 -- no source changes here. Resolved a real conflict in amuleDlg.cpp's event table: #642 added the Alt+<letter> EVT_MENU block right where this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept #642's block, dropped the connect-button line as intended. Regenerated again via scripts/update-po.sh after later rebasing onto master's #671 (Preferences sidebar) and #665/#662 (amuleapi credentials, sparse part-file setting), which had added strings this branch's catalogs didn't have yet. * feat(gui): turn the per-network Disconnect buttons into Connect/Cancel/Disconnect toggles Relocates the removed global toolbar button's functionality into the ED2K (IDC_ED2KDISCONNECT) and Kad (ID_KADDISCONNECT) panes instead of just dropping it, per got3nks's proposal on #663: each button now mirrors ed2kState/kadState (off/connecting/connected) using the existing connButImg() bitmaps, and can independently connect or cancel/disconnect its own network -- something the old OR-toggled global button never allowed. The ED2K pane also gains a Connect button it never had (previously Disconnect-only). CServerWnd::UpdateED2KConnectButton() / CKadDlg::UpdateConnectButton() are called from CamuleDlg::ShowConnectionState() alongside the existing UpdateED2KInfo()/UpdateKadInfo() calls, and once from each pane's own ctor/Init() for the initial paint. OnBnClickedED2KDisconnect and OnBnClickedDisconnectKad gained the missing "connect when off" branch; OnBnConnect and the tray icon are untouched. Verified interactively on Windows: both buttons show the correct label/icon for their live state, ED2K disconnect/reconnect works from its own button, and disabling ED2K in Preferences while Kad stays connected confirms the two toggle independently. po/ regenerated via scripts/update-po.sh to match the new button labels ("Disconnect Kad" is now unused and moves to the obsolete section; no new translatable strings). * fix(gui): drop the per-network connect-button state cache got3nks flagged two bugs in review (PR #663): the static cache's early return also skipped Enable(), so the button could get stuck disabled once IsReady()/network-pref state changed without the Connect/Cancel/Disconnect state itself changing (master had two now- deleted escape hatches -- the skinChanged force-path and an unconditional EnableTool call -- that used to paper over this). Worse, the cache was function-scope static, surviving widget recreation, so a fresh pane's Init() call could early-return and leave the wxDesigner default label ("Disconnect Kad") on screen. Dropping the cache removes both: SetLabel/SetBitmap/Enable are cheap on a plain wxButton (unlike the old toolbar tool, which had real native-image-list reasons to memoize, see amuleDlg.cpp:1054-ish), and master's own second call site already did an unconditional EnableTool every tick without issue. Second review round (macOS + Ubuntu ARM64, monolithic + amuleGUI) confirmed the toggle works functionally but flagged three cosmetic issues, all fixed here: - No gap between the connect-button icon and its label. The obvious fix, wxButton::SetBitmapMargins(), is NOT portable (wxOSX overrides it, wxGTK inherits the base no-op) -- prefixing the label with a literal space outside the _() call behaves identically on both and introduces no new msgid. - The Kad button was stretched full-width by its sizer's .Expand() flag, while the ED2K one sizes naturally; GTK and macOS then filled the slack differently (centered vs. icon-left). Dropped .Expand() so the two buttons size the same way -- trades away lining up with the "Bootstrap from known clients" button above it, but the two connect buttons reading as the same control matters more. - CServerWnd::UpdateED2KConnectButton() and CKadDlg::UpdateConnectButton() were near-identical (same 3-state enum, same switch, same three bitmaps). Factored into a shared SetConnectButtonState(button, state, enabled) helper in muuli_wdr.cpp/.h, next to connButImg() which it reuses -- so the icon-spacing fix above lives in one place instead of two. Verified interactively on Windows: re-tested full disconnect/reconnect cycle on both panes, toggling ED2K off and back on via Preferences (existing behavior: needs an app restart to re-enable) with correct state on the fresh post-restart paint, and confirmed by screenshot that both buttons now show a visible icon/label gap and size the same way (Kad no longer stretched). Also confirmed amulegui (CLIENT_GUI) builds clean with these changes; did not perform full interactive remote-daemon testing. * fix(gui): first-paint layout bug + connect-button placement, per review Two more issues from got3nks's Linux/GTK pass on #663. 1. Bug: the ED2K connect button painted with the icon overlapping the label on first show (Kad was fine, only by luck of timing). CServerWnd::UpdateED2KConnectButton() runs after sizer->Show(this, TRUE) in the ctor, so SetBitmap() grows the button's best size after the row was already measured against the text-only label, and nothing re-lays it out until a window resize. Fixed by calling Layout() on the button's parent at the end of the shared SetConnectButtonState() helper -- covers both panes, and stops Kad from relying on being laid out late (its notebook page is only measured once shown, after Init() already set its bitmap). 2. Layout: moved both connect/disconnect toggles to lead their pane instead of sitting among secondary controls, per got3nks's request: - ED2K (serverListDlgUp): toggle now on its own top row, right- aligned via a stretch spacer, directly above the server list. The "Add server manually" form (name/IP/port/Add) moved below the table instead of sharing a row with the toggle; the vertical separator that used to sit between them is gone (no longer needed once they're not adjacent). - Kad (KadDlg): toggle moved out of the "Bootstrap" static box (where it sat under "Bootstrap from known clients") to its own top row above the box, right-aligned the same way as ED2K's. item0 changed from a 1x1 FlexGridSizer (which only ever held the two-column area) to a plain vertical wxBoxSizer so it can hold the new top row plus the existing two-column layout. Both panes now read "primary control on top, secondary/manual controls below" -- matching shape on both tabs. Verified interactively on Windows: screenshotted both panes, confirmed no icon/label overlap on first paint (no window resize needed), and re-ran the disconnect/reconnect toggle cycle on both after the layout change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #656.
amuleapi's admin and guest passwords lived in two places at once: unsalted MD5 digests in
amule.conf, and anamuleapi-passwordsfile.amule.confalways won at load and was never written back, so a password set withamuleapi --set-admin-passlost silently and permanently to a stale one from the preferences dialog — deterministically wrong rather than racy, which is why it never looked like a bug.This collapses them onto
amuleapi-passwordsas the single store and gives every frontend the same ability to manage it: monolithic aMule, amulegui→amuled, and amuleapi itself.The store
The format and the hashing move to
src/libwebcommon/Credentials.{h,cpp}, linked into both the amuleapi binary and the aMule core, so the processes that write the file cannot drift apart on it. Records become PHC-stylepbkdf2-sha256$<iterations>$<salt>$<hash>instead of a bare digest a rainbow table resolves instantly.The KDF input stays the MD5 the preferences dialog already produces. That is deliberate: EC sessions are unencrypted, so asking for plaintext would put a new secret on the wire to gain nothing. The wire format is unchanged; only what lands on disk improves.
A bare-MD5 record from a development config still verifies and is rewritten at the current cost on the next login — without moving the file's mtime, because an internal re-hash is not a password change and must not sign anyone out.
Managing it
Because a stretched record cannot be read back, every interface treats its password field as write-only: it opens empty, and empty means "keep the current password". The preferences panel now says beside the admin field whether one is set, so an empty box no longer reads as "nothing configured". A new Enable guest access checkbox sits beside the guest field; unticking it clears the stored guest password, which is what turns guest access off.
Over EC both credential tags become a container with an optional hash child, carrying state from the daemon and a request from the client:
Admin deliberately has no clear state: a client that merely forgot the field would otherwise lock a non-loopback deployment out of its own API. The guest toggle goes through
ApplyBoolean, becauseCEC_Prefs_Packet::Applyserves two callers with opposite conventions — amulegui sends the whole group atEC_DETAIL_UPDATE(absent = off) whilePATCH /preferencessends only named fields atEC_DETAIL_FULL(absent = leave alone).The stale
/AmuleApi/Passwordand/AmuleApi/GuestPasswordkeys are deleted fromamule.confon load. Nothing to migrate — the digests cannot be converted to the stretched form without the password, so the operator sets it once more. amuleapi has not shipped, so there is no installed base.REST
New
GETandPATCH /api/v0/auth/passwords, both admin-only. The PATCH requirescurrent_passwordeven though the caller already holds an admin token — a stolen token alone should not be enough to lock the real operator out — and checks it against the same per-IP limiter as/auth/login.amuleapi_passwordis no longer accepted byPATCH /preferences(400 rather than silently ignored). It had neither re-auth nor rate limiting, and it would travel over EC to whichever aMule this amuleapi is attached to, landing in that host's config directory rather than the file this daemon reads.Rotation ends other sessions
A token whose
iatpredates the credential file's mtime is rejected. The cutoff is a property of the file, so this holds however the change was made — REST, CLI, preferences dialog, or amulegui over EC — and survives a restart. Without it, rotating a leaked password would leave whoever leaked it signed in for up to a day.PATCH /auth/passwordsre-issues the caller's own token in the same response, so the operator making the change is not signed out by their own request. The 60 identical auth call sites collapse onto oneCApiDispatcher::Authenticate()so the cutoff cannot be forgotten at a call site.Two bugs this found
PATCH /preferencesinvalidated every live session. aMule applies credentials after every preferences save, and the write was unconditional — so an unrelated preference change bumped the file's mtime and signed everyone out. Writes are now skipped when nothing actually changed. Caught by running the full curl suite: 96/102 assertions in15-preferences-patch.shfailed.PATCH /preferenceswould have wiped the guest credential, because the apply side read "tag absent" as "guest off" regardless of the sparse/full convention described above.Testing
CredentialsTest— 19 new cases: salt uniqueness, malformed records never verifying, legacy verify + upgrade, file round-trip, empty guest round-tripping as disabled, and the shared change semantics (empty admin leaves admin alone, guest disabled clears, guest enabled with no digest keeps).AmuleApiConfigTest— reworked to the new API, plus rotation-picked-up-without-restart and legacy-upgraded-on-login.unittests/curl-tests/amuleapi/02-auth.sh— extended to 66 assertions covering GET/PATCH, every validation error, the guest role gate, rotation revoking other sessions, and the caller's token surviving. It restoresADMIN_PASSafterwards since the suite shares one daemon../scripts/update-po.shregenerated for eight new dialog strings.docs/man/po/is untouched:amuleapi.1.inis not inpo4a.config.in, so regenerating produced only aPOT-Creation-Datebump.Not in scope
src/webapi/is absent frompo/POTFILES.in, so amuleapi's own CLI strings are still untranslated — pre-existing, and adding it would pull ~100 strings into 52 catalogs. Worth a separate change.Manual testing still to do
The amulegui → amuled EC round trip has no automated harness. Setting and clearing passwords from a remote GUI, and confirming the "is set" indicator reflects daemon state, needs a manual pass before merge.