Re: [bfcpbis] Joerg's review - Fwd: Reviewing draft-ietf-bfcpbis-rfc4582bis

[Tom Kristensen <[e-mail]>]

Inline below.

On 12/02/2013 09:31 AM, Tom Kristensen wrote:
 > Relaying Jörg's review sent to the authors of the draft 2013-11-30.
 >
 > Please, step forward with any reactions or input regarding the two
 > reviews we received recently. My plan is to go through them and provide
 > feedback to the list ASAP.
 >
 > (Jörg is added to the copy list, since he's not following the BFCPbis
 > mailing list)

No reactions or comments triggered on the list. (And apologize for a 
loooong yule and new year vacation).
However, here are my comments and suggestions for the issues Jörg had.

 > -------- Original Message --------
 >
 > Hi Gonzalo and all,
 >
 > thanks a lot for the diff, this is really useful!
 >
 > So, I have gone in some detail through the document and found a
 > bunch of issues to be addressed. Most of those revolve around
 > the use of unreliable transport, which appears to be underspecified
 > in a number of ways.

[...]

 > 5.1. General
 >
 > SHOULD -> MUST be cleared by the sender

I don't see the need for change since the flag has no meaning when using 
reliable transport and especially not since it is a MUST for the 
receiver to ignore it in that case. Anyway, changing it to MUST works 
fine with me if that's what the WG wants!

Also, since this is an extension of RFC 4582 we have used the same style 
as in the original draft. For instance in 5.2.6 (for the Padding bits) 
and in 5.2.6.1 (for the R bits), where the sender SHOULD set clear the 
bits and the receiver MUST ignore them.

 > When moving from a 16- or 32-bit "value" to an unsigned
 > integer, the byte order must be specified.

The second sentence in Section 5 says that all "the protocol values MUST 
be sent in network byte order". That should be sufficient, shouldn't it?

 > 5.1. R bit
 >
 > Question: Can it happen that transport changes in a relay? If
 > my dim memory serves me right, then STUN relay may change the
 > transport and thus the reliability. They won't interpret BFCP,
 > however, and so the bit would suddenly be wrong. Or is this
 > scenario prohibited?

You mean TURN not STUN I presume. A TURN relay may have an unreliable 
and reliable hop on each hop. However, the TURN relaying(/"tunneling") 
magic added to the packets will be removed before the BFCP packets 
reaches the BFCP stack. And since on hop is unreliable the extensions 
for unreliable transport is still needed.

 > 5.2. Use of SHOULD
 >
 > Quite a few places state a SHOULD requirement, e.g., for sending
 > error messages. But this doesn't make sense. Based upon which
 > grounds would an endpoint _not_ send an error message?

I do agree in principle, but again and if I remember correctly the 
SHOULD here was used to comply with the style used in the original RFC 
4582. See for instance the SHOULD requirement to send an Error message 
in Section 13 (for Unknown Primitive).

Since the revision of RFC 4582 was done to add support for unreliable 
transport and in addition to fix important issues (such known 
bugs/typos/unclear text), I'm not sure we want to change the SHOULD to 
MUST and potentially generate issues for existing RFC 4582 compliant 
implementations out there.

However, we can without any problem make the use of error messages 
mandatory (with MUST) in the sections added for unreliable transport of 
course. Such as in Section 6.2 and probably some more places.

 > 5.2.6 Generic error
 >
 > Is it specified what the receiver of a non-specific error is
 > supposed to do? Apparently, it cannot talk to the server. So,
 > abandon floor control?

Well, for me the Generic Error is kind of a safety net in situations 
where the existing error codes are not meaningful. So one implementation 
might simply abandon floor control/BFCP as a consequence another might 
have some error recovery that is possible - depending on context.

Any need for text describing this fluffy assumption? :) Or do people 
have other interpretations?

 > 5.3.14, 5.3.15., and 5.3.17
 >
 > It seems that FloorRequestStatusAck, FloorStatusAck, and GoodbyeAck
 > are essentially packet-level acks. So why not have just a single
 > code point for those, since they all carry a transaction id anyway?

True and if I remember correctly this has been discussed earlier on. 
Reasons like having explicit acks matching request primitive and 
potentially extensibility per request type later on might be enough to 
keep it as is? I think so at least.

 > 6.2 Unreliable Transport
 >
 > The text often talks about UDP datagrams, even though DTLS
 > transport appears assumed.
 >
 > "Entities MUST have at most one outstanding request transaction at
 > any one time."
 >
 > This probably wants the addition of a "per peer", otherwise a floor
 > control server would be in trouble. (Section 6.2.1 has this right)

Indeed, a good point! This will be changed to make it consistent and clear.

 > 6.2 Hello
 >
 > Clients MUST announce their presence to the floor control server by
 > sending a Hello message. The floor control server responds to the
 > Hello message with a HelloAck message. The client considers the
 > floor control service as present and available only upon receiving
 > the HelloAck message.
 >
 > When does the server consider the client to have disappeared so that
 > it can discard state?
 >
 > It seems I can run a client, send a Hello message, get the HelloAck,
 > and then send a floor control request with a spoofed IP address, and
 > predict and fake the response for the first message I am expecting
 > from the server in response. Then, I have state installed that will
 > generate packets and retransmissions to a random target until the
 > server discards the client state. But there is no mention when the
 > state would be discarded.
 >
 > Maybe this is less of an attack problem since this is bound to a
 > conference somewhere, but I still don't have a mechanism to get
 > rid of the transport state -- could be important particularly when
 > new connections are instantiated. While it is stated that the old
 > state is lost (for the new instance of the client), will it be
 > discarded? There is no guidance.

Hmmm, not sure how to best handle that situation.

Using DTLS would make the IP address spoofing harder at least, and it is 
recommended to use TLS for both TCP and UDP transport.

What can people propose for this? Is it sufficient to describe what the 
server should do when the number of retransmissions are tried?

 > 6.2.3 Fragmentation
 >
 > Is there any guidance on transmitting fragments? (any pacing?)

No, and one justification for keeping the fragmentation scheme simple 
and avoid specifying for instance pacing, is that the BFCP messages are 
binary and generally small in size. The probability for using the 
fragmentation scheme is smal and the need for pacing fragments even lower!

 > How is N defined? One would assume N = ceil (msgsize/MTUsize), but
 > an implementer might decide to use a larger N. In any case, when
 > we use N in the spec, it must be said how it is established.

I agree and I think using N = ceil(msgsize/MTUsize) is a good way to 
specify how to choose N.

 > 8.2 Transaction number re-use.
 >
 > The text currently states that a transaction number "MUST NOT be
 > reused in another message from the floor control server checks
 > whether until the appropriate response from the client sending is
 > received for the transaction".
 >
 > If the ID is monotonically increasing (modulo wrap-around) why not
 > make re-use illegal? Allowing re-use seems to be calling for
 > trouble, especially in case some transactions may refer to others
 > by their ID or if transactions last longer.

Very true and a good point. Re-use is potentially creating trouble... 
And since monotonical increasing values are required in the second last 
paragraph of Section 8, we should ban re-use in Section 8.2. OK?

 > 8.3.2. T2 is unclear

Yes, probably true! One way of dealing with that is to remove timer T2 
completely and let the implementations decide when to release knowledge. 
Another to brush up the specification of T2 of course.

I'll be back with a proposal if clarification is needed!

 > 9.1 Authentication
 >
 > The text states:
 >
 > BFCP messages received over an authenticated TLS/DTLS connection are
 > considered authenticated. A floor control server that receives a
 > BFCP message over TCP/UDP (no TLS/DTLS) can request the use of TLS/
 > DTLS by generating an Error message, as described in Section 13.8,
 > with an Error code with a value of 9 (Use TLS) or a value of 11 (Use
 > DTLS) respectively. Clients SHOULD simply ignore unauthenticated
 > messages.
 >
 > "can" -> MUST or MAY (this is normative behavior)

True.

 > But does this work? A server has one connection to a client. So,
 > if the server has at its disposition to accept TCP/UDP but the client
 > should ignore such messages, with the connection bidirectional, then
 > this de-facto mandates TLS/DTLS, doesn't it? Or the client would
 > never react.

I'n not that fluent in TLS/DTLS to make a definitive answer there. 
Anybody out there willing to explain why this works or not?

 > Appendix A: Wouldn't the monoticity of the transaction IDs become
 > clearer if increments or 1 in numbers were used?

True, but increments of 1 is not mandated. What do you mean by using 
increments, having an initial Transaction ID and using "+x" or something 
in later messages?

-- Tom