Add Enterprise WPA (802.1X/EAP) support #119
Merged
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
- Add detection of enterprise networks in scan results - Implement EnterpriseAuthentication dialog with support for: - EAP methods: PEAP, TTLS, TLS, LEAP, FAST, PWD - Phase 2 authentication: MSCHAPV2, GTC, PAP, CHAP, MD5 - CA certificate selection - Client certificate/key for TLS - Anonymous identity support - Generate proper wpa_supplicant.conf blocks for EAP - Add certificate validation helpers - Fix security issue: wpa_supplicant.conf permissions (0o765 -> 0o600) - Add unit tests for enterprise WPA functions - Add FreeRADIUS test setup documentation Addresses: ghostbsd#24
FreeBSD ifconfig scan doesn't distinguish PSK from EAP explicitly. Use heuristic: RSN without WPS and without typical consumer router features (HTCAP, VHTCAP, ATH, WME) suggests enterprise AP.
The setup_wpa_supplicant function was using index 6 (short CAPS like "EPS") instead of index 7 (full caps_string like "RSN HTCAP WME..."). Since "EPS" doesn't contain 'RSN' or 'WPA', networks were incorrectly falling through to WEP configuration, causing authentication failures.
Contributor
Reviewer's GuideImplements WPA-Enterprise (802.1X/EAP) Wi‑Fi support end‑to‑end, including network capability detection, EAP wpa_supplicant configuration generation, a new GTK dialog for enterprise credentials, more secure handling of wpa_supplicant.conf, a WireGuard configs bugfix, and test/docs coverage for the new behavior. Sequence diagram for enterprise Wi‑Fi connection setupsequenceDiagram
actor User
participant TrayIcon
participant EnterpriseDialog
participant NetApi
participant WpaSupplicantConf as Wpa_supplicant_conf
User->>TrayIcon: Click enterprise_ssid in menu
TrayIcon->>TrayIcon: menu_click_enterprise(ssid_info, wificard)
TrayIcon->>WpaSupplicantConf: Check for existing ssid network block
alt Config_exists
TrayIcon->>TrayIcon: connectToSsid(ssid, wificard)
else No_config
TrayIcon->>TrayIcon: EnterpriseAuthentication(ssid_info, wificard, failed=False)
TrayIcon->>EnterpriseDialog: Show EAP credential form
User->>EnterpriseDialog: Select EAP_method, phase2, CA_cert, credentials
User->>EnterpriseDialog: Click Connect
EnterpriseDialog->>TrayIcon: add_enterprise_to_wpa_supplicant(ssid_info, card)
TrayIcon->>NetApi: write_eap_config(ssid, eap_config)
NetApi->>WpaSupplicantConf: Append EAP network block (0600 perms)
TrayIcon->>TrayIcon: try_to_connect_to_ssid(ssid, ssid_info, card)
end
TrayIcon-->>User: Updated connection status in tray
Updated class diagram for trayicon enterprise support and net_api helpersclassDiagram
class TrayIcon {
+ssid_menu_item(sn, caps, ssid, ssid_info, wificard)
+menu_click_lock(widget, ssid_info, wificard)
+menu_click_enterprise(widget, ssid_info, wificard)
+disconnect_wifi(widget, wificard)
+setup_wpa_supplicant(ssid, ssid_info, pwd, card)
+Open_Wpa_Supplicant(ssid, card)
+add_enterprise_to_wpa_supplicant(widget, ssid_info, card)
+on_eap_method_changed(combo)
+close_eap_window(widget)
+on_eap_password_check(widget)
+EnterpriseAuthentication(ssid_info, card, failed)
-eap_window
-eap_method_combo
-phase2_combo
-identity_entry
-anon_identity_entry
-eap_password
-ca_cert_chooser
-client_cert_chooser
-private_key_chooser
-private_key_passwd
-client_cert_box
-private_key_box
-private_key_passwd_box
-phase2_box
-password_box
}
class NetApi {
<<module>>
+EAP_METHODS : list
+PHASE2_METHODS : list
+DEFAULT_CA_CERT : str
+card_online(netcard)
+barpercent(sn)
+is_enterprise_network(caps_string)
+get_security_type(caps_string)
+validate_certificate(cert_path)
+get_system_ca_certificates()
+networkdictionary()
+delete_ssid_wpa_supplicant_config(ssid)
+generate_eap_config(ssid, eap_config)
+write_eap_config(ssid, eap_config)
+wait_inet(card)
}
class WpaSupplicantConf {
<<file>>
+path : "/etc/wpa_supplicant.conf"
+mode : 0600
}
TrayIcon ..> NetApi : uses
TrayIcon ..> WpaSupplicantConf : appends_network_blocks
NetApi ..> WpaSupplicantConf : writes_EAP_config
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- In trayicon.py, the new uses of EAP_METHODS, PHASE2_METHODS, get_system_ca_certificates, and write_eap_config rely on these being imported into the module; consider adding explicit imports from net_api to avoid NameError at runtime.
- menu_click_enterprise currently does a synchronous open('/etc/wpa_supplicant.conf').read() without a context manager or error handling on every click; consider guarding for missing/permission-denied files and avoiding a full blocking read (e.g., use a context manager and short-circuit search).
- generate_eap_config directly interpolates user-provided values (ssid, identity, passwords, paths) into the wpa_supplicant.conf block without escaping quotes or special characters, which can break the config or be abused; consider sanitizing or escaping these strings before writing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In trayicon.py, the new uses of EAP_METHODS, PHASE2_METHODS, get_system_ca_certificates, and write_eap_config rely on these being imported into the module; consider adding explicit imports from net_api to avoid NameError at runtime.
- menu_click_enterprise currently does a synchronous open('/etc/wpa_supplicant.conf').read() without a context manager or error handling on every click; consider guarding for missing/permission-denied files and avoiding a full blocking read (e.g., use a context manager and short-circuit search).
- generate_eap_config directly interpolates user-provided values (ssid, identity, passwords, paths) into the wpa_supplicant.conf block without escaping quotes or special characters, which can break the config or be abused; consider sanitizing or escaping these strings before writing.
## Individual Comments
### Comment 1
<location> `NetworkMgr/trayicon.py:249-250` </location>
<code_context>
self.Authentication(ssid_info, wificard, False)
self.updateinfo()
+ def menu_click_enterprise(self, widget, ssid_info, wificard):
+ if f'"{ssid_info[0]}"' in open('/etc/wpa_supplicant.conf').read():
+ connectToSsid(ssid_info[0], wificard)
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Directly opening and reading /etc/wpa_supplicant.conf without a context manager or error handling is fragile.
If `/etc/wpa_supplicant.conf` is missing or unreadable, this will raise and break the menu action, and it loads the whole file just to check membership. Please (1) use a context manager, (2) catch `FileNotFoundError`/IO errors and fail gracefully, and (3) consider reading line-by-line or a more structured lookup instead of loading the entire file on the UI thread.
</issue_to_address>
### Comment 2
<location> `NetworkMgr/net_api.py:476-485` </location>
<code_context>
+ return ws
+
+
+def write_eap_config(ssid, eap_config):
+ """
+ Write EAP configuration to wpa_supplicant.conf with secure permissions.
+ """
+ config = generate_eap_config(ssid, eap_config)
+ wpa_conf_path = '/etc/wpa_supplicant.conf'
+
+ # Write with restrictive permissions (owner read/write only)
+ old_umask = os.umask(0o077)
+ try:
+ with open(wpa_conf_path, 'a') as wsf:
+ wsf.write(config)
+ finally:
+ os.umask(old_umask)
+
+ # Ensure file permissions are correct
+ try:
+ os.chmod(wpa_conf_path, 0o600)
+ except PermissionError:
+ pass # May need root, handled by sudoers
</code_context>
<issue_to_address>
**🚨 suggestion (security):** wpa_supplicant.conf permissions are only enforced here and in delete/setup paths, not after all writes.
`umask(0o077)` secures only newly created files. If `/etc/wpa_supplicant.conf` already exists with permissive permissions, the `setup_wpa_supplicant()` and `Open_Wpa_Supplicant()` paths won’t fix that, since they don’t call `chmod` like this function and `delete_ssid_wpa_supplicant_config()` do. Please also apply `os.chmod(wpa_conf_path, 0o600)` in those paths after writing to ensure existing configs are consistently locked down.
Suggested implementation:
```python
ws += '\n}\n'
return ws
def _ensure_wpa_conf_permissions(wpa_conf_path: str = '/etc/wpa_supplicant.conf') -> None:
"""
Ensure wpa_supplicant.conf has restrictive permissions (owner read/write only).
This is safe to call after any write to the file.
"""
try:
os.chmod(wpa_conf_path, 0o600)
except PermissionError:
# May need root; permission handling should be done by caller/sudoers
pass
def write_eap_config(ssid, eap_config):
```
```python
# Ensure file permissions are correct (even if file already existed)
_ensure_wpa_conf_permissions(wpa_conf_path)
```
To fully implement your suggestion, you should also:
1. Locate the `setup_wpa_supplicant()` implementation (or similarly named function that initializes `/etc/wpa_supplicant.conf`) and, immediately after any write that touches `/etc/wpa_supplicant.conf`, call:
```python
_ensure_wpa_conf_permissions('/etc/wpa_supplicant.conf')
```
2. Locate the `Open_Wpa_Supplicant()` path (or any function that updates `/etc/wpa_supplicant.conf` outside of `write_eap_config` and `delete_ssid_wpa_supplicant_config`) and likewise call:
```python
_ensure_wpa_conf_permissions('/etc/wpa_supplicant.conf')
```
right after the write completes.
3. If those functions write to `wpa_supplicant.conf` via a variable path (e.g., `wpa_conf_path`), pass that variable to `_ensure_wpa_conf_permissions` instead of the hard-coded string, to keep the behavior consistent with any future path changes.
</issue_to_address>
### Comment 3
<location> `NetworkMgr/trayicon.py:581-593` </location>
<code_context>
+ self.eap_window.add(main_box)
+
+ # Title
+ if failed:
+ title_text = _(f"{ssid_info[0]} Enterprise Authentication Failed")
+ else:
+ title_text = _(f"Enterprise Authentication for {ssid_info[0]}")
+ title_label = Gtk.Label()
+ title_label.set_markup(f"<b><span size='large'>{title_text}</span></b>")
</code_context>
<issue_to_address>
**suggestion:** Using f-strings inside _() breaks translation of these messages.
Wrapping an f-string in `_()` makes the entire interpolated text (including the SSID) the msgid, which translation tools can’t reuse. For strings with variables, keep a static msgid and interpolate afterward, e.g. `title_text = _('Enterprise Authentication for %s') % ssid_info[0]` (and similarly for the failure case), so the catalog entries stay stable and reusable.
```suggestion
# Title
if failed:
title_text = _("%s Enterprise Authentication Failed") % ssid_info[0]
else:
title_text = _("Enterprise Authentication for %s") % ssid_info[0]
title_label = Gtk.Label()
title_label.set_markup(f"<b><span size='large'>{title_text}</span></b>")
main_box.pack_start(title_label, False, False, 5)
# Security info
security_type = ssid_info[7] if len(ssid_info) > 7 else "WPA2-EAP"
security_label = Gtk.Label(_("Security: %s") % security_type)
main_box.pack_start(security_label, False, False, 0)
```
</issue_to_address>
### Comment 4
<location> `tests/unit/test_enterprise_wpa.py:70-73` </location>
<code_context>
+ """Test WPA-EAP detection."""
+ assert get_security_type("WPA EAP") == "WPA-EAP"
+
+ def test_get_security_type_wpa2_psk(self):
+ """Test WPA2-PSK detection."""
+ assert get_security_type("RSN HTCAP WME") == "WPA2-PSK"
+ assert get_security_type("RSN") == "WPA2-PSK"
+
+ def test_get_security_type_wpa_psk(self):
</code_context>
<issue_to_address>
**issue (testing):** The `"RSN"` case in `test_get_security_type_wpa2_psk` contradicts the documented enterprise detection heuristic and is likely an incorrect expectation.
Per the `is_enterprise_network` docstring, `"RSN"` without WPS/HTCAP/VHTCAP/ATH is classified as enterprise, so `get_security_type("RSN")` returns `"WPA2-EAP"`, not `"WPA2-PSK"`. This makes the test expectation inconsistent with the current heuristic and will fail.
Please either:
- Change the expectation to `"WPA2-EAP"` and add a brief comment noting that bare `RSN` is treated as enterprise, or
- Update the heuristic in `is_enterprise_network` if the intended behavior is different, and then align the tests accordingly.
The tests should clearly encode the intended classification of a minimal `"RSN"` beacon.
</issue_to_address>
### Comment 5
<location> `tests/unit/test_enterprise_wpa.py:15-23` </location>
<code_context>
+top_dir = str(Path(__file__).absolute().parent.parent.parent)
+sys.path.insert(0, top_dir)
+
+from NetworkMgr.net_api import (
+ EAP_METHODS,
+ PHASE2_METHODS,
+ DEFAULT_CA_CERT,
+ is_enterprise_network,
+ get_security_type,
+ validate_certificate,
+ get_system_ca_certificates,
+ generate_eap_config,
+)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `get_system_ca_certificates` and `write_eap_config`, especially around filesystem interactions and permissions.
The enterprise WPA logic depends on correct filesystem behavior and secure handling of `/etc/wpa_supplicant.conf` and system CA bundles, but there are no tests covering:
- `get_system_ca_certificates()`: returning only existing paths across the supported locations.
- `write_eap_config(...)`: appending the EAP config, using a restrictive umask, and setting `0o600` on the target file.
Please add tests that, for example:
- Monkeypatch `os.path.exists` (and optionally `os.path.isfile`) to simulate different CA bundle layouts and assert the returned paths.
- Monkeypatch `open`, `os.umask`, and `os.chmod` in `NetworkMgr.net_api` to write to a temp file and assert that:
- The appended EAP block matches `generate_eap_config`.
- `os.umask` is temporarily set to `0o077`.
- `os.chmod` is called with `0o600` on the file.
This will verify the secure-storage behavior and help prevent regressions.
Suggested implementation:
```python
import os
import sys
import io
import tempfile
from pathlib import Path
import pytest
# Add project root to path
top_dir = str(Path(__file__).absolute().parent.parent.parent)
sys.path.insert(0, top_dir)
from NetworkMgr import net_api as net_api_module
from NetworkMgr.net_api import (
EAP_METHODS,
PHASE2_METHODS,
DEFAULT_CA_CERT,
get_system_ca_certificates,
generate_eap_config,
write_eap_config,
)
def test_get_system_ca_certificates_returns_empty_when_no_paths_exist(monkeypatch):
"""get_system_ca_certificates should return an empty list when no CA bundle paths exist."""
def fake_exists(path):
return False
monkeypatch.setattr(net_api_module.os, "path", net_api_module.os.path)
monkeypatch.setattr(net_api_module.os.path, "exists", fake_exists)
result = get_system_ca_certificates()
assert isinstance(result, list)
assert result == []
def test_get_system_ca_certificates_returns_list_when_paths_exist(monkeypatch):
"""
get_system_ca_certificates should return a list of CA bundle paths when they exist.
We don't depend on specific paths, only that:
- it returns a non-empty list when all probed paths "exist"
- all returned entries are absolute paths.
"""
def fake_exists(path):
return True
monkeypatch.setattr(net_api_module.os, "path", net_api_module.os.path)
monkeypatch.setattr(net_api_module.os.path, "exists", fake_exists)
result = get_system_ca_certificates()
assert isinstance(result, list)
# If the implementation ever has no configured paths this will fail,
# which is fine because the logic would then be pointless.
assert len(result) > 0
assert all(os.path.isabs(p) for p in result)
def test_write_eap_config_secure_file_handling(monkeypatch):
"""
write_eap_config should:
- use generate_eap_config to build the EAP block
- temporarily set umask to 0o077
- call chmod with 0o600 on the target file
- append the generated EAP config to the file.
"""
# Capture the EAP config that write_eap_config attempts to write.
generated_block = "network={\n ssid=\"test-ssid\"\n}\n"
captured_generate_args = {}
def fake_generate_eap_config(*args, **kwargs):
captured_generate_args["args"] = args
captured_generate_args["kwargs"] = kwargs
return generated_block
monkeypatch.setattr(net_api_module, "generate_eap_config", fake_generate_eap_config)
# Capture umask changes.
umask_calls = []
def fake_umask(mode):
umask_calls.append(mode)
# Simulate a previous umask value that will be restored.
return 0o022
monkeypatch.setattr(net_api_module.os, "umask", fake_umask)
# Capture chmod calls.
chmod_calls = []
def fake_chmod(path, mode):
chmod_calls.append((path, mode))
monkeypatch.setattr(net_api_module.os, "chmod", fake_chmod)
# Capture file writes without touching the real filesystem.
written_content = {"data": ""}
class DummyFile(io.StringIO):
def __enter__(self):
return self
def __exit__(self, exc_type, exc, tb):
# Do not suppress exceptions
return False
def write(self, s):
written_content["data"] += s
return len(s)
def fake_open(*args, **kwargs):
# We don't care about the filename or mode here; we just capture the content.
return DummyFile()
monkeypatch.setattr(net_api_module, "open", fake_open)
# Call write_eap_config with dummy parameters; the exact signature is delegated
# to the implementation, but we assume a typical EAP config signature.
# Adjust arguments here if the real function signature differs.
write_eap_config(
ssid="test-ssid",
identity="user@example.com",
password="secret-password",
ca_cert=DEFAULT_CA_CERT,
eap_method=EAP_METHODS[0],
phase2_method=PHASE2_METHODS[0],
)
# Assert umask was set to 0o077 at some point.
assert 0o077 in umask_calls
# Assert chmod was called with 0o600 on the WPA supplicant file.
assert any(mode == 0o600 for _, mode in chmod_calls)
# Assert that the generated EAP block was appended to the file.
assert generated_block in written_content["data"]
```
These tests assume the following about `NetworkMgr.net_api`:
1. `get_system_ca_certificates()` uses `os.path.exists` on one or more configured CA bundle paths.
2. `write_eap_config(...)`:
- Calls `generate_eap_config(...)` (imported in the same module) to build the EAP block.
- Uses `os.umask` to set a restrictive `0o077` umask and restores the original value afterward.
- Uses `os.chmod(path, 0o600)` on the WPA supplicant configuration file.
- Opens the WPA supplicant configuration file via the module-level `open` (e.g. in append mode).
If the actual `write_eap_config` signature differs from the call in the test (e.g. different parameter names or order, or it accepts a configuration object instead of individual parameters), adjust the arguments in `test_write_eap_config_secure_file_handling` accordingly so they match the real function.
If `get_system_ca_certificates` or `write_eap_config` are implemented in a different module or under different names, update the imports and `monkeypatch.setattr` targets to match the real locations.
</issue_to_address>
### Comment 6
<location> `tests/unit/test_enterprise_wpa.py:110-114` </location>
<code_context>
+ assert is_valid is False
+ assert "not found" in error
+
+ def test_validate_certificate_directory(self):
+ """Test validation with directory instead of file."""
+ is_valid, error = validate_certificate("/tmp")
+ assert is_valid is False
+ assert "Not a file" in error
+
+ def test_validate_certificate_valid_file(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for `validate_certificate` where the file exists but is not readable, to cover the permission-error path.
Current tests cover empty/None inputs, non-existent paths, directories, and readable files. One implemented but untested path is when the file exists and is a regular file, but is not readable (`os.access(path, os.R_OK)` is `False`).
Please add a test that creates a temp file, revokes read permissions (or monkeypatches `os.access` to return `False` for that path), and asserts `validate_certificate(path)` returns `False` with an error containing "not readable". This will exercise the permission-check branch and verify the error message.
</issue_to_address>
### Comment 7
<location> `docs/TESTING_ENTERPRISE_WPA.md:9` </location>
<code_context>
+
+- FreeBSD/GhostBSD system
+- A wireless card that supports AP mode (for hostapd)
+- Another wireless card (or separate machine) for testing as client
+
+## Installation
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding an article: "for testing as a client".
You could revise this to: "Another wireless card (or separate machine) for testing as a client."
```suggestion
- Another wireless card (or separate machine) for testing as a client
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Add input escaping for wpa_supplicant.conf values (security) - Use context manager and error handling for config file reads - Fix i18n strings to use %s interpolation instead of f-strings - Fix test expectation for bare RSN classification
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
ericbsd
approved these changes
Jan 14, 2026
Member
|
Thanks for the contribution! |
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.
Closes #24
Instead of just providing
ifconfig wlan0 scanoutput, here's a complete implementation.What this PR adds
Enterprise WPA2 (802.1X/EAP) authentication support:
Changes
NetworkMgr/net_api.py: Addedis_enterprise_network(),get_security_type(),generate_eap_config(),write_eap_config()NetworkMgr/trayicon.py: AddedEnterpriseAuthenticationdialog classNetworkMgr/setup-nic.py: Fixed wpa_supplicant.conf permissions (0o600)NetworkMgr/wg_api.py: Fixed KeyError when WireGuard directory doesn't existBug fixes included
Testing
Summary by Sourcery
Add WPA-Enterprise (802.1X/EAP) Wi‑Fi support, improve wireless security configuration handling, and fix related permission and WireGuard configuration issues.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: