Use X-Forwarded-Host in Redirects #729
Conversation
NickMeves
force-pushed
the
x-forwarded-host-redirect
branch
from
8f2444e
to
04337b6
Compare
| ) | ||
|
|
||
| // MakeCookie constructs a cookie from the given parameters, | ||
| // discovering the domain from the request if not specified. | ||
| func MakeCookie(req *http.Request, name string, value string, path string, domain string, httpOnly bool, secure bool, expiration time.Duration, now time.Time, sameSite http.SameSite) *http.Cookie { | ||
| if domain != "" { | ||
| host := req.Host |
There was a problem hiding this comment.
Not sure why this one was left as req.Host in #412 when every other usage was changed to use GetRequestHost(req)
@edahlseng Do you happen to remember from your PR? (Sorry, I know it was a while ago 😅 )
| @@ -195,7 +197,7 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu | |||
|
|
|||
| err := l.authTemplate.Execute(l.writer, authLogMessageData{ | |||
| Client: client, | |||
| Host: req.Host, | |||
| Host: util.GetRequestHost(req), | |||
There was a problem hiding this comment.
These are the switches I'm most on the fence about. Looking for second opinions about which Host to log when Host & X-Forwarded-Host are present in a request.
There was a problem hiding this comment.
I think X-Forwarded would be the ones that makes sense to log. We should be inspecting that header when the mux determines where to route the traffic, so that's the host I would expect to be logged. It should reflect what the user put in their browser right?
There was a problem hiding this comment.
Yeah, if it exists - X-Forwarded-Host reflects what the user put into the browser where Host then reflects what the API Gateway changes the host to to work with the next hop internal proxy's Host based routing (IE K8s ingress pod).
I was trying to think through any implications of user or hacker setting a X-Forwarded-Host themselves manually... but I can't see any new issues or threat vectors that don't already exist since a client technically now could set the Host header to whatever they want now.
There was a problem hiding this comment.
This all seems sensible to me. You're right about the logging one, I think it makes sense to use the request host, but we should probably discuss the implications of this further. I'd like to play around with the logging and observe what the differences are on a nginx based example with and without this change
| @@ -195,7 +197,7 @@ func (l *Logger) PrintAuthf(username string, req *http.Request, status AuthStatu | |||
|
|
|||
| err := l.authTemplate.Execute(l.writer, authLogMessageData{ | |||
| Client: client, | |||
| Host: req.Host, | |||
| Host: util.GetRequestHost(req), | |||
There was a problem hiding this comment.
I think X-Forwarded would be the ones that makes sense to log. We should be inspecting that header when the mux determines where to route the traffic, so that's the host I would expect to be logged. It should reflect what the user put in their browser right?
NickMeves
force-pushed
the
x-forwarded-host-redirect
branch
from
04337b6
to
54f8292
Compare
|
I have nothing else to add, but I'd like to take this for a spin before we merge |
NickMeves
force-pushed
the
x-forwarded-host-redirect
branch
from
54f8292
to
29b2479
Compare
#724
Description
Use the
GetRequestHosthelper more consistently in the codebase. Previously it was only used in cookie setting logic while some redirect areas missed it and only usedreq.Host, missingX-Forwarded-Hostwhen it would've been appropriate to use it.Additionally, set the {{.Host}} logging to use this instead of plain
req.HostMotivation and Context
Redirects weren't aware of systems behind an access proxy that changed the
Hostheader and set anX-Forwarded-Host. This caused the redirects after the authentication process to go to the wrong URLs and be misaligned with various cookies.How Has This Been Tested?
Unit Tests
Checklist: