Path params with encoded '/' don't get re-directed properly through gateway?

[plavjanik]

Issue by stevenhorsman-ibm Tuesday Sep 18, 2018 at 09:09 GMT Originally opened as https://github.com/gizafoundation/api-layer/issues/110

---

An example of this (which blocks moving atlas fully behind the API Gateway): In the Atlas USS apis we use the uss file path as a path parameter in the URI eg. http --verify=no -a stevenh:XXXXX GET "https://winmvs3b.hursley.ibm.com:7443/Atlas/api/uss/files/%2Fu%2Fstevenh" HTTP/1.1 200 OK … { "children": [ { "link": "https://winmvs3b.hursley.ibm.com:7443/Atlas/api/uss/files/%2Fu%2Fstevenh%2FnewDirectory123", "name": "newDirectory123", "type": "directory" }, …

I've got the Atlas APIs generally working behind the gateway: http --verify=no -a stevenh:XXXXXX GET "https://localhost:10010/api/v1/explorer-server/datasets/STEVENH.JCL/members" [ "ANOTHER", "COPY", "FROMDATA" ]

but when I make a request which has the '%2f' in I get a bad request: http --verify=no -a stevenh:XXXXXX GET "https://localhost:10010/api/v1/explorer-server/uss/files/%2Fu%2Fstevenh" HTTP/1.1 400 Connection: close Content-Length: 0 Date: Wed, 12 Sep 2018 10:43:59 GMT

My guess is that the %2F is causing some issues in the Zuul filter, but I'm far from an expert!

[plavjanik]

Comment by stevenhorsman-ibm Tuesday Oct 30, 2018 at 16:26 GMT

---

So my latest update from testing this branch - the gateway seems to be correctly sending the request through now, but I think it might be scrambled on the way as the backend is rejecting it with pathname not valid. These few weeks are crazy for me, but when I get a chance I'll hook up a debugger and see if I can work out what the path is translated to.

[stevenhorsman]

I've hooked up a debugger and the request path is altered - My request of http --verify=no -a stevenh:dp109jp3 GET "https://localhost:10010/api/v1/explorer-server/uss/files/u%2Fstevenh" gets to the backend with a path parameter of 'u%252Fstevenh' (so the % has been expanded to %25 and the 2F unchanged). Whereas when we hit that endpoint directly Java gets the path param of 'u/stevenh' (decodes the %2F to a '/')

[stevenhorsman]

The latest update: The backend now recieves a path parameter of 'u%2Fstevenh', so the %25 has been fixed, but it's still not getting through as 'u/stevenh'

[cZikos]

The latest update: The backend now recieves a path parameter of 'u%2Fstevenh', so the %25 has been fixed, but it's still not getting through as 'u/stevenh'

The gateway's role is to pass the request as it is issued to the service. So '%2f' is not translated as '/' by the gateway, but passed as it is. The decoding process should be done individually by the service.

[stevenhorsman]

~I can understand what you are saying, but the problem is that the gateway breaks the existing behaviour. Maybe it is a quirk unique to JAX-RS, but without the gateway to get a parameter passed which has a '/' in you have to encode it as %2f and by the time it hits your coding it's converted. The gateway is stopping this from happening, so as it currently stands we cannot on-boarding our APIs behind it. I'm not going to re-write our backend so that it works differently when the gateway is in place as I wouldn't expect any of the services that that exist that we are trying to onboard to do that. In my mind the gateway/reverse proxy/mediation layer should be invisible to the back-end - if it's there then you get advantages, but you shouldn't have to re-write applications to assume that it is.~ I've done some testing on spring boot and it seems to work there, so happy to write this off as some bad server configuration. Sorry about the grouchiness earlier!

[cZikos]

I can understand what you are saying, but the problem is that the gateway breaks the existing behaviour. Maybe it is a quirk unique to JAX-RS, but without the gateway to get a parameter passed which has a '/' in you have to encode it as %2f and by the time it hits your coding it's converted. The gateway is stopping this from happening, so as it currently stands we cannot on-boarding our APIs behind it. I'm not going to re-write our backend so that it works differently when the gateway is in place as I wouldn't expect any of the services that that exist that we are trying to onboard to do that. In my mind the gateway/reverse proxy/mediation layer should be invisible to the back-end - if it's there then you get advantages, but you shouldn't have to re-write applications to assume that it is. I've done some testing on spring boot and it seems to work there, so happy to write this off as some bad server configuration. Sorry about the grouchiness earlier!

No probelm Steve, happy to help :)

[kingo999]

Hi Steve, I'm playing a bit of catch-up here so excuse any obvious questions Is your original problem that you could not send a GET request with an un-encoded URL ? https://localhost:10010/api/v1/_explorer-server/datasets_/STEVENH.JCL/members (with STEVENH.JCL/members being the variable section.

If so then if possible i'd recommend against using this method of URL for various reasons but I understand that you might not have a choice. If changing the path variable to a request body parm is not an option then could you change the url of the rest controller so it uses a /** section and then decode that in your controller (a simple helper function could be written? if so then the Gateway does not need to change and will allow an uncoded path variable through.

For example: Here's my GET calling the service directly (no gateway) : http://localhost:10012/discoverableclient/api/v1/path/my/variable/uss/path

And using the following REST method:

@GetMapping(value = "/api/v1/path/**")
 public String getPath(HttpServletRequest request) {
        ResourceUrlProvider urlProvider = (ResourceUrlProvider) request
            .getAttribute(ResourceUrlProvider.class.getCanonicalName());
        String restOfUrl = urlProvider.getPathMatcher().extractPathWithinPattern(
            String.valueOf(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE)),
       String.valueOf(request.getAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE)));
        return "Path: " + restOfUrl;
    }

I get a response of Path: my/variable/uss/path.

No changes were required in the Gateway to allow the extra / in the URL through: https://localhost:10010/api/v1/discoverableclient/path/myvariable/path returns: Path: my/variable/path

If the encoding is required then changes will be required in the Gateway which I think should be looked at in more detail as while it will solve your immediate issue it may have other consequences for other services as the current implementation is a global change.

Dave

[stevenhorsman]

Hey Dave, thanks for the suggestion. I'll be honest - I didn't know that /** was an option (I don't know if it also in in JAX-RS). I'm on the fence myself about whether I put to pass a file path through without encoding anyway. /api/v1/path//u/stevenh makes me twitch a bit with lack of clarity between path and params, and when you type /u/stevenh as a path parameter in swagger's 'try it out' section it automatically converts the '/'s to %2f. That said I'm happy to have a wider discussion if everyone else feels differently to me! That all being said - I think that the gateway is working okay for GET requests now. There is a slight difference in some request/response generation, but I'll inspect my code and see if that is to blame as I do the SpringBoot port and raise and discuss it at the time.

[kingo999]

Hi Steve, The issue is not with us or with you, the root of the problem is that internally, the tomcat portion of Zuul does some encoding. It's bad in my opinion as why should a proxy modify the request ? But it's there. The reason why the push back is that while the fix works, it's basically a hack for a specific case which does expose potential security issues. The other reservation is if we put logic in there for your case, someone else may have another case and another and we could end up with conflicts or worse, nightmare scenario of deprecated core functionality that breaks everything!

If it's possible we would like to try influence owners of the services to use 'good' (a relative term) API's which do not require us to put more logic in the Gateway than we have to.

I'l be glad to work with you on this, we we're also thinking about having some kind of post-send filter which will allow you service to send the encoded stuff but modify it before it gets to the gateway

Can I have a look at your code to see if there's something I can do to get past this quickly ?

Dave

[stevenhorsman]

Ok, so is it potentially a bug that we could raise against Zuul? (though I know that won't be an immediate fix). I can understand your point of view that you don't want to add hacks to Zuul - but in my opinion, we shouldn't have to sacrifice 'good' APIs to get things behind the gateway - encoding '/' in path params to %2f is just how things are done - and things like Swagger work this way probably because URLEncoder and other libraries do, rather than as a choice. Our current code is https://github.com/zowe/explorer-server/blob/master/src/main/java/com/ibm/atlas/webservice/resource/Files.java, but there is also the issue that some of the services behind the gateway may not have the option to re-write before on-boarding, so we could potentially limit users if we force that.

[kingo999]

I agree, it's a bit of a balancing act ... Michal will arrange a call so we can discuss

[plavjanik]

@stevenhorsman The APIs in Zowe should not rely on corner cases as encoded slash. But I get your point about other APIs that we will not be able to change. You know z/OSMF REST API very well. Do they use something similar?

[stevenhorsman]

I'm not sure that I agree that encoding slashes is a corner case - it seems to be standard practice: eg see https://stackoverflow.com/questions/2992231/slashes-in-url-variables, but if there is a security issue, then I guess we'll have to document that it doesn't work with the gateway. z/OS MF doesn't encode it's slashes, but that was a contributing factor to Atlas existing as a wrapper as people struggled to create working swagger hub for it as a result.

[kingo999]

Fix pushed for testing: https://github.com/zowe/api-layer/tree/encoded_strings_in_url_path

[varma09]

Continues in this PR: https://github.com/zowe/api-layer/pull/104

[kingo999]

I created the following jersey rest controller (no spring) in our jersey enabler sample service

 @GET
    @Path(value = "/atlas/{path}/content")
    @Produces(MediaType.APPLICATION_JSON)
    public String getContent(@PathParam("path") String path) {
        return path;
    }

And posted the following URL through the Gateway: https://localhost:10010/api/v1/helloworld-jersey/v1/atlas/%2faaa%2fbbbb/content

I received this response: %2faaa%2fbbbb

No double encoding.

[kingo999]

Debugged screenshot:

[kingo999]

Steve, can you debug class: GatewayRibbonLoadBalancingHttpClient in the Gateway Service, does method: reconstructURIWithServer modify the URL ? I want to track down exactly where your modification occurs. btw: we looking at trying to get Atlas installed here so we can help with debugging.

[stevenhorsman]

I made the following request: http --verify=no -a stevenh:dp109jp3 GET "https://localhost:10010/api/v1/explorer/uss/files/%2fu%2Fstevenh" (Just a quick thought - I'm using httpie as recommended, were you also using that in your testing?) GatewayRibbonLoadBalancingHttpClient. reconstructURIWithServer didn't change the URL, it was already 'double encoded'. [GS] original URI is :/api/v1/uss/files/%252fu%252Fstevenh [GS] uriToSend is :https://winmvs3b.hursley.ibm.com:43443/api/v1/uss/files/%252fu%252Fstevenh

[kingo999]

I'm using postman, but I cannot connect to that server

[stevenhorsman]

I tried using a different REST client and going straight through a browser - same result each time: original URI is :/api/v1/uss/files/%252fu%252Fstevenh When you trace using your jersey example are you seeing the same stuff in that class as our server doesn't seem to be an important factor as it hasn't even got that far before the double encoding happens?

[kingo999]

right, that helps

[kingo999]

I finally tracked down where the encoding takes place: org.springframework.web.util.HierarchicalUriComponents -> encodeUriComponent

This loop checks for invalid characters, / (encoded) is invalid so it goes into the else block and encodes it.

for (byte b : bytes) {
            if (b < 0) {
                b += 256;
            }
            if (type.isAllowed(b)) {
                bos.write(b);
            }
            else {
                bos.write('%');
                char hex1 = Character.toUpperCase(Character.forDigit((b >> 4) & 0xF, 16));
                char hex2 = Character.toUpperCase(Character.forDigit(b & 0xF, 16));
                bos.write(hex1);
                bos.write(hex2);
                changed = true;
            }
        }

Here's a debug shot showing the character being changed:

So the problem isn't in Zuul but rather the Spring-Web package. I'm looking into a way to get around this without hacking it.......

[stevenhorsman]

Thanks for the update Dave - let me know if there is anything I can do to help

[kingo999]

I've spent a fair bit of time looking at this issue. While there is a solution there to resolve it I recommend that we do NOT allow encoded /'s in the request URL. Steps to allow them through:

• Configure Tomcat (against their own advice to allow /)
• Configure Spring (including modifying firewall beans)
• Override default Zuul behaviour to allow unencoded request through (avoid double encoding)

Apart from the fact that encoded / in the request goes against standard W3C recommendations there's also the issue that this will allow potentially malicious code through. Finally if we override this default behaviour against what it was designed to do we could get into the situation whereby an upgrade to spring could render our changes useless or a future 'corner case' may not be possible because of these changes.

Bottom line: recommend to change the API to a more acceptable format.

[kingo999]

Agreed with @stevenhorsman that no hacks will be made. API will be redesigned to follow best practices

[zowe-robot]

The changes to the error code documentation are available in this PR: https://github.com/zowe/docs-site/pull/2082

[zowe-robot]

The changes to the error code documentation are available in this PR: https://github.com/zowe/docs-site/pull/3267