Authorizer "remote" throws exception "invalid Read on closed Body" if request body is present in request

[denysandriyanov]

Preflight checklist

• I could not find a solution in the existing issues, docs, nor discussions.
• I agree to follow this project's Code of Conduct.
• I have read and am following this repository's Contribution Guidelines.
• I have joined the Ory Community Slack.
• I am signed up to the Ory Security Patch Newsletter.

Ory Network Project

No response

Describe the bug

I am using the authorizer "remote".
When sending a request with some JSON payload and the remote authorizer returns status code 200 it is expected that Oathkeeper will allow the request and forward it to upstream service, while the actual result is I am getting 502 status code. In oathkeeper logs, i see the exception "invalid Read on closed Body".

The authorizer implementation that is used during this flow accepts POST request with a body and returns status code 200 or 403 based on body content.
Authorizer do not return any body in response, just a status code.

Reproducing the bug

1. Configure oathkeeper to use authorizer "remote"
2. Configure a dummy API service (mocking behavior of authorizer) that accepts POST request with some json payload as body and returns status code 200
3. Create oathkeeper rule to protect some api endpoint and use the configured authorizer
4. Make a call to the endpoint configured in step 3

Expected result:
Considering that our dummy authorizer returned status code 200 oathkeeper is expected to allow the request to upstream service

Actual result:
Exception is happened in oathkeeper indicating that "invalid Read on closed Body".

Relevant log output

time=2023-09-09T17:13:19Z level=warning msg=Access request denied because roundtrip failed audience=application error=map[message:readfrom tcp 10.81.28.46:51888->10.0.214.228:80: http: invalid Read on closed Body stack_trace:
github.com/ory/oathkeeper/proxy.(*Proxy).RoundTrip
	/project/proxy/proxy.go:80
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Transport).RoundTrip
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.36.4/transport.go:116
net/http/httputil.(*ReverseProxy).ServeHTTP
	/usr/local/go/src/net/http/httputil/reverseproxy.go:473
github.com/urfave/negroni.Wrap.func1
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:46
github.com/urfave/negroni.HandlerFunc.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:29
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2122
github.com/rs/cors.(*Cors).Handler.func1
	/go/pkg/mod/github.com/rs/cors@v1.8.2/cors.go:231
net/http.HandlerFunc.ServeHTTP
	/usr/local/go/src/net/http/server.go:2122
github.com/ory/x/corsx.ContextualizedMiddleware.func1
	/go/pkg/mod/github.com/ory/x@v0.0.565/corsx/middleware.go:30
github.com/urfave/negroni.HandlerFunc.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:29
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38
github.com/ory/x/reqlog.(*Middleware).ServeHTTP
	/go/pkg/mod/github.com/ory/x@v0.0.565/reqlog/middleware.go:142
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38
github.com/ory/oathkeeper/metrics.(*Middleware).ServeHTTP
	/project/metrics/middleware.go:103
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38
github.com/ory/x/metricsx.(*Service).ServeHTTP
	/go/pkg/mod/github.com/ory/x@v0.0.565/metricsx/middleware.go:272
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:38
github.com/urfave/negroni.(*Negroni).ServeHTTP
	/go/pkg/mod/github.com/urfave/negroni@v1.0.0/negroni.go:96
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Handler).ServeHTTP
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.36.4/handler.go:204
net/http.serverHandler.ServeHTTP
	/usr/local/go/src/net/http/server.go:2936
net/http.(*conn).serve
	/usr/local/go/src/net/http/server.go:1995
runtime.goexit
	/usr/local/go/src/runtime/asm_amd64.s:1598] granted=false http_host=proxy-upstream8128.us-east-1.cloud-int.avid.com http_method=POST http_url=http://echoserver/test http_user_agent=insomnia/2022.7.0 service_name=ORY Oathkeeper service_version=v0.40.6 subject=d2b3aa1a-1b5c-54ad-b088-51a5459105b2
time=2023-09-09T17:13:19Z level=error msg=http: gateway error audience=application error=map[message:readfrom tcp 10.81.28.46:51888->10.0.214.228:80: http: invalid Read on closed Body stack_trace:stack trace could not be recovered from error type *net.OpError] service_name=ORY Oathkeeper service_version=v0.40.6

Relevant configuration

authorizers:
        remote:
          enabled: true
          config:
            headers:
              X-Subject: '{{ print .Subject }}'
            remote: http://path-to-authorier/authorize

Version

v0.40.6

On which operating system are you observing this issue?

None

In which environment are you deploying?

Kubernetes with Helm

Additional Context

My assumption is that once the original request body is piped into the read end of the pipe and used as the request body for the new HTTP POST request to the remote authorizer service, it cannot be sent to the upstream service even if the authorizer returns a 200 status code to allow the request. I am not a Go programer but i guess you need to modify the code to save a copy of the original request body before it is piped into the read end of the pipe and then send that saved copy to the target service if authorization succeeds.

[joshm91]

I think that error is implying that your upstream echo server closed the body.

Is there any chance you have some sort of NetworkPolicy in your cluster that's rejecting the connection between pods or something? Does the request work correctly if you remove the authoriser?

[denysandriyanov]

Hi Josh. No, echo server (is just a dummy example service for test) could not read the body as it was closed before.
If the authorizer is removed request works correctly.
No any policies applied that might create such problems

[joshm91]

Okay, yeah, I can confirm I get the same issue. It looks like it was introduced in v0.40.2 and is something to do with the tracing implementation.

If these lines are commented out then it works correctly.

[denysandriyanov]

Do you think someone from Ory can fix that? I can assume the parts you said to comment our are also required in order to trace, so just disabling them is not an option?

[joshm91]

Sorry, I only disabled the tracing lines to test if that was the issue; it's not a suitable fix.

I spent some time yesterday trying to find the correct fix but couldn't. You'll likely need to wait for someone from Ory who knows the codebase better than me, unfortunately.

[denysandriyanov]

Anyone from Ory, could you please take a look here, this issue is a show stopper for remote authorizer

[denysandriyanov]

Up

[timthornton-avid]

I have looked into the root cause for Denys and here is a summary and fix:
../pipeline/authz/remote.go:

 (a *AuthorizerRemote) Authorize(r *http.Request, session *authn.AuthenticationSession, config json.RawMessage, rl pipeline.Rule) (err error) {
ctx, span := a.tracer.Start(r.Context(), "pipeline.authz.AuthorizerRemote.Authorize")
defer otelx.End(span, &err)

// Original code
//r = r.WithContext(ctx)
//
// Problem with the original code is when r.WithContext is called, it creates a new request with the same body as the original request with a modified context.
// r is a pointer and when its assigned to the new request message it scope becomes local to this function.
// The calling function expects the original request message to be modified as the message is processed by the pipeline.
// When message processing is complete the calling function does not have access to the modified http.Request message (r)
//
// There are several possible solutions:
// 1. Create a function in http.request to .SetContext(ctx) which replaces context but does not create new http.Request instance
// 2. Modify the Authorize function to return a new http.Request used by this function during message processing
// 3. Assign the new request to the original request pointer
//
// The change below implements option 3

*r = *(r.WithContext(ctx))

I've tested fix and run all unit tests with success.
I will look into submitting changes into repo, haven't done before.

[denysandriyanov]

I did CIT for this, and it passed

[denysandriyanov]

UP

[denysandriyanov]

Any updates?

[ouranders]

UP
Is it the failing "Docker Image Scanner" step the only thing blocking the pull request for this issue to be merged?
#1138

[ralfkrakowski]

UP, we would also like to have this fix please.