-
Notifications
You must be signed in to change notification settings - Fork 3.8k
simple-captive-portal: Multiple enhancements #28374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d91893e
a12882d
5361357
0c5945f
dd25277
f5839ff
cf8759c
4f2d776
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| config simple-captive-portal main | ||
| #option interface guest | ||
| #list interface guest | ||
| #option port_redirect 8888 | ||
| #option port_portal 8889 | ||
| #option timeout 86400 | ||
| #list exempt '01:23:45:67:89:0A' |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,28 @@ | ||
| #!/bin/sh /etc/rc.common | ||
| START=10 | ||
| USE_PROCD=1 | ||
| EXTRA_COMMANDS='firewall' | ||
| EXTRA_COMMANDS='flush_guests' | ||
|
|
||
| firewall() { | ||
| local INTF PORT_REDIRECT TIMEOUT | ||
| add_exempt() { | ||
| [ -z "${1}" ] || /usr/sbin/nft add element inet simple-captive-portal exempt_macs { "${1}" } | ||
| } | ||
|
|
||
| add_interface() { | ||
| [ -z "${1}" ] || /usr/sbin/nft add element inet simple-captive-portal portal_ifs { "${1}" } | ||
| } | ||
|
|
||
| service_triggers() { | ||
| procd_add_reload_trigger "simple-captive-portal" | ||
| } | ||
|
|
||
| start_service() { | ||
| local INTF INTF_COUNT PORT_REDIRECT PORT_PORTAL TIMEOUT | ||
| config_load "simple-captive-portal" | ||
| config_get INTF main interface | ||
| [ -z "${INTF}" ] && exit 0 | ||
| config_get INTF_COUNT main interface_LENGTH | ||
| config_get PORT_REDIRECT main port_redirect 8888 | ||
| config_get PORT_PORTAL main port_portal 8889 | ||
| config_get TIMEOUT main timeout 86400 | ||
|
|
||
| /usr/sbin/nft -f- <<EOF | ||
|
|
@@ -20,33 +34,35 @@ table inet simple-captive-portal { | |
| timeout ${TIMEOUT}s | ||
| } | ||
|
|
||
| set exempt_macs { | ||
| type ether_addr | ||
| } | ||
|
|
||
| set portal_ifs { | ||
| type ifname | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment than on first commit, add the interfaces in the set right away
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will likely have the same solution as |
||
| } | ||
|
|
||
| chain prerouting { | ||
| type nat hook prerouting priority mangle; policy drop; | ||
| iif != ${INTF} accept | ||
| iifname != @portal_ifs accept | ||
| ether saddr @exempt_macs accept | ||
| ether saddr @guest_macs accept | ||
| tcp dport 80 redirect to ${PORT_REDIRECT} | ||
| fib daddr . iif type { local, broadcast, multicast } accept | ||
| reject | ||
| } | ||
| } | ||
| flush set inet simple-captive-portal exempt_macs | ||
| flush set inet simple-captive-portal portal_ifs | ||
| EOF | ||
| } | ||
| config_list_foreach main exempt add_exempt | ||
|
|
||
| boot() { | ||
| BOOT=1 | ||
| start "$@" | ||
| } | ||
|
|
||
| start_service() { | ||
| # firewall() called by hotplug on boot | ||
| [ -z "${BOOT}" ] && firewall | ||
|
|
||
| local INTF PORT_REDIRECT PORT_PORTAL | ||
| config_load "simple-captive-portal" | ||
| config_get INTF main interface | ||
| [ -z "${INTF}" ] && exit 0 | ||
| config_get PORT_REDIRECT main port_redirect 8888 | ||
| config_get PORT_PORTAL main port_portal 8889 | ||
| # Detect whether the interface is an option or a list | ||
| if [ -n "${INTF_COUNT}" ]; then | ||
| config_list_foreach main interface add_interface | ||
| else | ||
| add_interface "${INTF}" | ||
| fi | ||
|
|
||
| procd_open_instance redirect | ||
| procd_set_param command /usr/sbin/uhttpd -f -c /dev/null -k0 -h /etc/simple-captive-portal/ -l / -L /usr/share/simple-captive-portal/redirect.lua -p "${PORT_REDIRECT}" | ||
|
|
@@ -79,3 +95,12 @@ start_service() { | |
| procd_set_param stderr 1 | ||
| procd_close_instance | ||
| } | ||
|
|
||
| stop_service() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we just implement stop, then I think restart/reload are implemented as stop/start,
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that is the case. I wasn't happy about the tiny gap during restart either. Stopping the firewall blocking on 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 |
||
| # Delete the rule that blocks traffic | ||
| /usr/sbin/nft delete chain inet simple-captive-portal prerouting &> /dev/null | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no reason to silence errors here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the I was concerned someone might see this message and attempt to delete the |
||
| } | ||
|
|
||
| flush_guests() { | ||
| /usr/sbin/nft flush set inet simple-captive-portal guest_macs | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Simple captive portal</title> | ||
| </head> | ||
| <body style="text-align:center"> | ||
| <h1>Free Wifi</h1> | ||
| <p>You are now connected. You may close this page.</p> | ||
| </body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,24 @@ function handle_request(env) | |
| return | ||
| end | ||
|
|
||
| uhttpd.send("Status: 200 OK\r\n") | ||
| if string.find(env.SERVER_ADDR, ":") == nil then | ||
| url = "http://" .. env.SERVER_ADDR .. ":" .. env.SERVER_PORT .. "/connected.html" | ||
| else | ||
| url = "http://[" .. env.SERVER_ADDR .. "]:" .. env.SERVER_PORT .. "/connected.html" | ||
| end | ||
|
|
||
| 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" | ||
|
Comment on lines
+29
to
+34
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not tested: |
||
|
|
||
| uhttpd.send("Status: 302 Found\r\n") | ||
| uhttpd.send("Server: simple-captive-portal\r\n") | ||
| uhttpd.send("Content-Type: text/plain\r\n\r\n") | ||
| uhttpd.send("You now have internet access\n") | ||
| uhttpd.send("Location: " .. url .. "\r\n") | ||
| uhttpd.send("Cache-Control: no-cache\r\n") | ||
| uhttpd.send("Content-Type: text/html\r\n") | ||
| uhttpd.send("Content-Length: " .. string.len(body) .. "\r\n\r\n") | ||
| uhttpd.send(body) | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,27 @@ | ||
| port_portal = os.getenv("PORT_PORTAL") | ||
|
|
||
| function handle_request(env) | ||
| uhttpd.send("Status: 302 Found\r\n") | ||
| uhttpd.send("Server: simple-captive-portal\r\n") | ||
| if string.find(env.SERVER_ADDR, ":") == nil then | ||
| uhttpd.send("Location: http://" .. env.SERVER_ADDR .. ":" .. port_portal .. "/\r\n") | ||
| url = "http://" .. env.SERVER_ADDR .. ":" .. port_portal .. "/" | ||
| else | ||
| uhttpd.send("Location: http://[" .. env.SERVER_ADDR .. "]:" .. port_portal .. "/\r\n") | ||
| url = "http://[" .. env.SERVER_ADDR .. "]:" .. port_portal .. "/" | ||
| end | ||
|
|
||
| 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" | ||
|
Comment on lines
+10
to
+18
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never seen a client that doesn't follow 302, can you share the one you saw ? Not tested:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.send("Status: 302 Found\r\n") | ||
| uhttpd.send("Server: simple-captive-portal\r\n") | ||
| uhttpd.send("Location: " .. url .. "\r\n") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't make a huge difference, but I think you can do |
||
| uhttpd.send("Cache-Control: no-cache\r\n") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a potential bug here (already existing), we should send
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, good catch, thanks. I'll add it in the next push. 👍 |
||
| uhttpd.send("Content-Length: 0\r\n\r\n") | ||
| uhttpd.send("Content-Type: text/html\r\n") | ||
| uhttpd.send("Content-Length: " .. string.len(body) .. "\r\n\r\n") | ||
| uhttpd.send(body) | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to add the members of the set directly here, and move the
flush set inet simple-captive-portal exempt_macswith 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
exemptoption can contain a space:I'm not aware of any
ether_addrvalues 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
exemptoptions configured, so theadd elementneeds to be done conditionally since you can't pass an empty string.Having
flush ... exempt_macsafter the table definition avoids the case during first boot where theexempt_macsset 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. :)