Subject: [sipcore] AD Review: draft-ietf-sipcore-sip-websocket-06.txt

From: Robert Sparks <[email-masked]>
Date: Tue, 27 Nov 2012 16:35:54 -0600

AD Review: draft-ietf-sipcore-sip-websocket-06.txt

Summary: There are a few issues to address with a revised ID before 
progressing to IETF LC

Overall, this is a very well written, easy to read draft. Thank you for 
making it this clear.
There are a couple of small problems to correct, and a few issues to 
consider before moving
forward:

* Section 6 starts with "_This section is non-normative_", but then contains
   normative statements. RECOMMENDED is the same as SHOULD. This is a 
stronger
   requirement for Ping than RFC 6455 places on websockets. If it was 
the intent
   to restate what RFC 6455 required, the language should be "RFC 6455 
says to...".
   If it was the intent to make a stronger requirement, then this 
section _is_
   normative.

   To avoid the appearance of placing a new normative requirement on the 
implementation
   of keep-alives, I suggest replacing the next to last paragraph of 
this section with
   "The indication and use of the CRLF NAT keep-alive mechanism defined 
for SIP
    connection-oriented transports in RFC5626 section 3.5.1 or RFC6223 
are, of course,
    usable over the transport defined in this specification."

* Section 7 starts with "_This section is non-normative_" but then
   contain normative statements.

   Section 7 could be rewritten to not use 2119, but that would take 
more than
   changing the words to lower case. The gist is 'the SIP and WebSocket 
specs
   tell you these things.'

   It also currently confuses SIP Digest with HTTP Digest, and 
incorrectly suggests
   that SIP Digest is a SHOULD strength requirement in 3261 (it is a 
MUST there).

   It would be better to say "This section describes how authentication is
   acheived through the requirements in RFCs 6445, 6265, and 3261."
   The MAY in paragraph 3 can be changed to "is allowed to".
   The last paragraph can say
    ...", authentication can be requested at the SIP protocol level. Note
     that RFC3261 requires that all SIP implementations (which includes
     implementations of this specification) implement Digest Authorization
     (see RFC3261, Section 26.3.1)."

* The document should more explicitly call out that none of SIP TLS 
certificate
   checks specified in RFC3261 or RFC5922 will be made when using this 
transport.
   Instead, only the checks specified by RFC6455 will be made. The 
certificates
   that are appropriate for SIP over TLS over TCP will probably not be 
appropriate
   for SIP over secure websockets (it would only be coincidence if the 
same cert
   worked - the two checks will look in different places in the 
certificate).

* Should there be some additional discussion of what an element should do if
   someone clicks on "sip:foo.example.com;transport=ws" or receives that 
in a
   contact in a 302 or in a Refer-To: header field?  To: header field?
   Taken to an extreme, what's a proxy that understands sip over websockets
   supposed to do if it gets a message containing Route header fields 
(think
   preloaded routes) where the next route value is for a transport=ws 
hop that
   a connection doesn't yet exist for?

* I'm ok proceeding with the IANA Considerations sections as they currently
   stand, but I wonder if instead of what's in 10.3 and 10.4, this document
   should create a new registry for the transport values. That way someone
   could search the registry for the strings "WS" or "WSS" and find 
something
   meaningful.

Nits:

  - RFCs never change. Years from now, this won't be "new". Please consider
    removing "new" from the abstract and the Introduction. The abstract 
would
    be stronger if you replaced "enable usage of SIP in new scenarios" with
    "enable usage of SIP in web-oriented deployments."

  - In the introduction, you say "a [new] reliable and message boundary 
transport".
    Did you mean "a reliable, and message-boundary preserving, transport" ?

  - At the end of 5.2.3, s/regardless the Via/regardless of whether the Via/

  - In A.1, to further avoid this section appearing to be normative, I 
suggest
    changing "Therefore the SIP WebSocket Client constructs" to "A SIP 
WebSocket
    client running in such an environment can construct".

  - In section 6, what is the value of the sentence "A future WebSocket 
protocol
    extension providing a similar keep alive mechanism could also be 
used"? This
    specification says the same thing if the sentence is removed, and I 
suggest
    removing it.

  - (no change suggested - just pointing something out for future work)
    The ABNF in 5.2.1 could have used =/ and not have restated the other 
values.
    It would have been nice if 5.2.2 could have as well, but the way 
transport-param
    is defined in RFC3261 prevents it. It's probably better to keep the 
form of 5.2.1
    and 5.2.2 the same.

---

From: Paul Kyzivat <[email-masked]>
Date: Wed, 28 Nov 2012 12:30:10 -0500

>     is defined in RFC3261 prevents it. It's probably better to keep the

There is a way to use =/ for both:

   transport =/ "WS" / "WSS"

   transport-param =/ "transport=" "ws"

This isn't pretty for transport-param, but it works and repeats less.
Whether this is better or worse than what is currently in the draft is a 
matter of taste. IMO it is marginally better.

This is definitely a lesson for writing ABNF in the future, so that it 
is extensible. (Maybe we need an ABNF directorate and an ABNF review of 
each RFC.)

	Thanks,
	Paul

---

From: Iñaki Baz Castillo <[email-masked]>
Date: Thu, 29 Nov 2012 12:44:02 +0100

Robert, thanks a lot for your useful comments. Please let's some days
until we can comment and address them into a new revision of the
draft.

Best regards.

2012/11/27 Robert Sparks <[email-masked]>:
>   requirement for Ping than RFC 6455 places on websockets. If it was the
> intent
>   to restate what RFC 6455 required, the language should be "RFC 6455 says
> to...".
>   If it was the intent to make a stronger requirement, then this section
> _is_
>   normative.
>
>   of keep-alives, I suggest replacing the next to last paragraph of this
> section with
>   "The indication and use of the CRLF NAT keep-alive mechanism defined for
> SIP
>    connection-oriented transports in RFC5626 section 3.5.1 or RFC6223 are,
> of course,
>   Section 7 could be rewritten to not use 2119, but that would take more
> than
>   changing the words to lower case. The gist is 'the SIP and WebSocket specs
>   tell you these things.'
>
>   It also currently confuses SIP Digest with HTTP Digest, and incorrectly
> suggests
>   that SIP Digest is a SHOULD strength requirement in 3261 (it is a MUST
> there).
>
>   for SIP over secure websockets (it would only be coincidence if the same
> cert
>   worked - the two checks will look in different places in the certificate).
>
>   someone clicks on "sip:foo.example.com;transport=ws" or receives that in a
>   supposed to do if it gets a message containing Route header fields (think
>   preloaded routes) where the next route value is for a transport=ws hop
> that
>   could search the registry for the strings "WS" or "WSS" and find something
>   meaningful.
>
> Nits:
>
>    removing "new" from the abstract and the Introduction. The abstract would
>    extension providing a similar keep alive mechanism could also be used"?
> This
>    is defined in RFC3261 prevents it. It's probably better to keep the form
> of 5.2.1
>    and 5.2.2 the same.
>
> _______________________________________________
> sipcore mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sipcore

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