Add TLS support #3389
Open
Add TLS support #3389
Conversation
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
Reviewer's GuideIntroduces end-to-end TLS support for the beacon service by migrating to a new settings version, managing certificates and Nginx configurations programmatically, and exposing toggle endpoints, while updating the frontend wizard and store to let users enable or disable TLS. Includes new Nginx templates and minor refactorings in the bootstrapper and helper service. Sequence diagram for toggling TLS via frontend and backendsequenceDiagram
actor User
participant Wizard as Setup Wizard (frontend)
participant BeaconStore as BeaconStore (frontend store)
participant API as Beacon API
participant Nginx as Nginx
User->>Wizard: Toggle 'Enable TLS' checkbox
Wizard->>BeaconStore: setTLS(enable_tls)
BeaconStore->>API: POST /use_tls (enable_tls)
API->>API: set_enable_tls(enable_tls)
alt enable_tls = true
API->>API: generate_cert()
API->>API: generate_new_nginx_config(use_tls=True)
API->>API: nginx_config_is_valid()
API->>API: nginx_promote_config()
API->>Nginx: reload_nginx_config()
else enable_tls = false
API->>API: generate_new_nginx_config(use_tls=False)
API->>API: nginx_config_is_valid()
API->>API: nginx_promote_config()
API->>Nginx: reload_nginx_config()
API->>API: Remove cert/key files
end
API-->>BeaconStore: Success/Failure
BeaconStore-->>Wizard: Update UI
Wizard-->>User: Show result
Class diagram for Beacon class with TLS managementclassDiagram
class Beacon {
- runners: Dict[str, AsyncRunner]
- manager: Manager
+ get_enable_tls() bool
+ set_enable_tls(enable_tls: bool) None
+ generate_cert() None
+ generate_new_nginx_config(config_path: str, use_tls: bool) None
+ nginx_config_is_valid(config_path: str) bool
+ nginx_promote_config(config_path: str, new_config_path: str, keep_backup: bool) None
+ reload_nginx_config() None
}
Class diagram for SettingsV5 (settings.py)classDiagram
class SettingsV4 {
+ VERSION: int
+ migrate(data: Dict[str, Any])
...
}
class SettingsV5 {
+ VERSION: int
+ use_tls: bool
+ migrate(data: Dict[str, Any])
}
SettingsV5 --|> SettingsV4
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Closed
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @matt-bathyscope - I've reviewed your changes - here's some feedback:
Blocking issues:
- Certificate verification has been explicitly disabled. This permits insecure connections to insecure servers. Re-enable certification validation. (link)
General comments:
- In
generate_cert, remove the use of shlex.quote when passing-subjand-addextto openssl withshell=False, since the literal quotes will end up in your certificate fields. - Avoid hardcoding IP addresses in the SAN list (
192.168.2.2,192.168.3.1); instead discover the actual interface addresses at runtime so certificates stay accurate across different network setups. - Before calling
os.unlinkon the TLS key/cert files in your enable/disable logic, check that the files actually exist to prevent uncaught file-not-found errors when toggling TLS.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `generate_cert`, remove the use of shlex.quote when passing `-subj` and `-addext` to openssl with `shell=False`, since the literal quotes will end up in your certificate fields.
- Avoid hardcoding IP addresses in the SAN list (`192.168.2.2`, `192.168.3.1`); instead discover the actual interface addresses at runtime so certificates stay accurate across different network setups.
- Before calling `os.unlink` on the TLS key/cert files in your enable/disable logic, check that the files actually exist to prevent uncaught file-not-found errors when toggling TLS.
## Individual Comments
### Comment 1
<location> `core/services/beacon/main.py:144` </location>
<code_context>
interface.domain_names = [f"{hostname}-hotspot"]
self.manager.save()
+ # if the hostname is changed and we have TLS enabled we need to regenerate the cert
+ if self.get_enable_tls():
+ os.unlink(TLS_KEY_PATH)
+ os.unlink(TLS_CERT_PATH)
+ self.generate_cert()
+ self.reload_nginx_config()
def get_hostname(self) -> str:
</code_context>
<issue_to_address>
Potential race condition when deleting and regenerating TLS certs.
If generate_cert fails after deleting the existing certs, the system will be left without valid TLS files. To prevent this, generate new certs in a temporary location and atomically replace the originals.
</issue_to_address>
### Comment 2
<location> `core/services/beacon/main.py:169` </location>
<code_context>
+
+ def set_enable_tls(self, enable_tls: bool) -> None:
+ # handle enabling/disabling tls
+ if not enable_tls and self.get_enable_tls():
+ # tls is currently enabled and we need to disable
+ # change nginx config
+ self.generate_new_nginx_config(use_tls=False)
+ # validate config
+ if not self.nginx_config_is_valid():
+ raise SystemError("Unable to validate staged Nginx config")
+ # bounce nginx
+ self.nginx_promote_config(keep_backup=True)
+ # remove old cert
+ os.unlink(TLS_CERT_PATH)
+ os.unlink(TLS_KEY_PATH)
+ elif enable_tls and not self.get_enable_tls():
+ # tls is currently disabled and we need to enable
</code_context>
<issue_to_address>
Unlinking TLS cert/key files without checking existence may raise exceptions.
os.unlink raises FileNotFoundError if the file is missing. Please check for file existence before unlinking or handle the exception to prevent errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# remove old cert
os.unlink(TLS_CERT_PATH)
os.unlink(TLS_KEY_PATH)
=======
# remove old cert
if os.path.exists(TLS_CERT_PATH):
os.unlink(TLS_CERT_PATH)
if os.path.exists(TLS_KEY_PATH):
os.unlink(TLS_KEY_PATH)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `core/services/beacon/main.py:225` </location>
<code_context>
+ TLS_KEY_PATH,
+ "-out",
+ TLS_CERT_PATH,
+ "-subj",
+ shlex.quote(f"/CN={self.DEFAULT_HOSTNAME}"),
+ "-addext",
+ shlex.quote(f"subjectAltName={','.join(alt_names)}"),
</code_context>
<issue_to_address>
Potential misuse of shlex.quote in openssl argument list.
shlex.quote adds literal quotes when used in argument lists for subprocess with shell=False, which can break the command. Remove shlex.quote and pass the arguments as plain strings.
</issue_to_address>
### Comment 4
<location> `core/services/beacon/main.py:274` </location>
<code_context>
+ Moves the file at new_config_path to config_path and bounces nginx, optionally keeping a backup of config_path
+ """
+ # do both files exist
+ if not os.path.exists(config_path):
+ raise FileNotFoundError("Old config not found")
+ if not os.path.isfile(new_config_path):
+ raise FileNotFoundError("New config not found")
+
</code_context>
<issue_to_address>
Checking for config_path existence before promoting may block first-time setup.
Consider handling the case where config_path does not exist, to support first-time setup scenarios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# do both files exist
if not os.path.exists(config_path):
raise FileNotFoundError("Old config not found")
if not os.path.isfile(new_config_path):
raise FileNotFoundError("New config not found")
=======
# ensure new config exists
if not os.path.isfile(new_config_path):
raise FileNotFoundError("New config not found")
# old config may not exist (first-time setup), so do not raise if missing
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `core/services/beacon/main.py:452` </location>
<code_context>
@version(1, 0)
def set_hostname(hostname: str) -> Any:
+ # beacon.ts has a regex to validate hostname format, but we should check here too
+ hostname_regex = re.compile(r"^[a-zA-Z0-9-]+$")
+ if not hostname_regex.match(hostname):
+ raise ValueError("Invalid characters in hostname")
return beacon.set_hostname(hostname)
</code_context>
<issue_to_address>
Hostname validation regex does not prevent leading/trailing hyphens or consecutive hyphens.
The current regex permits invalid hostnames per RFC 952/1123. Please update the validation to disallow leading, trailing, or consecutive hyphens.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# beacon.ts has a regex to validate hostname format, but we should check here too
hostname_regex = re.compile(r"^[a-zA-Z0-9-]+$")
if not hostname_regex.match(hostname):
raise ValueError("Invalid characters in hostname")
return beacon.set_hostname(hostname)
=======
# beacon.ts has a regex to validate hostname format, but we should check here too
# Hostname must not start or end with a hyphen, nor contain consecutive hyphens
hostname_regex = re.compile(r"^(?!-)[A-Za-z0-9-]+(?<!-)$")
if not hostname_regex.match(hostname) or "--" in hostname:
raise ValueError("Invalid hostname: must only contain alphanumeric characters and hyphens, cannot start or end with a hyphen, and cannot contain consecutive hyphens")
return beacon.set_hostname(hostname)
>>>>>>> REPLACE
</suggested_fix>
### Comment 6
<location> `core/services/beacon/main.py:493` </location>
<code_context>
+ return beacon.get_enable_tls()
+
+
[email protected]("/use_tls", summary="Set whether TLS should be enbabled")
+@version(1, 0)
+def set_enable_tls(enable_tls: bool) -> Any:
</code_context>
<issue_to_address>
Typo in endpoint summary: 'enbabled' should be 'enabled'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
@app.post("/use_tls", summary="Set whether TLS should be enbabled")
=======
@app.post("/use_tls", summary="Set whether TLS should be enabled")
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `core/services/helper/main.py:210` </location>
<code_context>
SKIP_PORTS: Set[int] = {
22, # SSH
80, # BlueOS
+ 443, # BlueoS TLS
5201, # Iperf
6021, # Mavlink Camera Manager's WebRTC signaller
</code_context>
<issue_to_address>
Typo in comment: 'BlueoS' should be 'BlueOS'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
443, # BlueoS TLS
=======
443, # BlueOS TLS
>>>>>>> REPLACE
</suggested_fix>
## Security Issues
### Issue 1
<location> `bootstrap/bootstrap/bootstrap.py:284` </location>
<issue_to_address>
**security (opengrep-rules.python.requests.security.disabled-cert-validation):** Certificate verification has been explicitly disabled. This permits insecure connections to insecure servers. Re-enable certification validation.
```suggestion
response = requests.get(
"http://localhost/version-chooser/v1.0/version/current",
timeout=10,
verify=True,
)
```
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+144
to
+148
| if self.get_enable_tls(): | ||
| os.unlink(TLS_KEY_PATH) | ||
| os.unlink(TLS_CERT_PATH) | ||
| self.generate_cert() | ||
| self.reload_nginx_config() |
There was a problem hiding this comment.
issue (bug_risk): Potential race condition when deleting and regenerating TLS certs.
If generate_cert fails after deleting the existing certs, the system will be left without valid TLS files. To prevent this, generate new certs in a temporary location and atomically replace the originals.
Comment on lines
+225
to
+226
| "-subj", | ||
| shlex.quote(f"/CN={self.DEFAULT_HOSTNAME}"), |
There was a problem hiding this comment.
issue (bug_risk): Potential misuse of shlex.quote in openssl argument list.
shlex.quote adds literal quotes when used in argument lists for subprocess with shell=False, which can break the command. Remove shlex.quote and pass the arguments as plain strings.
| if not enable_tls and self.get_enable_tls(): | ||
| # tls is currently enabled and we need to disable | ||
| # change nginx config | ||
| self.generate_new_nginx_config(use_tls=False) |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method)
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Contributor
Author
|
Awesome. Some of the suggested Sourcery changes violate |
joaoantoniocardoso
requested review from
Williangalvani,
joaoantoniocardoso,
joaomariolago and
patrickelectric
Member
|
Nice work! Since I don't know your docker account, I forked your branch and tested it here (deployed as
Problems:
|
Contributor
Author
You have to take the bootstrap update too, or this will fail once enabled. Bootstrap will think chooser is down and then revert to factory.
I'll check on this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This replaces the old PR (2697) and is based on the latest BlueOS master. The functionality is the same.
Summary by Sourcery
Enable TLS support for the beacon component by introducing a persisted use_tls setting, self-signed certificate generation, Nginx configuration management, and corresponding API and UI controls
New Features:
Enhancements:
Chores: