Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Case sensitivity #10

Open
minos314 opened this issue Dec 1, 2022 · 5 comments
Open

Case sensitivity #10

minos314 opened this issue Dec 1, 2022 · 5 comments
Labels
question Further information is requested

Comments

@minos314
Copy link

minos314 commented Dec 1, 2022

Hi,

Is there a specific reason why the stringmap matching in CoreLogic works NOT case sensitive? If the backend server treats paths case sensitive this could be unwanted behaviour to pass requests through.

`

FULL - LAN

add policy expression PE_CL1009_HTTP_CS_FULL_LAN_KEY "(HTTP.REQ.CS_VSERVER.NAME + ";lan;" + HTTP.REQ.HOSTNAME.SERVER + HTTP.REQ.URL.PATH).TO_LOWER"
add policy expression PE_CL1009_HTTP_CS_FULL_LAN_KEY_EXISTS "((HTTP.REQ.CS_VSERVER.NAME + ";lan;" + HTTP.REQ.HOSTNAME.SERVER + HTTP.REQ.URL.PATH).TO_LOWER.IS_STRINGMAP_KEY("SM_CL1009_CS_CONTROL"))"
add policy expression PE_CL1009_HTTP_CS_FULL_LAN_VALUE_DST "(HTTP.REQ.CS_VSERVER.NAME + ";lan;" + HTTP.REQ.HOSTNAME.SERVER + HTTP.REQ.URL.PATH).TO_LOWER.MAP_STRING("SM_CL1009_CS_CONTROL").AFTER_STR("dst=").BEFORE_STR(";")"
add policy expression PE_CL1009_HTTP_CS_FULL_LAN_VALUE_VS "(HTTP.REQ.CS_VSERVER.NAME + ";lan;" + HTTP.REQ.HOSTNAME.SERVER + HTTP.REQ.URL.PATH).TO_LOWER.MAP_STRING("SM_CL1009_CS_CONTROL").AFTER_STR("vs=").BEFORE_STR(";")"
add policy expression PE_CL1009_HTTP_NO_SERVICE_SWITCH_FULL_LAN_KEY_EXISTS "((HTTP.REQ.CS_VSERVER.NAME.BEFORE_STR_ANY("PS_CL1009_NAME_PROTOCOLS") + "_ssl;lan;" + HTTP.REQ.HOSTNAME.SERVER + HTTP.REQ.URL.PATH).TO_LOWER.IS_STRINGMAP_KEY("SM_CL1009_CS_CONTROL"))"
`

Do you see any problems if we try to change our CoreLogic to case sensitive? It seems that the multiple TO_LOWER expressions are the only thing to remove.

Thanks,
Simon

@jantytgat
Copy link
Contributor

jantytgat commented Dec 1, 2022

Hi Simon,

  • The first reason we force lower-case entries in the key, is to avoid confusion on how input must be made.
  • The second reason is because it would be bad API design if you were to have to routes in a web application, such as /api/auth and /api/Auth doing different things.
  • The third reason is because data is passed from the user's browser into the HTTP request as-is, which means that if a user types https://fqdn/api/Auth as the URL for the request, it wouldn't match the entry in the stringmap.

We are looking at removing the TO_LOWER in the full rebuild of CoreLogic, but I think the TO_LOWER would then be replaced by SET_TEXT_MODE(IGNORECASE), giving the same result.

Does that make sense to you?

Cheers,
Jan

@jantytgat jantytgat added the question Further information is requested label Dec 1, 2022
@minos314
Copy link
Author

minos314 commented Dec 1, 2022

Hi Jan,

Thanks for explanation.
I fully understand reason #1 and #3, but i am not 100% convinced if a case sensitive webserver is really bad API design. For example also github differentiates paths, the URL to this question delivers a 404 if I change /issues/ to uppercase in the URL (https://github.com/corelayer/corelogic/Issues/10).

So the main point is: should URL paths with different casing be treated as unique ressources, or are they different. As most webservers running on linux differentiates those paths I would say they shouldn't be treated the same. In our setup we do also have an appfw before the ADC which works case sensitive. So in our setup almost everthing works case sensitive except core logic.

FYI: The TO_LOWER or even the planned SET_TEXT_MODE(IGNORECASE) in CoreLogic wouldn't be really necessary in most cases, because the usage of HTTP.REQ.HOSTNAME within the expression sets the text_mode to case insensitive implicitly. It took me quite a while to figure this out. See https://developer-docs.citrix.com/projects/citrix-adc-advanced-policy-expression-reference/en/latest/http_req_t/.

Cheers,
Simon

@jantytgat
Copy link
Contributor

What I meant in my second post, is that there will/should never be two endpoints at the same URL which differ only by case, for example:

The above would be bad API design in my opinion.

That brings us back to case-sensitivity, I've checked the link you provided.
While HTTP.REQ.HOSTNAME is setting text mode to case insensitive, I'm not sure if the same applies to HTTP.REQ.URL (and related expressions). Also from the documentation:

IS_STRINGMAP_KEY ([text_t](https://developer-docs.citrix.com/projects/citrix-adc-advanced-policy-expression-reference/en/latest/text_t/) string_map_name)

Returns true if the string that it operates on is present in the string map. Text mode IGNORECASE, NOIGNORECASE - controls whether key lookup in string map is case sensitive or not. For example:

HTTP.REQ.URL.IS_STRINGMAP_KEY("url_stringmap") returns true if value of HTTP.REQ.URL is present as key in the string map whose name is "url_stringmap".
HTTP.REQ.URL.SET_TEXT_MODE(IGNORECASE). IS_STRINGMAP_KEY("url_stringmap") returns true if value of HTTP.REQ.URL is present as key in the string map whose name is "url_stringmap". Key lookup in this case is done case-insensitively.
Parameters (expressions not allowed):

string_map_name- identifies the string map.

Returns: [bool_at](https://developer-docs.citrix.com/projects/citrix-adc-advanced-policy-expression-reference/en/latest/bool_at/)

To make sure the IS_STRINGMAP_KEY function returns true for one value, we needed to make sure that we can match it (even if the end-user, or even the admin, is unaware of the correct casing).
At the time, TO_LOWER was the easiest option, given that we wanted to take into account my first remark about avoiding confusion.

To summarize,
While I would love to have the stringmap key to be case-sensitive, especially in regards to the URL specification, it would require a lot of testing to make it always works the way it is supposed to work (from an end-user's perspective who doesn't care about case sensitivity in the browser bar) in CoreLogic.

Of course, we don't touch the information in the request, so if the backend webserver is case-sensitive, the request would still fail.
The intent of CoreLogic here, is to route the traffic through NetScaler, and perform some basic checks (hence our view that good API-design will not have two endpoints which only differ by applying case sensitivity).

So allow me to turn the question back to you; how would you suggest to handle it?

  • Enforce case-sensitivity in the stringmap key (for the csvserver name and the fqdn/url entry)?
  • How would you go about make sure it works if the user misspells the URL but it should work on the backend (maybe we should provide a mechanism to notify the user that we do find an entry where the casing is different?)

Don't get me wrong I'm definitely willing to change the behaviour, but it needs to be tested throughly 😄

@minos314
Copy link
Author

minos314 commented Dec 1, 2022

Hi Jan,
For me it seems it would be the best to let the end user decide wether he wants to have the simplicity and convenience with case insensitivity, or the more secure and explicit case sensitive routing behaviour.

Variant A
This could be a global setting (CoreLogic case sensitive edition), which means there would be 2 seperate versions.

Variant B
Alternatively this could be based on each mapping line in the control stringmap:

cs_demo;any;anycase;myhost.com/rest  -> mapping as usual, case insensitive
cs_demo;any;casesensitive;myhost.com/AnotherPath  -> matches only if case matches exactly

The logic then has then to respect these settings. So imho if a user misspells a URL which is configered explicitly case sensitive by the admin it should simply route to NOTFOUND. But this change would almost double the code of CoreLogic, and could also doubles the amount of lookups needed for each request.

So feel free to take this as feature request for the next version, but I would also understand if you decide to leave it that way. It is quite a lot of work for a relatively small feature.

In our case: We already branched CoreLogic and did quite some modifications over the time (use X-Forwarded-For instead of the client IP, added some logic for special routing for authenticated users, custom error pages), so we have to address the case sensitivity in our own code. I think we would go with variant A.

Cheers,
Simon

@jantytgat
Copy link
Contributor

Hi Simon,

I'll have to investigate what it means for the code to support both modes of operation (as the package engine isn't flexible in that regard in the way we have to define/develop expressions). It might double or even triple the amount of code required.

However, my biggest fear is that it will be more cumbersome to troubleshoot. The strength in the current code is that it just works, always (if your input as an administrator is correct, obviously).

If it is too much of a hassle, I would opt for the case-sensitive mode (with the addition that we can enable a redirect if we find an entry that resembles the failed request, i.e., the different casing of the request).

Would you be willing to share the changes you made, so I can see whether it is in the general interest to have them included in the public version? There are already many changes and improvements coming in the new release, so taking on some other stuff might be simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants