NOVER-5: add authz http mw #31
Conversation
common/claim/claim.go
Outdated
| if len(newBearerErrorList) > 0 || len(bearerErrorList) > 0 { | ||
| //fishy Should not have multiple newBearers | ||
| } |
go.mod
Outdated
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/pmezard/go-difflib v1.0.0 // indirect | ||
| github.com/stretchr/objx v0.1.1 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect |
There was a problem hiding this comment.
are these new packages required for http middleware ?
There was a problem hiding this comment.
out.txt
this is the graph file for the dependencies, as per the file, these might have been introduced due to go.uber.org/multierr or github.com/stretchr/testify. Middleware is using these libraries
There was a problem hiding this comment.
this is middleware, lets use standard go libraries.
There was a problem hiding this comment.
Hi @drewwells , I see that these two libraries are widely used in infoblox.
https://github.com/search?q=org%3AInfoblox-CTO%20github.com%2Fstretchr%2Ftestify&type=code
https://github.com/search?q=org%3AInfoblox-CTO%20go.uber.org%2Fmultierr&type=code
http_opa/authorizer/authorizer.go
Outdated
| defer func() { | ||
| span.SetStatus(trace.Status{ | ||
| Code: int32(grpc.Code(err)), | ||
| Message: grpc.ErrorDesc(err), | ||
| }) | ||
| span.End() | ||
| logger.WithFields(log.Fields{ | ||
| "opaResp": opaResp, | ||
| "elapsed": time.Since(now), | ||
| }).Debug("authorization_result") | ||
| }() | ||
| if err != nil { | ||
| return false, ctx, err | ||
| } |
There was a problem hiding this comment.
I would comment out these traces, and remove references to grpc. http_opa shouldn't require any version of grpc. Commenting out the traces will also remove the dependence on opentracing found in here. Maybe we should publish http_opa as a separate module too... so people aren't bound to the grpc version being imported in other parts of atlas-authz-middleware.
There was a problem hiding this comment.
Are we sure we want to remove the opentracing? Wasn't there a requirement to add them?
There was a problem hiding this comment.
@cbarik-infoblox Please remove grpc related dependencies from http middleware. While we would need tracing for http; add a TODO and work towards getting the initial functionality.
|
@cbarik-infoblox you mention "Most of the public methods/functions/interfaces have been copied". I'm not what you mean by this. My concern if there is ANY public method/function/interface/type/variable/constant/whatever that has been removed, it may break backwards compatibility. |
@rchowinfoblox when I said copied, I meant that the new package |
What is your suggestion, @rchowinfoblox ? Shouldn't the current test cases be capable of detecting those issues? |
I'm not sure our current test cases provide 100% coverage. Anyways I'm good with @cbarik-infoblox 's reply above. |
The
http_opapackage contains the new http middleware implementation for authorising http requests.Most of the public methods/functions/interfaces have been copied from the
grpc_opapackage to allow for backward compatibility.Some of the util functions have been moved to
commonfolder to be reused by grpc and http and yet a stub file has been created to avoid build issues for the existing consumers.For the opa client, it expects a jwt token in the
Authorizationheader, prefixed withBearerand an optionalX-Request-IDheader.Unauthorised requests will usually be rejected with a status of
401 Unauthorized