Re: [sipcore] AD review: draft-ietf-sipcore-event-rate-control-03

[Robert Sparks <[e-mail]>]

inline:

On Jul 12, 2010, at 7:13 PM, [e-mail] wrote:

> Hi Robert,
> 
> I submitted the -04 version today addressing your comments: http://www.ietf.org/id/draft-ietf-sipcore-event-rate-control-04.txt 
> 
> Please find my answers in-line with [KK]:
> 
> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On Behalf Of ext Robert Sparks
> Sent: 2010. június 10. 13:14
> To: SIPCORE
> Subject: [sipcore] AD review: draft-ietf-sipcore-event-rate-control-03
> 
> Summary: This draft has a few adjustments that are needed before moving it into IETF Last Call.
> 
> Major question:
> 
> Why isn't this an Update to 3265? 
> 
> [KK] It's an extension to 3265. Implementations of 3265 not interested in rate control don't need to implement it. If people think it's an essential part of event-notifications, we could make it as an update of 3265. Any recommendations?

We have a separate thread going for this one.

> 
> Is there text here that prevents a subscriber
> from generating Event headers in 200 OKs to NOTIFYs mid-subscription (when he
> didn't probe for support using the SUBSCRIBE?) How would they know the request
> got honored?  The possibility of running into implementations that break should
> be called out.  
> 
> [KK] I added text in -04 to address this: 
> Section 4.1: "If the Event header field of the SUBSCRIBE request did not include the "min-interval" parameter, the subscriber MUST NOT include an initial value of the "min-interval" Event header field parameter in a 200-class response to the NOTIFY request." 
> Section 4.2: "If the Event header field of the SUBSCRIBE request did not include the "min-interval" parameter, the notifier MUST ignore an initial value of the "min-interval" Event header field parameter in a 200-class response to the NOTIFY request, if present."
> ...and similar text covering max-interval and average-interval mechanisms.

So, which SUBSCRIBE? I think you mean the most recent SUBSCRIBE request received on this dialog (and not just the initial SUBSCRIBE)?

> 
> 4.2 indicates the subscriber only gets a "hint" about support
> for rate-control in the notifier - is the condition it describes really only a
> hint?
> 
> [KK] Replaced "serves as a hint" with "indicates".
> 
> In several places, the notifier is given permission to adjust an interval based
> on local policy.  The document should be explicit about allowing the adjustment
> in any direction (increasing or decreasing) since there are so many other uses
> of intervals in SIP and SIP Events that allow adjustment only in one direction.
> A few places I noted when reading the document were the Note in REQ7, 4.3 4th
> paragraph, 5.2 paragraph 3, 6.2 paragraph 3.
> 
> [KK] I spelled out the possibility of increasing and decreasing at all of these sections you referenced.
> 
> The last paragraph of section 3.6 claims "exactly the same properties" except
> for being generated constrained to a schedule. Can you clarify which properties
> you mean? Many properties of the notifications beside their timing are clearly
> different (for instance, you may miss state transitions).
> 
> [KK] I just deleted that sentence, there was no real added value in it.
> 
> The security considerations section deserves more text: 
> * What is the forward reference from section 3.4 supposed to be pointing to?
> 
> [KK] Deleted.
> 
> * Call out the implications on a Notifier having to store/aggregate partial state
> 
> [KK] I added a reference to the security considerations listed in RFC 5263 (partial notifications).
> 
> * Note that the Event header (particularly in 200 OKs) is not integrity protected. 
>  This would allow anything that could modify the message in flight (or an 
>  eavesdropper that could race a 200 OK in) to suppress (or flood) notifications 
>  without the subscriber seeing what caused it.
> 
> [KK] I added a new paragraph on this:
> "RFC 3265 [RFC3265] recommends the integrity protection of the Event header field of SUBSCRIBE requests. Implementations of this extension SHOULD also provide integrity protection for the Event header field included in the 200-class response to the NOTIFY request."

I would still call out the attacks that are possible when it isn't integrity protected.

> 	
> The assertion that applying rate limiting and compression together results in
> savings as good as the sum of applying them independently should be supported
> or adjusted. I think it's sufficient to say they can be applied together.
> 
> [KK] OK, so no changes needed, right?

No, not OK. I think you misunderstood my comment.

You should either add text justifying the claim, or change the text. I propose above to change the text
to simply say they can be applied together and not make an unsupported claim about the savings being as
good as the sum of applying them together.

> 
> Below are several suggestions for text tweaks. The first few (staring with *)
> are the most important. 
> 
> * Section 3.2 paragraph 4: suggest replacing "does not typically" with "may not"
> 
> [KK] Fixed.
> 
> * Section 3.2 last paragraph: The sentence 'The "max-interval" parameter 
>      indicates ... complete state information' is difficult to parse. Could it
>      be simplified?
> 
> [KK] Done.
> 
> * Section 4.3 first paragraph, last sentence: "For such cases" is ambiguous.
>  Suggest "If the min-interval value is greater than the subscription expiry".
> 
> [KK]. Done. Re-wrote as: "If the subscription expiry is shortened during an active subscription,..."
> 
> * Section 6.2 last paragraph: This currently says the timeout mechanism does
>  not affect when 3261 transaction retransmissions are generated. It should
>  also explicitly note that retransmissions do not affect the calculation of
>  the next timeout.
> 
> [KK]. Fixed by extending the definition of "count":  
> "count: The number of notifications that have been sent during the last "period" of seconds ***not including any retransmissions of	requests***." New text is between *** marks.
> 
> Introduction, paragraph 2: suggest replacing "congestion" with "load"
> 
> [KK] Fixed.
> 
> Section 3.1  paragraph 3: suggest replacing "amount of traffic" with 
> "number of notifications"
> 
> [KK] Fixed.
> 
> Section 3.5 paragraph 1: Suggest a reference for RLS after "list subscription".
> 
> [KK] Fixed.
> 
> The sentence "Moreover, the list event notifier..." should be more explicit
> about using the rate mechanism for any back-end subscriptions it might have.
> 
> [KK] Fixed.
> 
> Suggest referencing 3261 in the last paragraph of 4.2
> 
> [KK] Fixed. Same applies to section 6.2.
> 
> Section 4.3, 3rd paragraph last sentence. The only way the subscriber _can_
> resume notifications is to renew the subscription with a resubscribe request.
> Would this text work? "This results in receiving no further notifications until
> the subscription expires or the subscriber sends a SUBSCRIBE request refreshing
> the subscription (perhaps resuming notifications)".
> 
> [KK] Fixed as "or the subscriber sends a SUBSCRIBE request resuming notifications."
> 
> The text needs to be adjusted to reflect subnot-etags being issued as an RFC
> 
> [KK] Fixed throughout the document.
> 
> Adam suggested some RFC-Editor notes in the proto writeup (which may address
> some of the above comments). Please be sure to incorporate those when revising
> the draft.
> 
> [KK] Done. Fixed the text for subnot-etags references throughout the document.
> 
> One last question:
> 
> If the combination of min-interval, max-interval, and average-interval make
> little sense, why does the document allow them to be combined? I think what the
> group was trying to say is that we currently don't forsee a use for combining
> those options, but do not wish to forbid their combination.
> 
> [KK] Fixed as "this combination makes little sense to be used although not forbidden". Recommendations added on choosing the right values similarly to previous options for combinations.
> 
> 
> Thanks,
> Krisztian
> 
> _______________________________________________
> sipcore mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/sipcore