Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle 302 Found Redirects when using the webhook filter #3130

Open
FergusInLondon opened this issue Jun 27, 2024 · 4 comments
Open

Handle 302 Found Redirects when using the webhook filter #3130

FergusInLondon opened this issue Jun 27, 2024 · 4 comments
Assignees
@FergusInLondon

Comments

@FergusInLondon
Copy link

FergusInLondon commented Jun 27, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.
The Skipper webhook filter allows requests to be filtered based on the status code received from an authorisation/authentication endpoint. By default, Skipper will return an empty response with a status code of 401 or 403 if a request is rejected.

This behaviour is well-suited to API traffic, but doesn't work for frontend traffic where the user agent should be redirected upon rejection.

Describe the solution you would like
When an authentication endpoint returns a HTTP redirect - i.e. a HTTP 302 Found - then Skipper should return that redirect to the user agent.

These changes would be limited to:

  1. Introducing a new conditional to catch responses with a status code of 302 - in webhook.go#L124-L128.
  2. Introducing a new function - redirect - which is the equivalent of the existing reject function - in auth.go#L136-L142 - but copies the Location header from the authentication endpoint response.

This should not be a breaking change as currently 302 Found has no specific meaning to the webhook filter: it's caught by the default behaviour which returns 401 Unauthorized.

Would you like to work on it?
Yes, I have a Pull Request prepared - #3131
FergusInLondon added a commit to FergusInLondon/skipper that referenced this issue Jun 27, 2024
@FergusInLondon
webhook filter: allow client redirects via 302 responses (zalando#3130)
fe41415 
This commit changes the behaviour of the webhook filter when a 302 Found
response is recieved from the AuthN/AuthZ endpoint. As a result, it allows
front-end facing (i.e. non-API) traffic to be filtered via the webhook.

Documentation updates and increased test coverage is included.

Incidental: Prevent the webhook client from following redirects from the
AuthN/AuthZ endpoint: during testing I realised that the default `net/http`
behaviour was in use - i.e. redirects were followed.

Signed-off-by: Fergus Morrow <[email protected]>
@FergusInLondon FergusInLondon mentioned this issue Jun 27, 2024
Open
@szuecs
Copy link
Member

szuecs commented Jul 1, 2024

thanks, reviewed the PR, feature wise I think it's fine but implementation needs to be changed as I wrote in PR comment. Discussion in PR.

@AlexanderYastrebov
Copy link
Member

This behaviour is well-suited to API traffic, but doesn't work for frontend traffic where the user agent should be redirected upon rejection.

I can see the point, e.g. if webhook authenticates Cookie and issues redirect for new visitors.

From #3131

Incidental: Prevent the webhook client from following redirects from the AuthN/AuthZ endpoint: during testing I realised that the default net/http behaviour was in use - i.e. redirects were followed. Is this an intended behaviour of the filter: it's not documented, and it seems potentially risky?

Its not risky assuming operator trusts the webhook server. Following redirects may be used e.g. for migrating webhook url to another domain.

I am not sure I like the idea of forwarding webhook redirect to the client on the other hand there is a usecase.

Maybe we can add a third webhook filter parameter to control redirect behaviour. By default and for backward compatibility (important), when parameter is omitted, webhook client follows redirects just like now. When parameter is specified then webhook client does not follow redirects (i.e. 301, 302, 303 and 307, 307 like in http.Client redirectBehavior) and returns redirect to the client:

* -> webhook("https://auth.test", "X-Foo,X-Bar", "allow-redirect")

Also it makes sense to return whole webhook redirect response to the client - this way webhook server can send additional data like:

HTTP/1.1 301 
Location: https://www.example.test/
Content-type: text/html; charset=UTF-8

<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>301 Moved</TITLE></HEAD><BODY>
<H1>301 Moved</H1>
The document has moved
<A HREF="https://www.example.test/">here</A>.
</BODY></HTML>

or just

HTTP/1.1 308 Permanent Redirect
Location: https://www.example.test/
Content-Length: 0

@AlexanderYastrebov AlexanderYastrebov mentioned this issue Dec 5, 2024
Open
@AlexanderYastrebov
Copy link
Member

Incidental: Prevent the webhook client from following redirects from the AuthN/AuthZ endpoint: during testing I realised that the default net/http behaviour was in use - i.e. redirects were followed. Is this an intended behaviour of the filter: it's not documented, and it seems potentially risky?

Its not risky assuming operator trusts the webhook server. Following redirects may be used e.g. for migrating webhook url to another domain.

Here is an example #3329

@szuecs
Copy link
Member

szuecs commented Dec 9, 2024

This behaviour is well-suited to API traffic, but doesn't work for frontend traffic where the user agent should be redirected upon rejection.

I can see the point, e.g. if webhook authenticates Cookie and issues redirect for new visitors.

From #3131

Incidental: Prevent the webhook client from following redirects from the AuthN/AuthZ endpoint: during testing I realised that the default net/http behaviour was in use - i.e. redirects were followed. Is this an intended behaviour of the filter: it's not documented, and it seems potentially risky?

Its not risky assuming operator trusts the webhook server. Following redirects may be used e.g. for migrating webhook url to another domain.

I am not sure I like the idea of forwarding webhook redirect to the client on the other hand there is a usecase.

Maybe we can add a third webhook filter parameter to control redirect behaviour. By default and for backward compatibility (important), when parameter is omitted, webhook client follows redirects just like now. When parameter is specified then webhook client does not follow redirects (i.e. 301, 302, 303 and 307, 307 like in http.Client redirectBehavior) and returns redirect to the client:


* -> webhook("https://auth.test", "X-Foo,X-Bar", "allow-redirect")

Also it makes sense to return whole webhook redirect response to the client - this way webhook server can send additional data like:

HTTP/1.1 301 

Location: https://www.example.test/

Content-type: text/html; charset=UTF-8



<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">

<TITLE>301 Moved</TITLE></HEAD><BODY>

<H1>301 Moved</H1>

The document has moved

<A HREF="https://www.example.test/">here</A>.

</BODY></HTML>

or just

HTTP/1.1 308 Permanent Redirect

Location: https://www.example.test/

Content-Length: 0

I like your idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@FergusInLondon FergusInLondon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@szuecs @AlexanderYastrebov @FergusInLondon