Fix for KEYCLOAK-10864

[Nuru]

I opened pull request 535 (PR #535) against keycloak-gatekeeper to fix KEYCLOAK-10864 which will also fix KEYCLOAK-11276, KEYCLOAK-13315, and GitHub issue keycloak-gatekeeper/#528 and replaces keycloak-gatekeeper/pull/483 by making this a bug fix and not a new option.

It does not remove FlagRemoveDuplicateSlashes as suggested in comments to PR 483 here because that change is not needed to fix KEYCLOAK-10864. Instead, the existing sanitization of URLs is kept for internal processing. If you want to remove FlagRemoveDuplicateSlashes I think that should be added to keycloak-gatekeeper/pull/510 as part of the fix for KEYCLOAK-10633

I have included tests that illustrate the problem that fail on master and succeed with my changes.

I'm sorry I did not find the previous work earlier, so I did not build on it. Instead, I diagnosed the issue and fixed it from scratch. I think my PR is better because

1. It does not change internal processing of the URL, so it does not risk creating problems with how Gatekeeper itself interprets URLs or applies policies to them
2. It is easy to reason about and see that the URL path forwarded upstream is the path received
3. It has more thorough tests included

If you use the test setup at https://github.com/primeroz/keycloak-10864-test-jenkins (not written by me) you will see the error message:

Then, in the docker-compose.yml, replace the proxy image "quay.io/keycloak/keycloak-gatekeeper:8.0.1" with "nuru/key-gate:KEYCLOAK-10864" and then docker down, docker up, and you will see the error message is gone. 

While the issue is old, it is new for me, and getting close to production, so I would like to get this approved quickly. Please respond on GitHub in comments to the PR.

[Bruno Oliveira]

Thanks for the heads up. I left some comments here
https://github.com/keycloak/keycloak-gatekeeper/pull/483#issuecomment-618548447.

There's already a pull-request for this as you already mentioned. My
suggestion is to join efforts, so we can benefit from a single solution.

On 2020-04-17, Nuru wrote:
>I opened pull request 535 (PR #535

><https://github.com/keycloak/keycloak-gatekeeper/pull/535>) against
>keycloak-gatekeeper <https://github.com/keycloak/keycloak-gatekeeper> to
>fix KEYCLOAK-10864 <https://issues.redhat.com/browse/KEYCLOAK-10864> which

>will also fix KEYCLOAK-11276, KEYCLOAK-13315, and GitHub issue
>keycloak-gatekeeper/#528

><https://github.com/keycloak/keycloak-gatekeeper/issues/528> and replaces
>keycloak-gatekeeper/pull/483
><https://github.com/keycloak/keycloak-gatekeeper/pull/483> by making this a

>bug fix and not a new option.
>
>It does not remove FlagRemoveDuplicateSlashes as suggested in comments to
>PR 483 here

><https://github.com/keycloak/keycloak-gatekeeper/pull/483#issuecomment-598881265>

>because that change is not needed to fix KEYCLOAK-10864. Instead, the
>existing sanitization of URLs is kept for internal processing. If you want
>to remove FlagRemoveDuplicateSlashes I think that should be added to
>keycloak-gatekeeper/pull/510

><https://github.com/keycloak/keycloak-gatekeeper/pull/510> as part of the
>fix for KEYCLOAK-10633 <https://issues.redhat.com/browse/KEYCLOAK-10633>

>
>I have included tests that illustrate the problem that fail on master and
>succeed with my changes.
>
>I'm sorry I did not find the previous work earlier, so I did not build on
>it. Instead, I diagnosed the issue and fixed it from scratch. I think my PR
>is better because
>

> 1. It does not change internal processing of the URL, so it does not

> risk creating problems with how Gatekeeper itself interprets URLs or
> applies policies to them

> 2. It is easy to reason about and see that the URL path forwarded

> upstream is the path received

> 3. It has more thorough tests included

>
>
>If you use the test setup at
>https://github.com/primeroz/keycloak-10864-test-jenkins (not written by me)
>you will see the error message:
>
>

>Then, in the docker-compose.yml, replace the proxy image "
>quay.io/keycloak/keycloak-gatekeeper:8.0.1" with "
>nuru/key-gate:KEYCLOAK-10864" and then docker down, docker up, and you will
>see the error message is gone.
>
>While the issue is old, it is new for me, and getting close to production,
>so I would like to get this approved quickly. Please respond on GitHub in
>comments to the PR

><https://github.com/keycloak/keycloak-gatekeeper/pull/535>.
>
>--
>You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.
>To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/aee050aa-0445-40a9-9f20-7cfad3573409%40googlegroups.com.



--

abstractj

[Nuru]

I think joining efforts to merge #483 with #535 runs counter to the idea of having just 1 issue per PR and one squashed commit per PR. #535 is a bug fix and #483 is a feature enhancement plus a bugfix for something not listed as a bug in JIRA. Also #535 is ready to go, including testing. I would much prefer you accept #535 as is and then open separate PRs for this "-loose-path-normalization" flag (or maybe "--santize-path") if it is still needed and for removing `FlagRemoveDuplicateSlashes` from the internal processing of paths to implement access control if indeed that is a bug (which should be filed in JIRA and discussed there first).

#535 is ready to go and could be in the next patch release. Let us please not delay it by adding extras.

[Bruno Oliveira]

Your PR will be reviewed, but we are not going to rush into things. PR
https://github.com/keycloak/keycloak-gatekeeper/pull/483 and
https://github.com/keycloak/keycloak-gatekeeper/pull/535 as far as I
know try to fix the same issue.

If that's not the case, one of them must be revisited.

> --
> You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/c07c2c9a-f945-419c-b416-7a779721c419%40googlegroups.com.



--
- abstractj

[Nuru]

PR 483 and 535 have, at their source, the same motivation of fixing the error message from Jenkins that the reverse proxy is misconfigured. Other than that, they are quite different.

PR 483:

• Creates a new flag/option, "loose-path-normalization", that defaults to false
• When "loose-path-normalization" is set to true
• duplicate slashes are no longer removed from the URL before processing, but "dot segments" are still removed
  
• req.URL.RawPath and req.RequestURI are changed differently than when loose-path-normalization is false, creating inconsistent treatment withing Gatekeeper
  
• Fails these tests from the master branch when loose-path-normalization is enabled
  
• TestCookieBasePath
• TestAuthorizationTemplate
• TestAuthorizationURLWithSkipToken
  
• TestCallbackURL
• TestExpirationHandler
• TestHealthHandler
• TestLoginHandler
• TestLoginHandlerDisabled
• TestLogoutHandlerBadRequest
• TestLogoutHandlerBadToken
• TestLogoutHandlerGood
• TestMetricsMiddleware
• TestOauthRequestNotProxying
• TestStrangeAdminRequests
• TestTokenHandler

PR 535:

• Is a bug fix and does not add or remove options
• Does not change how URLs are processed by Gatekeeper internally
• Ensures that the upstream server gets the identical path portion of the resource request that was presented to Gatekeeper
  
• Fixes an error in the testing rig so that the upstream URL tested is the URL that will be sent to the upstream server, not the URL that was sent to the Gatekeeper proxy itself. (This fix exposes additional errors with PR 483, including that it does not fix )
• Passes all tests, including a new one that specifically tests the URLs mentioned in KEYCLOAK-10864, KEYCLOAK-11276, and KEYCLOAK-13315

So I would prefer to proceed with PR 535, which would close KEYCLOAK-10864, KEYCLOAK-11276, and KEYCLOAK-13315 (because they all have the same underlying cause) and if possible get it in the next point release of Gatekeeper. Then, if you want other features from PR 483, you can certainly consider them apart from this bug fix.

Fundamentally, my point is PR 535 completely succeeds as-in in accomplishing a desired goal. I would not like to delay that progress (or put in more of personal time) in pursuit of additional goals right now.

On Thursday, April 23, 2020 at 1:07:09 PM UTC-7, Bruno Oliveira wrote:

Your PR will be reviewed, but we are not going to rush into things. PR
https://github.com/keycloak/keycloak-gatekeeper/pull/483 and
https://github.com/keycloak/keycloak-gatekeeper/pull/535 as far as I
know try to fix the same issue.

If that's not the case, one of them must be revisited.

On Thu, Apr 23, 2020 at 4:46 PM Nuru <goo...@nuru.com> wrote:
>
> I think joining efforts to merge #483 with #535 runs counter to the idea of having just 1 issue per PR and one squashed commit per PR. #535 is a bug fix and #483 is a feature enhancement plus a bugfix for something not listed as a bug in JIRA. Also #535 is ready to go, including testing. I would much prefer you accept #535 as is and then open separate PRs for this "-loose-path-normalization" flag (or maybe "--santize-path") if it is still needed and for removing `FlagRemoveDuplicateSlashes` from the internal processing of paths to implement access control if indeed that is a bug (which should be filed in JIRA and discussed there first).
>
> #535 is ready to go and could be in the next patch release. Let us please not delay it by adding extras.
>
> On Thursday, April 23, 2020 at 10:53:24 AM UTC-7, Bruno Oliveira wrote:
>>
>> Thanks for the heads up. I left some comments here
>> https://github.com/keycloak/keycloak-gatekeeper/pull/483#issuecomment-618548447.
>>
>> There's already a pull-request for this as you already mentioned. My
>> suggestion is to join efforts, so we can benefit from a single solution.
>>
> --
> You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.

> To unsubscribe from this group and stop receiving emails from it, send an email to keyclo...@googlegroups.com.

[Bruno Oliveira]

As already mentioned, your PR will have a proper review, but we're not going to rush into things. 

To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/5b2a4c5d-c23b-4722-ae3c-315da2fca101%40googlegroups.com.

[Nuru]

Of course I do not want you to hastily rush into anything.

On the other hand, I note that PR 483 has been open for more than 8 months, and KEYCLOAK-10864 has been open for closer to a year. I crafted PR 535 to be easier to analyze and reason about and to be smaller in scope (and added tests for additional confidence) specifically so that you could be confident in approving it in a shorter time frame. PR 535 is a "single solution" to a specific problem on which there is clear agreement of the nature of the problem. PR 483 is actually several solutions to several problems, some of which are not as clearly agreed upon. I am just asking that you not delay the progress made by PR 535 by requesting that it do more than it set out to do and/or tying it to anything else.

On Thursday, April 23, 2020 at 7:18:51 PM UTC-7, Bruno Oliveira wrote:

As already mentioned, your PR will have a proper review, but we're not going to rush into things. 

On Thu, Apr 23, 2020, 7:01 PM Nuru <goo...@nuru.com> wrote:

PR 483 and 535 have, at their source, the same motivation of fixing the error message from Jenkins that the reverse proxy is misconfigured. Other than that, they are quite different.

[Bruno Oliveira]

I understand. We're working to improve the PR review process. Please
be patient and your PR will be reviewed.

> --
> You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/6ba859b3-7457-433a-848f-c1e1d1a31d25%40googlegroups.com.



--
- abstractj