Skip to content
This repository was archived by the owner on Dec 7, 2020. It is now read-only.

Keycloak-gatekeeper - path normalization break some use cases ( i.e. Proxying to Jenkins ) #594

Closed
abstractj opened this issue Apr 28, 2020 · 2 comments

Comments

@abstractj
Copy link

abstractj commented Apr 28, 2020

What:

When using keycloak-gatekeeper to Reverse Proxy to a Jenkins instance jenkins will actually fail to validate the reverse proxy configuration ( see [this|https://wiki.jenkins.io/display/JENKINS/Jenkins+says+my+reverse+proxy+setup+is+broken])

The problem is with the [func entrypointMiddleware|https://github.com/keycloak/keycloak-gatekeeper/blob/master/middleware.go#L41] function where the URL path is normalized.

Jenkins will test the reverse proxy configuration with a request similar to
{{https://localhost:6001/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https%3A%2F%2Flocalhost%3A6001%2Fmanage/}}

The 2 problems i found are

  1. doubleslashes will be removed
  2. rawPath is overwritten with the "decoded" Path

I Assume that the normalization is for security reasons since also the ".." are removed.

I am currently running a simple forked version of the code to check it actually work with jenkins ( see [here|https://github.com/keycloak/keycloak-gatekeeper/compare/master...primeroz:jenkins-rp-support] )

Do you think that adding a flag to "--disable-normalized-req-path" would be feasable / safe enough ?"

Reference:

Relates to #591

@Nuru
Copy link
Contributor

Nuru commented Apr 29, 2020

Do you think that adding a flag to "--disable-normalized-req-path" would be feasable / safe enough ?"

No, I do not. The changes you propose are very similar to and I expect inspired by PR #483, but as I explained, on the Keycloak Dev list those changes cause a large number of existing tests to fail.

Also, as I explained in #483 (comment), The normalized path was introduced in #201 and #202 to fix #200 and I suspect your "--disable-normalized-req-path" would re-introduce #200, which is why it fails all the tests.

I also question the value of having this be a flag/option. I do not see anyone calling for transforming the requests that are sent upstream except for changing hostname and scheme, certainly not URL sanitization. That leaves the only use for a flag to be a way to affects how URLs are parsed for security, as this would be asking for trouble.

I believe the situation is that we need the normalizations to harden the security model, as see in #200, but we want to let go of the normalizations to pass upstream. Which is what #535 does.

@abstractj
Copy link
Author

Solved by #626. If there are any comments or questions, of if the issue persists. Please, let me know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants