Re: [bfcpbis] Comments on draft-ietf-bfcpbis-rfc4582bis-13

["Paul E. Jones" <[e-mail]>]

Ben,

Following up on this email (and building a growing to-do list)....

------ Original Message ------
From: "Ben Campbell" <[e-mail]>
To: "[e-mail]" 
<[e-mail]>
Cc: "Richard Barnes" <[e-mail]>; [e-mail]
Sent: 3/2/2015 10:23:58 PM
Subject: [bfcpbis] Comments on draft-ietf-bfcpbis-rfc4582bis-13

>(no hats—please treat as any other IETF last call feedback)
>
>Hi,
>
>Here are some comments on draft-ietf-bfcpbis-rfc4582bis-13.
>
>I've divided them into substantive and editorial comments. I've tried 
>to limit comments to the actual "bis" parts, but I don't know how 
>successful I've been at that.
>
>Thanks!
>
>Ben.
>
>
>*** substantive comments ***
>
>
>— 5.1, "ver"
>
>Am I correct that the guidance for receiving an unsupported version 
>applies when you get the wrong version for the transport? (even if you 
>might have supported that version on a different transport?) If so, it 
>would be good to mention that.

Valid point.  I'll add it to the to-do list.


>Also it would be nice to elaborate a bit more on the case of a device 
>only supporting 4582. Given that you aren't allowed to send version 1 
>over UDP, how would this ever be an issue? Are you worried about early 
>UDP implementations that still use version 1? If so, that would be 
>worth a note.

Devices supporting 4582 will use a reliable transport.  Personally, I 
would not worry about the "why".  If it's the wrong/unexpected value at 
the wrong/unexpected place, return an error.


>— 5.1, payload length:
>
>How is a TCP recipient expected to know a payload length was wrong? 
>(Other than the parser failing on the next message?)

This is an issue in framing any protocol over TCP.  If it's wrong, 
subsequent messages just start looking like garbage and messages are 
rejected.


>Also, in the "note", what is the "message size" for a fragmented 
>message over UDP? Is it the size of a fragment, or the size of the 
>message before fragmentation plus the additional common headers? In 
>either case, the REQUIRED part seems more a statement of fact (i.e. you 
>mathematically can't express a size that won't fit) than a normative 
>requirement.

This refers to the BFCP message generally, before it's framed and 
transmitted.  Agree on REQUIRED ... make it lowercase.


>5.3.11:
>
>It seems like "Hello" does quite a bit more than liveness checks. 
>(e.g., establishing the connection for UDP, capabilities query). Also, 
>if Hello is used for liveness checks, I missed any discussion about the 
>procedures for using it for that purpose. (Note that 6.2.4 says to use 
>STUN for liveness checks for non-reliable transports.)

Yep, valid points.  I'll add it to my list.


>-- 6.2.1:
>
>Is this section only applicable to non-reliable transports? Also, the 
>bit about only sending one message until you get an ack is not quite 
>true even for UDP if you consider fragmented messages.

I think this is just for UDP (since it's in the unreliable transport 
sub-section); TCP handles congestion itself.  There is a single message, 
but it might be fragmented over several packets.  That's really no 
different than if fragmentation happened at the IP layer.  Do you want 
something else stated here?


>  -- 6.2.1, last paragraph:
>
>Can you elaborate on what it means to say the "default MUST be" a 
>certain value? Do you mean an implementation MUST use that for a 
>default if no other value is selected? Or is this more a statement of 
>fact (the default value _is_...)?

Given Gorry's comments, I think we need to revisit this assignment.


>-- 6.2.3, paragraph 5: "The Fragment Offset field denotes the number of 
>bytes..."
>
>The definition said the unit was 4-octet units.

Yeah, that needs to be clarified.  Will put it on the list.


>  -- 8:
>
>If a request times out completely on a non-reliable transport, does 
>that impact the state of the "bfcp connection"?
Also needs to be clarified.  Thanks.


>  -- 9.1:
>
>The text mentions sending self-signed certs or fingerprints in an 
>"initial integrity protected channel". I assume that means the 
>signaling protocol? The language does not seem to allow for a CA issued 
>certs validated with PKI techniques. Is that the intent? If so, can 
>they be mixed and matched? (i.e. a self-signed client cert and a CA 
>issued server cert?)

We'll need to revisit this.  The protocol itself does not care about the 
certificates, only that we need some way to ensure we're talking to the 
right endpoint.  These certs could be CA-issued.  I'll put this on the 
list.


>13.1.1, last paragraph:
>
>What if the Ack is never received? What if it is received after T2 
>expires? (In the latter case, does the device need to be prepared to 
>receive an Ack for which it no longer has transaction state?)

For some reason, I'm not matching 13.1.1 against the question you are 
asking, but I understand your point.  If a message sender sends a 
message and does not get a reply, it sends it again a few times using 
exponential back-off.  If a response is not received, then ... that 
speaks to your question of "is this a connection failure?"  That's on my 
list.


>13.7, paragraph 2:
>
>Is the normative requirement to send an error 12 if you don't support 
>the version redundant with similar normative language in the definition 
>of "ver"?

It is redundant.


>  -- section 14, last sentence:
>
>I think you mean to require the use of encryption if TLS/DTLS is used, 
>but the language can be read to globally require TLS/DTLS, which is not 
>consistent with guidance elsewhere.

I'm not quite sure what the issue is.  TLS or DTLS are optional, but 
they are the only means through which authentication and confidentiality 
are provided in the protocol.


>-- Appendix A:
>
>For non-reliable transports, do both endpoints share a transaction ID 
>sequence, or does each end keep it's own independent sequence? I would 
>have guessed the latter, but in step 5 of the example, the server 
>originated transaction has a transaction ID that is one higher than the 
>latest value sent by the client.

The transaction number spaces are independent and the "R" bit is used to 
help make it clear that a message relates to a server-initiated 
transaction or a client-initiated transaction.  What I think is needed 
in that section is showing the R bit value.


>  -- Note following Figure 48: "... does not and SHOULD not"
>
>That's an odd construction. Do you mean a statement of fact, or a 
>normative requirement? If the latter, why is this a SHOULD and not a 
>MUST? Can you imagine a situation where it might be valid for the 
>client to ack the initial FloorRequestStatus message?

Given this is just an example to illustrate the protocol, I would 
suggest we change the sentence to read:
"As such, it is not followed by a ..."

There is nothing new here.


>
>*** editorial comments ***
>
>— section 3, 1st paragraph:
>
>The bis version considers new requirements beyond those mentioned, 
>doesn't it? (e.g., UDP support?) It would be nice to mention that here.

I'm personally not sure what other requirements are considered.  It 
appears to be an enhancement only to add UDP.  We need to get clarity 
from the co-authors.  That said, exactly where were you looking for new 
text?  And you want a list of "what's new"?


>— 4.1, paragraph 7:
>
>I'm a bit surprised to see the unreliable transport message flows 
>buried in an appendix. Aren't those the whole point of the update?

Given they are examples, I personally thought this was reasonably, 
especially since the area there the TCP-based flows are would get really 
cluttered.  We could swap the flows or move the reliable transport 
examples to an appendix, too, but I like having examples up front.  If 
anything, I'd suggest swapping the flows (moving the reliable flows to 
an appendix).  Actually, given the list of things on the to-do list, I'm 
inclined to say "leave it" to avoid introducing instability.


>  — 5.1, Res:
>
>The phrase "At this point," doesn't seem to add anything, and may 
>confuse readers.
Yeah, others noted that and I agree we should remove it.


>
>—5.2, "M" bit:
>
>The phrase "In all other cases,..." is a little  ambiguous. I assume 
>you mean "Attributes specified in future extensions that do not have 
>the "M" bit set..."

Agree this needs adjustment, but it's not necessarily attributes 
specified in the future, but just ones that are not recognized.  Perhaps 
this?

     Unrecognized attributes, such as those that might be specified in 
future extensions, that do
     not have the "M" bit set are ignored, but the message is processed.

Paul