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

[Christer Holmberg <[e-mail]>]

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."


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.

-------

>* 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.

-------
 
>* 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