Rough draft of reciever rate limiting for discussion #584

karmanyaahm · 2023-01-17T01:31:14Z

This is the original proposal from the Matrix room
me:

My main idea, the ideal solution (as PeterCxy proposed) is to move the cost, for at least UP rate limits, to the receiver instead of the sender.

There are many ways to do it, but one potential solution (a simple but clean-ish one I could think of):

Like id: and user:, make up* topics a up: visitor. [1]

This up: visitor (the UP topic) has large-ish rate limits (to prevent obvious spam/misconfiguration), but doesn't care too much.

Topic.Subscribe and Topic.Unsubscribe keep track of the most recent visitor to subscribe to a topic, and the time (say prev. 12hrs is valid). [2]

On Topic.Publish, the message is 'billed' to the most-recent-visitor's rate limit (either ip: or user:), rejected if there is no recent visitor.

Delete the message if successfully delivered to at least one client. UP topics are unique to each client anyways. [3]

[1] In which case up* topics have to be reserved explicitly for receiver-based rate limiting. We cannot rely on up=1 because the client doesn't control that.
[2] If all of this is in-memory: say someone loses their connection, wait 1 hour, ntfy restarts, wait 1 hour, they resubscribe. If there was a notification at T+90 minutes, that notification would be lost, since the topic would show up as 'fresh', and the notification rejected. However, this seems like a fair compromise for the simplicity of avoiding a database.
[3] Attempt delivering to multiple clients if they are online and subscribed though, for debugging purposes. This is just an extra resource-saving measure and not part of the core plan.

I can actually help you make it now, though, once we agree on a plan, since this is needed by a clear deadline.

binwiederhier:

I think I understand this plan. In my own words:

For topics starting with up*, we keep track of the visitors that are subscribed to a topic in the topic struct.

When a message is published, we determine the visitor based on the topic name instead of the IP (v := s.visitorByTopic("upABCDEF..")). If there are multiple visitors, we pick the most recent one.

The rest of the logic remains the same.

Correct?

me:

Basically, yes. The goal is to 'bill' that messsage to the most recently subscribed visitor instead of to the sender.

Additionally, this would require formally defining 'up*' as a special namespace, since messages are "billed" differently.

karmanyaahm · 2023-01-17T01:33:18Z

server/topic.go

+	// what if someone unsubscribed and DOESNT want their sub to count against them anymore, maybe the app server lost sync and will keep on sending stuff
+	// I guess they suffer for unifiedPushSubscriptionDuration?
+
+	// if lastunsub v exists, and the time since it was unsubbed is longer than our limit, it should not exist
+	if t.lastUnsub.v != nil && time.Since(t.lastUnsub.unsubTime) > unifiedPushSubscriptionDuration {
+		t.lastUnsub.v = nil
+	}


That. How do we know whether the last subscriber wants to cache messages (gone temporarily) or not (unsubscribed permanently)?

I don't understand what's happening here. Sorry.

Messages are being billed to the last visitor (either someone currently subscribed or the last person to unsubscribe). This bill-the-last-unsusbscriber behavior remains for unifiedPushSubscriptionDuration.

Let's say user:KM is subscribed to ntfy.sh/upABCD, and unifiedPushSubscriptionDuration := 10 * time.Hour

Scenario 1:
06:00 user:KM's internet went out and they unsubscribed from ntfy.sh/upABCD
06:15: ntfy.sh receives a message at upABCD. It is stored and billed to KM
06:30 user:KM is online again. The missed message is delivered and everyone's happy.

Scenario 2:
06:00 user:KM's internet went out and they unsubscribed from ntfy.sh/upABCD
06:15: ntfy.sh receives a message at upABCD. It is stored and billed to KM
15:59: ntfy.sh receives a message at upABCD. It is stored and billed to KM
16:01: ntfy.sh receives a message at upABCD. t.lastUnsub.v is set to nil. The message is rejected (412 or whatever else).
23:59 user:KM is online again. They missed some messages, but it's not that big of a deal.

Scenario 3:
06:00 user:KM's intentionally unsubscribed from ntfy.sh/upABCD. They don't want upABCD messages anymore, but the app server keeps sending them.
06:15: ntfy.sh receives a message at upABCD. It is stored and billed to KM
15:59: ntfy.sh receives a message at upABCD. It is stored and billed to KM
16:01: ntfy.sh receives a message at upABCD. t.lastUnsub.v is set to nil. The message is rejected (412 or whatever else).
user:KM is angry that 2 (or 200 or 2000) messages from their rate limit were eaten up by this random app server.

karmanyaahm · 2023-01-17T01:36:11Z

server/server.go

+	var v_billing *visitor
+	if strings.HasPrefix(t.ID, unifiedpushTopicPrefix) {
+		v_billing := t.getBillee()
+		if v_billing != nil {
+			// instant reject and won't even store it if there's no one registered for a UP topic in the past some time
+	// need to find error code for device not available try again later
+			return nil, errHTTPInternalError
+		}
+	}


If there is no last subscriber, or there was a subscriber very long ago, what should ntfy do?

Cache it anyway but bill it to the app server - inconsistent

Reject it. Which status code? 500?

*should be == not !=

I think this is kind of up to the UP spec, no? "What should the push server do if there is no one listening" seems like a spec question.

Obviously the easiest thing would be to reject it. Everything else would cause lots of headscratching.

The response code may be https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/412?

karmanyaahm · 2023-01-17T01:36:36Z

server/topic.go

 	}
 }

+// need a better name for bill?


Is the term "Bill" fine?

(apparently Billee is not a real word)

Invoiced? Billed visitor/account? Charged? Accounted?

Other "Bill" synonyms: cost, expense, tally

t.SubscriberVisitor()?

binwiederhier · 2023-01-22T02:10:28Z

server/server.go

+	var v_billing *visitor
+	if strings.HasPrefix(t.ID, unifiedpushTopicPrefix) {
+		v_billing := t.getBillee()
+		if v_billing != nil {
+			// instant reject and won't even store it if there's no one registered for a UP topic in the past some time
+	// need to find error code for device not available try again later
+			return nil, errHTTPInternalError
+		}
+	}


I think this is kind of up to the UP spec, no? "What should the push server do if there is no one listening" seems like a spec question.

Obviously the easiest thing would be to reject it. Everything else would cause lots of headscratching.

The response code may be https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/412?

binwiederhier · 2023-01-22T02:11:53Z

server/server.go

+		v_billing.IncrementMessages()
+	} else {
+		v.IncrementMessages()
+	}


I would generally opt for replacing v early in this function, instead of adding if-then-elses everywhere.

Case and point: There's a v.MessageAllowed() at the top which would be run against the IP-visitor and not the subscriber visitor.

Yeah, but in that case, logs and email-allowed need to be if-then-elsed (which makes more sense I guess, since there's fewer of those things). I'll figure out a way to minimize complications.

binwiederhier · 2023-01-22T02:12:43Z

server/topic.go

 	}
 }

+// need a better name for bill?


t.SubscriberVisitor()?

binwiederhier · 2023-01-22T02:14:03Z

server/topic.go

+	// what if someone unsubscribed and DOESNT want their sub to count against them anymore, maybe the app server lost sync and will keep on sending stuff
+	// I guess they suffer for unifiedPushSubscriptionDuration?
+
+	// if lastunsub v exists, and the time since it was unsubbed is longer than our limit, it should not exist
+	if t.lastUnsub.v != nil && time.Since(t.lastUnsub.unsubTime) > unifiedPushSubscriptionDuration {
+		t.lastUnsub.v = nil
+	}


I don't understand what's happening here. Sorry.

binwiederhier · 2023-01-22T02:15:44Z

server/server.go

@@ -544,6 +546,17 @@ func (s *Server) handlePublishWithoutResponse(r *http.Request, v *visitor) (*mes
 	if err != nil {
 		return nil, err
 	}
+
+	var v_billing *visitor
+	if strings.HasPrefix(t.ID, unifiedpushTopicPrefix) {


Here's an idea: instead of using the prefix, could we use the presence of the ?up=1 query param? If the qurey param is present, bill differently? I vaguely remember this being discussed before, but I do not recall what the outcome was

From the UP dev chat

up=1 is sent by the server, whereas these messages are being billed to the subscriber. I thought about that too, but then came up with this:

I am subscribed to ntfy.sh/videogameupdates, an open channel for anyone to post

A malicious actor comes along and sends 10000 messages to ntfy.sh/videogameupdates?up=1

Me, poor subscriber, is now rate-limited

So, we need a way in which the subscriber knows that they are subscribing to a subscriber-billed topic instead of trusting the sender to inform us. The easiest way is the topic.

karmanyaahm · 2023-02-13T01:48:50Z

Closing in favor of a new PR (coming soon)

very very rought draft of reciever rate limiting

84e2ee9

karmanyaahm commented Jan 17, 2023

View reviewed changes

binwiederhier reviewed Jan 22, 2023

View reviewed changes

karmanyaahm mentioned this pull request Jan 22, 2023

Retransmission is not specified UnifiedPush/specifications#34

Open

karmanyaahm closed this Feb 13, 2023

karmanyaahm mentioned this pull request Feb 15, 2023

Subscriber Billed Topics #609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rough draft of reciever rate limiting for discussion #584

Rough draft of reciever rate limiting for discussion #584

karmanyaahm commented Jan 17, 2023 •

edited

Loading

karmanyaahm Jan 17, 2023

binwiederhier Jan 22, 2023

karmanyaahm Jan 22, 2023

karmanyaahm Jan 17, 2023

karmanyaahm Jan 17, 2023

binwiederhier Jan 22, 2023

karmanyaahm Jan 22, 2023

karmanyaahm Jan 17, 2023

karmanyaahm Jan 17, 2023

MayeulC Jan 17, 2023

binwiederhier Jan 22, 2023

binwiederhier Jan 22, 2023

binwiederhier Jan 22, 2023

karmanyaahm Jan 22, 2023

binwiederhier Jan 22, 2023

binwiederhier Jan 22, 2023

binwiederhier Jan 22, 2023

karmanyaahm Jan 22, 2023

karmanyaahm commented Feb 13, 2023

Rough draft of reciever rate limiting for discussion #584

Rough draft of reciever rate limiting for discussion #584

Conversation

karmanyaahm commented Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmanyaahm commented Feb 13, 2023

karmanyaahm commented Jan 17, 2023 •

edited

Loading