Kong Rate Limiting Plugin not working as expected when limit by path

cunkz commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Kong version (`$ kong version`)

2.5

Current Behavior

When I add Rate Limiting plugin, I do this following step :

set config.limit_by with path
set config.path with /abc
hit other path like /zxc

Kong also do rate limit in /zxc path

Expected Behavior

I assume Kong didn't rate limit it. Then I check this line : https://github.com/Kong/kong/blob/009ee44a11b1d6d3fd5426a0b79f4892152f48ad/kong/plugins/rate-limiting/handler.lua#L73

And https://github.com/Kong/kong/blob/009ee44a11b1d6d3fd5426a0b79f4892152f48ad/kong/plugins/rate-limiting/handler.lua#L79

When request_path didn't same with config.path why Kong didnt bypass it ? But, he change identifier to forwarded ip ?

Steps To Reproduce

set config.limit_by with path
set config.path with /abc
hit other path like /zxc

Anything else?

No response

hanshuebner commented 1 year ago

Hello @cunkz,

sorry that it took so long to respond. I notice that you're using a very old version of Kong that we're not maintaining anymore. Please consider upgrading to the latest release. If the problem exists in Kong Gateway 3.2, please feel free to reach out again.

Kind regards, Hans

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Clint-Chester commented 1 year ago

We are seeing this issue as well with the Advanced Rate Limiting Plugin. It doesn't seem like matching by path is actually matching any paths specified, and is applied to the lot.

hanshuebner commented 1 year ago

@Clint-Chester The Rate Limiting Advanced plugin is an enterprise feature - Please reach out to your support contact for assistance. Thank you!

Clint-Chester commented 1 year ago

@Clint-Chester The Rate Limiting Advanced plugin is an enterprise feature - Please reach out to your support contact for assistance. Thank you!

Thanks @hanshuebner, tried the standard Rate Limiting Plugin and got the same behaviour unfortunately.

hanshuebner commented 1 year ago

@Clint-Chester I'm going to re-open this ticket. You may get expedited responses through the enterprise support channel, though.

Clint-Chester commented 1 year ago

Current configuration is the following.

apiVersion: configuration.konghq.com/v1
kind: KongPlugin
metadata:
  name: website-login-rate-limiting
plugin: rate-limiting
config:
  limit_by: path
  path: /api/login
  minute: 70
  sync_rate: -1
  hide_client_headers: true
  error_message: "Login Rate Limit Exceeded"

With the following as my Ingress Controller Configuration.

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend

Is the behaviour that it's going to try and match all paths?

Thanks for re-opening @hanshuebner, appreciate it! We've got Enterprise Support Open as well but was very interested in seeing if others had experienced this in the past.

StarlightIbuki commented 1 year ago

@Clint-Chester By the name ("login") I guess smaller limitations are configured than general requests? limit-by uses a fallback mechanism to determine its limit scope, that is if the request path does not match the configured path, it will use IP to limit the request. So the behavior is kind of expected. I also feel this behavior makes little sense. I'm looking for more information and opinions about this.

mheap commented 1 year ago

I can't comment on the rate limiting defaults, but to achieve what you're looking for with Ingress you can define two Ingress resources:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: login-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend

The konghq.com/plugins annotation applies to all routes in an Ingress definition, which means they should be split if you only want the plugin to apply to specific routes

Clint-Chester commented 1 year ago

@Clint-Chester By the name ("login") I guess smaller limitations are configured than general requests? limit-by uses a fallback mechanism to determine its limit scope, that is if the request path does not match the configured path, it will use IP to limit the request. So the behavior is kind of expected. I also feel this behavior makes little sense. I'm looking for more information and opinions about this.

Yes that's correct, it would be looking to have a smaller limit on a particular path.

So the behaviour in this example is that you'd want to be mitigating a DDoS attack or a credential stuffing attack used a bot network. Rate limiting by IP address doesn't work here because the attack is cycling through different IP addresses. By doing rate limiting via the path, it acts as a kill switch on the API to slow down the damage.

Is there something missing in my path matching? As I've tried a lot of different combinations to no avail.

Clint-Chester commented 1 year ago

I can't comment on the rate limiting defaults, but to achieve what you're looking for with Ingress you can define two Ingress resources:
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: login-ingress
  annotations:
    konghq.com/plugins: website-login-rate-limiting
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Exact
        path: "/api/login"
        backend:
          service:
            name: web-api-backend
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: web-ingress
spec:
  ingressClassName: kong
  rules:
  - host: "website.com"
    http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: web-frontend
The konghq.com/plugins annotation applies to all routes in an Ingress definition, which means they should be split if you only want the plugin to apply to specific routes

I did actually try this but it still didn't seem to work. It makes me think that what @StarlightIbuki was talking about that the path match must not have been detecting at all, and it was defaulting to IP rate limiting.

StarlightIbuki commented 1 year ago

@Clint-Chester The plugin falls back to limit by IP when the request path does not match the configured one. I do not understand the design's intention so I'm trying to confirm.

StarlightIbuki commented 1 year ago

The fallback mechanism seems to be introduced when we have only consumer and credential for limit-by, and it can't take later introduced type into consideration.

Please refer to the doc:

If the underlying service or route has no authentication layer, the Client IP address is used. Otherwise, the consumer is used if an authentication plugin has been configured.

I think it makes no sense to fall back when we limit by path.

tzssangglass commented 1 year ago

If there is no fallback, when limit by path fails, it is equivalent to no limit, which is similar to bypass.

StarlightIbuki commented 1 year ago

We had a discussion and this seems a legacy of old design, and the fallback here is not useful at all. We are going to redesign the plugin. For now, a possible workaround is to have the path limited split to a separate route and config the plugin on it. @cunkz @Clint-Chester

dilanka-cacib commented 6 months ago

Hello @StarlightIbuki, Can I know possible fix date of this bug? Thanks

nvervelle commented 1 month ago

Hello !

I just found this issue after opening a discussion on this subject.

I think the fix is quite simple, and I can open a PR for it if it helps. Just tell me

StarlightIbuki commented 1 month ago

Hello !

I just found this issue after opening a discussion on this subject.

I think the fix is quite simple, and I can open a PR for it if it helps. Just tell me

Hi. Thanks for your kind offering. This is more of a design issue than an implementation bug, and unfortunately, it did not draw enough attention for a production decision. I'm creating an internal ticket and asking for comments.

Tracked by KAG-5426

Personally, I agree with the solution you proposed (that's also the same as I thought of). PRs are welcome. Please refer to https://github.com/Kong/kong/blob/master/CONTRIBUTING.md

Kong / kong