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

From: Robert Sparks <[email-masked]>
Date: Tue, 23 Nov 2010 14:30:50 -0600

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:

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

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

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

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

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

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

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

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

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

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

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 24 Nov 2010 20:19:26 +0100

Hi Robert,

Comments inline ([CHH])

>The text needs to be adjusted to reflect the security issue I 
>called out in a separate message to the list today.

[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

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

[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

[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

[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

[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

[CHH] Done.
 
-------

Thank You for reviewing the document!

Regards,

Christer

---

From: Christer Holmberg <[email-masked]>
Date: Fri, 26 Nov 2010 10:13:11 +0100

Hi Robert,

-------

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

[CHH] I took a look at this. Using syntactically correct header fields would make the example quite messy, due to the length of the header fields, so my suggestion would be to instead add the following paragraph to the general section of the examples:

	"NOTE: The examples do not show the actual syntactical encoding of the request lines, response lines
	and the Via header fields, but rather a pseudo code in order to identity the message type and to 
	which entity a Via header field is associated."

-------

Regards,

Christer

---

From: Robert Sparks <[email-masked]>
Date: Mon, 29 Nov 2010 10:12:05 -0600

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

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

---

From: Robert Sparks <[email-masked]>
Date: Mon, 29 Nov 2010 10:17:54 -0600

I didn't intend for you to modify the existing examples. The Note is good.
But I suggest you _add_ a single instance of a Via header field that is syntactically
correct near the beginning of the document as an explanatory figure.
Something like:

Topmost Via header field value in a request:
  Via: SIP/2.0/UDP pc33.example.com;branch=z9hG4bK776asdhds;keep;rport
Topmost Via header field value in an associated response:
  Via : SIP/2.0/UDP pc33.example.com;received=192.0.2.2;rport=18250;keep=20;branch=z9hG4bK776asdhds

RjS

---

From: Christer Holmberg <[email-masked]>
Date: Mon, 29 Nov 2010 21:51:55 +0100

Hi,

Comments inline ([CHH]).

---------

>(you stop when the last dialog using the 5-tuple terminates), but that seems pretty error-prone.

[CHH]  I suggest the following modification (<new>...</new>)

"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<new>, for 
each dialog on which it is willing the receive keep-alives,</new> as soon as possible."

---------

[CHH] Done.

"Entities that want to reuse connections need to use another mechanism to ensure
that security aspects associated with connection reuse are taken into consideration."

---------

Regards,

Christer