Pass through certain forward auth negative response headers

[wheresmysocks]

This alters forward authentication allowing servers to set cookies and redirect clients to other locations if needed.

Description

The current version of httpClient automatically follows redirects, so if a forward auth server decides to redirect the original client to a different page to continue authentication, Go will simply follow the redirect rather than passing the redirection back to the original client.

This is problematic when the forward auth function returns an unexpected success if it was able to follow a redirection chain until it encountered a 200 response. This PR alters the httpClient to stop following redirects automatically and pass a Location header from a negative forward auth response, if one exists.

Additionally, the forward auth server is not able to set any cookies (other than including JavaScript in the response body, I suppose). This PR also passes the Set-Cookie header on a negative forward auth response.

[emilevauge]

Design LGTM

[jkaberg]

@emilevauge @wheresmysocks any ETA on this one❓

[wheresmysocks]

Should now be back up to date with master.

[ldez]

LGTM

[nmengin]

LGTM 👏 👏

[ldez]

Thanks 👍

[ldez]

Could you explain err != http.ErrNoLocation is this case?
I think you want to check: err == http.ErrNoLocation && redirectURL.String() != ""

If I'm true, the code can be rewrite like that:

if err != nil {
	if err != http.ErrNoLocation {
		log.Debugf("Error reading response location header %s. Cause: %s", config.Address, err)
		w.WriteHeader(http.StatusInternalServerError)
		return
	}
        if redirectURL.String() != "" {
		// Set the location in our response if one was sent back.
		w.Header().Add("Location", redirectURL.String())
	}
}

[wheresmysocks]

There are three things to handle with Response.Location() return values:

1. Handle the case where a Location header was present but it did not have a parsable URL (some non-nil error).
2. Handle the case where no Location header was present (err == http.ErrNoLocation).
3. Handle the case where a Location header was present and it had a parsable URL.

The second case is common and not an error as far as we are concerned as any non-redirect response code would trigger this case, so it's important to ensure it passes through. Any other error was likely a non-parsable URL. With a small edit, this can be simplified in mainly the manner you describe:

redirectURL, err := forwardResponse.Location()

if err != nil {
	if err != http.ErrNoLocation { // Case #1
		log.Debugf("Error reading response location header %s. Cause: %s", config.Address, err)
		w.WriteHeader(http.StatusInternalServerError)
		return
	} /* else {
		// Case #2, do nothing with http.ErrNoLocation
	} */
} else if redirectURL.String() != "" { // Case #3
	// Set the location in our response if one was sent back.
	w.Header().Add("Location", redirectURL.String())
}

[ldez]

if err != nil {
	if err != http.ErrNoLocation {
// ...
    }
} else if redirectURL.String() != "" {
// ...
}

It's not the same thing as:

if err != nil && err != http.ErrNoLocation {
// ...
}

if err != http.ErrNoLocation && redirectURL.String() != "" {
// ...
}

It's the same as:

if err != nil && err != http.ErrNoLocation {
// ...
}

if err == nil && redirectURL.String() != "" {
// ...
}

Are you sure of your changes?

[wheresmysocks]

Yes, the change is as intended. I believe that because the first if clause in each of the last two snippets you presented above should contain a return statement, the second if clause in both snippets are functionally equivalent (as the only value err may contain after the first if clause should be nil or http.ErrNoLocation).

I believe another way to write this would be with a switch statement:

redirectURL, err := forwardResponse.Location()

switch {

// Do nothing, as there was no Location header.
case err == http.ErrNoLocation:
    break

// Return an error, as we were (likely) unable to parse the Location header.
case err != nil:
    log.Debugf("Error reading response location header %s. Cause: %s", config.Address, err)
    w.WriteHeader(http.StatusInternalServerError)
    return

// Set the location in our response if one was sent back.
case redirectURL.String() != "":
    w.Header().Add("Location", redirectURL.String())

}

[ldez]

You can rewrite this part like that:

// Pass any Set-Cookie headers the forward auth server provides
for _, cookie := range forwardResponse.Cookies() {

[wheresmysocks]

Fixed, was leftover from testing.

[ldez]

LGTM

[mmatur]

LGTM 👏