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

[Tom Kristensen <[e-mail]>]

On 13 January 2014 19:41, Charles Eckel (eckelcu) <[e-mail]> wrote:

>
> On 1/9/14 4:53 AM, "Tom Kristensen (tomkrist)" <[e-mail]> wrote:
>
> >
> >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.
> [...]
> > > -------- 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.
>
> After rereading RFC 2119, this looks like a case where it is better to fix
> the original RFC as part of extending it rather than follow the
> conventions put in place by the origin RFC. As there does not seem to be
> any reason or circumstances to justify NOT clearing the flag/bits, I think
> both instances should be changed from SHOULD to MUST.


Changed across the document a number of places where the sender earlier
SHOULD clear bits, that the receiver MUST ignore. From now MUST + MUST!


> > > 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?
>
> I think so.
>

No changes, since it was taken care of.

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

No changes, is taken care of below BFCP.


> > > 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.
>
> Here as well, I think it best to adhere to RFC 2119 rather than follow
> conventions used previously in RFC 4582. We have fixed some other issues
> with the original already, and I think we should fix these as well. I
> believe this to be more of matter of interpretation of the guidance of RFC
> 2119 rather than a real change to the BFCP protocol.
>

Changed accordingly. Sending Error messages is now required throughout the
document.


> >
> > > 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?
>
> The paragraph above the table states:
>
> If an error code is not recognized by the receiver,
>    then the receiver MUST assume that an error exists, and therefore
>    that the original message that triggered the Error message to be sent
>    is processed, but the nature of the error is unclear.
>
> I would expect the same to be true when receiving a "Generic Error". The
> action taken in response to such an error is implementation and context
> dependent.


No changes, the behaviour is described sufficiently already.


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


No changes. Earlier agreement still in place.


> >
> > > 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.
>
> Yes, good catch.
>

Fixed!


> >
> > > 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?
>
> That sounds sensible for UDP. For TCP, it can be based on not being able
> to establish a connection to the client. Both of these are similar to
> receiving an ICMP error, as described in section 6.2.2.


No fix needed, since this was already described where the retransmission
timer and behaviour for max number of retries was specified (Section 8).


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

Added this to define how to choose the value for 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?
>
> Works for me. Of course the ID will need to be able to wrap around once
> the defined range is exhausted.


Fixed accordingly. Also added MUST for using monotonically increasing IDs
for it to be consistent.


> > > 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.
>
> Perhaps it is better to remove T2 and instead rely on T1 expiration
> following final  retransmission serving as trigger to release knowledge of
> transaction.


All traces of T2 removed. Added a sentence describing that the entity was
free to release the knowledge of the transaction in question at that point.


> > > 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.
>
> MAY seems appropriate to me.
>

Me too. Fixed.

> > 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?
>
> I think the last sentence need to be changes to read as follows:
>
> "Clients configured to require the use of TLS/DTLS MUST ignore
> unauthenticated messages."
>

Yes, changed to that sentence.

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

Well, I see the point and the Transaction IDs used are no incremented by 1
in the sequence.

Updated draft will be submitted ASAP.

-- Tom