Re: [sipcore] AD review: draft-ietf-sipcore-keep-08

[Robert Sparks <[e-mail]>]

On Nov 24, 2010, at 1:19 PM, Christer Holmberg wrote:

> 
> Hi Robert,
> 
> Comments inline ([CHH])
> 
>> Summary: Revised ID needed
>> 
>> The text needs to be adjusted to reflect the security issue I 
>> called out in a separate message to the list today.
>> During that adjustment, there are a few other places to 
>> consider making changes:
> 
> [CHH] See other thread.
> 
> ------
> 
>> * Section 4.4, second paragraph "The parameter can contain" 
>> is confusing - it could be read to imply that the parameter 
>> can contain other things as well. I suggest rewriting this 
>> sentence as "The parameter value, if present, represents a 
>> recommended keep-alive frequency..."
> 
> [CHH] Done.
> 
> "The parameter value, if present and with a value other than zero, represents a recommended keep-alive frequency, given in seconds."
> 
> 
> -------
> 
>> * The document talks about invoking keep for INVITE initiated 
>> dialogs, but not about SUBSCRIBE initiated dialogs. Was it the 
>> intent to not allow the use of the mechanism for SUBSCRIBE initiated 
>> dialogs?
> 
> [CHH] The intent was to allow it for INVITE initiaded dialogs.
> 
> I can't remember any technical reasons why keep could not be applied also to SUB dialogs. Nobody has ever requested it, however.
> 
> 
> -------
> 
>> * Section 4.4 talks about normative server behavior for using 
>> this mechanism with INVITE initiated dialogs.
>> where is the companion set of normative requirements for 
>> using the mechanism to keep alive a REGISTRATION
>> flow?
> 
> [CHH] The 2nd paragraph is the generic rule, which covers both INVITE and REGISTER.
> 
> The 3rd paragraph is an addition that covers the case where multiple responses are sent to a request, which is INVITE specific.
> 
> But, maybe to make it more clear, and based on your comment on section 5 (see further down), I could rewrite the 3rd paragraph:
> 
> "There might be multiple responses to an INVITE request.
> When a SIP entity indicates willingness to receive keep-alives in a
> response to an INVITE request, it MUST add a parameter value to the 
> "keep" parameter in at least one reliable response to the request. The SIP entity MAY
> add identical parameter values  to the "keep" parameters in other responses to the
> same request.  The SIP entity MUST NOT add different parameter value to the "keep" 
> parameters in responses to the same request. The SIP entity SHOULD indicate 
> the willingness to receive keep-alives as soon as possible."

This text is better, but it uncovers another issue. For these keep-alives associated with
a dialog, you need to adjust the text to reflect responses _in that dialog_. In the presence
of forking, you may have multiple 183s from different branches (or multiple 200 OKs from
different branches. If the keep-alive activity is intended to be associated with the dialogs
(which is what I think the text is currently trying to do), you need to adjust to note that the
element needs to include its keep value as early as possible on each dialog, and note that
the keep-alive messages will be sent reflecting each dialog that was set up.

An alternative is to negotiate one keepalive stream per 5-tuple, using a reference count
(you stop when the last dialog using the 5-tuple terminates), but that seems pretty error-prone.

> 
> 
> The same editorial change would also be done in section 3:
> 
> ""keep" parameter: A SIP Via header field parameter that a SIP entity
> can insert in its Via header field of a request to explicitly
> indicate willingness to send keep-alives towards its adjacent
> downstream SIP entity.  A SIP entity can add a parameter value to 
> the "keep" parameter in a response to explicitly indicate willingness to 
> receive keep-alives from its adjacent upstream SIP entity."
> 
> ------- 
> 
>> *  In several places, the document refers to an element 
>> inspecting "its" Via header. This is imprecise and will
>> lead to arguments among implementers. I suggest 
>> explicitly calling out "the topmost Via header field value"
>> and being very specific about when you are talking about a 
>> message being received or a message being sent.
> 
> [CHH] I will look into that.
> 
> -------
> 
>> * In section 5's fourth paragraph, it would help avoid 
>> confusion  to say "adds a value the a 'keep' parameter to
>> indicate willingness" instead of "uses the 'keep' parameter...".
> 
> [CHH] Done.
> 
> "If a SIP entity that adds a parameter value to the "keep" parameter, in order to indicate willingness to receive keep-alives, also inserts..."
> 
> -------
> 
>> *In the last sentence of Section 5 - what value is "without 
>> forcing entities to re-write the value of Flow-Timer header 
>> field" adding? I suggest deleting the phrase.
> 
> [CHH] Done.
> 
> -------
> 
>> * Section 6's first paragraph says "Entities that want to 
>> reuse connections MUST use a another mechanism".
>> (Note the spurious 'a'). Why is this a 2119 MUST? I 
>> suggest saying "would need to" instead.
> 
> [CHH] Done. 
> 
> The reason behind the "MUST" was to clearly indicate that keep as such is not a connection reuse mechanism.

That's an incorrect use of the 2119 term. 
I can't test implementation of that requirement - there's no way to account for it in an interoperability report, and there's
no good reference to what an implementer would need to read in order to satisfy the requirement. You are really just
trying to say this mechanism doesn't provide connection reuse. Just say that.


> 
> -------
> 
>> * The first sentence in 7.4 would be better if it said Figure 
>> 3 instead of "The figure". The end of the first
>> paragraph has a spurious period.
> 
> [CHH] Done.
> 
> -------
> 
>> * All of the examples showing Via header fields use a 
>> shorthand, pseudo-code style representation of the
>> header field format. It would be good to explicitly note 
>> that in the document. It would be better to provide
>> one example that was syntactically correct.
> 
> [CHH] I assume you mean that, instead of "Via:UAC" and "Via:P1" I would use something like "Via:uac.operator.com" and "Via:p1.operator.com"?
> 
> Personally I think it's more clear to use pseudo-style (as the example is not supposed to be teach-yourself-SIP), but I can change according to your suggestion.
I'll respond to this through your followup message.

> 
> -------
> 
>> * In Section 10's section paragraph. I suggest that you be 
>> explicit that you are talking about hop-by-hop
>> integrity protection (calling out TLS or DTLS), to avoid 
>> confusion with end-to-end integrity protection
>> mechanisms (S/MIME) which will not help this case.
> 
> [CHH] I suggest the follwoing change (<new></new>):
> 
> "Unless SIP messages are integrity protected <new>hop-by-hop (e.g. using TLS or DTLS)</new>, 
> a man-in-the-middle can modify Via header fields...."
> 
> -------
> 
>> * In section 10's third paragraph, you call out "(at high 
>> rates)". This is potentially misleading - the rates
>> are at the highest somewhere just under once a second. I 
>> suggest replacing "(at high rates)" with
>> "(up to approximately once a second)".
> 
> [CHH] Done.
> 
> -------
> 
> Thank You for reviewing the document!
> 
> Regards,
> 
> Christer