core icon indicating copy to clipboard operation
core copied to clipboard

Change Captive Portal Redirection Port

Open alexyao2015 opened this issue 2 years ago • 2 comments

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

  • [x] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
  • [x] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue

Is your feature request related to a problem? Please describe.

I would like to run the captive portal interface behind a reverse proxy using a custom FQDN. I am currently running into a limitation with this as even after changing the hostname, it still is hardcoded to use an 8000+ port number. I simply want a way to use a custom hostname with standard HTTP/S ports while still having the listen port remain the same.

Describe the solution you like

Add some way to adjust the hardcoded redirection port.

Describe alternatives you considered

Not using a reverse proxy, but this makes ssl certificate management a huge pain.

Additional context

https://github.com/opnsense/core/blob/90f471cb1999d2efca5319ad89074f499f53280e/src/opnsense/service/templates/OPNsense/Captiveportal/lighttpd-zone.conf#L27

alexyao2015 avatar Jun 15 '22 05:06 alexyao2015

@fichtner Do you have an update on possible solutions for this? I see you've added a support label, but I don't believe this is support. Its a feature request.

alexyao2015 avatar Jul 24 '22 18:07 alexyao2015

+1

s0mm3rb avatar Aug 18 '22 23:08 s0mm3rb

This issue has been automatically timed-out (after 180 days of inactivity).

For more information about the policies for this repository, please read https://github.com/opnsense/core/blob/master/CONTRIBUTING.md for further details.

If someone wants to step up and work on this issue, just let us know, so we can reopen the issue and assign an owner to it.

OPNsense-bot avatar Dec 12 '22 05:12 OPNsense-bot

@fichtner Could this be reopened because this is still a real issue and something that would be great if it could be changed.

alexyao2015 avatar Dec 12 '22 07:12 alexyao2015

@alexyao2015 if someone offers to work on it, we can reopen when a PR emerges, no problem.

AdSchellevis avatar Dec 12 '22 08:12 AdSchellevis

I would like the very same thing. I could consider making a PR, the GUI/plugin part I could manage, but some pointers would be very welcome as to where to start to even change these port numbers...

toxic0berliner avatar Jan 14 '24 21:01 toxic0berliner

@toxic0berliner I would start to look at the templates in https://github.com/opnsense/core/tree/master/src/opnsense/service/templates/OPNsense/Captiveportal

When opening a PR, please make sure the issue it should solve is clear in the text as well. As changing the port number consistently will not offer the possibility to use a different handler, it merely makes sure localhost for the calculated numbers is not bound to something different.

AdSchellevis avatar Jan 15 '24 08:01 AdSchellevis

Thanks @AdSchellevis , not sure I get the point with the handler thing.

Looks to me opnsense config is already available in the tempaltes and probably only adding a port setting on the zone as well as reverse-proxy field to list trusted proxies should suffice as pointed in the first post mentioning the template https://github.com/opnsense/core/blob/90f471cb1999d2efca5319ad89074f499f53280e/src/opnsense/service/templates/OPNsense/Captiveportal/lighttpd-zone.conf#L27

Looking at it further I do believe changing the port might not be the proper way to do as @alexyao2015 said certificates also need to be managed, so will probably focus on adding wht is needed to allow to reverse-proxy in front of the zone lighttpd, I'm just not clear yet where the IP of the client is taken from to pass it to the allow.py script, I need to ensure at this place it does use the X-Real-IP if coming from a trusted proxy.

Still a bit fuzzy how this all works, looks to me there is a process running in the background that will check IP against MAC and move the session to a new IP if the client changes IP, but that I believe happens without the user going on the lighty pages anyway so hopefully this will continue to work even when reverse-proxy is in front of the zone lighty.

Will first do some tests to get my reverse proxy working, make light stop redirecting to a specific port, make it take the X-Real-IP header... And will report further here

toxic0berliner avatar Jan 15 '24 08:01 toxic0berliner

I think I've found the place I was most looking for https://github.com/opnsense/core/blob/5b5cf45846c679b6c90d4447b4493bf63c5d68cf/src/opnsense/mvc/app/controllers/OPNsense/CaptivePortal/Api/AccessController.php#L98 There it will accept x-forwarded-for but only if the caller IP is 127.anything (strange check though...)

Still looking for the place that is redirecting to the portal url with the :8000 port because that too needs disabling if a custom port or proxy is to be used

toxic0berliner avatar Jan 15 '24 17:01 toxic0berliner

There it will accept x-forwarded-for but only if the caller IP is 127.anything (strange check though...)

Originally there was the intention to support a list of trusted proxies as everyone can send a header. This was removed years ago as not fully implemented.

AdSchellevis avatar Jan 15 '24 17:01 AdSchellevis

The relevant code for redirection was already noted in my original issue.

alexyao2015 avatar Jan 15 '24 22:01 alexyao2015

Wow, took me quite a bit of brain juice to try to understand how IPFW is being used by the captive portal, but now I think I get it and probably finally understand the original warning by @AdSchellevis .

Need to discuss as I would really like to make the PR and get it into main as I'm targetting it at a system I'll share management for and want it to be fully gui managed and not fear pressing the upgrade button...

If I don't touch the IPFW rules it means that the reverse-proxy has to run either on the firewall itself, else all traffic to it would be fwd by IPFW and next-hop would be on the lighty running the captive portal for the zone if it's tcp 80 or 443, or simply blocked if it's anything else anyways...

So, my current battle plan :

  • edit the GUI to add new advanced settings on each FW Zone :
    • Allowed and Trusted Proxy IP - Traffic to this proxy IP on the below port will be possible even to unauthenticated users
    • Allowed and Trusted Proxy Port - Port on which the proxy shall be accessible
  • edit ipfw.conf template to allow all traffic to this proxy even for unauthenticated users
  • edit lighty config to
    • make it redirect to the "allowed and trusted proxy port" instead of zoneid+8000 (line 88 might already be a nightmare, depends if lighty handles absence of port being equal to 443 for https and such... not familiar with light, yet :wink: )
    • make it trust X-Forwarded-for or X-RealIp from the trusted proxy (seems to already be the case in lighty, at least some already happens smoothely between the zone and the api lighty)
    • ensure the accesscontroller reads the X-RealIP or X-Forwarded-For headers properly if they come from the above "Allowed and Trusted Proxy IP". There might actually be quite a pile of those headers, the last proxy in the line beeing 127.0.0.1 from lighty-zone to lighty-api I believe, not sure yet how it will sho up in this getClientIp function on the request object...

I think that should do the trick. So do you agree it is safe to assume someone using these new advanced settings is responsible to secure his proxy and ensure it serves only what it needs to on the specified port.

I also plan to "reuse" the existing "Hostname" setting in the zone it's already here to set the redirect properly in lighty I see no reason to add another one.

For my personal use-case I originally planned to use traefik which defaults to using X-Real-IP instead of X-Forwarded for, I'd also like your opinion for this non-standard header, shall I :

  • support these 2 I know of ?
  • support X-forwarded-for and leave it to the user to configure his proxy to send this
  • add another advanced setting in the UI to spell out the header to be used to fetch the real client IP to be added in our IPFW rules ?

Given my previous experience making a PR to enhance ddclient preparing for the demise of the previous dyndns php plugin, I fear this PR might be larger and will take time, if anyone has experience and can tell me if it's probably more a question of weeks or months between my PR and it's merge for such a topic I'm all ears ;)

toxic0berliner avatar Jan 18 '24 00:01 toxic0berliner

My only advice here would be take it slow and split it into achievable goals/PRs that fit your priorities. Some things may be debatable or take more work than initially thought making a large scale PR unmergable and hard to work through from both sides.

Cheers, Franco

fichtner avatar Jan 18 '24 08:01 fichtner

Thanks @fichtner. Not sure how I could split this even further without making a PR that doesn't bring any feature, but I'll definitely do my best to remember to keep things simple, especially when in the PHP code that I know best, I'll do my best to not get sidetracked with checks & subchecks and improovments that are not needed and complex ;) keep it simple shall be my moto !

But happy to see that experienced devs here have only this advice looking at my plan ;) Hope the PR will look as good ;)

toxic0berliner avatar Jan 18 '24 22:01 toxic0berliner