Add IPv6 configuration support #121
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
Implement IPv6 support in the Network Configuration dialog: - Add SLAAC (Stateless Address Autoconfiguration) support - Add static IPv6 address configuration - Add IPv6 gateway configuration (with link-local address support) - Add IPv6 DNS server configuration - Fix existing GUI bugs (labels, connect handlers) New functions in net_api.py: - start_static_ipv6_network() - enable_slaac() / disable_slaac() - get_ipv6_addresses() - has_slaac_enabled() - get_ipv6_gateway() New function in query.py: - get_interface_settings_ipv6() Tested on GhostBSD 14.3-RELEASE with FritzBox router. Closes ghostbsd#120
Contributor
Reviewer's GuideImplements full IPv6 configuration support in the Network Configuration dialog, including SLAAC and static IPv6 workflows, wiring the GUI to new query and net API helpers, and hardening rc.conf/resolv.conf handling and labels for IPv6-specific semantics. Sequence diagram for applying IPv6 settings from the GUIsequenceDiagram
actor User
participant ConfigWindow as NetworkConfigurationWindow
participant Query as query
participant NetAPI as net_api
participant RcConf as rc.conf
participant Resolv as resolv.conf
participant Ifconfig as ifconfig_route_netstat
User->>ConfigWindow: Launch NetworkMgr
ConfigWindow->>Query: get_interface_settings(active_nic)
ConfigWindow->>Query: get_interface_settings_ipv6(active_nic)
Query-->>ConfigWindow: IPv4 and IPv6 settings
ConfigWindow->>ConfigWindow: update_interface_settings_ipv6
ConfigWindow->>ConfigWindow: set rb_slaac6 or rb_manual6
User->>ConfigWindow: Change IPv6 Method radio (SLAAC/Manual)
ConfigWindow->>ConfigWindow: edit_ipv6_setting
ConfigWindow->>ConfigWindow: Toggle sensitivity of IPv6 inputs
User->>ConfigWindow: Edit IPv6 address, prefix, gateway, DNS
User->>ConfigWindow: Click Save
ConfigWindow->>ConfigWindow: commit_pending_changes
ConfigWindow->>ConfigWindow: update_system
ConfigWindow->>ConfigWindow: update_system_ipv6(nic)
alt Method Manual
ConfigWindow->>RcConf: update_rc_conf(ifconfig_nic_ipv6="inet6 inet6 prefixlen prefixlen")
ConfigWindow->>RcConf: update_rc_conf(rtsold_enable="NO")
opt IPv6 address provided
ConfigWindow->>NetAPI: disable_slaac(nic)
NetAPI->>Ifconfig: ifconfig nic inet6 -accept_rtadv
ConfigWindow->>NetAPI: start_static_ipv6_network(nic, inet6, prefixlen)
NetAPI->>Ifconfig: ifconfig nic inet6 inet6 prefixlen prefixlen
NetAPI->>Ifconfig: service routing restart
end
opt Gateway provided
ConfigWindow->>ConfigWindow: Add %nic suffix for fe80:: gateway
ConfigWindow->>RcConf: update_rc_conf(ipv6_defaultrouter="gateway6_full")
ConfigWindow->>Ifconfig: route delete -inet6 default
ConfigWindow->>Ifconfig: route add -inet6 default gateway6_full
end
opt DNS provided
ConfigWindow->>Resolv: add_ipv6_dns(dns6)
Resolv->>Resolv: Append nameserver dns6
end
else Method SLAAC
ConfigWindow->>RcConf: update_rc_conf(ifconfig_nic_ipv6="inet6 accept_rtadv")
ConfigWindow->>RcConf: update_rc_conf(rtsold_enable="YES")
ConfigWindow->>ConfigWindow: remove_rc_conf_var(ipv6_defaultrouter)
ConfigWindow->>Ifconfig: sysrc -x ipv6_defaultrouter
ConfigWindow->>NetAPI: enable_slaac(nic)
NetAPI->>Ifconfig: ifconfig nic inet6 -accept_rtadv
NetAPI->>Ifconfig: ifconfig nic inet6 accept_rtadv
NetAPI->>Ifconfig: rtsol nic
end
ConfigWindow-->>User: Window closes, IPv6 config applied
Flow diagram for IPv6 method handling and gateway logicflowchart TD
Start[Start IPv6 update_system_ipv6] --> ReadInputs[Read GUI values inet6, prefixlen, gateway6, dns6, method6]
ReadInputs --> CheckMethod{Assignment method Manual?}
CheckMethod -- Yes --> ManualCfg[Prepare static IPv6 config ifconfig_nic_ipv6]
ManualCfg --> RcConfStatic[update_rc_conf ifconfig_nic_ipv6]
RcConfStatic --> DisableRtsold[update_rc_conf rtsold_enable=NO]
DisableRtsold --> HasInet6{inet6 non empty?}
HasInet6 -- Yes --> DisableSlaac[disable_slaac on interface]
DisableSlaac --> ApplyStatic[call start_static_ipv6_network inet6 prefixlen]
HasInet6 -- No --> CheckGateway
ApplyStatic --> CheckGateway{Gateway provided?}
CheckGateway -- Yes --> GatewayType{Gateway starts with fe80:: and no %?}
GatewayType -- Yes --> AddSuffix[Append %nic to gateway6]
GatewayType -- No --> UseGateway[Use gateway6 as is]
AddSuffix --> SaveGwRcConf[update_rc_conf ipv6_defaultrouter=gateway6_full]
UseGateway --> SaveGwRcConf
SaveGwRcConf --> RouteDelete[route delete -inet6 default]
RouteDelete --> RouteAdd[route add -inet6 default gateway6_full]
CheckGateway -- No --> CheckDNS{DNS provided?}
RouteAdd --> CheckDNS
CheckDNS -- Yes --> AddDns[add_ipv6_dns dns6 to resolv.conf]
CheckDNS -- No --> EndManual[End Manual path]
AddDns --> EndManual
CheckMethod -- No --> SlaacCfg[Prepare SLAAC config ifconfig_nic_ipv6=accept_rtadv]
SlaacCfg --> RcConfSlaac[update_rc_conf ifconfig_nic_ipv6]
RcConfSlaac --> EnableRtsold[update_rc_conf rtsold_enable=YES]
EnableRtsold --> RemoveStaticGw[remove_rc_conf_var ipv6_defaultrouter]
RemoveStaticGw --> EnableSlaac[enable_slaac on interface]
EnableSlaac --> EndSlaac[End SLAAC path]
EndManual --> End[Done]
EndSlaac --> End
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 19 security issues, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'check_output' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'check_output' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
- Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
enable_slaac, the comment says you remove existing IPv6 addresses but the code only togglesaccept_rtadv; if the intent is truly to clear old IPv6 addresses you may want to explicitly flush existing inet6 addresses or adjust the comment to reflect the actual behavior. - The implementation of
get_ipv6_addressesconflicts with its docstring: it claims to exclude link-local and localhost addresses but currently returns all inet6 addresses that match the regex; either filter outfe80::/::1as described or update the docstring to match the current behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `enable_slaac`, the comment says you remove existing IPv6 addresses but the code only toggles `accept_rtadv`; if the intent is truly to clear old IPv6 addresses you may want to explicitly flush existing inet6 addresses or adjust the comment to reflect the actual behavior.
- The implementation of `get_ipv6_addresses` conflicts with its docstring: it claims to exclude link-local and localhost addresses but currently returns all inet6 addresses that match the regex; either filter out `fe80::`/`::1` as described or update the docstring to match the current behavior.
## Individual Comments
### Comment 1
<location> `NetworkMgr/query.py:58-59` </location>
<code_context>
+ ipv6_settings["Prefix Length"] = "64"
+
+ # Get IPv6 default gateway from rc.conf or routing table
+ gateway_search = re.search(
+ r'^ipv6_defaultrouter="([0-9a-fA-F:]+)"',
+ rc_conf,
+ re.MULTILINE
</code_context>
<issue_to_address>
**issue (bug_risk):** ipv6_defaultrouter regex will miss link-local gateways stored with an interface suffix (e.g., fe80::1%em0).
In `update_system_ipv6` link-local gateways are stored with an interface suffix (e.g., `ipv6_defaultrouter="fe80::1%em0"`), but this regex only permits `[0-9a-fA-F:]`, so values with `%<iface>` won’t match and existing gateways won’t appear in the GUI. Please extend the pattern to allow an optional `%iface` suffix and handle it consistently with how the value is written.
</issue_to_address>
### Comment 2
<location> `NetworkMgr/net_api.py:329-337` </location>
<code_context>
+ run('service routing restart', shell=True)
+
+
+def enable_slaac(netcard):
+ """Enable SLAAC (Stateless Address Autoconfiguration) on the interface."""
+ # Remove any existing IPv6 addresses first
+ run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
+ sleep(0.5)
</code_context>
<issue_to_address>
**suggestion:** enable_slaac comment and implementation diverge regarding removal of existing IPv6 addresses.
The code only toggles the `accept_rtadv` flag (`inet6 -accept_rtadv`) and does not actually remove any existing IPv6 addresses as the comment suggests. Please either adjust the comment to match the behavior, or, if you do need to clear stale/static IPv6 addresses, add the explicit address flush logic (and/or document that behavior in an appropriate place).
```suggestion
def enable_slaac(netcard):
"""Enable SLAAC (Stateless Address Autoconfiguration) on the interface by
toggling accept_rtadv and soliciting router advertisements."""
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
sleep(0.5)
# Enable accept_rtadv for SLAAC
run(f'ifconfig {netcard} inet6 accept_rtadv', shell=True)
# Start rtsold to solicit router advertisements
run(f'rtsol {netcard}', shell=True)
```
</issue_to_address>
### Comment 3
<location> `NetworkMgr/net_api.py:345-346` </location>
<code_context>
+ run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
+
+
+def get_ipv6_addresses(netcard):
+ """Get all IPv6 addresses configured on the interface."""
+ try:
+ output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** get_ipv6_addresses returns all IPv6 addresses but the comment suggests excluding link-local/localhost.
The docs/comments state this should exclude link-local (fe80::/10) and ::1, but the implementation currently returns all `inet6` entries. Please either filter the results to match the documented semantics or update the docs/comments to reflect the actual behavior so callers aren’t misled.
</issue_to_address>
### Comment 4
<location> `NetworkMgr/configuration.py:586-595` </location>
<code_context>
+ # Enable SLAAC
+ enable_slaac(nic)
+
+ def add_ipv6_dns(self, dns6):
+ """Add IPv6 DNS server to resolv.conf without removing existing entries."""
+ resolv_path = '/etc/resolv.conf'
+ try:
+ with open(resolv_path, 'r') as f:
+ content = f.read()
+ # Check if this IPv6 DNS is already present
+ if f'nameserver {dns6}' not in content:
+ with open(resolv_path, 'a') as f:
+ f.write(f'nameserver {dns6}\n')
+ except Exception:
+ pass # resolv.conf may be managed by dhclient
+
+ def remove_rc_conf_var(self, varname):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** add_ipv6_dns blindly appends `nameserver` lines without validating IPv6 format or deduplicating by address family.
Because this only checks for exact text matches, changing the IPv6 DNS leaves the old entry in place and appends a new one. There’s also no validation of `dns6`, so invalid IPv6 strings could end up in resolv.conf. Consider validating that `dns6` is a well-formed IPv6 address and either replacing/pruning existing IPv6 `nameserver` entries or updating the first one instead of always appending.
Suggested implementation:
```python
def add_ipv6_dns(self, dns6):
"""
Ensure resolv.conf has a single, valid IPv6 DNS nameserver entry.
- Validate dns6 as an IPv6 address.
- Replace the first existing IPv6 nameserver with dns6 and remove any
additional IPv6 nameserver entries.
- If no IPv6 nameserver exists, append one.
"""
resolv_path = '/etc/resolv.conf'
try:
# Validate that dns6 is a well-formed IPv6 address
try:
import ipaddress
ip = ipaddress.ip_address(dns6)
if ip.version != 6:
return # Not IPv6, do nothing
except Exception:
# Invalid IP or ipaddress not available; avoid writing bad entries
return
try:
with open(resolv_path, 'r') as f:
lines = f.readlines()
except FileNotFoundError:
lines = []
new_lines = []
ipv6_replaced = False
for line in lines:
stripped = line.strip()
if not stripped.startswith('nameserver'):
new_lines.append(line)
continue
parts = stripped.split()
if len(parts) != 2:
# Keep malformed or unexpected lines as-is
new_lines.append(line)
continue
_, addr = parts
# Try to classify existing nameserver IP (IPv4 vs IPv6)
try:
existing_ip = ipaddress.ip_address(addr)
except ValueError:
# If it doesn't parse, preserve it
new_lines.append(line)
continue
if existing_ip.version == 4:
# Always keep IPv4 nameservers untouched
new_lines.append(line)
else:
# IPv6 nameserver: replace the first, drop the rest
if not ipv6_replaced:
new_lines.append(f'nameserver {dns6}\n')
ipv6_replaced = True
# Subsequent IPv6 nameservers are skipped (pruned)
if not ipv6_replaced:
# No existing IPv6 nameserver: append a new one
new_lines.append(f'nameserver {dns6}\n')
with open(resolv_path, 'w') as f:
f.writelines(new_lines)
except Exception:
# resolv.conf may be managed by dhclient or be otherwise immutable
pass
```
To keep imports clean and avoid importing inside the function, consider adding at the top of `NetworkMgr/configuration.py`:
<<<<<<< SEARCH
# existing imports...
=======
# existing imports...
import ipaddress
>>>>>>> REPLACE
If you make this top-level import, remove the inner `import ipaddress` and use the already-imported module inside `add_ipv6_dns`.
</issue_to_address>
### Comment 5
<location> `NetworkMgr/configuration.py:564` </location>
<code_context>
run(f'route add -inet6 default {gateway6_full}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `NetworkMgr/configuration.py:564` </location>
<code_context>
run(f'route add -inet6 default {gateway6_full}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'route add -inet6 default {gateway6_full}', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `NetworkMgr/configuration.py:601` </location>
<code_context>
run(f'sysrc -x {varname}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `NetworkMgr/configuration.py:601` </location>
<code_context>
run(f'sysrc -x {varname}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'sysrc -x {varname}', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 9
<location> `NetworkMgr/net_api.py:324` </location>
<code_context>
run(f'ifconfig {netcard} inet6 {inet6} prefixlen {prefixlen}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 10
<location> `NetworkMgr/net_api.py:324` </location>
<code_context>
run(f'ifconfig {netcard} inet6 {inet6} prefixlen {prefixlen}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'ifconfig {netcard} inet6 {inet6} prefixlen {prefixlen}', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 11
<location> `NetworkMgr/net_api.py:332` </location>
<code_context>
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 12
<location> `NetworkMgr/net_api.py:332` </location>
<code_context>
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 13
<location> `NetworkMgr/net_api.py:335` </location>
<code_context>
run(f'ifconfig {netcard} inet6 accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 14
<location> `NetworkMgr/net_api.py:335` </location>
<code_context>
run(f'ifconfig {netcard} inet6 accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'ifconfig {netcard} inet6 accept_rtadv', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 15
<location> `NetworkMgr/net_api.py:337` </location>
<code_context>
run(f'rtsol {netcard}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 16
<location> `NetworkMgr/net_api.py:337` </location>
<code_context>
run(f'rtsol {netcard}', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'rtsol {netcard}', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 17
<location> `NetworkMgr/net_api.py:342` </location>
<code_context>
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 18
<location> `NetworkMgr/net_api.py:342` </location>
<code_context>
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
run(f'ifconfig {netcard} inet6 -accept_rtadv', shell=False)
```
*Source: opengrep*
</issue_to_address>
### Comment 19
<location> `NetworkMgr/net_api.py:348` </location>
<code_context>
output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 20
<location> `NetworkMgr/net_api.py:348` </location>
<code_context>
output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'check_output' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
output = check_output(f'ifconfig {netcard}', shell=False, universal_newlines=True)
```
*Source: opengrep*
</issue_to_address>
### Comment 21
<location> `NetworkMgr/net_api.py:359` </location>
<code_context>
output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 22
<location> `NetworkMgr/net_api.py:359` </location>
<code_context>
output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'check_output' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
output = check_output(f'ifconfig {netcard}', shell=False, universal_newlines=True)
```
*Source: opengrep*
</issue_to_address>
### Comment 23
<location> `NetworkMgr/query.py:68-71` </location>
<code_context>
netstat_output = check_output(
'netstat -rn -f inet6'.split(),
universal_newlines=True
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*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
+586
to
+595
| def add_ipv6_dns(self, dns6): | ||
| """Add IPv6 DNS server to resolv.conf without removing existing entries.""" | ||
| resolv_path = '/etc/resolv.conf' | ||
| try: | ||
| with open(resolv_path, 'r') as f: | ||
| content = f.read() | ||
| # Check if this IPv6 DNS is already present | ||
| if f'nameserver {dns6}' not in content: | ||
| with open(resolv_path, 'a') as f: | ||
| f.write(f'nameserver {dns6}\n') |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): add_ipv6_dns blindly appends nameserver lines without validating IPv6 format or deduplicating by address family.
Because this only checks for exact text matches, changing the IPv6 DNS leaves the old entry in place and appends a new one. There’s also no validation of dns6, so invalid IPv6 strings could end up in resolv.conf. Consider validating that dns6 is a well-formed IPv6 address and either replacing/pruning existing IPv6 nameserver entries or updating the first one instead of always appending.
Suggested implementation:
def add_ipv6_dns(self, dns6):
"""
Ensure resolv.conf has a single, valid IPv6 DNS nameserver entry.
- Validate dns6 as an IPv6 address.
- Replace the first existing IPv6 nameserver with dns6 and remove any
additional IPv6 nameserver entries.
- If no IPv6 nameserver exists, append one.
"""
resolv_path = '/etc/resolv.conf'
try:
# Validate that dns6 is a well-formed IPv6 address
try:
import ipaddress
ip = ipaddress.ip_address(dns6)
if ip.version != 6:
return # Not IPv6, do nothing
except Exception:
# Invalid IP or ipaddress not available; avoid writing bad entries
return
try:
with open(resolv_path, 'r') as f:
lines = f.readlines()
except FileNotFoundError:
lines = []
new_lines = []
ipv6_replaced = False
for line in lines:
stripped = line.strip()
if not stripped.startswith('nameserver'):
new_lines.append(line)
continue
parts = stripped.split()
if len(parts) != 2:
# Keep malformed or unexpected lines as-is
new_lines.append(line)
continue
_, addr = parts
# Try to classify existing nameserver IP (IPv4 vs IPv6)
try:
existing_ip = ipaddress.ip_address(addr)
except ValueError:
# If it doesn't parse, preserve it
new_lines.append(line)
continue
if existing_ip.version == 4:
# Always keep IPv4 nameservers untouched
new_lines.append(line)
else:
# IPv6 nameserver: replace the first, drop the rest
if not ipv6_replaced:
new_lines.append(f'nameserver {dns6}\n')
ipv6_replaced = True
# Subsequent IPv6 nameservers are skipped (pruned)
if not ipv6_replaced:
# No existing IPv6 nameserver: append a new one
new_lines.append(f'nameserver {dns6}\n')
with open(resolv_path, 'w') as f:
f.writelines(new_lines)
except Exception:
# resolv.conf may be managed by dhclient or be otherwise immutable
passTo keep imports clean and avoid importing inside the function, consider adding at the top of NetworkMgr/configuration.py:
<<<<<<< SEARCH
existing imports...
=======
existing imports...
import ipaddress
REPLACE
If you make this top-level import, remove the inner import ipaddress and use the already-imported module inside add_ipv6_dns.
- Fix ipv6_defaultrouter regex to match link-local with interface suffix - Fix enable_slaac docstring to match actual behavior - Fix get_ipv6_addresses docstring (returns all addresses, not filtered)
|
|
||
| def remove_rc_conf_var(self, varname): | ||
| """Remove a variable from rc.conf using sysrc.""" | ||
| run(f'sysrc -x {varname}', shell=True) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| """Get all IPv6 addresses configured on the interface. | ||
| Returns list of tuples: (address, prefixlen).""" | ||
| try: | ||
| output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| def has_slaac_enabled(netcard): | ||
| """Check if SLAAC (accept_rtadv) is enabled on the interface.""" | ||
| try: | ||
| output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.subprocess-shell-true): Found 'subprocess' function 'check_output' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
Suggested change
| output = check_output(f'ifconfig {netcard}', shell=True, universal_newlines=True) | |
| output = check_output(f'ifconfig {netcard}', shell=False, universal_newlines=True) |
Source: opengrep
ericbsd
approved these changes
Jan 14, 2026
Member
ericbsd
left a comment
ericbsd
left a comment
There was a problem hiding this comment.
All those Sourcery comments did make it harder to review.
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.
Summary
Changes
New Features
Bug Fixes
connect()handler on wrong widgetremove_rc_conf_line()ValueError when line doesn't existedit_ipv6_setting()AttributeError during initializationNew Functions
net_api.py:
start_static_ipv6_network(netcard, inet6, prefixlen)enable_slaac(netcard)/disable_slaac(netcard)get_ipv6_addresses(netcard)has_slaac_enabled(netcard)get_ipv6_gateway()query.py:
get_interface_settings_ipv6(active_nic)Test Plan
Tested on GhostBSD 14.3-RELEASE with FritzBox router:
Screenshots
The IPv6 Settings tab is now functional with SLAAC/Manual radio buttons and input fields for Address, Prefix Length, Gateway, DNS, and Search Domain.
Closes #120
Summary by Sourcery
Add functional IPv6 configuration support to the network manager, including SLAAC and static setup in the GUI and backend helpers.
New Features:
Bug Fixes:
Enhancements: