Last Call Review of draft-ietf-mmusic-rfc2326bis-34
review-ietf-mmusic-rfc2326bis-34-genart-lc-sparks-2013-06-12-00

Request Review of draft-ietf-mmusic-rfc2326bis
Requested rev. no specific revision (document currently at 40)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2013-06-04
Requested 2013-05-23
Authors Henning Schulzrinne, Anup Rao, Rob Lanphier, Magnus Westerlund, Martin Stiemerling
Draft last updated 2013-06-12
Completed reviews Genart Last Call review of -34 by Robert Sparks (diff)
Genart Last Call review of -38 by Robert Sparks (diff)
Genart Last Call review of -34 by Elwyn Davies (diff)
Genart Telechat review of -38 by Elwyn Davies (diff)
Secdir Last Call review of -34 by Chris Lonvick (diff)
Secdir Telechat review of -38 by Chris Lonvick (diff)
Assignment Reviewer Robert Sparks
State Completed
Review review-ietf-mmusic-rfc2326bis-34-genart-lc-sparks-2013-06-12
Reviewed rev. 34 (document currently at 40)
Review result Ready with Issues
Review completed: 2013-06-12

Review
review-ietf-mmusic-rfc2326bis-34-genart-lc-sparks-2013-06-12

I am the assigned Gen-ART reviewer for this draft. For background on 


Gen-ART, please see the FAQ at




<

http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.



Please resolve these comments along with any other Last Call comments 


you may receive.




Document: draft-ietf-mmusic-rfc2326bis-34
Reviewer: Robert Sparks
Review Date: 05-Jun-2013
IETF LC End Date: 04-Jun-2013
IESG Telechat date: not yet on a telechat



Summary: This draft is on the right track but has open issues, described 


in the review.






I have not reviewed this document at the level of detail I prefer for 


Gen-art reviews, but have tried to be thorough in the sections I have 


reviewed.




In particular,
I didn't verify the tables and the text agree
I didn't carefully check the ABNF


I haven't thought about possible edge cases and race conditions as much 


as I would have liked


I didn't closely review the security mechanisms in section 19 or the 


specialized MIKEY keying mechanism in the appendix - both need careful 


review from security experts.




One observation I would like to make before calling out issues and nits:



The document is very long, and the structure is unusual - much of the 


definition of the protocol itself is in the appendices. You are missing 


an opportunity to make this document much shorter (thereby likely 


increasing its effectiveness). Much of the jump in from RFC2326 was 


importing the description of headers and responses from HTTP, tailoring 


them to RTSP. That was a good exercise in that it highlights some issues 


that would otherwise be non-obvious (and raises a few questions below). 


But you followed the style from HTTP perhaps too closely - much shorter 


descriptions without examples might have done the job better. Overall, 


separating exposition and examples from the protocol definition would 


make it much easier for an implementer to find the definition of the 


protocol, and use the document as a reference when diagnosing a failure 


to interoperate.




Major Issues



I'm not seeing what instructs an RTSP element on how to find the server 


it would try to open a connection to given an rtsp or rtsps URI? Are you 


assuming it would be doing A or AAAA DNS lookups? Should this protocol 


use NAPTR/SRV? The document should be explicit. On a related note, (and 


maybe I missed this), but where do you talk about what an element should 


check in the server's certificate when connecting over TLS? Are you 


assuming a common name match? Or are you expecting some SubjectAltName 


processing?






The document should say more about when connection reuse is appropriate, 


particularly when the connections happened because of an rtsps uri. Two 


different names might resolve to the same IP address - reusing a 


connection formed when looking at the first name (and verifying the 


server's cert) is dangerous. A new connection needs to be formed (and 


verified) instead.






The text talks about the option to queue S->C requests if there isn't a 


connection to the client available. Ostensibly, this means the request 


can go down some future connection. It's not clear how the server can 


tell the right client connected. In particular, when using rtsps, how do 


you prevent a malicious client from getting such a queued message that 


wasn't meant for him?






Given that the text talks about queuing S-C requests, it should 


explicitly call out whether a server can queue a response if the 


connection the associated request arrived on is no longer available. I 


think what you want to say is that such a response must not be queued, 


and must be dropped.






There are several places in the text that talk about using a 503 


response with a Retry-After header to push back on traffic from an 


element (the first is section 10.7).


* It's not clear what the subject of a 503 is. Is it intended to be 


scoped to requests to the resource in the RURI of the associated 


request? Is it intended to be scoped to the domain in that resource? Or 


is it, like in SIP, intended to be scoped to the server issuing the 


response, independent of what the RURI contained?


* If the intent is that the 503 talks about the server, then you should 


clarify that a proxy doesn't simply forward 503 responses (after 


rewriting CSeqs), and should probably have a response of its own. 


Otherwise, clients that might be talking to two different servers 


through one proxy would lose access to both of them when one of the 


servers 503ed.






Section 4.9.1 lists values the Seek-Style header can take, but 18.45 


lists a completely different set (which most of the rest of the document 


uses). Should 4.9.1 be changed to use the values in 18.45? Are the right 


strings being used in sections 13.4.4 through 13.4.6? Those appear to be 


using strings from 4.9.1.






The document will not stand if you delete some of the appendices. They 


aren't supplementary material. Please consider restructuring the 


normative sections back into the main document, or clearly identifying 


which ones define protocol and which are background information.






Section 15 talks about a "transparent' proxy, but there is no 


description in this document of what protocol such a thing should 


follow. What bad thing happens if you remove all mention of 


"transparent" proxies from the document? As far as I can tell, that 


won't affect the protocol definition at all.






The list of steps for establishing an SRTP cryptographic context in 


C.1.4.1 says "This specification does not specify an explicit method for 


indicating this SRTP cryptographic context establishment method, but 


future specifications may." in the context of looking at the SDP. SDP 


_has_ methods of indicating what keying mechanism is used with SRTP - 


are you talking about something different? Why doesn't this section say 


something like "the use of SRTP with RTSP is only defined with MIKEY 


with keys established as defined in this section. Future documents may 


talk about how an RTSP implementation might treat SDP that indicates 


some other key mechanism be used"? Could you provide an example in the 


document of an RTSP message carrying SDP reflecting the use of SRTP as 


defined in this document?





Minor Issues



The document uses the notion of a "control connection" (it appears first 


in section 2.5) without defining what that is, or what kind of 


connections you might have that aren't control connections.






The update to the registration of the rtsp and rtsps schema call out 


that there are changes that can result in interoperability issues. It 


doesn't say what the issues are, or who might encounter them. I can 


infer that the point is that there might be problems if a URI following 


this updated registration were to be processed by an RTSP 1.0 


implementation (or any other application that relied on the previous 


definition). It would be better to say that explicitly, and talk about 


how a previous implementation is likely to act when presented with a URI 


that exercised these new changes. (It's not clear to me that all of the 


thread at www.ietf.org—msg01567.html 


<

http://www.ietf.org/mail-archive/web/uri-review/current/msg01567.html

> 


concluded - I see how the fragments question ended, but what about 


things like the addition of IPV6 literals? In particular, I'm not 


finding a response where Ted Hardie agreed that the potential for 


interoperability failure had been adequately addressed).







I think you've said something different than you mean in 5.4 item 1. I 


think you mean to say that an RTSP implementation would ignore any body 


that happened to come in message that MUST NOT contain one. What you've 


said is that the part of the parser that's doing framing has to stop 


framing at the end of the headers, and that such an errant body would be 


parsed as a separate message. As written this would keep someone from 


using an Internet Message Format parser since it would have to ignore 


any Content-Length that might have appeared, even though it wasn't 


supposed to.






In section 10.4, it looks like a server can keep an RTSP transaction 


pending forever if it sends 100 responses often enough. I assume the 


client's recourse if it doesn't want to remain involved is to close the 


connection. If that's right, it's worth calling it out explicitly in 


this section.






Somewhere, probably in section 15.2, you should be explicit that a proxy 


that is multiplexing requests MUST keep the requests in the same order 


as they are folded together. You can infer that, or things simply break, 


but saying it explicitly would be better.






Both GET_PARAMETER and SET_PARAMETER are listed as keep-alive methods in 


10.5, but the note in section 13 on table 7 only uses that as a reason 


for requiring SET_PARAMETER. Why doesn't it also apply to GET_PARAMETER? 


And why is this only required at servers? "Required" in this section 


means Mandatory to Implement, I think. If that's right, it should be 


made explicit. If that's not right, what does it mean?






Section 13.8 makes a good argument for always putting parameters to be 


retrieved in a body. Why are you keeping the complexity of also allowing 


them in headers?






Section 13.10: "That should not provide any benefit." is not clear - can 


you expand it a little?






In Section 13.10 you talk about clients _sending_ media (search for 


"send or receive media"). Do you mean anything more than possibly RTCP? 


Can you make that clear in the section?






Section 15.2's second paragraph appears to be the only place the proxy 


rewriting of CSeq in order to multiplex requests is defined, and it is 


very loose. It would be easy for a reader to fail to recognize this as 


an interoperability requirement. Please consider expanding how this is 


specified when addressing the comment about above about keeping requests 


in relative order when multiplexing.






You follow the principle of saying a client should treat a response with 


an unknown response code, the same as it would treat the x00 of that 


class (e.g. 400 for an unknown 4xx). However, you do not define what a 


response code of 300 means. How is a client supposed to handle an 


unknown 3xx response?






As written, a client and server can use HTTP Basic authentication over 


TCP that is not protected with TLS. Consider restricting it's use to TLS 


only. Are you sure you don't need to say anything RTSP specific about 


DIGEST computations (I don't think you need to go as far as SIP did 


talking about DIGEST, but I'm surprised you haven't run into issues 


relying only on 2617 literally.







How does 411 Length Required for this protocol make sense? Given that 


you've restricted the protocol to TCP and require the Content-Length 


header to be present if there is a body, in what circumstance would a 


server need to send a 411?







Section 18.19 requires senders to increment CSeq by 1. We have a 


reliable transport with no request retransmission, so there should never 


be gaps at the receiver. Should the receiver check and react some way if 


there are gaps? I think you should explicitly tell them not to even 


look. If you agree, why is incrementing by one important as long as you 


are always strictly increasing?






You call out several places where RTCP is essential to allow RTSP using 


RTP to work correctly, yet section C.1.2 sets up conditions where RTCP 


MUST NOT be sent. Why are you allowing those instead of treating the 


conditions you describe as errors?





Nits


The definition of Message still talks about connectionless transports



22.14 says "These URI schemes are defined in existing registries which 


are not created by RTSP." Should it say that they are _registered_ 


rather than defined, and "not created by this document" instead of by 


RTSP? In section 4.2 in the paragraph discussing ports 554 and 322, why 


is the language different for rtsp and rtsps? "MUST be used" vs "is 


registered and MUST be assumed"?






Consider a reference in 4.4 to where SMPTE 30 drop is defined, 


particularly for where the 1/100th frame measurement comes from.






Section 4.5 introduces NPT as indicating "the absolute position" and 


then defines something that can carry a point or a range. I suggest 


adjusting the first sentence to allow for a range.






Section 4.9.1's description of Conditional Random Access contains some 


ambiguity. It would help be more explicit. I think it's trying to say 


"would move the client's play point further away from the requested play 


point than it's play point currently is."?




Section 5 : what do you mean by "tricky" switching?

Section 7: consider octets instead of bytes

Section 10.3: what is "take appropriate action" trying to say?



The document mixes using "3rr" and "3xx" to talk about redirect 


responses. Why are there two ways to say the same thing?






(Total nit, but) The examples show dates in 1997, when there could be no 


RTSP/2.0. Are there other aspects of the examples that might not have 


been updated?






Is "will need to use" at the bottom of page 85 in section 13.5.2 trying 


to say something that should be said with 2119?






Consider saying that a client must not put a Terminate-Reason in a C->S 


Teardown request, or tweak the definition of Terminate-Reason to discuss 


client use and the possibility of C->S specific values.






These two sentences in 13.7.1 are particularly hard to parse together: 


"The time period is considered extended when it is 10 times the Session 


Timeout period. Consideration of the application of the server and its 


content should be performed when configuring what is considered as 


extended period of time." I suspect they were not written at the same 


time? Can you simplify what they are trying to say, noting that the 


suggested "10 times" is a default, and that if you want to change that 


default, you need to consider the context?






Section 14: "Interleaved binary data SHOULD only be used if RTSP is 


carried over TCP". Is this leftover from when UDP was an option?






(small nit) Section 14, you mention an ASCII dollar sign - maybe you 


should just say an octet with value 0x24?






It's not clear how far up to go with "upper-layer protocol headers" in 


section 14, given that you are trying to speak generally about carrying 


things in interleaved data. Your example for RTP may cover the only case 


that matters. Consider speaking more normatively (instead of with an 


example) for RTP and provide some guidelines about where to cut the 


prefixes for other things you might want to carry (lest you end up with 


the v6 headers too when tunneling some new foo protocol?)






In section 17, you say "Status codes that have the same meaning are not 


repeated here." But you did repeat them (see 200 OK). Should that 


sentence be deleted?






304 (section 17.3.4) probably should have been a different class 


response, maybe even a 2xx. The text does call it out as "unusual" for 


the 3xx, but it would be better to say why this ended up in this 


response class.






I might be confused, but I'm not sure the redirection semantics defined 


in this document lead to the "black hole" case discussed in 17.4.14. If 


I'm confused, could you send me an example of such a redirection black 


hole happening?




Section 18.11 first paragraph should be split into two sentences.



Is "MUST be transported over TCP" in C.2.1 stale now that no 


connectionless transport is defined?






The end of Appendix G still implies that CSeq was primarily for 


unreliable transports. There are other places that out and point to 


proxy multiplexing. All of them should acknowledge that CSeq is 


necessary for associating responses with requests.