[KEYCLOAK-10864] - Add support for Looser path normalization

[primeroz]

Fix https://issues.jboss.org/browse/KEYCLOAK-10864

Make working with backends like jenkins , which expect an undecoded path with "//" , easier

Add a new flag to enable "--loose-path-normalization" with default to false so not to affect any current setup and not open up security issues

[primeroz]

Is it normal to get no feedback on PR in this repo ? :)

Posted on the mailing list, Create a Jira case and everything else

thanks

[bruegth]

Tested this, looks good.

Additionally I think it's a bug, that the normal behavior is to sent the decoded URL to upstream.

[primeroz]

Nice, i kinda had lost faith on this :)

Additionally I think it's a bug, that the normal behavior is to sent the decoded URL to upstream.

I do agree , as a user when i started looking into my issue i was buffled as to why i was not getting the raw path when it was not required

[abstractj]

@primeroz @bruegth if I understood correctly the issue, the bug you reported applies only to Jenkins and no other server, correct? If yes, I don't see us moving forward with this, but if you have more use case scenarios I'm more than happy to take a look.

@primeroz I understand your frustration, but please keep in mind that people inside Keycloak organization, are working not only on Gatekeeper and the sentence in the mailing list: "But this is more of a POC than anything else since I am not sure the implication of those changes and if is a good idea at all to do this.", rings a bell that this PR requires more time for testing.

@stianst I still need to test the changes here, but if it's specific to Jenkins, I'd say that Gatekeeper should not cover it. Otherwise, we need to adapt to every particularity from other servers outside.

[primeroz]

Nice, i kinda had lost faith on this :)
I understand your frustration, but please keep in mind that people inside Keycloak organization

@abstractj , let me apologize if what i wrote was offensive , i did not mean that and i guess i did not think enough when i wrote it ... i hoped the smiley face was enough but sometimes communication over github can be tricky and i made a mistake.
I sincerely appreciate all the work on the project and use it extensively every day.

on the more technical side

But this is more of a POC than anything else since I am not sure the implication of those changes and if is a good idea at all to do this
if I understood correctly the issue, the bug you reported applies only to Jenkins and no other server, correct?

That is not really fair to say in my opinion.
The issue is that the url path received by the gatekeeper is not the same that is then pushed upstream, because of the normalization applied.
Jenkins is indeed the only service i found so far that is suffering from this issue so i would definitely agree it is an edge case but i don't think we can really say the issue is with the client.

The question that stands, and the reason why i wrote that this was a POC , is that I am sure there are plenty of reason to do a very hard normalization of the path before passing it to the upstream and so i wanted, with this PR and the Jira case i created, to show an issue and also propose a solution hoping that it could start a conversation with those who know more than me about gatekeeper and proxying in general and get to a solution that i don't necessarily need to be my code.

Also , to be on the safe side, i ended up writing a separate function https://github.com/keycloak/keycloak-gatekeeper/pull/483/files#diff-4c166b743d95629a9ab37eb6fe3df22aR74 rather than changing the current normalization function ... again because i did not want to introduce any security issues in a piece of code that seems to work just fine in 99.999% of cases.

but if it's specific to Jenkins, I'd say that Gatekeeper should not cover it. Otherwise, we need to adapt to every particularity from other servers outside.

Again i don't think this is the right point of view on the issue, i would ask more if gatekeeper should support a looser normalization of the path before proxying the request upstream to allow services like jenkins, that do exploit the url in ways that are edgy at best to work with gatekeeper.

If the answer is no i would totally be fine with it by the way

thanks

[razzeee]

We actually have this problem with rabbitmq when you leave the default vhost setting.

And some internal angular apps, as they try to use encoded / in their page urls, but gatekeeper causes the requests belonging to those pages to fail.

[abstractj]

@primeroz it's all good. Would be possible to add more details about how to reproduce the issue you mentioned in the same Jira (step by step)? That would help with the review process. We did something similar in this Jira.

@razzeee thanks for reporting this. Would you mind adding your scenario in the comments of the same Jira + steps to reproduce? The reason why I'm asking this is that I would like to make sure that the change submitted by @primeroz, also covers the issues you have with rabbitmq.

[bruegth]

@abstractj
I wonder about the use about the unsafe purell.FlagRemoveDuplicateSlashes flag.
Unsafe defined here: https://github.com/PuerkitoBio/purell/blob/master/purell.go#L41
I started debugging and found many incorrect handling of paths with self create double slashes in code & tests.
E.g. :

// WithOAuthURI returns the oauth uri
func (r *Config) WithOAuthURI(uri string) string {
	if r.BaseURI != "" {
		return fmt.Sprintf("%s/%s/%s", r.BaseURI, r.OAuthURI, uri)
	}

	return fmt.Sprintf("%s/%s", r.OAuthURI, uri)
}

Together with default OAuthURI = "/oauth" and optinally BaseURI = "/base-uri" and hardcoded:

	authorizationURL = "/authorize"
	callbackURL      = "/callback"
	expiredURL       = "/expired"
	healthURL        = "/health"
	loginURL         = "/login"
	logoutURL        = "/logout"
	metricsURL       = "/metrics"
	tokenURL         = "/token"
	debugURL         = "/debug/pprof" 

all will result in e.g.: "/base-uri//oauth//authorize"

If I fix WithOAuthURI by removing slashes between, nearly all tests will fail with 404.

This is also useless in entryMiddleware:

		req.RequestURI = req.URL.RawPath
		req.URL.RawPath = req.URL.Path

But in forwarding.go this is missing:

		if req.URL.RawPath != "" {
			req.URL.Path = req.URL.RawPath
		}

because of this:

Note that the Path field is stored in decoded form: /%47%6f%2f becomes /Go/. A consequence is that it is impossible to tell which slashes in the Path were slashes in the raw URL and which were %2f. This distinction is rarely important, but when it is, the code should use RawPath, an optional field which only gets set if the default encoding is different from Path.
(https://golang.org/pkg/net/url/#URL)

So at all, I think this PR should be refactored as bugfix, so that FlagRemoveDuplicateSlashes not needed anymore and also RawPath will be used in forwarder to prevent sending decoded path to upstream.

See my branch: https://github.com/bruegth/keycloak-gatekeeper/tree/native-go-proxy

[abstractj]

@bruegth I couldn't agree more with you, thanks for looking into this and such detailed feedback. As I mentioned to @primeroz and @razzeee we need more details, otherwise we are at the risk of introducing new bugs, instead of fixing them.

[razzeee]

I'm working on the same project as @bruegth is and before he cooked up the custom version we're running right now, we could just not query routes that included encoded /.
Same applied to accessing rabbitmqs management frontend with the default vhost of / and our route ending with / so that you end up with // unfortunately. I'm still not 100% sure if that's something we could/should fix on our end in a config or if that would break rabbitmq.

[primeroz]

In my case i can report the same url i did report in the JIRA case , when using jenkins a URL like

https://localhost:6001/administrativeMonitor/hudson.diagnosis.ReverseProxySetupMonitor/testForReverseProxySetup/https://localhost:6001/manage/

hit the proxy , but when is proxied upstream, after normalization, the // of the inner url is removed and so it breaks the request jenkins expects.

[abstractj]

@razzeee @primeroz do you have an environment to reproduce the issue? Something on docker-compose or a virtual machine? If yes, that would be helpful.

[primeroz]

I can try to get something , the only issue i can see for jenkins is that since the configuration that requires that special url is the reverse proxy one i will need to make it work with the whitelisting on the proxy ... but yeah i think it should work. i will give it a shot

[primeroz]

@abstractj note that in the case of jenkins you will just see an error in the ui , it won't really tell you that the problem is a malformed url ... to figure that was the issue i had to go through logs an use an upstream service to print back the url it received and noticed it was malformed.

[primeroz]

@abstractj example here https://github.com/primeroz/keycloak-10864-test-jenkins

instructions in readme

[abstractj]

@primeroz do you think you and @Nuru could merge the changes here into a single codebase?@Nuru submitted another PR #535, but I believe we should join the efforts and discuss on top of a single change.

[Nuru]

@abstractj I think joining efforts to merge this 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 this 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.

Let us please not make the perfect the enemy of the good.

[primeroz]

@abstractj @Nuru my understanding is that the #535 would superseed this one, looking at https://github.com/keycloak/keycloak-gatekeeper/pull/535/files#diff-4c166b743d95629a9ab37eb6fe3df22aR70 for everything other thant the FlagRemoveDuplicateSlashes ?

If that is the case i can wait for #535 to be merged in and rework this ( or maybe close and recreate) to only handle the use of different flags in the same middleware handler by using a flag at cli.

[Nuru]

@primeroz Yes, thank you, #535 is meant to supersede this PR for the purpose of ensuring the upstream server gets the exact same request that Gatekeeper gets, thus closing KEYCLOAK-10864 and some other open bugs. It does not provide a flag or remove FlagRemoveDuplicateSlashes from the URL sanitization options.

@abstractj This PR (#483) also (in addition to fixing KEYCLOAK-10864) makes changes to how Gatekeeper parses the request for the purpose of applying access control, and #535 purposely avoids making any such changes because that is a separate issue about which I think there has not been enough discussion. I would prefer we do as @primeroz proposes: merge #535 and get that into the release pipeline, after which everyone can continue to work on whatever remaining features you want that it does not provide.

[stianst]

I honestly don't see why Gatekeeper would make any changes/normalize the request path at all. I can honely see that causing problems like this.

[Nuru]

@stianst You can see #200, #201, and #202 for the motivation to normalize the URLs before applying resource protection rules. The idea was to make it harder to circumvent resource protections through generating URLs that the upstream would consider equivalent but that Gatekeeper, in its implementation details, would consider different.

I have my doubts about whether #202 was the right solution to #200, but I do not want to spend the time or energy getting into that. What I can say is that a naïve solution to KEYCLOAK-10864 that simply strips out the URL normalization will re-introduce #200. This is why I submitted #535: to fix KEYCLOAK-10864 without re-introducing #200 or getting into a discussion about whether #201 and/or #202 was the right fix. I am happy to leave that question open and have the discussion about it continue here or elsewhere. This PR has already been open 8 months; I would just like to get KEYCLOAK-10864 fixed sooner, and #535 can do that without risk of reintroducing #200, so I think it is a safer and easier option.

[stianst]

@Nuru Makes sense to normalize the path within Gatekeeper for sure, while use the originial path when invoking upstream. I assume that's what your PR is doing.

[abstractj]

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