Remove custom port from CSRF cookie suffix

[Fmstrat]

• Your changes are not possible to do through a plugin and relevant
  to a large audience (ideally all users of OctoPrint)
• If your changes are large or otherwise disruptive: You have
  made sure your changes don't interfere with current development by
  talking it through with the maintainers, e.g. through a
  Brainstorming ticket
• Your PR targets OctoPrint's devel branch if it's a completely
  new feature, or maintenance if it's a bug fix or improvement of
  existing functionality for the current stable version (no PRs
  against master or anything else please)
• Your PR was opened from a custom branch on your repository
  (no PRs from your version of master, maintenance, or devel
  please), e.g. dev/my_new_feature or fix/my_bugfix
• Your PR only contains relevant changes: no unrelated files,
  no dead code, ideally only one commit - rebase and squash your PR
  if necessary!
• Your changes follow the existing coding style
• If your changes include style sheets: You have modified the
  .less source files, not the .css files (those are generated
  with lessc)
• You have tested your changes (please state how!) - ideally you
  have added unit tests
• You have run the existing unit tests against your changes and
  nothing broke
• You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

Addresses: #4740 by utilizing 443 or 80 as the port for the CSRF cookie name even if a custom port is being used.

How was it tested? How can it be tested by the reviewer?

The best method is to set up an NGINX proxy outside of OctoPi running on https://octoprint.domain.com:12345 and confirm that https://octoprint.domain.com:12345/reverse_proxy_test/ is all green. You will see 443 as the port (to align to the server port from OctoPi). Without this fix, logins will fail with a 400, with it, they will succeed regardless of if you are running a standard port, non-standard port, going through haproxy, or direct.

Any background context you want to provide?

#4740 has a link to the forum post with a full description and debugging of this issue.

What are the relevant tickets if any?

#4740

Screenshots (if appropriate)

N/A

Further notes

N/A

[GitIssueBot]

This pull request has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/csrf-reverse-proxy-configuration-issue-or-bug/50066/13

[cp2004]

Should the fix not be to go the other way around - rather than get the port wrong in both places, OctoPrint should get the correct port? It should match what is actually happening, which is you using the UI from port 12345, not 443.

[Fmstrat]

Should the fix not be to go the other way around - rather than get the port wrong in both places, OctoPrint should get the correct port? It should match what is actually happening, which is you using the UI from port 12345, not 443.

Yes.. but no?

The current CSRF methodology of OctoPrint is really setting the cookie name via schema, it's just aliasing the port. For instance, _P80 could be _SHTTP and _P443 could be _SHTTPS. While it might "look" odd to have 443, that's the port the server believes it is listening on, since there is no baseurl set. I.E. the current cookie naming convention is somewhat arbitrary.

This could be handled with an overhaul to force setting a baseurl with custom port when using a proxy (including haproxy in OctoPi), and then updating OctoPrint to always leverage a "true port" for the cookie name instead of P80/P443. But since the cookie name is arbitrary and set via schema (just with the port as the name), it doesn't add any additional security.

So: Yes, if we want the reverse proxy page to "look better." But no if we want the same functionality as today, ease of setup via not requiring users with custom ports to set custom configurations in config.yaml, and a simpler fix with little impact to other pieces of code, all while maintaining the same security posture. Given the benefits of the "no", I personally lean that way.

[cp2004]

The current CSRF methodology of OctoPrint is really setting the cookie name via schema, it's just aliasing the port. For instance, _P80 could be _SHTTP and _P443 could be _SHTTPS.

While I agree (to the best of my knowledge) the cookie suffix is a label (and it could be whatever we decide), it's not right to say it only takes either 80 or 443. When I access OctoPrint directly (no proxy) through localhost:5000 I get the correct suffix:

And if I use haproxy to send it through http port 4000, I still get a correct suffix:

So it does listen to the port number, not just using the scheme, and when properly setup they match.

In your forum post, it looks to me like the client is generating the cookie suffix correctly as _P12345 (in the screenshot), but this is not matching the server. The client is correct, the server is wrong, so rather than make the client also wrong (but the same wrong as the server) the server should be made right to tell the truth about what port you access OctoPrint on. That could either be a configuration issue or a bug, I haven't yet tried that but it is up next for testing (likely at the weekend).

EDIT: To add, the port definitely needs to be correct server-side for more than just the CSRF. You'll be able to log in, but I suspect some things won't work. If you go to the files section & try to download a file, I think it would then point to the wrong port there for example?

[cp2004]

OK, it didn't take that long to test in the end...

I setup haproxy with a self-signed cert, on port 3000 for https, and everything works as expected. So I would be inclined to say there is still something wrong with your nginx configuration, and so I'll hop back to the community forums to have a poke at what might be the issue there.

For reference, the haproxy config I used:

haproxy.cfg

global
        maxconn 4096
        user haproxy
        group haproxy
        log /var/log/haproxy.log local1 debug
        tune.ssl.default-dh-param 2048

defaults
        log     global
        mode    http
        compression algo gzip
        option  httplog
        option  dontlognull
        retries 3
        option redispatch
        option http-server-close
        option forwardfor
        maxconn 2000
        timeout connect 5s
        timeout client  15m
        timeout server  15m

frontend public
        bind :::4000 v4v6
        bind :::3000 v4v6 ssl crt /etc/ssl/snakeoil.pem
        option forwardfor except 127.0.0.1
        default_backend octoprint

backend octoprint
        acl needs_scheme req.hdr_cnt(X-Scheme) eq 0

        http-request replace-path ^([^\ :]*)\ /(.*) \1\ /\2
        http-request add-header X-Scheme https if needs_scheme { ssl_fc }
        http-request add-header X-Scheme http if needs_scheme !{ ssl_fc }
        option forwardfor
        server octoprint1 127.0.0.1:5000

[Fmstrat]

Interesting, I wonder why you're getting a different result. I guess what I'm saying is that the function I changed isn't leveraged by any part of the system other than the CSRF cookie name. The only reason I can think of to even have pre-defined cookie names like this would be to separate HTTP from HTTPS, but if that's the case, there's no reason to use a port in the cookie name at all. It could just be changed to "_SHTTP" and "_SHTTPS" to remove the complexity altogether. The proxy port in headers and/or cookies have no impact on the proxy itself (including file downloads) since the only place it's used is with CSRF (at least that I see calling that function).

In any event, if there's a cleaner way to do this, it works for me. I'm just not finding it with nginx.

[Fmstrat]

@cp2004 Trying to figure out what's different about your setup and mine, and I noticed host_to_server_and_port does have separate code for determining port when IPV6 vs IPV4. I don't see any "issue" per-se, but if you are willing to take a look, you know this stuff better than me.

EDIT: I have confirmed the above function does return 443 for me, and the resulting environ variable is incorrect on the server (when passing via nginx in ipv4). Not sure if that helps or not.

[Fmstrat]

@cp2004 Thank you for the direction, I have figured this out, and it is nginx specific, though would still impact anyone running nginx with default configurations.

I traced the calls back to the use of HTTP_X_FORWARDED_HOST for the port, and the default of this header in an nginx reverse proxy would be $server_name. I have forced the mapping of this with:

proxy_set_header X-Forwarded-Host $host:12345;

With success. I will close this PR, resolve my issue, and make a request in the forum for an update on https://community.octoprint.org/t/reverse-proxy-configuration-examples/1107 to include this specificity in the nginx configuration example.

Thanks again!

[GitIssueBot]

This pull request has been mentioned on OctoPrint Community Forum. There might be relevant details there:

https://community.octoprint.org/t/reverse-proxy-configuration-examples/1107/47

[cp2004]

That's great that you've managed to sort it out - and we've learnt something about how nginx works with https ports then I guess. I'll have to play around with nginx closer (I tend to always use haproxy as that's what's included with OctoPi) to see if it has any other slight quirks.