Subject: [MMUSIC] Comments on ice-tcp-11

From: Hadriel Kaplan <[email-masked]>
Date: Mon, 15 Nov 2010 00:36:57 -0500

Howdy,
I've read the latest v.11 draft, and have the following comments so far:

1) It would be really nice to get some examples, including example SDP.  I know that's a pain, but it's hard to grok for those of us who don't live/breathe ICE.

2) I don't see any ABNF for the SDP, which would be cleaner than inline in text I think.  Section 4.4 talks about the tokens for the "transport-extension" expansion for the candidate SDP attribute, but instead of using "TCP" it uses "tcp-act", "tcp-so" and so on.  That seems really weird to me, since the "transport-extension" field was supposed to be for transport types, and none of these are a transport type per se.  I believe you're doing this because you need to encode the TCP connection mode (active/passive/so) but the ABNF for ICE doesn't give you any place to put it, right?  Couldn't you put it as a new extension-att-name = extension-att-value? (not a big deal, just seems odd)

3) I don't see any language about an m-line mismatch with candidate lines causing ICE to be ignored and reverting to basic behavior, as is done for ICE-UDP.  This needs to be called out in here as well for the same reasons, unfortunately.

4) Using RFC4571 to frame non-RTP stream content, as described in section 10.1, is very hokey.  If the "media" was a file being transferred, for example, then you're making the sender check every ~64KB for several values that could match a STUN header, and if it determines the leading bytes could be a STUN header, then truncating the stream payload to be smaller to fail the STUN length field check.  And then the receiver gets it and checks the STUN length and if it's wrong decides to treat this as a data stream payload.  That's a really weird way of distinguishing a STUN message from stream content for the receiver, and a painful way to have the sender transmit stream content safely.  

Have you thought of just biting the bullet and defining a STUN message type of "DATA"?  In other words, forget RFC4571 framing, and instead send only STUN messages in the TCP stream: if the STUN message type is a "DATA" indication message type, it's payload belongs to a higher application.  This way a sender can just write data without worrying about its payload content pattern, and a reader can easily distinguish/demux payload, as well as treat an invalid STUN header as an invalid STUN header (ie, a protocol error). If you're concerned about the additional 18 bytes of overhead, I think one could argue the transaction-id/magic fields are unnecessary for this message type, and it would be left out when the new message type is defined.  So another way to describe it is a slightly modified rfc4571 framing, where each frame header is 2 bytes msg-type and 2-bytes length, as opposed to just 2-bytes length.

-hadriel

---

From: Ari Keranen <[email-masked]>
Date: Tue, 16 Nov 2010 15:32:55 +0100

Hi Hadriel,

Thanks for the comments! Responses inline.

15.11.2010 6:36, Hadriel Kaplan wrote:
> 1) It would be really nice to get some examples, including example
> SDP.  I know that's a pain, but it's hard to grok for those of us who
> don't live/breathe ICE.

True. As I mentioned in the previous mail, we're working on getting an 
SDP example there.

> 2) I don't see any ABNF for the SDP, which would be cleaner than
> inline in text I think.  Section 4.4 talks about the tokens for the
> "transport-extension" expansion for the candidate SDP attribute, but
> instead of using "TCP" it uses "tcp-act", "tcp-so" and so on.  That
> seems really weird to me, since the "transport-extension" field was
> supposed to be for transport types, and none of these are a transport
> type per se.  I believe you're doing this because you need to encode
> the TCP connection mode (active/passive/so) but the ABNF for ICE
> doesn't give you any place to put it, right?  Couldn't you put it as
> a new extension-att-name = extension-att-value? (not a big deal, just
> seems odd)

Good point. I would also support making this extension-att instead.

> 3) I don't see any language about an m-line mismatch with candidate
> lines causing ICE to be ignored and reverting to basic behavior, as
> is done for ICE-UDP.  This needs to be called out in here as well for
> the same reasons, unfortunately.

I think that this already applies to ICE TCP since you do everything 
like with ICE UDP unless otherwise specified here. That said, we could 
make it more explicit in the draft too. I guess a single sentence with a 
reference to the ICE RFC's Section 18.6 would be sufficient?
(http://tools.ietf.org/html/rfc5245#section-18.6)

> 4) Using RFC4571 to frame non-RTP stream content, as described in
> section 10.1, is very hokey.  If the "media" was a file being

Frankly, I'm not a big fan of the RFC4571 shim either. I spent some time 
thinking whether we could live completely without it, but the final 
conclusion was that having this makes many things simpler.

> transferred, for example, then you're making the sender check every
> ~64KB for several values that could match a STUN header, and if it
> determines the leading bytes could be a STUN header, then truncating
> the stream payload to be smaller to fail the STUN length field check.

I don't see this as a big problem, neither performance- nor code-wise. 
It is highly unlikely that one would need to check STUN identifiers past 
the magic cookie and even much less likely that the values would 
actually match and require truncating of the payload. Also, one can use 
the existing STUN code for the checks, so the amount of new code due to 
to this is quite minimal.

> And then the receiver gets it and checks the STUN length and if it's
> wrong decides to treat this as a data stream payload.  That's a
> really weird way of distinguishing a STUN message from stream content
> for the receiver, and a painful way to have the sender transmit
> stream content safely.

It is a bit ugly, I admit that. But I believe it's best we got.

> Have you thought of just biting the bullet and defining a STUN
> message type of "DATA"?  In other words, forget RFC4571 framing, and

Actually, TURN has already defined DATA attribute:
http://tools.ietf.org/html/rfc5766#section-14.4

> instead send only STUN messages in the TCP stream: if the STUN
> message type is a "DATA" indication message type, it's payload
> belongs to a higher application.  This way a sender can just write
> data without worrying about its payload content pattern, and a reader
> can easily distinguish/demux payload, as well as treat an invalid
> STUN header as an invalid STUN header (ie, a protocol error). If
> you're concerned about the additional 18 bytes of overhead, I think
> one could argue the transaction-id/magic fields are unnecessary for
> this message type, and it would be left out when the new message type
> is defined.  So another way to describe it is a slightly modified
> rfc4571 framing, where each frame header is 2 bytes msg-type and
> 2-bytes length, as opposed to just 2-bytes length.

This is an interesting idea, but I think the overhead caused by this is 
a bigger issue than the hokeyness of the current approach. Also, I would 
prefer not to break STUN by defining new message type without the 
required fields, but rather use the shim. Any other opinions on this?

Cheers,
Ari