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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pingiun
Copy link

@pingiun pingiun commented May 15, 2021

This is similar to Envoys external authorization, and can be used for example in combination with Ory Oathkeeper.

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
@nathanejohnson nathanejohnson self-requested a review July 15, 2022 01:14
@nathanejohnson nathanejohnson added this to the 1.6.2 milestone Jul 15, 2022
@wfaler
Copy link

wfaler commented Aug 30, 2022

👍 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.

@nathanejohnson nathanejohnson modified the milestones: 1.6.2, 1.6.3 Sep 13, 2022
Copy link
Member

@nathanejohnson nathanejohnson left a 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 }}
Copy link
Member

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()
}

Copy link
Member

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, "/") {
Copy link
Member

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?

@nathanejohnson nathanejohnson removed this from the 1.6.3 milestone Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants