feat: pass only essential and configured headers to authenticator #952

gen1us2k · 2022-04-11T17:04:57Z

BREAKING CHANGE: From now on, the bearer_token and cookie_session handlers pass only the needed header (Authorization, Cookie) to the check URL. To pass additional headers, use the forward_http_headers configuration key.

Closes #954
Closes ory/network#76

gen1us2k · 2022-04-11T17:10:08Z

I tried to use oathkeeper with Ory cloud for the local project. I set up everything and it worked except the header mutator.
After debugging, I found that the response from Ory Cloud comes with Accept-Encoding gzip header and all requests are gzipped by default.

Another solution to solve that case is to create an HTTP client with the DisableCompression option for http.Client.Transport

// DisableCompression, if true, prevents the Transport from
// requesting compression with an "Accept-Encoding: gzip"
// request header when the Request contains no existing
// Accept-Encoding value. If the Transport requests gzip on
// its own and gets a gzipped response, it's transparently
// decoded in the Response.Body. However, if the user
// explicitly requested gzip it is not automatically
// uncompressed.

but it doesn't handle deflate encoding.

codecov · 2022-04-11T17:10:28Z

Codecov Report

Merging #952 (fdd0ca1) into master (c4836f5) will increase coverage by 0.25%.
The diff coverage is 89.39%.

❗ Current head fdd0ca1 differs from pull request most recent head 028c091. Consider uploading reports for the commit 028c091 to get more accurate results

@@            Coverage Diff             @@
##           master     #952      +/-   ##
==========================================
+ Coverage   66.01%   66.26%   +0.25%     
==========================================
  Files         106      107       +1     
  Lines        4723     4764      +41     
==========================================
+ Hits         3118     3157      +39     
- Misses       1324     1325       +1     
- Partials      281      282       +1

Impacted Files	Coverage Δ
...ernal/httpclient/models/health_not_ready_status.go	`0.00% <ø> (ø)`
internal/httpclient/models/json_web_key.go	`0.00% <ø> (ø)`
internal/httpclient/models/rule.go	`0.00% <ø> (ø)`
internal/httpclient/models/rule_handler.go	`0.00% <ø> (ø)`
internal/httpclient/models/upstream.go	`0.00% <ø> (ø)`
pipeline/authn/authenticator.go	`100.00% <ø> (ø)`
pipeline/authn/authenticator_cookie_session.go	`83.01% <85.41%> (+2.53%)`	⬆️
pipeline/authn/authenticator_bearer_token.go	`73.77% <100.00%> (+8.55%)`	⬆️
x/header/header.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58c7fdf...028c091. Read the comment docs.

aeneasr · 2022-04-11T17:18:06Z

pipeline/authn/authenticator_cookie_session.go

-		body, err := ioutil.ReadAll(res.Body)
+		var reader io.Reader
+		switch res.Header.Get("Content-Encoding") {
+		case "gzip":


http.DefaultTransport automatically handles gzip decoding if the correct headers are set: https://stackoverflow.com/questions/13130341/reading-gzipped-http-response-in-go

So this should not be required!

the http.DefaultTransport does not use the DisableCompression property, and it's disabled by default. Also, the standard HTTP client does not support automatic decompression of deflate encoding.

I can give you a way how to reproduce the issue because I reproduced it by having this setup.

Ory proxy proxies traffic from Ory Cloud

Oathkeeper uses ory proxy endpoint and cookie_session authenticator

Oathkeeper as one upstream to proxy traffic to
I found that with mutator header X-User that comes from the cookie_authenticator is always empty and after a couple of hours of debugging I found that's a gzip issue.

I think that I'll write a couple of tests to prove the bug, and maybe I'll find a better solution, but also a couple of users had the same issue in our Slack community

However, the default client handles this by itself . I think I'll start with the tests first and try to reproduce it on a clean setup

Ok, that sounds great! We had issues with GZIP in the past in Ory Oathkeeper. I think it depends a bit on where in the pipeline the issue is. I think the proxy had issues with GZIP as it did not decode them automatically - here however I think we do?

Another problem that can happen is that we forward requests with all the headers that might include e.g. Content-Encoding: gzip but we actually do not use gzip in the body. So basically an incoming request to oathkeeper is:

GET /foo Content-Encoding: gzip ...gzip-body...

then Ory Oathkeeper reads that data and decompresses it. Next, it forwards the request to e.g. the session endpoint with all the HTTP headers:

GET /sessions/whoami Content-Encoding: gzip ...here the body is actually not gzip any more because it was decoded in oathkeeper already...

which then causes issues at the receiver.

I think that, in the end, we will need to refactor this so that we only send "wanted" headers to the upstream services instead of sending a catch-all. The same underlying issues is the reason for this: #954

I think the problem is the opposite way around... The behaviour of the Go HTTP client depends on whether the http.Request explicitly sets the Accept-Encoding: gzip header or not. If no header is set, the default http client automatically adds it before firing the request, and transparently handles decompression of the response for you (stripping out any Content-Encoding response headers).

If the request header is set though, it assumes you want to handle the decompression manually, and so returns the response compressed.

This can happen and cause problems if the service you're proxying serves compressed content in response to a request with Accept-Encoding: gzip (e.g. an nginx ingress with transparent compression enabled) - Oathkeeper would receive the response which would normally be decompressed, but because the inbound request had the Accept-Encoding header set, and we pass it on to the the Go http client, the Go client would not transparently decompress it.

The offending lines are below, the blind copy of all inbound request headers onto the outbound request on L157.

for k, v := range r.Header { req.Header[k] = v }

We should add a check for the Accept-Encoding header here, and not copy it, to avoid this

Right, that can also very well be the root cause! I think the effect and best resolution is to only pass essential and configured headers to the upstream and leave the user the ability to pass additional headers through the config too.

I added a draft implementation to pass only essential headers. It's still a work in progress and has a lack of tests for gzip/deflate encodings.

It would be a breaking change for the oathkeeper. I added "Authorization" and "Cookie" headers as default values to proxy_headers to provide backward compatibility and not break things.

aeneasr

Nice, this goes in a very good direction! It will also need a breaking change note as well as some documentation in ory/docs :)

.schema/config.schema.json

gen1us2k · 2022-05-16T10:20:27Z

@aeneasr I think this is ready for review.

Benehiko

I think this looks quite good :)
Just a few questions

Benehiko · 2022-05-20T13:37:46Z

pipeline/authn/authenticator_bearer_token_test.go

@@ -96,7 +96,7 @@ func TestAuthenticatorBearerToken(t *testing.T) {
 					w.WriteHeader(200)
 					w.Write([]byte(`{"sub": "123"}`))
 				},
-				config:    []byte(`{"preserve_query": true}`),
+				config:    []byte(`{"preserve_query": true, "forward_http_headers": ["Authorization"]}`),


Just a question, i see here we are adding forward_http_headers for Authorization in many of these tests. Aren't they included automatically now?

Yep. I wrote tests first and later I decided to implement some backward compatibility. I can remove it from the tests if you want.

yeah i think it makes sense to remove it. otherwise it will just confuse in future.

We should just add a test though to make sure these headers are always present in these authentictors (explicitly even though the other tests infer it).

Fixed it :)

Benehiko · 2022-05-20T13:40:13Z

pipeline/authn/authenticator_bearer_token.go

+	if len(c.ForwardHTTPHeaders) == 0 {
+		c.ForwardHTTPHeaders = []string{header.Authorization, header.Cookie}
+	}


Just to make sure I'm understanding this correctly, if we have no forward_http_headers we would always add Authorization and Cookie. But shouldn't that be the case for bearer and cookie authenticators anyway, even if we add more forward_http_headers?

referring to this discussion #952 (comment)

Yeah. I found the issue here. Right now oathkeeper adds Authorization and Cookie headers only when we have no forward_http_headers configured. However, I think that the oathkeeper should have them in any case because they're essential.

The issue may occur when someone configures something like

forward_http_headers: - Accept - X-User

In that case, the authentication flow breaks because the oathkeeper does not proxies essential headers.

I think that we can remove if len(c.ForwardHTTPHeaders) == 0 and append []string{header.Authorization, header.Cookie} anyway. WDYT?

i agree since it is required for bearer or cookie authenticator to work properly

Fixed it too

pipeline/authn/authenticator_cookie_session.go

Benehiko

LGTM :)

Co-authored-by: hackerman <[email protected]>

gen1us2k · 2022-06-09T08:56:50Z

@aeneasr It's ready for another round of review.

Benehiko

A lot better :)

Just a few more questions and minor changes

pipeline/authn/authenticator_bearer_token_test.go

pipeline/authn/authenticator_cookie_session.go

pipeline/authn/authenticator_bearer_token.go

Benehiko

nice :)

aeneasr

Thank you, we're getting there :) Can you please take a look at the comments? Additionally, can you please add two tests:

A test for bearer token where a custom header is forwarded
A test for session cookie where a custom header is forwarded

Regarding the CI format failure, @kevgo will take a look

pipeline/authn/authenticator_cookie_session_test.go

x/header/header.go

aeneasr

Great job :)

fix: added gzip support for cookie_session authenticator

f0cb78f

gen1us2k requested a review from aeneasr as a code owner April 11, 2022 17:04

aeneasr requested changes Apr 11, 2022

View reviewed changes

aeneasr mentioned this pull request Apr 17, 2022

fix: response handling for session store (bearer + cookie) #916

Open

fix: added proxy_headers config variable for authenticators

6999a5f

gen1us2k changed the title ~~fix: added gzip support for cookie_session authenticator~~ feat: pass only essential and configured headers to authenticator May 9, 2022

gen1us2k added 2 commits May 9, 2022 21:35

fix: better design for forwardRequestToSessionStore

5b94457

fix: better solution to convert array to map

50f01b1

aeneasr reviewed May 12, 2022

View reviewed changes

.schema/config.schema.json Outdated Show resolved Hide resolved

gen1us2k added 6 commits May 14, 2022 00:48

renamed field

85d391d

added header package to improve readability

0cceab9

fixed build

6cbc29a

updated schema

f4b9936

removed old test

4f5d8af

added tests

5c35b6d

gen1us2k requested a review from aeneasr May 17, 2022 13:54

Benehiko suggested changes May 20, 2022

View reviewed changes

gen1us2k added 2 commits May 23, 2022 14:03

better backwards compatibility

27e785a

Merge branch 'master' into gzip_support

e456520

aeneasr force-pushed the gzip_support branch from e456520 to 993eeff Compare May 23, 2022 20:30

Benehiko previously approved these changes May 25, 2022

View reviewed changes

gen1us2k added 6 commits May 26, 2022 13:22

fix: added gzip support for cookie_session authenticator

0a9c066

fix: added proxy_headers config variable for authenticators

9906ce4

fix: better design for forwardRequestToSessionStore

198902c

fix: better solution to convert array to map

134c9dc

renamed field

eea93ee

added header package to improve readability

3d4333d

aeneasr dismissed Benehiko’s stale review via aeba3ce May 26, 2022 15:04

gen1us2k and others added 5 commits May 30, 2022 14:46

Update pipeline/authn/authenticator_cookie_session.go

b671e8b

Co-authored-by: hackerman <[email protected]>

Merge

f7f6da9

Small refactoring

0ca5c51

fix: Don't use maps anymore

339e8e7

Merge branch 'master' into gzip_support

affc702

gen1us2k mentioned this pull request Jun 9, 2022

feat: Added documentation for forward_http_headers feature ory/docs#858

Merged

6 tasks

gen1us2k requested a review from aeneasr June 9, 2022 09:26

Benehiko suggested changes Jun 10, 2022

View reviewed changes

gen1us2k added 2 commits June 10, 2022 15:09

Small fixes

0bbc5fd

fixed test

da038f8

Benehiko previously approved these changes Jun 10, 2022

View reviewed changes

gen1us2k and others added 2 commits June 21, 2022 15:22

Merge branch 'master' into gzip_support

690d766

chore: code review

ba19be3

aeneasr dismissed Benehiko’s stale review via ba19be3 June 23, 2022 07:14

aeneasr requested changes Jun 23, 2022

View reviewed changes

pipeline/authn/authenticator_cookie_session_test.go Show resolved Hide resolved

x/header/header.go Show resolved Hide resolved

gen1us2k added 10 commits June 23, 2022 15:54

Remove header constants

afaf5af

fixed tests. Check only canonical header names

fe4bd05

Merge branch 'master' into gzip_support

5e16805

Drop header values

046422f

Added tests for the cookie session authenticator

f2d31ea

Added test for token authenticator

a19b283

run prettifier

c66ba2a

Fixed linter

ab72681

Drop dead headers

4b735e3

not canonical header

028c091

aeneasr approved these changes Jun 23, 2022

View reviewed changes

aeneasr merged commit e5e4de4 into ory:master Jun 23, 2022

gen1us2k deleted the gzip_support branch June 24, 2022 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pass only essential and configured headers to authenticator #952

feat: pass only essential and configured headers to authenticator #952

gen1us2k commented Apr 11, 2022 •

edited by aeneasr

Loading

gen1us2k commented Apr 11, 2022

codecov bot commented Apr 11, 2022 •

edited

Loading

aeneasr Apr 11, 2022

gen1us2k Apr 13, 2022

gen1us2k Apr 13, 2022

aeneasr Apr 14, 2022

eh-steve Apr 28, 2022 •

edited

Loading

aeneasr May 4, 2022

gen1us2k May 9, 2022

gen1us2k May 9, 2022

aeneasr left a comment

gen1us2k commented May 16, 2022

Benehiko left a comment

Benehiko May 20, 2022

gen1us2k May 20, 2022

Benehiko May 20, 2022

gen1us2k May 23, 2022

Benehiko May 20, 2022

gen1us2k May 20, 2022

gen1us2k May 20, 2022

Benehiko May 20, 2022

gen1us2k May 23, 2022

Benehiko left a comment

gen1us2k commented Jun 9, 2022

Benehiko left a comment

Benehiko left a comment

aeneasr left a comment

aeneasr left a comment

feat: pass only essential and configured headers to authenticator #952

feat: pass only essential and configured headers to authenticator #952

Conversation

gen1us2k commented Apr 11, 2022 • edited by aeneasr Loading

gen1us2k commented Apr 11, 2022

codecov bot commented Apr 11, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eh-steve Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

gen1us2k commented May 16, 2022

Benehiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

gen1us2k commented Jun 9, 2022

Benehiko left a comment

Choose a reason for hiding this comment

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

gen1us2k commented Apr 11, 2022 •

edited by aeneasr

Loading

codecov bot commented Apr 11, 2022 •

edited

Loading

eh-steve Apr 28, 2022 •

edited

Loading