From: Christer Holmberg <christer.holmberg@ericsson.com>
To: Robert Sparks <rjsparks@nostrum.com>, SIPCORE <sipcore@ietf.org>
Date: Wed, 24 Nov 2010 20:19:26 +0100
Thread-Topic: AD review: draft-ietf-sipcore-keep-08
Thread-Index: AcuLTVOcJ65h0amkTRmAnyyGNzH3WwAifNKQ
Message-ID: <224F5CB1ECAB2C45BC64065AFD0339650406B5FC94@ESESSCMS0356.eemea.ericsson.se>
References: <4892E0CC-52C7-4422-A5DA-C6A7553A4053@nostrum.com>
In-Reply-To: <4892E0CC-52C7-4422-A5DA-C6A7553A4053@nostrum.com>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [sipcore] AD review: draft-ietf-sipcore-keep-08
Precedence: list

Hi Robert,

Comments inline ([CHH])

>Summary: Revised ID needed
>=20
>The text needs to be adjusted to reflect the security issue I=20
>called out in a separate message to the list today.
>During that adjustment, there are a few other places to=20
>consider making changes:

[CHH] See other thread.

------

>* Section 4.4, second paragraph "The parameter can contain"=20
>is confusing - it could be read to imply that the parameter=20
>can contain other things as well. I suggest rewriting this=20
>sentence as "The parameter value, if present, represents a=20
>recommended keep-alive frequency..."

[CHH] Done.

"The parameter value, if present and with a value other than zero, represen=
ts a recommended keep-alive frequency, given in seconds."


-------

>* The document talks about invoking keep for INVITE initiated=20
>dialogs, but not about SUBSCRIBE initiated dialogs. Was it the=20
>intent to not allow the use of the mechanism for SUBSCRIBE initiated=20
>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 t=
o SUB dialogs. Nobody has ever requested it, however.


-------
   =20
>* Section 4.4 talks about normative server behavior for using=20
>this mechanism with INVITE initiated dialogs.
>where is the companion set of normative requirements for=20
>using the mechanism to keep alive a REGISTRATION
>flow?

[CHH] The 2nd paragraph is the generic rule, which covers both INVITE and R=
EGISTER.

The 3rd paragraph is an addition that covers the case where multiple respon=
ses are sent to a request, which is INVITE specific.

But, maybe to make it more clear, and based on your comment on section 5 (s=
ee 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=20
"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"=20
parameters in responses to the same request. The SIP entity SHOULD indicate=
=20
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=20
the "keep" parameter in a response to explicitly indicate willingness to=20
receive keep-alives from its adjacent upstream SIP entity."

-------=20

>*  In several places, the document refers to an element=20
>inspecting "its" Via header. This is imprecise and will
>lead to arguments among implementers. I suggest=20
>explicitly calling out "the topmost Via header field value"
>and being very specific about when you are talking about a=20
>message being received or a message being sent.

[CHH] I will look into that.

-------
=20
>* In section 5's fourth paragraph, it would help avoid=20
>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 or=
der to indicate willingness to receive keep-alives, also inserts..."

-------
=20
>*In the last sentence of Section 5 - what value is "without=20
>forcing entities to re-write the value of Flow-Timer header=20
>field" adding? I suggest deleting the phrase.

[CHH] Done.

-------

>* Section 6's first paragraph says "Entities that want to=20
>reuse connections MUST use a another mechanism".
>(Note the spurious 'a'). Why is this a 2119 MUST? I=20
>suggest saying "would need to" instead.

[CHH] Done.=20

The reason behind the "MUST" was to clearly indicate that keep as such is n=
ot a connection reuse mechanism.

-------

>* The first sentence in 7.4 would be better if it said Figure=20
>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=20
>shorthand, pseudo-code style representation of the
>header field format. It would be good to explicitly note=20
>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 n=
ot supposed to be teach-yourself-SIP), but I can change according to your s=
uggestion.

-------
=20
>* In Section 10's section paragraph. I suggest that you be=20
>explicit that you are talking about hop-by-hop
>integrity protection (calling out TLS or DTLS), to avoid=20
>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 TL=
S or DTLS)</new>,=20
a man-in-the-middle can modify Via header fields...."

-------
=20
>* In section 10's third paragraph, you call out "(at high=20
>rates)". This is potentially misleading - the rates
>are at the highest somewhere just under once a second. I=20
>suggest replacing "(at high rates)" with
>"(up to approximately once a second)".

[CHH] Done.
=20
-------

Thank You for reviewing the document!

Regards,

Christer=