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 10 commits into
Conversation
…e telegram settings check
… _tt() integration
helldweller
left a comment
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.
| # Fallback: try username-based lookup | ||
| if username: | ||
| username_clean = str(username).lstrip("@").lower() | ||
| for u in data.get("users", []): | ||
| stored = str(u.get("telegramId", "") or "").lstrip("@").lower() | ||
| if stored and stored == username_clean: | ||
| return u |
There was a problem hiding this comment.
🔴 Critical — this turns telegramId into a spoofable identifier.
On main, _find_user() matched telegramId against the numeric Telegram ID only. This fallback also accepts an @username, and a Telegram username is not an identity: the owner can change it at any time, it is released when the account is renamed or deleted, and anyone can then claim it (Fragment even sells them).
_require_admin() (line 893) resolves through this same function. So if an admin's profile has @handle in the telegramId field — which is the natural thing to type into a field labelled "Telegram ID" — whoever holds that handle next gets full admin control of the bot: /addserver, the server list with user@host:port, client_cfg for every client on every server (those configs contain PrivateKey), toggle_proto (docker start/stop of the protocol container), remove_client. For a non-admin it is straightforward impersonation of another panel user.
Suggestion: drop the fallback and keep numeric-ID matching only. Getting your own ID costs one message — e.g. @userinfobot replies with it — so the onboarding friction is negligible compared to the exposure. Validating the field as digits-only in the user form would close this for good.
If username lookup really has to stay for first-time linking, scope it strictly to that: on first contact write msg["from"]["id"] into a separate numeric telegram_id field and match on that from then on. In any case _require_admin() must never resolve by username.
| async with lock: | ||
| async with self.data_lock: | ||
| data = self.load_data() | ||
| settings = self._settings(data) | ||
| user = self._validate_create_request(data, settings, user_id, server_id, protocol, clean_name, source) | ||
| self._check_rate_limit(user_id, source, settings) | ||
| server = data['servers'][server_id] | ||
| port = server.get('protocols', {}).get(protocol, {}).get('port', '55424') | ||
| ssh = self.get_ssh(server) | ||
| remote_client_id = None | ||
| manager = None | ||
| try: | ||
| await asyncio.to_thread(ssh.connect) | ||
| manager = self.get_protocol_manager(ssh, protocol) |
There was a problem hiding this comment.
🟠 The global DATA_LOCK is held for the entire SSH provisioning run.
self.data_lock is app.DATA_LOCK, the panel-wide write lock. add_client() for AWG is a chain of SSH calls (_insert_peer_sorted with a backup, wg syncconf, docker cp), each with a 60 s timeout, on top of a 15 s connect. For that whole window every other writer in the panel (save_data_async, reorder, auto-backup) blocks.
That means any regular self-service user can stall panel-wide writes for minutes just by targeting a server that is slow or down. The admin path (api_add_connection, app.py:3169) takes no lock at all, so this is new behaviour introduced here.
Suggested shape: hold DATA_LOCK only around the read-modify-write of the JSON (validation, and later the append + save_data), and run the SSH work outside it, guarded by _provision_locks[(server_id, protocol)] alone.
| await self._rollback_client(manager, protocol, remote_client_id) | ||
| remote_client_id = None | ||
| raise | ||
| self._record_rate_event(user_id, source) |
There was a problem hiding this comment.
🟠 Rate limiting only counts successes, so it does not limit load on the VPN server.
_record_rate_event() runs only after a connection was fully created. An attempt that dies in ssh.connect or add_client is never counted, so failed attempts are unlimited — a user can hammer /api/my/connections/add and open unbounded root SSH sessions against the VPN node. Same for anything that fails earlier: _check_rate_limit() is called after _validate_create_request() (line 94), so validation failures are free too.
Related gaps in the same mechanism:
- the key is
(source, user_id), so web and telegram get independent quotas — effectively 2× the configured limit; delete_user_connection()has no rate limit at all, and each call is an SSH round trip plus a config rewrite;- state is per-process and in-memory, so it resets on restart, and
_rate_events/_provision_locksaredefaultdicts that grow without bound.
Minimal fix: record the attempt before provisioning (or in a finally), not only on success.
| def _check_rate_limit(self, user_id, source, settings): | ||
| count = int(settings.get('rate_limit_count', 3)) | ||
| window = int(settings.get('rate_limit_window_seconds', 60)) | ||
| if count <= 0 or window <= 0: |
There was a problem hiding this comment.
🟡 rate_limit_count = 0 silently means "unlimited".
The settings form (templates/settings.html) presents this as a request count, so 0 reads as "no requests allowed" — here it disables the limiter entirely. Same for rate_limit_window_seconds. Either treat 0 as "deny" or reject it at the model level so the UI cannot produce it.
| except SelfServiceError: | ||
| raise | ||
| except Exception as e: | ||
| logger.warning("Failed to parse expiration_date '%s': %s", expiration, e) |
There was a problem hiding this comment.
🟡 Fail-open on an unparsable expiration_date.
If the date does not parse we log a warning and fall through, treating the user as active. For an expiry check the safe default is the other way round — refuse and let the admin fix the record.
| except Exception as e: | ||
| logger.exception("Error getting self-service options") | ||
| return JSONResponse({'error': str(e)}, status_code=500) |
There was a problem hiding this comment.
🟡 Internal exception text is returned to the client.
str(e) here carries whatever paramiko or the protocol manager raised — hostnames, remote paths, stderr from remote commands. Same pattern at lines 3771 and 3787. logger.exception is already doing the right thing; the response should be a generic message.
| await api.edit_message(chat_id, loading_msg_id, f"✅ {_tt(lang, 'connection_created_success')}") | ||
| except Exception as e: | ||
| logger.exception("Bot: self-service create failed") | ||
| await api.send_message(chat_id, f"❌ {_tt(lang, 'error')}: {_e(e)}") |
There was a problem hiding this comment.
🟡 Same as the API handlers — raw exception text is sent to the user.
_e(e) only HTML-escapes it; the content is still the internal error (host, path, remote stderr). Also at line 1547 in _user_delete_confirm. Keep the detail in logger.exception and send a generic failure message.
|
|
||
| await api.answer_callback(callback_id) | ||
|
|
||
| _pending_inputs[str(chat_id)] = { |
There was a problem hiding this comment.
🟡 _pending_inputs is keyed by chat_id alone.
In a group chat the wizard state is shared by everyone in that chat, so a second linked user can complete the flow another user started. Ownership is re-derived from msg["from"]["id"] in _handle_pending_input, so no connection is created under the wrong account — but the state should still be keyed by (chat_id, from_id). Same at line 1134.
| async def _set_default_commands(api: TelegramAPI): | ||
| await api.call( | ||
| "setMyCommands", | ||
| commands=[ | ||
| {"command": "start", "description": "Open bot menu"}, | ||
| {"command": "connections", "description": "Show my connections"}, | ||
| {"command": "connect", "description": "Create a new connection"}, | ||
| {"command": "disconnect", "description": "Delete a connection"}, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🟡 Nothing in the bot checks chat["type"], and these commands are now advertised to everyone.
/connect, /connections and the cfg: callback all work in any chat the bot has been added to, and the resulting config — which contains PrivateKey — is delivered with sendMessage/sendDocument to that same chat. In a group every member sees it.
Some of this predates the PR (cfg:), but registering /connect and /disconnect in the command menu makes the scenario much more likely. Suggest gating the self-service flows (and ideally config delivery in general) on chat["type"] == "private".
| class SelfServiceSettings(BaseModel): | ||
| enabled: bool = False | ||
| web_enabled: bool = True | ||
| telegram_enabled: bool = True | ||
| max_connections_per_user: int = 5 | ||
| rate_limit_count: int = 3 | ||
| rate_limit_window_seconds: int = 60 | ||
| allowed_protocols: List[str] = ['awg', 'awg2'] |
There was a problem hiding this comment.
🟢 Minor: no bounds on these fields.
templates/settings.html enforces min/max on the inputs, but the API accepts anything — max_connections_per_user: 100000 or a negative rate_limit_count go straight through. Admin-only, so low impact, but Field(ge=..., le=...) here would keep the model and the form in agreement.
2 participants
Core service
Telegram bot
API
Web UI
Settings
Translations