Repository: oauth2-proxy/oauth2-proxy
Pull request #799 (2020-09-28 19:22:23)

Use comma separated multiple values for header
Use set instead of add for the X-Auth-Request-Groups header.

## Description

When trying to test the latest group support https://github.com/oauth2-proxy/oauth2-proxy/pull/616, I found out the X-Auth-Request-Groups header has been truncated and only first header is returned. My environment is nginx-ingress 0.30.0. I tried to dig out why this happened. It turned out if we set the header multi-values directly instead add each value, it works fine. I suspect somewhere in the proxy code it calls header.Get() which only returns the first value in the header. But I am not able to find it out

## Motivation and Context
This is a bug needed to be fixed

## How Has This Been Tested?
Tested the headers are appended correctly for single and multiple group value

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


lilida: cc: @stefansedich @NickMeves  This is one issue I have seen when tested the latest groups change
lilida: @NickMeves @JoelSpeed Any idea on the issue and how we can proceed?
JoelSpeed: This is actually a bug in Nginx and not a bug in OAuth2 Proxy. If you load up your environment and take a look at the response headers from `/oauth2/auth` you will see that there is in fact multiple values for your groups under multiple headers

For example:
![Capture](https://user-images.githubusercontent.com/9232216/94996277-871fa580-059b-11eb-8a7e-f40d13dbb8e9.PNG)

This is how the Go libraries set multi valued headers and it is in accordance with the [HTTP specification](https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2).

> Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

NGINX's problem is that when you call `auth_request_set`, it is seeing the three values and, where it should concatenate them and make them comma separated (`Group1,Group2,Everyone`), it just uses the first value.

I believe people have worked around this issue in the past using the NGINX Lua module but I'm not certain of that
NickMeves: So I actually didn't fully process the behavior of this was split headers (and therefore think we should look at `X-Forwarded-Groups` again as well if that has each group in a split header) -- reading that spec is a little confusing though, so feel free to disregard me if I'm understanding things incorrectly 😅 

I see the merits in slitting is this were called `X-Forwarded-Group` (singular).  But naming it `Groups` I think this should be consolidated to one header with a separator. (I dug up this old discussion): https://github.com/oauth2-proxy/oauth2-proxy/pull/616#discussion_r441064196

Even though it is less clean per HTTP spec, it fits better what enterprise upstream apps expect for SSO roles:

Here's an an example of Jenkins wanting that scheme for SSO groups: https://learn.akamai.com/en-us/webhelp/enterprise-application-access/enterprise-application-access/GUID-C5FBC2F3-EEC4-4C43-82BC-94E4B72B051F.html

And a sample from Graylog SSO looking for a single comma separated header for roles: https://docs.graylog.org/en/3.3/pages/users_and_roles/external_auth.html#single-sign-on

@stefansedich mentioned he did some investigation and saw a split of `,` `;` and `|` with `,` being most common - hence revisiting the groups array separator might be valid.
alanjcastonguay: I am responsible for the environment @lilida mentioned earlier, now running https://github.com/kubernetes/ingress-nginx/releases/tag/controller-v0.35.0 with `nginx.ingress.kubernetes.io/auth-response-headers: "x-auth-request-user, x-auth-request-email, x-auth-request-groups, authorization"` and `nginx.ingress.kubernetes.io/auth-url` pointed to an `oauth2-proxy` built from @lilida 's `multivalueheaders` branch and `--upstream=file:///dev/null`. The way nginx collects auth-subrequest response headers and sends them on to a backend application is important.

I can see merits of both a `something, delimited, list` of groups in one header, or a header repeated multiple times. I have only seen the single-header-with-list style in the wild. It seems pragmatic to support, even if less clean.

I would like to get back to mainline. Go is not my first language. I'm not sure what I could contribute. I am happy to research or test things.
NickMeves: @JoelSpeed Circling back on this one for your final opinion.

I personally lean toward comma delimited instead of split headers.  Even though split headers might be more to spec, the more prevalent implementation in upstream Commercial-off-the-Shelf software is to pull a comma delimited list of groups from a single header.

We also could add another config option to flip which type of group headers we support... But I hate the configuration explosion for simple things like this and would lean toward us picking a route.
alanjcastonguay: FWIW, I would want CSV as the primary mode, with split-headers or alternate-delimiters options added later if/when demand appears.
NickMeves: Something we debated in the original PR that looks like it will be an issue for some users afterall -- whether to allow configuring the group separator in the headers.  This question came up in Slack and this sample groups-like claim would have issues with a `,` separator by default:

```
  "memberof": [
        "CN=EMSUDEMY,OU=Groups,O=cco.xxx.com",
        "CN=all-xxxx-people,OU=xxxx Groups,O=cco.xxxx.com"
    ],
``` 
alanjcastonguay: Now that #839 is merged, can this move forward?
NickMeves: Can you take a peek at this PR: https://github.com/oauth2-proxy/oauth2-proxy/pull/826

When @JoelSpeed is back that is the next PR that is a baseline for a bunch of others we are looking to wrap up. It completely removes this codepath, but we are trying to work out the right design for `Get` vs `Set` that you have uncovered in this PR.
JoelSpeed: I'm wondering if we leave all of the existing logic as is, post #826, then maybe add some sort of flattening logic that reads the headers and flattens them as desired? Could become a new part of the header middlewares 🤔  
JoelSpeed: #826 has now merged, I think the route forward is to have some logic that flattens headers in the header injection stuff, should be able to iterate through the headers, get the `Values()` (`[]string`), then do the `strings.Join` on that and finally `Set` the header to that value.

Do we want to support different separators for this or can we stick to comma separated in the first instance?
therc: Is this in scope for 7.0? We'd rather not run a fork to get the issue fixed.
Comma separated values might cover the vast majority of users for the initial release, it seems. No objection to that from here. :)

If the X.500-like Common Names are a concern, because multiple groups would be joined incorrectly into one line, breaking things even further, then one could check if the headers already contain a comma. If so, just return the first header as is, which is the same as the current behaviour for such environments, but unblocks the common case, no? Those users could then file an issue for custom separators — hoping that e.g. they don't require also some escaping/massaging of the values.

The logic above could be extended so that you go through e.g. the string `,;|` and use the first separator that is not present in the first header. There might be too much magic in that, though. For one, it considers the semicolon as the best choice if the incoming headers have a comma, which might or might not be a major assumption to make. (Also: do you repeat this for every request, or do you cache the inferred separator?)
JoelSpeed: This is a change in behaviour for us that could break users, I prefer we get this in before we release v7
NickMeves: > #826 has now merged, I think the route forward is to have some logic that flattens headers in the header injection stuff, should be able to iterate through the headers, get the `Values()` (`[]string`), then do the `strings.Join` on that and finally `Set` the header to that value.
> 
> Do we want to support different separators for this or can we stick to comma separated in the first instance?

Let's just support comma and see if anyone complains. At that point we can embark on another flag to set the array header separator.

I think in my research on this, comma-separated seemed to be the resounding standard for header consumers.
JoelSpeed: @lilida Do you have any time over the next couple of weeks to get this rebased and finished up? This is one of three PRs we are looking at for v7 which we are hoping to release before the end of the month. If you don't have time, do you mind if one of our maintainers finishes it up? (With appropriate attribution of course)
lilida: > @lilida Do you have any time over the next couple of weeks to get this rebased and finished up? This is one of three PRs we are looking at for v7 which we are hoping to release before the end of the month. If you don't have time, do you mind if one of our maintainers finishes it up? (With appropriate attribution of course)

Sure! I will get it finished
lilida: @JoelSpeed  I updated the pull request after rebase. I only update the claim injector and let me know if this is what you want
JoelSpeed: Great, I think this looks good now, could you add a changelog entry and make sure your branch is up to date with master please? (Rebase or merge, up to you)

@NickMeves Just wanted to check you agree this is good?
NickMeves: > Please add a changelog entry and a note in the breaking changes section as well explaining the change in behaviour, and then I'm happy to get this merged! Thanks @lilida

@JoelSpeed Do we have any breaking changes that need documenting? I think all the array based headers this affects are related to `X-Forwarded-Groups` which are also new in v7.

I don't think this breaks existing functionality in v6.1.1, right?
JoelSpeed: > I don't think this breaks existing functionality in v6.1.1, right?

Good point... It's been quite some time since the last release that it's completely blurred in my head 😅

In which case, never mind, just a regular changelog will be fine
lilida: > Please add a changelog entry and a note in the breaking changes section as well explaining the change in behaviour, and then I'm happy to get this merged! Thanks @lilida

Done!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3f29a99d..8c0d9ff5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -67,6 +67,7 @@
 
 ## Changes since v6.1.1
 
+- [#799](https://github.com/oauth2-proxy/oauth2-proxy/pull/799) Use comma separated multiple values for header (@lilida)
 - [#903](https://github.com/oauth2-proxy/oauth2-proxy/pull/903) Add docs and generated reference for Alpha configuration (@JoelSpeed)
 - [#995](https://github.com/oauth2-proxy/oauth2-proxy/pull/995) Add Security Policy (@JoelSpeed)
 - [#964](https://github.com/oauth2-proxy/oauth2-proxy/pull/964) Require `--reverse-proxy` true to trust `X-Forwareded-*` type headers (@NickMeves)
diff --git a/pkg/middleware/headers.go b/pkg/middleware/headers.go
index b79b547b..0e47085a 100644
--- a/pkg/middleware/headers.go
+++ b/pkg/middleware/headers.go
@@ -3,6 +3,7 @@ package middleware
 import (
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/justinas/alice"
 	middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware"
@@ -40,6 +41,14 @@ func newStripHeaders(headers []options.Header) alice.Constructor {
 	}
 }
 
+func flattenHeaders(headers http.Header) {
+	for name, values := range headers {
+		if len(values) > 1 {
+			headers.Set(name, strings.Join(values, ","))
+		}
+	}
+}
+
 func stripHeaders(headers []string, next http.Handler) http.Handler {
 	return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 		for _, header := range headers {
@@ -67,6 +76,7 @@ func injectRequestHeaders(injector header.Injector, next http.Handler) http.Hand
 		// If scope is nil, this will panic.
 		// A scope should always be injected before this handler is called.
 		injector.Inject(req.Header, scope.Session)
+		flattenHeaders(req.Header)
 		next.ServeHTTP(rw, req)
 	})
 }
@@ -98,6 +108,7 @@ func injectResponseHeaders(injector header.Injector, next http.Handler) http.Han
 		// If scope is nil, this will panic.
 		// A scope should always be injected before this handler is called.
 		injector.Inject(rw.Header(), scope.Session)
+		flattenHeaders(req.Header)
 		next.ServeHTTP(rw, req)
 	})
 }


Test patch:
diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index 3366ef5f..41ac81bb 100644
--- a/oauthproxy_test.go
+++ b/oauthproxy_test.go
@@ -612,7 +612,7 @@ func TestPassGroupsHeadersWithGroups(t *testing.T) {
 	rw = httptest.NewRecorder()
 	proxy.ServeHTTP(rw, req)
 
-	assert.Equal(t, groups, req.Header["X-Forwarded-Groups"])
+	assert.Equal(t, []string{"a,b"}, req.Header["X-Forwarded-Groups"])
 }
 
 type PassAccessTokenTest struct {
diff --git a/pkg/middleware/headers_test.go b/pkg/middleware/headers_test.go
index a9c6d73e..4cfd0c1b 100644
--- a/pkg/middleware/headers_test.go
+++ b/pkg/middleware/headers_test.go
@@ -55,11 +55,11 @@ var _ = Describe("Headers Suite", func() {
 		Entry("with no configured headers", headersTableInput{
 			headers: []options.Header{},
 			initialHeaders: http.Header{
-				"foo": []string{"bar", "baz"},
+				"Foo": []string{"bar", "baz"},
 			},
 			session: &sessionsapi.SessionState{},
 			expectedHeaders: http.Header{
-				"foo": []string{"bar", "baz"},
+				"Foo": []string{"bar,baz"},
 			},
 			expectedErr: "",
 		}),
@@ -77,13 +77,13 @@ var _ = Describe("Headers Suite", func() {
 				},
 			},
 			initialHeaders: http.Header{
-				"foo": []string{"bar", "baz"},
+				"Foo": []string{"bar", "baz"},
 			},
 			session: &sessionsapi.SessionState{
 				IDToken: "IDToken-1234",
 			},
 			expectedHeaders: http.Header{
-				"foo":   []string{"bar", "baz"},
+				"Foo":   []string{"bar,baz"},
 				"Claim": []string{"IDToken-1234"},
 			},
 			expectedErr: "",
@@ -133,7 +133,7 @@ var _ = Describe("Headers Suite", func() {
 				IDToken: "IDToken-1234",
 			},
 			expectedHeaders: http.Header{
-				"Claim": []string{"bar", "baz", "IDToken-1234"},
+				"Claim": []string{"bar,baz,IDToken-1234"},
 			},
 			expectedErr: "",
 		}),
@@ -176,7 +176,7 @@ var _ = Describe("Headers Suite", func() {
 			},
 			session: nil,
 			expectedHeaders: http.Header{
-				"Claim": []string{"bar", "baz"},
+				"Claim": []string{"bar,baz"},
 			},
 			expectedErr: "",
 		}),