IP check policy shoud check all "client_ip_sources"

[y-tabata]

I think if a user selects multiple "client_ip_sources", IP check policy should check all "client_ip_sources".
Current behavior is not intuitive.

For example, if the following API request comes with the following configuration, this request should be denied.

• API request

"X-Forwarded-For": 2.2.2.2
"X-Real-IP": 3.3.3.3
"last_caller": 3.4.5.6

• configuration

{
  "name": "ip_check",
  "configuration": {
    "ips": [ "3.4.5.6", "1.2.3.0/4" ],
    "check_type": "blacklist",
    "client_ip_sources": ["X-Forwarded-For", "X-Real-IP", "last_caller"]
  }
}

Current behavior:

• whitelist

first "client_ip_source" check	second "client_ip_source" check	third "client_ip_source" check	result
include client ip	-	-	not deny
not include client ip	-	-	deny

• blacklist

first "client_ip_source" check	second "client_ip_source" check	third "client_ip_source" check	result
include client ip	-	-	deny
not include client ip	-	-	not deny

Expected behavior:

• whitelist

first "client_ip_source" check	second "client_ip_source" check	third "client_ip_source" check	result
include client ip	-	-	not deny
not include client ip	include client ip	-	not deny
not include client ip	not include client ip	include client ip	not deny
not include client ip	not include client ip	not include client ip	deny

• blacklist

first "client_ip_source" check	second "client_ip_source" check	third "client_ip_source" check	result
include client ip	-	-	deny
not include client ip	include client ip	-	deny
not include client ip	not include client ip	include client ip	deny
not include client ip	not include client ip	not include client ip	not deny

[davidor]

Hi @y-tabata

I think I understand better the issue now. The code of the policy assumes that the list of sources (X-Forwarded-For, X-Real-IP, last_caller) can be sorted. However, the list cannot be sorted in the 3scale UI.

We'll need to make it sortable somehow or if it's not possible, we'll need to change the behavior to what you described.

Ref: https://issues.jboss.org/browse/THREESCALE-1692

[davidor]

The list of IP sources cannot be sorted using the current schema of the policy. For some reason, the list becomes sortable in react-jsonschema-form when the uniqueItems attribute is removed: https://github.com/3scale/apicast/blob/3fa1f9eb21f83d41f2b0495014ded2c10df9e8d9/gateway/src/apicast/policy/ip_check/apicast-policy.json#L46

I see 2 options depending on what we want to achieve.

1. Check whitelisted/blacklisted IPs in all the sources selected. In this case, we just need to modify the code to change its behavior.
2. Check whitelisted/blacklisted IPs in just the first source that is not empty. In this case, we'll need to modify the schema, remove the uniqueItems attribute so the list becomes sortable, and then, remove duplicates in the code ourselves.

Both options can be implemented easily. I don't have a strong preference. @mikz what's your opinion on this?

[davidor]

After speaking with @mikz , we don't see any strong points in favor of any of the 2 options I described in my previous comment, so we think that introducing a new config param to choose between those 2 options makes sense.

[thomasmaas]

Maybe take 1 option as the default one in the spirit of 'convention over configuration' if that makes sense?

[davidor]

@thomasmaas Yes, I think there should be a default one. Which one would you choose?

[thomasmaas]

Seems 1. is closest to the what's requested at the top of the issue so i would pick 1. as the default. Is there really any reason somebody wouldn't want 1., as in, is 2. really needed as an option?

Maybe we can start by just implementing 1. and only add config param if people ask for it?

[davidor]

I can't think of any strong argument in favor of either, so I prefer to offer both and let the users decide. We can change this later if we receive more feedback.

Regarding the default one, I'm leaning towards option 2. The reason is that it was the original behavior and we do not want to break backwards compatibility. It's true that the policy did not work properly when configured via the admin portal, but it worked using a config file.

[thomasmaas]

Once added, a feature is hard to take away. That’s why I think we shouldn’t add it if people are not asking for it and our only motivation is that we think both options are valid. The option that puts less mental burden on the user would be the one to pick and it seems to me that is option 1 as it would just always work without any added user actions like re-ordering.

I understand that option 2 reflects the current behavior and that re-ordering would allow the user to fix the problem herself. But I’m guessing this policy is not that widely used and therefore backward compatibility should not drive the decision here.

But maybe I’m overlooking a scenario where option 1 is not acceptable. In that case I stand corrected.

[davidor]

@thomasmaas The original way was defined in the requirements of a previous APIcast version and there might be some users with workflows depending on that behavior. I know the risk is low, but as I said before, there are not any strong arguments in favor of any of those 2 options in this issue so I prefer to offer both options.

[thomasmaas]

ok, seems we're repeating ourselves. Maybe @vramosp and @mikz can shine their lights on this. Or we could opt for a sword dual at sunset.

[davidor]

OK. Let's wait for product decision on this one.

There's a JIRA issue where someone asked to fix the original behavior: https://issues.jboss.org/browse/THREESCALE-1692

And the original poster of this issue described an alternative, which in my opinion is valid as well.

/cc @vramosp

[vramosp]

From a product perspective, the preference would be to provide a fix without introducing any breaking changes, no matter how low the risk is. So preference would be option 2.