OSC / ondemand

Supercomputing. Seamlessly. Open, Interactive HPC Via the Web
https://openondemand.org/
MIT License
287 stars 106 forks source link

mod_auth_openidc: newer versions cause lots of warnings about OIDCXForwardedHeaders #3880

Open CSC-swesters opened 2 weeks ago

CSC-swesters commented 2 weeks ago

We upgraded mod_auth_openidc to a newer version (we have tested 2.4.15.7 and 2.4.16.4 specifically), in order to get certain site-specific functionality working. We noticed that there is a noticeable amount of warning logs from the module, which complain about mismatches in expectations of X-Forwarded-* headers. See the configuration documentation here for details.

The default value of OIDCXForwardedHeaders none causes warnings each time one of the headers appears, and if one were to set it to X-Forwarded-Proto for example, it would warn whenever the header is missing.

The X-Forwarded-Proto header is the only one we've observed in our OOD deployments, and the total amount of logging is roughly 35% of the OOD instance's log line count, when using the default OIDCXForwardedHeaders none configuration. So it's a significant issue.

The mod_auth_openidc project seems to not offer any way of avoiding this check, or warning log, which indicates to me that sites should have this working correctly.

My questions for OOD are:

  1. Why are we seeing X-Forwarded-Proto warnings from making a request to the dashboard?

I noticed that the header is set in the Lua proxy code, and this is what triggers the warnings from mod_auth_openidc. Commenting out this line in proxy.lua removes the warning logs altogether.

  1. What is this header used for downstream?
  2. Could it simply be removed?
  3. If not, could it be injected in some other way, which would not irritate mod_auth_openidc?

Some additional info:

Thanks in advance!

johrstrom commented 2 weeks ago
  1. Why are we seeing X-Forwarded-Proto warnings from making a request to the dashboard?

  2. What is this header used for downstream?

Unfortunately the commit that adds it doesn't give us much information. Ostensibly, it's so the origin server can make some sort of toggle. While the dashboard may not care, it's sort of a larger issue when we talk about what other Passenger apps could be out there in the wild, and how they may be reacting to this.

https://github.com/OSC/ondemand/commit/c7ab1c71af4aa606e491424a8ef83eb200bc6dbd

  1. Could it simply be removed?

Maybe. But again, it brings into question comparability with Passenger apps in the wild. Someone may be using it, though I can't say for sure.

Why not just configure OIDCXForwardedHeaders to allow the X-Forwarded-Proto header?

CSC-swesters commented 2 weeks ago

While the dashboard may not care, it's sort of a larger issue when we talk about what other Passenger apps could be out there in the wild, and how they may be reacting to this.

Based on my understanding this is the risk with disabling it, yes. Downstream apps might behave differently if they do not know whether the connection is secure or not. I suppose we would need to test this and make a decision based on the outcomes. But good to know that OOD's default apps don't rely on it.

Why not just configure OIDCXForwardedHeaders to allow the X-Forwarded-Proto header?

Good question, sorry for not bringing that up earlier. I already tested this as well, and it also causes a non-zero amount of warnings due to the logic I described above:

if one were to set it to X-Forwarded-Proto for example, it would warn whenever the header is missing.

A quick 1-minute test with this option on yields 89 warning lines in a total of 256 httpd log lines. So this is not really a viable option either.

$ ts="2024-10-21T14:43:00" ; \
>    gawk "{if (\$1 > $(date +%s --date=${ts:?})) print }" < log | grep -cE ' ondemand_httpd ' ;  \
>    gawk "{if (\$1 > $(date +%s --date=${ts:?})) print }" < log | grep -cE 'oidc_check_x_forwarded_hdr' ;
256 # Total httpd log lines
89  # OIDC warning lines

I believe that the mod_auth_openidc that OOD bundles doesn't yet have these warnings, but if you plan on upgrading that to something newer, other sites will run into this as well. The parsing of these headers seems to have been introduced in version 2.4.11rc0 (commit).

It would be nice to understand what the correct way of handling this might be, and how to avoid the warning logs, so please let us know if you have ideas here.