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

Wip/bewest/express slow down #7469

Closed
wants to merge 4 commits into from
Closed

Conversation

bewest
Copy link
Member

@bewest bewest commented Jun 30, 2022

Introduce using https://www.npmjs.com/package/express-slow-down. The express-slow-down library allows specifying when the delay will start taking affect without any additional maintenance on on our end.

Some potential discussion points:

  • Only authorization endpoints have rate limiting.
    Should all API endpoint have rate limiting?
    • should different rules apply to different endpoints?
  • Are the defaults reasonable defaults to accomplish our goals? This defaults to using a window over 30 seconds. The first 100 requests can happen at full speed, after which all requests will experience an additional 500ms delay.

Replace usage of our delayIp with a popular middleware.
@bewest bewest requested a review from sulkaharo June 30, 2022 21:29
This restores a delay when verifyauth indicates that a request is not
authorized.
This allows implementing a longer delay with the request is
unauthenticated.
@sulkaharo
Copy link
Member

Is the change based on speculation or observed issues with the existing implementation?

@bewest
Copy link
Member Author

bewest commented Jul 11, 2022

Good questions, @sulkaharo. Observed, although the environment and harness may have had unrelated issues. I believe I had issues with valid concurrent requests getting delayed. I'm fine with delaying this while I collect more data.

My usage was via staging site exercising https://github.com/t1pal/nightscout-roles-gateway. I can use these patches to help evaluate the behavior I saw.

@sulkaharo
Copy link
Member

There should effectively be no cases where valid usage of Nightscout gets delayed. The only case I know where this has failed is with misconfigured proxies that fail to forward the IP in the header, resulting in all requests seemingly coming from the same IP.

@bewest
Copy link
Member Author

bewest commented Jul 11, 2022

I can remove the added delays to restrict only the unauthorized delays. I did observe delays on valid requests unrelated to a misconfigured load balancer. My reading suggested that concurrent requests would be delayed before checking validity, although there are other possibilities I'm still closing out.

@sulkaharo
Copy link
Member

Delays on valid requests sounds curious, since the logic only delays an IP in case the IP repeatedly trying to authenticate using the wrong credentials, so the only means to trigger this behavior is through a user having misconfigured an access key and the client repeatedly trying to authenticate without user realising what's going on, or a hacker trying to guess a passkey. Also btw note the admin message UI that informs a valid user their client is trying to use wrong credentials or someone is trying to hack their site relies on the IP list, so if the old logic is removed, you still need to have logic in place to support the admin message feature.

@sulkaharo
Copy link
Member

Related: we should probably change the flag that disables the admin message feature into only disabling the alarm on Nightscout being visible to the world. Right now the flag is being used by people who have no idea about the implications of disabling the feature and thus not being informed if they have a misconfigured client spamming their site (causing support load for the community, as the admin message feature also intended to help debug misconfigured clients) or if someone is trying to hack their site (at which point it makes sense to change the URL and keep it secret).

@bewest
Copy link
Member Author

bewest commented Jul 16, 2022

Great points, @sulkaharo. Could you say more about what you mean for the notifies flag? I think I agree if you mean that flags should be able to filter kinds of messages rather than turning the messaging feature itself off. Are there categories of messages that can/should be filtered?
Another related feature: In terms of detecting valid fowarded-by IPs, it may be useful to specify a limited set of trusted CIDR ranges to help specify only valid traffic flows, this can help reduce exposure to malicious traffic.

After some further experiments, it looks like the delay was due to an issue with my environment, and unrelated to the delay implementation. I think this alternative was a nice case study/exercise but probably doesn't offer any value for the remaining effort left. FWIW, it looks like I had two Nightscout instances that were load balanced under the same interface, but out of sync in terms of tokens. Eg: requests for xyz.backend-ns.local were load balanced between NS A and NS B, both instances pointed at the same mongo database with same API Secret. If we get a valid token from NS A, what happens when NS B attempts to verify it? It looks like NS B may not verify that token? In any case, this isn't caused by any issue with the delay logic, as you pointed out. We can close or archive this for now.

@bewest bewest closed this Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants