Captive portal: wrong way to get the mac address

[QuentinC]

Hello,

I've made a post in the user forum: https://forum.opnsense.org/index.php?topic=4136.msg15121#msg15121

In this post, I explain a bug with the captive portal.
The arp.py file won't give the mac address, and so will break the captive portal functionality.

Here is the fix: changing this line:
mac = line.split('at')[-1].split('on')[0].strip()
by this one:
mac = line.split('at')[1].split('on')[0].strip()

in arp.py ( https://github.com/opnsense/core/blob/master/src/opnsense/scripts/OPNsense/CaptivePortal/lib/arp.py#L64 ) worked for me.

Thanks,

Quentin

[fichtner]

Hi Quentin,

Nice! I will defer to @AdSchellevis as he needs to inspect this and it means it can only hit 17.1.x as 17.1 is currently building for next week.

Can you provide output of "/usr/sbin/arp -an" from the command line for the case where your version works but the current code doesn't? I don't know if the output for arp shifts. Just so we can double-check what's going on. :)

Thanks,
Franco

[QuentinC]

Here is what I get :

root@router:~ # /usr/sbin/arp -an
? (10.1.2.1) at aa:bb:cc:dd:ee:ff on bridge1 permanent [bridge]
? (10.1.2.2) at aa:bb:cc:dd:ee:ff on bridge1 expires in 692 seconds [bridge]
? (10.1.1.1) at aa:bb:cc:dd:ee:ff on bridge0 expires in 1180 seconds [bridge]
? (10.1.1.3) at aa:bb:cc:dd:ee:ff on bridge0 expires in 1153 seconds [bridge]
...
...
? (192.168.1.254) at aa:bb:cc:dd:ee:ff on re0 expires in 1186 seconds [ethernet]

Thanks,
Quentin

[fichtner]

Ok, I think we should rather change the whole parsing logic for the arp output to make the code less ambiguous.

[AdSchellevis]

ok, that's weird.... my previous line seems te be delivering the correct output using the input stated above:

`

tmp ="""
... ? (10.1.2.1) at aa:bb:cc:dd:ee:ff on bridge1 permanent [bridge]
... ? (10.1.2.2) at aa:bb:cc:dd:ee:ff on bridge1 expires in 692 seconds [bridge]
... ? (10.1.1.1) at aa:bb:cc:dd:ee:ff on bridge0 expires in 1180 seconds [bridge]
... ? (10.1.1.3) at aa:bb:cc:dd:ee:ff on bridge0 expires in 1153 seconds [bridge]
... ...
... ...
... ? (192.168.1.254) at aa:bb:cc:dd:ee:ff on re0 expires in 1186 seconds [ethernet]
... """
map(lambda x:x.split('at')[-1].split('on')[0].strip(), tmp.split('\n'))
['', 'aa:bb:cc:dd:ee:ff', 'aa:bb:cc:dd:ee:ff', 'aa:bb:cc:dd:ee:ff', 'aa:bb:cc:dd:ee:ff', '...', '...', 'aa:bb:cc:dd:ee:ff', '']

`

Not sure what caused the change to work, but both statements provide the same output.... (first entry v.s. last entry, but only one occurs).

[fichtner]

can we simplify this by doing a split(' ') kind of thing? the output is fixed and relying on subelement context seems to be prone to problems later on

[AdSchellevis]

.split()[3] should be fine too, as long as you validate the length of the string first....
But to be honest, the format is fixed as well, I'm not 100% sure there is no way for another space or tab to slip in between (if you know for sure, you can replace all of them, otherwise it's safer to keep it like it is.

The current loop always returns output.

'some random text'.split('at')[-1].split('on')[0].strip()
returns :
'some random text'

[fichtner]

it's simply hard to read / spot errors. :)

we could also split by regex "\s+"

[AdSchellevis]

That's a matter of taste :) I rather use the normal split.
Feel free to change.

[fichtner]

split() default seems to do exactly this which is perfectly fine... "If sep is not specified or is None, a different splitting algorithm is applied: runs of consecutive whitespace are regarded as a single separator, and the result will contain no empty strings at the start or end if the string has leading or trailing whitespace."

returning invalid / non-parsable data without being able to catch it is tricky, may have led into this very ticket.

[QuentinC]

Hello,

I'm coming back to you with this problem.
I've upgraded my router to latest version, and I didn't experiences this problem anymore.

But, as the code was the same, and arp -an had the same output, I didn't understand how it felt un a working state...

But I finally have found the real issue !
When I discovered the bug, I was using an internal WiFi board for the guest network. The interface name was "ath0_wlan2".

If I try your script with this interface, the problem occurs:

>>> tmp="""
... ? (10.1.2.1) at aa:bb:cc:dd:ee:ff on bridge1 permanent [bridge]
... ? (10.1.1.3) at aa:bb:cc:dd:ee:ff on ath0_wlan2 expires in 1153 seconds [bridge]
... """
>>> map(lambda x:x.split('at')[-1].split('on')[0].strip(), tmp.split('\n'))
['', 'aa:bb:cc:dd:ee:ff', 'h0_wlan2 expires in 1153 sec', '']

The problem is the "at" in "ath0" !

You should maybe just change 'at' to ' at ' (with space before and after "at"), it works in my case:

>>> map(lambda x:x.split(' at ')[-1].split('on')[0].strip(), tmp.split('\n'))
['', 'aa:bb:cc:dd:ee:ff', 'aa:bb:cc:dd:ee:ff', '']

I don't have this problem anymore just because I'm now using a bridge for my guest network... And of course, "bridge2" doesn't contain 'at' :)

Thanks,

Quentin Canel

[fichtner]

Thanks Quentin, very nice catch! All the more reason to refactor this as proposed to get rid of character-based matching.

[AdSchellevis]

refactored using default split() 3151c87, at my end it looks good, feel free to reopen if the issue reappears.