Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Why does louketo send User Agents to the authorization server with 307? #627

Closed
ackerleytng opened this issue Jun 1, 2020 · 2 comments · Fixed by #628
Closed

Why does louketo send User Agents to the authorization server with 307? #627

ackerleytng opened this issue Jun 1, 2020 · 2 comments · Fixed by #628
Milestone

Comments

@ackerleytng
Copy link

ackerleytng commented Jun 1, 2020

Summary

Was testing a protected endpoint using POST, and I was redirected to /oauth/authorize on louketo with a 307, and then subsequently redirected to my authorization server (Keycloak) with a 307.

Environment

Looking at

func (r *oauthProxy) oauthAuthorizationHandler(w http.ResponseWriter, req *http.Request) {

(Not environment specific)

Expected Results

I thought we should be sending User-Agents to the authorization server with a 303, as 303 hints the User-Agent to use a GET (https://tools.ietf.org/html/rfc2616#section-10.3.4)

I think the method itself isn't so much of an issue, but sending a 307 suggests that the User-Agent can repeat the original request on the new URI, which might leak something to the authorization provider.

Actual Results

Louketo tries to send the User-Agent to the authorization server with a 307.

Steps to reproduce

Set up louketo to proxy a protected endpoint and try POSTing to an endpoint requiring authentication

Additional Information

Related question: In redirectToAuthorization, why is the default 307? I was thinking that there isn't really a reason to have 307, since 307 would encourage the User-Agent to repeat the original request on the new URI, with the original method and data, etc, which probably isn't necessary since louketo's /oauth/authorize endpoint isn't going to make use of all that.

abstractj pushed a commit to abstractj/louketo-proxy that referenced this issue Jun 2, 2020
…edirects, instead HTTP 303 should be used.

The option `InvalidAuthRedirectsWith303` was removed, as does not make
sense to have it anymore.

More details:
https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9

Resolves louketo#627
@abstractj
Copy link

@ackerleytng thanks for reporting this, it should be fixed here #628. Please review, whenever you have some time.

abstractj pushed a commit to abstractj/louketo-proxy that referenced this issue Jun 2, 2020
Based on the security best practices we should not use HTTP 307 for
redirects, instead HTTP 303 should be used.

The option `InvalidAuthRedirectsWith303` was removed, as does not make
sense to have it anymore.

More details:
https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9

Resolves louketo#627
@abstractj abstractj added this to the 1.0.0-alpha.1 milestone Jun 2, 2020
@ackerleytng
Copy link
Author

Thanks @abstractj , it looks great! I've also left a comment on your PR.

abstractj pushed a commit to abstractj/louketo-proxy that referenced this issue Jun 2, 2020
Based on the security best practices we should not use HTTP 307 for
redirects, instead HTTP 303 should be used.

The option `InvalidAuthRedirectsWith303` was removed, as does not make
sense to have it anymore.

More details:
https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9

Resolves louketo#627
abstractj pushed a commit that referenced this issue Jun 4, 2020
Based on the security best practices we should not use HTTP 307 for
redirects, instead HTTP 303 should be used.

The option `InvalidAuthRedirectsWith303` was removed, as does not make
sense to have it anymore.

More details:
https://tools.ietf.org/id/draft-ietf-oauth-security-topics-08.html#rfc.section.3.9

Resolves #627
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants