- Notifications
You must be signed in to change notification settings - Fork1.1k
chore: make authz recorder opt in#20310
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderd/httpmw/authz.go Outdated
| // Only enabled if the x-authz-checks header is set to a truthy value. | ||
| // The requester is expected to explicitly ask for this header to be set | ||
| // in the response. | ||
| ifenabled,_:=strconv.ParseBool(r.Header.Get("x-authz-checks"));enabled { |
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.
Should we make this more explicit, likex-enable-authz-checks?
x-authz-checks is what gets returned, so it could get confusing.
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.
Maybe it should bex-enable-authz-response? Something more explicit then?
I was thinking of just setting it to the same value returned. So it's a singular header to remember 🤷
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.
orx-record-authz-checks, since that's what the middleware is called
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.
changed to that
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.
thank you for acquiescing to my request, I will remember this come turkey day 🦃
| returnfunc(next http.Handler) http.Handler { | ||
| returnhttp.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) { | ||
| ifenabled,_:=strconv.ParseBool(r.Header.Get("x-record-authz-checks"));enabled||always { | ||
| r=r.WithContext(rbac.WithAuthzCheckRecorder(r.Context())) | ||
| } | ||
| next.ServeHTTP(rw,r) | ||
| }) | ||
| } |
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.
minor suggestion (that maybe would fix what the linter is complaining about?): ifalways == true you could return a version of the handler that doesn't have theif and just always adds the context
but this is also only enabled in dev builds so the performance gain wouldn't really matter. just a thought!
86f0f39 intomainUh oh!
There was an error while loading.Please reload this page.
The authz recorder is causing a lot of memory to be allocated, and is a memory leak for websocket connections.
This change makes it opt-in on a per request basis (ontop of
isDev). To get the authz headers, useCopy as cURLon chrome and append the headerx-authz-checks=true.This is preferred to disabling this imo.