Support of X-Forwarded-Host

[pamiel]

Hi all,
In case both lua-resty-openidc (deployed as Relying Party) and the Authorization server are deployed behind a front-end reverse proxy, hostnames of URLs used "externally" (i.e. from the Web Browser to the front-end reverse proxy) are different from the hostnames of URLs used "internally (i.e. from the front-end reverse proxy to the Relying Party and the Authorization server).
As a consequence, when lua-resty-openidc is called, internal hostnames are used, so the way the redirect URI is build (here, using ngx.var.http_host) makes the redirect_uri query parameter of the returned URL contain an "internal" hostname that will unfortunately not be accessible from the external Web Browser.

To prevent from this situation, the X-Forwarded-Host header is usually used. This header will be set by the front-end reverse proxy.
Impact on the code of lua-resty-openidc is quite limited in a first approach. Instead of:

return scheme.."://"..ngx.var.http_host ..opts.redirect_uri_path

we could have:

local host = ngx.req.get_headers()['X-Forwarded-Host'] or ngx.var.http_host
return scheme.."://"..host ..opts.redirect_uri_path

In a second approach :P, it looks the X-Forwarded-Host header might be multi-valued, each value being separated by a comma "," (for example: X-Forwarded-Host: host1, host2). A better implementation would be to parse the content of X-Forwarded-Host and use the first host of the list…

local host = ngx.var.http_host
local forwarded_host = ngx.req.get_headers()['X-Forwarded-Host']
if forwarded_host then
    local sep = ","
    local hosts = string.gmatch (forwarded_host, "([^"..sep.."]+)")
    host = hosts()
end
return scheme.."://"..host ..opts.redirect_uri_path

It is maybe required to remove some leading or trailing spaces...

[zandbelt]

agree; a tested pull request would be great an easy to apply!

[pamiel]

Thanks a lot @bodewig to take the relay on the implementation: I'm still trying to understand my company policy regarding contributions to open sources...

In parallel, I however made some tests and research, and it appears that the X-Forwarded-Host field can definitely be multi-valued (within a multi-layer architecture, it is most likely that each reverse proxy will append a new host value to X-Forwarded-Host)... so my second proposal of implementation is very much required. Sorry for making these tests after you contributed.

Would it be possible that you switch to this second implementation, adding the missing leading and trailing spaces removal (should look like the following: host:gsub("^%s*(.-)%s*$", "%1")) ?

Thanks a lot for your help!

[bodewig]

First of all, we probably want to support Forwarded even though it doesn't seem to be in wide use so far.

In the multi-layer architecture you describe I would have expected a single header per proxy (and we'd need the outermost one). mod_proxy's dicumentation hints at having multiple comma separated values instead. I assume we'd have to deal with get_headers returning either an array or a comma separated list.

[pamiel]

Oops, I did not realize the existence of the Forwarded header... As you say, it is not widely used (it looks even not directly supported by mod_proxy, for example)...

Regarding the format of the X-Forwarded-Host, sorry if I was not clear when using the "multi-valued" term, but I'm fully in line with mod_proxy documentation: what I've seen so far is only a single header with a comma separated list of values. The implementation proposal I made in my first post is indeed compliant with this format.

Do you think we can take all of this (X-Forwarded-Host usage + comma-separated list format) as an assumption?

[bodewig]

Do you think we can take all of this (X-Forwarded-Host usage + comma-separated list format) as an assumption?

Yes, of course. It may just take a few days.

[pamiel]

Sure, sure, we'll see this in 2018 !! :P
Thanks a lot, again.

[bodewig]

I've updated #129 to deal with multiple headers (and use only the first one) and a comma-separated list of hostnames. I simply throw away all whitespace which may even help the X-Forwarded-Proto case.

For Forwarded without the X- I'd rather create a separate PR next year.

[zandbelt]

both included in release 1.5.3 now