fix: Authorizer "remote" throws exception

[timthornton-avid]

…ed Body" if request body is present in request

#1136

Related issue(s)

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.60%. Comparing base (8fc9b7a) to head (6eaedba).
Report is 2 commits behind head on master.

❗ Current head 6eaedba differs from pull request most recent head febbe7d. Consider uploading reports for the commit febbe7d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
- Coverage   77.90%   77.60%   -0.31%     
==========================================
  Files          80       79       -1     
  Lines        3929     4014      +85     
==========================================
+ Hits         3061     3115      +54     
- Misses        595      618      +23     
- Partials      273      281       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[taisph]

I'm probably wrong but this looks odd to me. WithContext creates a shallow clone of the request which might be the actual issue, given how r is used below. Have you tried r = r.Clone(ctx) instead?

[timthornton-avid]

Hi taishp.. Yes I tried Clone() also. Using either function r.WithContext(ctx) or r.Clone(ctx) create a new request message which gets assigned to r (HTTP Request ). The calling function to Authorizer expects r it passed to persist throughout the pipeline as it will use the context and payload for further processing. Once we assign r (via WithContext or Clone) in the Authorize function, it's pointing to a new HTTP Request * which goes out of scope when the Authorize function returns.

It appears this issue was introduced in #1079 when adding tracing improvements.
While testing, if we remove the lines added from #1079:
ctx, span := a.tracer.Start(r.Context(), "authz.remote")
defer otelx.End(span, &err)
r = r.WithContext(ctx)
The issue does not occur as the scope of r does not change.

As another test, I added a function SetContext(ctx) to http.request to assign the context to the request messaeg but not create a new http.http request. This also worked as again the scope of r persisted.

[timthornton-avid]

To address the syntax looking odd, here is another datapoint that follows the same pattern as this code change. Line 133 calls HandleRequest which calls the remote::Authorize function where the fix was made. In the below code snippet we confirm that the r.Out scope must be retained throughout the message processing.

proxy.go
132 *r.Out = *r.Out.WithContext(context.WithValue(r.Out.Context(), ContextKeyMatchedRule, rl))
133 s, err := d.r.ProxyRequestHandler().HandleRequest(r.Out, rl)
134 if err != nil {
135 *r.Out = *r.Out.WithContext(context.WithValue(r.Out.Context(), director, err))
136 return
137 }
138 *r.Out = *r.Out.WithContext(context.WithValue(r.Out.Context(), ContextKeySession, s))

[timthornton-avid]

Just enhanced and added to pull the unit testing in remote_test.go. To test this failure the message body sent to the authorize function is compared to the message body after the authorize pipeline completes. There are two tests that already pass a message body into the Authorize function and these were both verified to fail without 1136 fix and pass after 1136 fix.

[alnr]

Thanks for the PR! Would you mind adding a failing test case as a first step? That way we're sure to catch any regressions in the future. Also, it makes it easier to understand the problem and convince ourselves the fix is correct.

[ouranders]

@timthornton-avid Really nice of you to fix this. Could you get the branch up-to-date with the base branch and try to resolve the failing "Docker Image Scanners"? We would really love to have this issue fixed.
Thanks!

[ralfkrakowski]

Any progress on this pull? We would really love to have this issue fixed.
Thank you!

[timthornton-avid]

Hi, I'll try to get this branch up to date and retest this week or next. As per the docker image failure, there was nothing I added that would impact docker image. There was a 1 line code change + comments and a unit test added. I don't recall the docker image scan failure but they would have existed prior to my changes. Lets see what it looks like after I rebase.

[timthornton-avid]

Ok, had a few minutes to rebase code and test. Looks like everything in pipeline has passed. Should be good to go :)

[ralfkrakowski]

Hi, is there any progress with the review of this issue @taisph and @aeneasr ?
This fix would be really valuable for us.
Thanks for releasing such a great product!

[timthornton-avid]

Hey Guys,
Any update on this ? Updated branch a month ago.

[keyur-scogo]

Hi any plans on releasing patch for this issue?

[ouranders]

As others has previously mentioned, we are also very eager to have this issue fixed and merged :) @taisph @aeneasr any news on when this can be looked on? Kudos for a great product!

[mwahlhuetter]

Also wanted to chime in, would be great if this could be resolved and merged :)
Just had a very long troubleshooting session to find out that there is a bug in oathkeeper and not in my setup...

[alnr]

Fixed in #1185. Thanks for the PR and your patience.