Subject: [Gen-art] Gen-ART Last Call review of draft-ietf-sipcore-sip-websocket-08

From: Suresh Krishnan <[email-masked]>
Date: Tue, 16 Apr 2013 02:31:04 -0400

I am the assigned Gen-ART reviewer for
draft-ietf-sipcore-sip-websocket-08

For background on Gen-ART, please see the FAQ at
<http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html>.

Please resolve these comments along with any other Last Call comments
you may receive.

Summary: This draft is almost ready for publication as a Proposed
Standard, but has some minor issues that need to be fixed.

* Section 4.1

There is no error handling specified. i.e. What happens if the server
does not send a 101 reply with sip in the Sec-WebSocket-Protocol header?

* Section 5.2

The updated ABNF provided in section 5.2.1. is confusing. e.g. RFC4168
specifies the entire transport rule as follows

transport         =  "UDP" / "TCP" / "TLS" / "SCTP" / "TLS-SCTP"
                        / other-transport

while this document just uses an incremental alternative and calls it an
updated rule. I would have expected an *updated* rule to say

transport         =  "UDP" / "TCP" / "TLS" / "SCTP" / "TLS-SCTP" / "WS"
		        / "WSS" / other-transport

* Section 5.2.2

RFC4168 does not update transport-param. Is this a copy and paste error?

* Section 5.2.4

Maybe it is just me, but the way I read it, this section leaves the
client without any mandatory to implement transport protocol for SIP
since it relaxes the need to implement UDP and TCP, as well as allowing
the implementation of the websocket transport using a MAY.

Thanks
Suresh

---

From: Iñaki Baz Castillo <[email-masked]>
Date: Tue, 16 Apr 2013 10:40:36 +0200

Thanks a lot for your review Suresh. Please read replies inline:

> does not send a 101 reply with sip in the Sec-WebSocket-Protocol header?

It should be considered a SIP transport error, so a 503 SIP response
should be locally generated. This will be added to that section.

>                         / "WSS" / other-transport

There was a long discussion in SIPCORE WG about this subject and
finally it was chosen to use the incremental alternative (so the
updated ABNF would be also longer in the future when new SIP
transports are defined).

Do you consider anyhow that using the fully updated rule is required?

> RFC4168 does not update transport-param. Is this a copy and paste error?

Yes, will be fixed.

> the implementation of the websocket transport using a MAY.

Yes, such a MAY sounds a look confusing. What the text wants to say is
that a SIP WebSocket Client (which obviously implements the SIP
WebSocket transport) is not mandated to implement UDP and TCP
transports. I wil improve the text by removing the last words and
leave it as follows:

----------------
   The specification of this transport enables SIP to be used as a
   session establishment protocol in scenarios where none of other
   transport protocols defined for SIP can be used.  Since some
   environments do not enable SIP elements to use UDP and TCP as SIP
   transport protocols, a SIP element acting as a SIP WebSocket Client
   is not mandated to implement support of UDP and TCP.
----------------

Thanks a lot.

--
Iñaki Baz Castillo
<[email-masked]>

---

From: Robert Sparks <[email-masked]>
Date: Tue, 16 Apr 2013 09:40:19 -0500

Suresh -

One more comment (trimming heavily)

> Do you consider anyhow that using the fully updated rule is required?
Suresh -

The issue is that we have many header fields where more than one 
document may
add a token. Restating the full production each time has proven problematic,
particularly when more than one draft that proposes adding a token is in 
flight at the
same time. Instead, we're adopting a consistent approach of using the 
the incremental
alternative rule to add tokens and IANA registries to make it easy to 
find the
tokens that have been documented to date.

RjS

---

From: Suresh Krishnan <[email-masked]>
Date: Tue, 16 Apr 2013 19:02:57 -0400

Hi Iñaki,
  Thanks for the quick reply. Please find responses inline.

> should be locally generated. This will be added to that section.

Great.

> Do you consider anyhow that using the fully updated rule is required?

I saw Robert's response on the same topic and I will reply to that. If
the WG's consensus is to go incremental, please stick with that. I just
want to ensure that there is no ambiguity as to the final result.

>> RFC4168 does not update transport-param. Is this a copy and paste error?

OK.

> transports. I wil improve the text by removing the last words and

Looks good.

Thanks
Suresh

---

From: Suresh Krishnan <[email-masked]>
Date: Tue, 16 Apr 2013 19:03:23 -0400

Hi Robert,

> tokens that have been documented to date.

That sounds like a good plan. I was more concerned with the wording that
calls this an "updated rule" and the fact that the title of Section 5.2
"Updates to RFC3261" that implies the incremental rule could be
interpreted to add to the rule specified in RFC3261 and lose other
additions since (in this case losing TLS-SCTP). If you have a plan in
place with IANA to take care of this I am fine with this approach.

Thanks
Suresh

---

From: Iñaki Baz Castillo <[email-masked]>
Date: Wed, 17 Apr 2013 10:45:18 +0200

> place with IANA to take care of this I am fine with this approach.

Hi Suresh. Section 5.2.1 says:

----------------------
   The updated augmented BNF (Backus-Naur Form) [RFC5234] for this
   parameter is the following (the original BNF for this parameter can
   be found in [RFC3261], which was then updated by [RFC4168]):

     transport  =/  "WS" / "WSS"
-----------------------

So this BNF updates RFC 3261 but it's also said that same BNF in RFC
3261 was previously updated by RFC 4168. So IMHO it is clear that an
implementor should also check RFC 4168 to get the complete value for
this BNF, am I wrong?

Thanks a lot.

--
Iñaki Baz Castillo
<[email-masked]>