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

From: Iñaki Baz Castillo <[email-masked]>
Date: Mon, 18 Feb 2013 02:31:31 +0100

Hi Robert, I've submited a new revision 07 of
draft-ietf-sipcote-sip-websocket, some comments inline:

2012/11/27 Robert Sparks <[email-masked]>:
> 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."

Done in rev 07.

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

Done in rev 07.

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

Done in rev 07 within the "Secure WebSocket Connection" subsection
under "Security Considerations" section.

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

Typicall WebSocket capable SIP proxies/servers won't be able to
establish outgoing WS connections and, ever less, typical SIP WS
clients (browsers) won't be able to receive incoming WS connections.
Thus a SIP proxy receiving a request with a ;ws preloaded Route would
fail due to "unsupported transport" when performing RFC 3263 rules.

However I expect that SIP WebSocket clients should/will implement
Outbound and GRUU and work in enviroments in which proxies and
registrars also implement Outbound and GRUU. Therefore the problem you
mention would not occur since both Outbound and GRUU avoid it. The SIP
WebSocket UA would provide a GRUU based Contact when establishing
dialogs and thus the remote peer would send a GRUU URI in the Refer-To
header.

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

I'm ok with what you suggest, but I don't think I can start such a new
registry. If you consider that the document requires a new registry
please let me know how to help with it.

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

Done in rev 07.

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

Done in rev 07.

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

Fixed in rev 07.

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

Done in rev 07.

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

You are right. Done in rev 07.

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

I've rewritten it as Paul and you suggest:

  transport =/ "WS" / "WSS"

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

Thanks a lot for you useful feedback and improvements.

Best regards.

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

---

From: Paul Kyzivat <[email-masked]>
Date: Mon, 18 Feb 2013 18:10:08 -0500

> please let me know how to help with it.

We just went through this by establishing a new registry for the 
priority header field.

But in that case, the draft that provoked the creation of the new 
registry was in a different WG, and we thought it weird for that draft 
to establish a new registry for a sip header field.

In this case everything is within sipcore, so I don't see any reason why 
we couldn't do everything within this draft.

Robert, what do you think?

	Paul

---

From: Robert Sparks <[email-masked]>
Date: Mon, 25 Feb 2013 15:18:03 -0600

On 2/18/13 5:10 PM, Paul Kyzivat wrote:
> In this case everything is within sipcore, so I don't see any reason 
> why we couldn't do everything within this draft.
>
Yes, in this draft is the right place to create the registry if the 
working group thinks a registry is a good idea.

Is there anyone who thinks this would be a bad step?

---

From: Robert Sparks <[email-masked]>
Date: Mon, 25 Feb 2013 15:38:44 -0600

On 2/25/13 3:18 PM, Robert Sparks wrote:
>> registry was in a different WG, and we thought it weird for that 
>> draft to establish a new registry for a sip header field.
>>
If people _do_ think this is the right thing to do, this is the text I 
think we would need. (Based on a quick steal from
the sipcore-priority document):

10.6 SIP Transports Registry

   This document adds a new registry, "SIP Transport".
   Its format and initial values are as shown in the following table:

                         +------------+-----------+
                         | Priority   | Reference |
                         +------------+-----------+
                         | UDP        | RFC 3261  |
                         | TCP        | RFC 3261  |
                         | TLS        | RFC 3261  |
                         | SCTP       | RFC 3261  |
                         | WS         | RFC XXXX  |
                         | WSS        | RFC XXXX  |
                         +------------+-----------+

    The policy for registration of values in this registry is "Standards Action",
    as that term is defined by [RFC5226].

      RFC Editor Note: Please replace XXXX of the RFC number assigned 
for this document and
      remove this note.

---

From: Robert Sparks <[email-masked]>
Date: Mon, 25 Feb 2013 15:52:43 -0600

Looks like I missed 4168 - correction below:

>    Its format and initial values are as shown in the following table:

                         +------------+---------------------+
                         | Priority   | Reference           |
                         +------------+---------------------+
                         | UDP        | RFC 3261            |
                         | TCP        | RFC 3261            |
                         | TLS        | RFC 3261            |
                         | SCTP       | RFC 3261, RFC 4168  |
                         | TLS-SCTP   | RFC 4168            |
                         | WS         | RFC XXXX            |
                         | WSS        | RFC XXXX            |
                         +------------+---------------------+

> _______________________________________________
> sipcore mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sipcore

---

From: Robert Sparks <[email-masked]>
Date: Mon, 25 Feb 2013 16:00:13 -0600

Hi Iñaki (and SIPCORE participants):

One thing from my initial review that I don't think I've seen a response 
to (nor do I see how the draft revision addressed it) -
apologies if we had discussed this and I've forgotten:

On 11/27/12 4:35 PM, Robert Sparks wrote:
>   contact in a 302 or in a Refer-To: header field?
>   preloaded routes) where the next route value is for a transport=ws 
> hop that
RjS

---

From: "DRAGE, Keith (Keith)" <[email-masked]>
Date: Tue, 26 Feb 2013 01:25:37 +0100

The IANA considerations for the new registry needs to indicate it is grouped with the other SIP registries.

I see Robert has already spotted RFC 4168.

Keith

________________________________
From: [email-masked] [mailto:[email-masked]] On Behalf Of Robert Sparks
Sent: 25 February 2013 21:39
To: Paul Kyzivat
Cc: [email-masked]
Subject: Re: [sipcore] AD Review: draft-ietf-sipcore-sip-websocket-06.txt

We just went through this by establishing a new registry for the priority header field.

But in that case, the draft that provoked the creation of the new registry was in a different WG, and we thought it weird for that draft to establish a new registry for a sip header field.

In this case everything is within sipcore, so I don't see any reason why we couldn't do everything within this draft.

Robert, what do you think?
Yes, in this draft is the right place to create the registry if the working group thinks a registry is a good idea.

Is there anyone who thinks this would be a bad step?
If people _do_ think this is the right thing to do, this is the text I think we would need. (Based on a quick steal from
the sipcore-priority document):

                        | Priority   | Reference |

                        | UDP        | RFC 3261  |

                        | TCP        | RFC 3261  |

                        | TLS        | RFC 3261  |

                        | SCTP       | RFC 3261  |

                        | WS         | RFC XXXX  |

                        | WSS        | RFC XXXX  |

   as that term is defined by [RFC5226].
     RFC Editor Note: Please replace XXXX of the RFC number assigned for this document and
     remove this note.

Paul

---

From: Iñaki Baz Castillo <[email-masked]>
Date: Tue, 26 Feb 2013 11:16:14 +0100

Hi Robert, we are currently at MWC in Barcelona. We will address the
issues you and Paul have reported during next days.

Thanks a lot.

2013/2/25 Robert Sparks <[email-masked]>:
> One thing from my initial review that I don't think I've seen a response to
> (nor do I see how the draft revision addressed it) -

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