feat: add ability to users to manage their vpn connections via Telegram and web portal - #92
feat: add ability to users to manage their vpn connections via Telegram and web portal#92vikbaranov wants to merge 11 commits into
Conversation
…e telegram settings check
… _tt() integration
helldweller
left a comment
There was a problem hiding this comment.
Security review — self-service connections
Reviewed the whole branch against main with a focus on what a regular (non-admin) user can reach, and specifically on whether any of this can be escalated into control of a VPN server through the Telegram bot.
The self-service core itself holds up well. ConnectionService validates ownership, intersects the protocol against a hard-coded {awg, awg2} allow-list twice, re-validates inside the lock (so the quota/status TOCTOU is closed), rolls the remote peer back when save_data fails, restricts deletion to created_by == 'self_service', and moves the blocking SSH work into asyncio.to_thread — better than the existing admin path, which blocks the event loop. The feature is off by default both globally and per server. The connection name reaches the server as json.dumps over SFTP (managers/awg_manager.py:804), never through a shell, so there is no command injection there. Test coverage is genuinely good.
The findings below are ordered by severity; details are in the inline comments.
Blockers, in my view
-
_find_user()now resolves users by Telegram@username(telegram_bot.py:409). This is the one item that is not really about self-service. A username is mutable, released on rename/deletion, and re-claimable by anyone._require_admin()uses the same lookup, so an admin whosetelegramIdfield holds a handle rather than a numeric ID can be taken over by whoever claims that handle — which yields/addserver, every client config on every server (PrivateKeyincluded), anddocker start/stopof protocol containers. Recommendation: match on the numeric ID only. Users can obtain their own ID in one message via @userinfobot, so the onboarding cost is negligible. -
Stored XSS via the connection name.
templates/users.html:863interpolatesc.nameintoinnerHTMLunescaped. That file is untouched here, but this PR is what makes the field user-controlled, so it becomes a user → admin-session escalation path. The new client-side rendering intemplates/my_connections.html:203-224also regresses from Jinja auto-escaping to partial manual escaping. -
The panel-wide
DATA_LOCKis held across the whole SSH provisioning run (connection_service.py:97). Any user can stall every write in the panel for minutes by targeting a slow or unreachable server.
Worth fixing, but could be a follow-up
- Rate limiting counts only successful creations, so failed attempts — the ones that actually hammer the VPN node over SSH — are unlimited; deletion is not limited at all.
- Raw exception text is returned to API clients and Telegram users.
- The bot never checks
chat["type"], so/connectin a group posts a config containingPrivateKeyinto the group. - Fail-open when
expiration_datecannot be parsed.
Two notes that are not inline
-
Stale-write race.
save_data()rewrites the entiredata.json, and several admin handlers (api_delete_serverat app.py:2366,api_edit_server) write without takingDATA_LOCK.create_user_connectionreadsdata, spends up to a couple of minutes on SSH, then writes the whole blob back — clobbering anything an admin changed meanwhile, including "disable user" or "delete server" (which would resurrect the entry together with its stored SSH credentials). A long user-triggered read → SSH → write-whole-blob cycle did not exist before this PR. Worth at least re-reading and merging just before the write. Separately,save_datawrites in place with no tmp+rename, so a crash mid-write truncates the file that holds every server's SSH password and private key. -
Blast radius of the feature itself. A self-service peer gets network access into the VPN subnet. If the panel or its admin UI is reachable from there, any linked user can issue themselves a route to it. Probably worth stating explicitly in the docs and/or constraining via
AllowedIPs/firewall.
Happy to look again once the identity question in (1) is settled — that one changes the threat model for everything else here.
|
We should consider potential improvements to the current implementation. I also came across this project, which looks relevant and may be worth reviewing as a reference: https://github.com/v3new/amnezia-prvtpro-miniapp It could be useful to compare its approach with our current implementation and identify any ideas, architectural improvements, or features that we could adopt. |
|
Thank you for the detailed review process. I'll carefully study all issues and fix them soon |
Some ai research |
|
@helldweller Good day! |
Core service
Telegram bot
API
Web UI
Settings
Translations