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

Use HTTP 303 for redirects, instead HTTP 307 #628

Merged
merged 1 commit into from
Jun 4, 2020

Conversation

abstractj
Copy link

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

@abstractj abstractj requested a review from stianst June 2, 2020 00:14
@abstractj abstractj self-assigned this Jun 2, 2020
@abstractj abstractj changed the title Based on the security best practices we should not use HTTP 307 for redirects, instead HTTP 303 should be used Use HTTP 307 for redirects, instead HTTP 303 Jun 2, 2020
@abstractj abstractj added this to the 1.0.0-alpha.1 milestone Jun 2, 2020
@ackerleytng
Copy link

ackerleytng commented Jun 2, 2020

Thanks @abstractj !

Didn't know about the RFC you quoted!

Strictly speaking, I think section 3.9 refers to the redirect from the authorization server back to the client (whose role louketo takes, in this case), so it's slightly different from the changes in the code in this PR, which is about redirects from the client to the authorization server in the earlier phase of the auth code flow prior to authentication with the authorization server.

I think the concerns still apply, though - we shouldn't be leaking information posted to the client to the authorization server either.

Also, I was confused by the header of this PR, I think you meant "Use HTTP 303 for redirects, instead of HTTP 307".

Thanks for your work on louketo! It's an awesomely useful proxy.

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 changed the title Use HTTP 307 for redirects, instead HTTP 303 Use HTTP 303 for redirects, instead HTTP 307 Jun 2, 2020
@abstractj
Copy link
Author

Hi @ackerleytng thanks for reviewing it. Indeed the title was incorrect and should be fixed now. Reading your last comments, what exactly is still being leaked now that we are using HTTP 303?

Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but would probably be good to get someone with more knowledge on Go and the code base to review as well

@ackerleytng
Copy link

Thanks again @abstractj ! I think there's some misunderstanding there. I meant that the concerns expressed in the RFC are for redirection from authorisation server back to client, but the same concerns apply for us, even though we're redirecting from client to authorization server.

Using a 303 should address those concerns! Thanks for making this change.

@abstractj abstractj merged commit 33c23da into louketo:master Jun 4, 2020
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 this pull request may close these issues.

Why does louketo send User Agents to the authorization server with 307?
3 participants