Conversation
- Show IPv4 and IPv6 addresses in IP Address column - Aggregate traffic statistics across all IPs for a session - Add tooltip to display full IP addresses when truncated - Update accounting to include traffic from all associated IPs
…tack support - Resolved conflicts in captiveportal.inc: Use upstream's getValues() method while keeping IPv6 rules - Resolved conflicts in AccessController.php: Merged upstream's hostwatch dump with our IPv6 NDP fallback - Resolved conflicts in clients.volt: Use upstream's zone selection placement while keeping tooltip initialization - Resolved conflicts in pf.py: Preserved IPv6 protocol handling (0x86dd) and accounting methods - Resolved conflicts in db.py: Merged our aggregation logic with upstream's prev_* fields for counter reset detection - Resolved conflicts in cp-background-process.py: Adapted dual-stack MAC handling to use upstream's helper methods All IPv6 dual-stack functionality is preserved while incorporating upstream improvements.
| $backend = new Backend(); | ||
| $backend->configdRun('template reload OPNsense/IPFW'); | ||
| $backend->configdRun("ipfw reload"); |
There was a problem hiding this comment.
is this side effect a good idea? feels a bit too clever :)
There was a problem hiding this comment.
I expected comments here ;)
To be honest this was the quickest win here, the IPFW reload is a requirement though, and missing since the re-introduction of it and I didn't spot any other obvious hooks in the right place.
There was a problem hiding this comment.
for context: the spot where this code resided previously was cleaned up in d8519a0
There was a problem hiding this comment.
Looks duplicate now, as in the following line both dnctl and ipfw are triggered when no argument is specified:
Line 179 in 36b17ad
src/opnsense/mvc/app/models/OPNsense/CaptivePortal/CaptivePortal.xml
Outdated
Show resolved
Hide resolved
| <label>Allow multiple client IPs</label> | ||
| <help>Allow a connecting client to use multiple IPs (bound to the same MAC). For IPv4, these can be virtual IPs on the client. For IPv6, this option is needed for maximum compatibility because a client may actively use multiple IPv6 addresses.</help> | ||
| <label>Client roaming</label> | ||
| <help>Allow a connecting client to use multiple IPs (bound to the same MAC) over the course of its session. For IPv4, these can be virtual IPs on the client. For IPv6, this option is needed for maximum compatibility because a client may actively use multiple IPv6 addresses.</help> |
There was a problem hiding this comment.
strictly speaking the more common term is "alias" for an extra address (as per ifconfig). Both could still be confusing. Perhaps we can remove the IPv4 explanation or change it to a minor note "This also affects IPv4."
Wouldn't that also allow login from multiple IPs? Didn't we have a setting for that? Concurrent something?
There was a problem hiding this comment.
(if it's strictly tied to the MAC/DUID that's something else and please ignore me)
There was a problem hiding this comment.
"roaming" keys clients (with IP aliases) to a MAC address, if they connect from a different device the "concurrent user logins" setting still applies, different MAC.
| 'to' => "<__captiveportal_zone_{$zoneid}>", | ||
| 'to_not' => true, | ||
| 'to_port' => $to_port, | ||
| 'target' => $intf . 'ip', |
There was a problem hiding this comment.
Is the rdr working in an IPv6 only network?
There was a problem hiding this comment.
With the rdr now bound to the primary IPv6 address of the specified interface, yes :)
| {% do item.update({'interface_hostaddr':conf_inf.ipaddr}) %} | ||
| {% if conf_key == intf_tag %} | ||
| {# prefer IPv6 if available, fallback to IPv4 #} | ||
| {% if conf_inf.ipv6 and conf_inf.ipv6|length > 0 %} |
There was a problem hiding this comment.
this looks highly suspicious, there is no interface.XXX.ipv6 in our configuration.
|
|
||
| foreach ($zone->interfaces->getValues() as $intf) { | ||
| // allow DNS | ||
| // allow DNS (IPv4 + IPv6) |
There was a problem hiding this comment.
I think we can just kill the whole comment, it doesn't add much to be honest
| ] | ||
| ); | ||
|
|
||
| // Allow access to the captive portal (IPv4 + IPv6) |
There was a problem hiding this comment.
same is the case for this one I guess, the description is already pretty clear.
| } | ||
|
|
||
| // block all non-authenticated users | ||
| // block all non-authenticated users (IPv4 + IPv6) |
| } | ||
| } | ||
|
|
||
| // IP-based lookup failed - try MAC-based lookup for MAC-authenticated sessions |
There was a problem hiding this comment.
This whole block is a bit questionable in my opinion, list clients should be responsible for tracking the changes via the sqlite database. When we query specifically on mac here we might end with the situation where the frontend reports "AUTHORIZED", but in reality I'm still reflected to the login page for not being in the list yet.
If we do want to wait for the "accounting department" to be finished with the registration, we can make the frontend query again in the background to see if the "connected" state changed after 30 seconds or so. But this would be more a default template thing....
| current_ips = {row[0] for row in cur.fetchall()} | ||
|
|
||
| # if identical (order-independent), do nothing | ||
| if current_ips == new_ips: |
There was a problem hiding this comment.
nice touch using a set here
| - roaming IPs are cp_client_ips.ip_address | ||
| Returns a de-duplicated set[str] (order not guaranteed). | ||
| """ | ||
| if isinstance(sessionid, bytes): |
There was a problem hiding this comment.
same as in update_roaming_ips(), how?
| # Ensure primary IP is included for each session | ||
| for rec in sessions.values(): | ||
| pip = rec.get('primary_ip') | ||
| if pip is not None and str(pip).strip() != '': |
There was a problem hiding this comment.
going further through the code, I notice a lot of "consumers" strip() their results, it feels a bit like we're either doing something wrong with storing the data or these are not needed at all.
| create index cp_clients_ip ON cp_clients (ip_address); | ||
| create index cp_clients_zone ON cp_clients (zoneid); | ||
|
|
||
| -- multiple IPs per session |
There was a problem hiding this comment.
we might consider always calling init.sql so we can change the create table statements to create table if not exists and remove the create when not there yet code earlier.
| args = parser.parse_args() | ||
|
|
||
| response = DB().list_clients(int(args.z) if str(args.z).isdigit() else None) | ||
| response = DB().list_clients(int(args.z) if str(args.z).isdigit() else None, True) |
There was a problem hiding this comment.
DB().list_clients(...,True) doesn't seem to have an actual meaning, so either the default None is an issue or adding the bool here was not needed.
|
Thanks for taking this over! |
Closes #8761
TODO: banner showing hostwatch disabled -> no ipv6 support