Revert "Serverless WireGuard VPN Bot v3.0 Update" - #5
Conversation
Summary of ChangesHello @kuoyaoming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request reverts the changes introduced by a previous update (v3.0), effectively transitioning the Serverless Telegram VPN Bot back to an OpenVPN-based implementation. The core purpose is to simplify the bot's setup and management by moving from Google Secret Manager for configuration and authorization to a more direct, in-code approach with in-memory activation codes. It also updates the VM provisioning strategy to use OpenVPN via Docker on Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts the previous update, changing the VPN solution from WireGuard to OpenVPN and introducing a new authorization mechanism. While the README has been significantly updated to reflect these changes and provides a clearer overview, there are several critical issues in the main.py file related to security, state management, and error handling that need immediate attention. The new authorization system relies on ephemeral in-memory state, which is unsuitable for a serverless environment, and sensitive credentials are now hardcoded directly into the source code.
| CFG = { | ||
| "token": None, | ||
| "project": None, | ||
| "authorized_users": [], | ||
| "machine": "f1-micro", | ||
| "prefix": "vpn-svr" | ||
| "token": "####your_token####", | ||
| "chat_id": "####your_chat_id####", # This is the Admin ID | ||
| "project": "majestic-cairn-487303-b3", | ||
| "default_zone": "asia-east1-c", | ||
| "prefix": "vpn-svr", | ||
| "machine": "e2-micro", | ||
| "hourly_rate": 0.005 |
There was a problem hiding this comment.
Hardcoding sensitive information like Telegram bot tokens, chat IDs, and GCP project IDs directly into the source code is a critical security vulnerability. This information should be stored securely, for example, using Google Secret Manager or environment variables, and loaded at runtime. The previous version of the code correctly used Secret Manager for this purpose.
"token": "YOUR_TELEGRAM_BOT_TOKEN_SECRET_ID",
"chat_id": "YOUR_TELEGRAM_USER_ID_SECRET_ID", # This is the Admin ID
"project": "YOUR_GCP_PROJECT_ID_SECRET_ID",
"default_zone": "asia-east1-c",
"prefix": "vpn-svr",
"machine": "e2-micro",
"hourly_rate": 0.005
}| ACTIVE_CODES = {'111111', '222222', '333333', '444444', '555555'} | ||
| AUTHORIZED_USERS = {CFG['chat_id']} # Admin is authorized by default |
There was a problem hiding this comment.
Using global in-memory variables like ACTIVE_CODES and AUTHORIZED_USERS for state management in a serverless Cloud Function is highly problematic. Cloud Functions are stateless; instances can be recycled or new ones spun up at any time, causing these variables to reset. This will lead to inconsistent authorization, activation codes disappearing, and users losing access unexpectedly. For persistent state, consider using a database (e.g., Firestore, Cloud SQL) or a distributed cache.
| # Trigger Deployment (Background or direct call?) | ||
| # For GCF, we can call the deploy logic directly here. | ||
| # But we need to make sure we don't time out the HTTP request if it's too long. | ||
| # GCF HTTP timeout is usually 60s. Deployment takes 2-3 mins. | ||
| # We must respond "OK" to Telegram and let the process run? | ||
| # Standard GCF v2 can run up to 60 mins but we need to return response to TG quickly? | ||
| # Actually, if we block here, the webhook might timeout. | ||
| # However, the user wants 'active code' logic first. | ||
| # Let's call a placeholder deploy function for now. | ||
| deploy_vm_logic(chat_id, region) |
There was a problem hiding this comment.
The comment correctly identifies a potential issue: calling deploy_vm_logic directly from the handle_callback function might lead to Cloud Function timeouts. HTTP-triggered Cloud Functions typically have a maximum timeout of 60 seconds, while VM deployment can take 2-3 minutes. If the deployment logic blocks the HTTP response for too long, Telegram's webhook might retry or consider the webhook failed. It's best to offload long-running operations to a background task (e.g., Cloud Tasks, Pub/Sub) and respond to Telegram immediately.
| def get_regions(): | ||
| global ALL_REGIONS | ||
| if not ALL_REGIONS: | ||
| try: | ||
| request = compute_v1.ListRegionsRequest(project=CFG['project']) | ||
| ALL_REGIONS = sorted([r.name for r in regions_client.list(request=request)]) | ||
| except Exception as e: | ||
| print(f"Error fetching regions: {e}") | ||
| return [] | ||
| return ALL_REGIONS |
There was a problem hiding this comment.
The ALL_REGIONS global cache will also suffer from the same ephemeral nature as ACTIVE_CODES and AUTHORIZED_USERS. While caching regions might seem like an optimization, its effectiveness will be limited by the short lifespan of Cloud Function instances. If region data is static or changes infrequently, consider fetching it once at cold start or storing it in a more persistent, accessible location.
| requests.post(url, json=payload, timeout=10) | ||
| except Exception as e: | ||
| print(f"Error editing message: {e}") | ||
| requests.post(url, json=payload) |
There was a problem hiding this comment.
The requests.post call for edit_msg lacks error handling. Network requests can fail due to various reasons (e.g., network issues, API errors, timeouts). It's crucial to wrap such calls in a try...except block to catch exceptions and log them, or inform the user if the message could not be edited.
try:
requests.post(url, json=payload)
except requests.exceptions.RequestException as e:
print(f"Error editing message: {e}")| payload = {"chat_id": chat_id, "text": text, "parse_mode": "Markdown"} | ||
| if reply_markup: | ||
| payload["reply_markup"] = reply_markup | ||
| requests.post(url, json=payload) |
There was a problem hiding this comment.
Similar to edit_msg, the requests.post call for send_msg also lacks error handling. This can lead to silent failures if Telegram's API is unreachable or returns an error. Implement a try...except block to handle potential requests.exceptions.RequestException and log the error.
try:
requests.post(url, json=payload)
except requests.exceptions.RequestException as e:
print(f"Error sending message: {e}")| requests.post(url, json=payload, timeout=10) | ||
| except Exception as e: | ||
| print(f"Error answering callback: {e}") | ||
| requests.post(url, json=payload) |
There was a problem hiding this comment.
The requests.post call for answer_callback is missing error handling. If the callback query cannot be answered, the user might experience a hanging or unresponsive bot. Add a try...except block to gracefully handle network or API errors.
try:
requests.post(url, json=payload)
except requests.exceptions.RequestException as e:
print(f"Error answering callback: {e}")| script = f"""#!/bin/bash | ||
| curl -s -X POST "https://api.telegram.org/bot{CFG['token']}/sendMessage" -d "chat_id={chat_id}" -d "text=🛠️ **Initializing {region}...**" | ||
| docker volume create ovpn-data | ||
| IP=$(curl -s ifconfig.me) | ||
| docker run -v ovpn-data:/etc/openvpn --rm kylemanna/openvpn ovpn_genconfig -u udp://$IP | ||
| echo "yes" | docker run -v ovpn-data:/etc/openvpn --rm -i kylemanna/openvpn ovpn_initpki nopass | ||
| docker run -v ovpn-data:/etc/openvpn -d -p 1194:1194/udp --cap-add=NET_ADMIN --restart always --name ovpn kylemanna/openvpn | ||
| sleep 80 | ||
| docker exec -t ovpn easyrsa build-client-full client1 nopass | ||
| docker exec -t ovpn ovpn_getclient client1 > /tmp/client1.ovpn | ||
| curl -F "chat_id={chat_id}" -F "document=@/tmp/client1.ovpn" -F "caption=✅ **VPN Ready** ({region})" https://api.telegram.org/bot{CFG['token']}/sendDocument |
There was a problem hiding this comment.
The startup script includes curl commands to send Telegram messages. While functional, this couples the VM's startup process directly to the Telegram API and the bot's token. If the token changes or the Telegram API is temporarily unavailable, the VM might fail to report its status or send the .ovpn file. Consider a more robust mechanism, such as the VM reporting its status to a central logging service or a Pub/Sub topic that the Cloud Function monitors.
| google-cloud-secret-manager==2.22.0 | ||
| requests==2.31.0 | ||
| functions-framework==3.4.0 | ||
| requests==2.31.0 No newline at end of file |
There was a problem hiding this comment.
The functions-framework package is imported in main.py (line 4) but is missing from requirements.txt. This will cause deployment failures for the Cloud Function. Please add functions-framework to the requirements.txt file.
requests==2.31.0
functions-framework
1 participant
Reverts #4