zalando/skipper

Handle `302 Found` Redirects when using the webhook filter

FergusInLondon opened this issue · 2 comments

FergusInLondon commented

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

szuecs commented

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 commented

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