feat: Add support for X-Forwarded-Proto header

[amoshydra]

Related issue

Addresses #153
Context: #153 (comment)

Proposed changes

Check X-Forwarded-Proto header inside proxy.(*Proxy) EnrichRequestedURL using the same logic in #638

Checklist

• I have read the contributing guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further comments

[aeneasr]

Thank you, this looks great!

Could you please add a test or two to ensure this doesn't regress in future changes?

[aeneasr]

Are you still up for the changes? :) If you need any help, let us know!

[amoshydra]

Hi @aeneasr I am very sorry for the lack of update. I hadn't been spending time on using oathkeeper in the past weeks.

---

Could you please add a test or two to ensure this doesn't regress in future changes?

I have originally assumed that I could add additional test onto test/reload/run.sh script. But I later realise it is meant for testing the reload functionality of oathkeeper.

Where should I add the test to? Is it appropriate to create a new folder for this? For example:

test/forwarded-for-header/run.sh

[aeneasr]

No problem! The best place to test this is probably here: https://github.com/ory/oathkeeper/blob/0a089d9f513160c9792027bd2057a2180a1be3a6/proxy/proxy_test.go#L47

[amoshydra]

Hi @aeneasr, I have added some new tests that add X-Forwarded-Proto header. The tests are added in test/forwarded-header/run.sh

---

Before commit: 62e657e

PASS: 4
- Executing request against a HTTP rule -> 200
- Executing request against a HTTP rule with forwrded proto HTTP -> 200
- Executing request against a HTTPS rule -> 404
- Executing request against a HTTPS rule with forwarded proto HTTP -> 404
FAILED: 2
- Executing request against a HTTP rule with forwrded proto HTTPS -> 404
- Executing request against a HTTPS rule with forwarded proto HTTPS -> 200

After commit: 62e657e

PASS: 6
- Executing request against a HTTP rule -> 200
- Executing request against a HTTP rule with forwrded proto HTTP -> 200
- Executing request against a HTTP rule with forwrded proto HTTPS -> 404
- Executing request against a HTTPS rule -> 404
- Executing request against a HTTPS rule with forwarded proto HTTP -> 404
- Executing request against a HTTPS rule with forwarded proto HTTPS -> 200

---

However,

(1)
in all of these tests, r.TLS is always nil. I am not sure how to setup oathkeeper in such a way that r.TLS is not nil.

Reference

func EnrichRequestedURL(r *http.Request) {
	r.URL.Scheme = "http"
	r.URL.Host = r.Host
	if r.TLS != nil || strings.EqualFold(r.Header.Get("X-Forwarded-Proto"), "https") {
        // ^^^^^
        //
        // In the tests, r.TLS is always `nil`.

		r.URL.Scheme = "https"
	}
}

[aeneasr]

Cool!

Yeah, to enable HTTPS you would need to use httptest.NewTLSServer similar to here. This will set up a TLS server with a self-signed certificate. To avoid errors, you can use the Client method.

If you want, you can set up a minimal new test case just for that function with both TLS enabled and disabled!

[aeneasr]

Are you still up for the changes? 🧐 If you need any help, let us know!

I'll convert this to a draft in the meanwhile. When it's ready for review again, please remove the draft indicator! 👀

[vallahaye]

Hi @aeneasr, do you really think testing is necessary for such a simple feature?
It's literally a one-line change that seems to have little impact on the behavior of the proxy.
Also, the same functionality was added to the decision API and merged without adding additional tests (#638).
Let me know what you think.

[aeneasr]

Tests are important to open source projects because the allow reviewers to better judge if the changes are doing what was intended. They also ensure that your changes never regress / break when someone else works on code that is related to this.

If you need help with the tests feel free to push your work in progress and ping myself or another maintainer 🙋

[dwmunster]

Does the ./test/forwarded-header/run.sh file not sufficiently test the intended behavior? After all, the context of this request is when Oathkeeper is behind an SSL-terminating proxy (e.g. the request to Oathkeeper is HTTP but represents a forwarded HTTPS connection), which is exactly the behavior that file tests.

[aeneasr]

Added the test I was looking for: 67221fb

Will merge when tests pass :)

[codecov]

Codecov Report

Merging #665 (cb3cded) into master (057293f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   62.47%   62.51%   +0.04%     
==========================================
  Files         102      102              
  Lines        4813     4813              
==========================================
+ Hits         3007     3009       +2     
+ Misses       1531     1530       -1     
+ Partials      275      274       -1     

Impacted Files	Coverage Δ
proxy/proxy.go	72.34% <100.00%> (+2.12%)	⬆️

---

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 c89737b...cb3cded. Read the comment docs.