-
Notifications
You must be signed in to change notification settings - Fork 618
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.
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
Add external authorization via http request #830
base: master
Are you sure you want to change the base?
Conversation
This is similar to Envoys external authorization[0], and can be used for example in combination with Ory Oathkeeper. [0] https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto
a55de9d
to
04f958c
Compare
👍 for this PR! Maybe a future refinement would be to use service-discovery to discover the endpoint by service name? But I'd take this as is now to get the ball rolling, have the same use-case in mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this will be a great feature, just have a few tweaks before it can be merged. Sorry this has taken so long to get back to.
} | ||
|
||
func (b *external) Authorized(request *http.Request, response http.ResponseWriter) AuthDecision { | ||
client := &http.Client{CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This client should specify a timeout at least, and possibly a transport too depending on whether tls configurations would need to be overridden. Also, it would be much better to store this client inside the external struct instead of recreating it each time.
log.Println("[ERROR] External request error:", err.Error()) | ||
return unauthorized() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resp.Body is never drained or closed. if err is nil, body needs to be dealt with.
|
||
if strings.HasSuffix(a.External.Endpoint, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to the reasoning for this restriction?
This is similar to Envoys external authorization, and can be used for example in combination with Ory Oathkeeper.