simple-captive-portal: Multiple enhancements #28374
Conversation
This change adds the ability to configure a list of MAC addresses which should bypass the captive portal. This is useful for devices that have no ability to interact with the login page. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
This change adds a command to the service script which will flush any known guest MACs. This is useful for testing or for resetting the guest list at a specific time via a con job or similar. # /etc/init.d/simple-captive-portal flush_guests Clients will likely need to disconnect and reconnect to the network in order to trigger captive portal detection and authenticate again. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
Clients that do not automatically follow redirects can get stuck on the HTTP redirect to the login page. This change introduces a link on the transition page to allow such clients to continue. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
Some devices don't close the captive portal page after login. This allows the operator to configure what page is shown via a HTML file. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
This change adds the ability to run the portal on multiple interfaces. The login page, guest list, and exemption list are shared between all included interfaces. In addition, changing the rule to use an interface name enables using the portal with ephemeral interfaces without restarting the service. As an added bonus, this decoupling also allows the firewall to start at any time, removing the need for hotplug. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
This change removes the blocking firewall rule when the service is stopped. This allows pausing the captive portal without changing the configuration. Instead of flushing the whole table, we just remove the rule to avoid losing the guest list during a reload. The interface and exempt lists will be refreshed when the service starts again. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
This just adds a reload handler so the `reload_config` command can reconfigure the captive portal. Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
Signed-off-by: Ryan Turner <zdbiohazard2@gmail.com>
ZDBioHazard
force-pushed
the
simple-captive-portal_add-features
branch
from
64e1d96 to
4f2d776
Compare
champtar
left a comment
champtar
left a comment
There was a problem hiding this comment.
Nice to see someone using this, and thanks for the contribution !
Separated commits with proper description are perfect, do not squash or open multiple PR.
I've reviewed the commits in order, not sure how the comments are going to show up.
| } | ||
| set exempt_macs { | ||
| type ether_addr |
There was a problem hiding this comment.
You should be able to add the members of the set directly here, and move the flush set inet simple-captive-portal exempt_macs with the other flush so we call nft once, and the change is atomic.
Here when you reconfigure there is a small window of time in which the set is empty, so all traffic is going to be rejected.
There was a problem hiding this comment.
Yes, I also wasn't thrilled about losing the atomicity here. This definitely needs to be addressed then.
I could use something like this to generate the list, but this assumes no exempt option can contain a space:
local EXEMPT
config_get EXEMPT main exempt
[ -z "${EXEMPT}" ] || EXEMPT="\"$(echo "${EXEMPT}" | sed 's/ /", "/g' )\""
I'm not aware of any ether_addr values that would contain a space, but I wasn't 100% sure.
I also considered using an alternative method based on config_list_foreach, a global variable, and a couple string builder functions that would preserve spaces, but it was really gross and I didn't think it would review well, haha.
There's also the case where there are no exempt options configured, so the add element needs to be done conditionally since you can't pass an empty string.
Having flush ... exempt_macs after the table definition avoids the case during first boot where the exempt_macs set does not exist yet. Defining it quickly beforehand isn't as simple as it is for the table.
I have a couple ideas depending on whether the spaces issue is relevant or not. Please advise and I'll get it figured out and updated. :)
| body = "<!DOCTYPE html>\r\n" .. | ||
| "<html lang=\"en\"><head>\r\n" .. | ||
| "<title>Redirecting to login</title>\r\n" .. | ||
| "</head><body>\r\n".. | ||
| "<p>Redirecting to login page...</p>\r\n" .. | ||
| "<p><a href=\"" .. url .. "\">" .. | ||
| "Click here if the page does not automatically redirect." .. | ||
| "</a></p>\r\n" .. | ||
| "</body></html>\r\n" |
There was a problem hiding this comment.
Never seen a client that doesn't follow 302, can you share the one you saw ?
In any case this is a good addition, but no need for \r\n for the body, \n is fine.
Not tested:
body = string.format([[
<!DOCTYPE html>
<html lang="en"><head>
<title>Redirecting to the captive portal</title>
</head><body>
<p>Redirecting to the captive portal...</p>
<p><a href="%s">Click here if the page does not automatically redirect.</a></p>
</body></html>]], url)
There was a problem hiding this comment.
I haven't seen one in quite a while now that I think of it, but I always try to add guard links to redirects when I can. Old habits die hard, heh.
I'm not thrilled that the HTML is hard-coded when other pages are configurable, but I don't know enough about how uhttpd/LUA works to safely attempt loading and templating something from /etc/simple-captive-portal, and hardly anyone will see this page anyway.
| uhttpd.send("Status: 302 Found\r\n") | ||
| uhttpd.send("Server: simple-captive-portal\r\n") | ||
| uhttpd.send("Location: " .. url .. "\r\n") | ||
| uhttpd.send("Cache-Control: no-cache\r\n") |
There was a problem hiding this comment.
There is a potential bug here (already existing), we should send Connection: close to be sure the hijacked http connection is closed and not reused.
There was a problem hiding this comment.
Ah, good catch, thanks. I'll add it in the next push. 👍
|
|
||
| uhttpd.send("Status: 302 Found\r\n") | ||
| uhttpd.send("Server: simple-captive-portal\r\n") | ||
| uhttpd.send("Location: " .. url .. "\r\n") |
There was a problem hiding this comment.
doesn't make a huge difference, but I think you can do
uhttpd.send("Location: ", url, "\r\n")
| body = "<!DOCTYPE html>\r\n" .. | ||
| "<html lang=\"en\"><head>\r\n" .. | ||
| "<title>Connected</title>\r\n" .. | ||
| "</head><body>\r\n".. | ||
| "<p>You are now connected. You may close this page.</p>\r\n" .. | ||
| "</body></html>\r\n" |
There was a problem hiding this comment.
Not tested:
body = [[
<!DOCTYPE html>
<html lang="en"><head>
<title>Connected</title>
</head><body>
<p>You are now connected. You may close this page.</p>
</body></html>]]
| } | ||
| set portal_ifs { | ||
| type ifname |
There was a problem hiding this comment.
same comment than on first commit, add the interfaces in the set right away
There was a problem hiding this comment.
This will likely have the same solution as exempt_macs.
I'm pretty sure ifname names can't have spaces in them, right?
|
|
||
| stop_service() { | ||
| # Delete the rule that blocks traffic | ||
| /usr/sbin/nft delete chain inet simple-captive-portal prerouting &> /dev/null |
There was a problem hiding this comment.
no reason to silence errors here
There was a problem hiding this comment.
If the stop command is run when the service is already stopped, the following error message is printed:
Error: No such file or directory; did you mean chain 'prerouting' in table inet 'fw4'?
delete chain inet simple-captive-portal prerouting
I was concerned someone might see this message and attempt to delete the prerouting chain on fw4, so I opted to silence it, heh.
| procd_close_instance | ||
| } | ||
|
|
||
| stop_service() { |
There was a problem hiding this comment.
if we just implement stop, then I think restart/reload are implemented as stop/start,
I don't think we want to flush the guests / temporarily open on restart/reload
There was a problem hiding this comment.
Yeah, that is the case. I wasn't happy about the tiny gap during restart either.
Stopping the firewall blocking on stop was intended to keep the state of the firewall and uhttpd in sync with each other. I experimented with adding a pause command that would stop the blocking, but it left uhttpd running, and calling start/stop/pause in different ways could end up getting the system into some confusing states.
Before, the only way to stop the blocking was to remove the interface from the config and reboot. With the other changes in this PR, you can remove interfaces and reload quickly. If it's not an issue that the firewall can be blocking and the login page is unavailable, then this addition of stop probably isn't necessary.
|
Excellent, thanks for the review. - Looks like you weren't happy with the same parts I wasn't super happy with either. :) I'll be able to work on this again in a few days. We'll get the issues addressed and get it nice and cleaned up for the next go-around. Thanks for the suggestions. |
2 participants
📦 Package Details
Maintainer: @champtar
Description:
simple-captive-portalis a nice and simple implementation of a click-through captive portal. It is significantly easier to get working than the alternatives, so thanks a lot!While I was implementing this service in my network, I found a need to add a few features and smooth over a few issues with devices that I ran into during the deployment. Hopefully these improvements can be useful to everyone. :>
Please check the individual commit descriptions for explanations of each change.
I wasn't sure how best to submit multiple changes like this since there isn't really a proper upstream. Let me know if I should instead squash or split this up into separate PRs for each change.
🧪 Run Testing Details
✅ Formalities
If your PR contains a patch: