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

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

Closed
5 tasks done
denysandriyanov opened this issue Sep 9, 2023 · 14 comments · Fixed by #1185
Closed
5 tasks done
Labels
bug Something is not working.

Comments

@denysandriyanov
Copy link

denysandriyanov commented Sep 9, 2023

Preflight checklist

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/[email protected]/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/[email protected]/negroni.go:46
github.com/urfave/negroni.HandlerFunc.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:29
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/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/[email protected]/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/[email protected]/corsx/middleware.go:30
github.com/urfave/negroni.HandlerFunc.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:29
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:38
github.com/ory/x/reqlog.(*Middleware).ServeHTTP
	/go/pkg/mod/github.com/ory/[email protected]/reqlog/middleware.go:142
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/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/[email protected]/negroni.go:38
github.com/ory/x/metricsx.(*Service).ServeHTTP
	/go/pkg/mod/github.com/ory/[email protected]/metricsx/middleware.go:272
github.com/urfave/negroni.middleware.ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:38
github.com/urfave/negroni.(*Negroni).ServeHTTP
	/go/pkg/mod/github.com/urfave/[email protected]/negroni.go:96
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Handler).ServeHTTP
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/[email protected]/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.

@denysandriyanov denysandriyanov added the bug Something is not working. label Sep 9, 2023
@denysandriyanov denysandriyanov changed the title Authorizer "remote" throws exception if request body is present in request Authorizer "remote" throws exception "invalid Read on closed Body" if request body is present in request Sep 9, 2023
@joshm91
Copy link
Contributor

joshm91 commented Sep 18, 2023

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
Copy link
Author

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
Copy link
Contributor

joshm91 commented Sep 20, 2023

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
Copy link
Author

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
Copy link
Contributor

joshm91 commented Sep 21, 2023

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
Copy link
Author

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

@denysandriyanov
Copy link
Author

Up

@timthornton-avid
Copy link

timthornton-avid commented Oct 12, 2023

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.

timthornton-avid added a commit to timthornton-avid/oathkeeper that referenced this issue Oct 18, 2023
…ad on closed Body" if request body is present in request
@denysandriyanov
Copy link
Author

I did CIT for this, and it passed

@denysandriyanov
Copy link
Author

UP

@denysandriyanov
Copy link
Author

Any updates?

@ouranders
Copy link

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

@ralfkrakowski
Copy link

ralfkrakowski commented Apr 9, 2024

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

alnr added a commit that referenced this issue Sep 11, 2024
@alnr
Copy link
Collaborator

alnr commented Sep 11, 2024

Sorry, I've not had time to wrap my head around what the actual problem was here. #1185 should resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants