Subject: Re: [hybi] Comments on draft-ietf-hybi-permessage-compression-05

From: Takeshi Yoshino <[email-masked]>
Date: Tue, 12 Mar 2013 21:13:07 +0900

On Sun, Feb 24, 2013 at 11:59 AM, SM <[email-masked]> wrote:

> Hi Takeshi,
>
> This is an individual comment on draft-ietf-hybi-permessage-**compression-05.
>  I did not cover all the issues.  Sorry for not providing ample explanation.
>
>
Thank you so much. It's fine but let me ask you to elaborate some of them.

> In Section 1:
>
>  "_This section is non-normative._"
>
> This style is unusual for IETF Stream documents.  I suggest switching back
> to the usual style.
>

Yes. I've removed them.

>
>   "As well as other communication protocols, the WebSocket Protocol
>    [RFC6455] can benefit from compression technology."
>
> The sentence would benefit from a rewrite.  It could alternatively be
> dropped.
>

Dropped.

>
> I suggest first specifying the framework and then go into the details.  I
> would look at the Introduction Section in terms of providing an
> introduction to the topic and after that a view of what to expect in the
> other sections so that there is a "logical" flow.  Sorry for not describing
> this in better terms.
>
>
Yes. I've revised overall organization of the document.

> In Section 3:
>
>   'Extensions build based on this framework are collectively
>    called "Per-message Compression Extensions".'
>
> Where is there an explanation for the framework?
>
>
Added.

>   'To request use of a Per-message Compression Extension, a client
>    MUST include an element with its extension token in the
>    "Sec-WebSocket-Extensions" header in its opening handshake.'
>
> What is "element" and what is "extension token"?
>

I tried to point "extension" in the ABNF for the Sec-WebSocket-Extensions,
but there's no good name (such as extension descriptor, extension
definition, maybe) given for that part in RFC 6455.

"Sec-WebSocket-Extensions header element" might be shortest description.
Otherwise, we need to write clauses for each occurrence of element or
define our own alias for it.

Regarding introduction section, I just removed the detail.

>
>   "A client MAY list multiple Per-message Compression Extensions
>    with the same name to offer use of the same algorithm with
>    different configurations."
>
> Why is this a MAY?
>

It's intended to clarify it's ok to send multiple offers with the same
name. I've rewritten the sentence using the text suggested by Barry.

>
>   "The parameters MUST be derived from the parameters sent by the
>    client and the server's capability."
>
> What is the meaning of "the parameters must be derived"?
>

Erased during addressing Barry's comments.

>
>   'To reject use of a Per-message Compression Extension, a server MUST
>    simply ignore the element in the "Sec-WebSocket-Extensions" header in
>    the client's opening handshake.'
>
> This is not clear.
>

Erased during addressing Barry's comments.

>
>   "If a client doesn't support the extension and its parameters replied
>    from the server, the client MUST _Fail the WebSocket Connection_."
>
> What is the meaning of "MUST _Fail"?
>

It's commonly used in RFC 6455. Maybe you meant that I should cite RFC 6455
and define the term in this document. Done.

Done too for _the WebSocket Connection is established_ event.

>
>   "Otherwise, once _the WebSocket Connection is established_, both
>    endpoints MUST use the algorithm described in Section 4 to exchange
>    messages."
>
> This comes out as IF requirement1 ELSE requirement2.
>
> I suggest explaining how the protocol should work.  Error handling could
> be part of that.
>
> In Section 4.1:
>
>   "To send a compressed message, an endpoint MUST use the following
>    algorithm."
>
> I only need to know the format to use to send the compressed message.  I
> suggest looking at it from that angle instead of specifying an algorithm.
>
> In Section 5.2.1:
>
>   "The ABNF [RFC5234] for the value of this parameter is 1*DIGIT."
>
> I suggest following the usual practices for ABNF (see existing RFCs for
> examples).
>
>
Not sure which is the most supported format...

Rewrote referring to httpbis documents. I.e.

  c2s_max_window_bits = 1*DIGIT

and added a section about terminology and cited RFC 5234 in it.

> Each section seems like a standalone part of the draft.  In Section
> 5.3.2.1:
>
>   "The first 2 octets are the WebSocket protocol's overhead (FIN=1,
>    RSV1=1, RSV2=0, RSV3=0, opcode=text, MASK=0, Payload length=7)."
>
> I suggest using ASCII art for illustration.
>

Basically I feel like it's too much. But yes, it's good to show an example
of a diagram with the RSV1 is set once.

Done.

>
>   "To send it after fragmentation, split the compressed payload and
>    build frames for each of split data as well as fragmentation process
>    done when the compression extension is not used."
>
> I suggest avoiding the tutorial approach.  I would look at this as what
> goes on the wire.
>

Sorry I couldn't get what you mean.

Using "To A do B" format is bad?

> In Section 6:
>
>   "There are no security concerns for now."
>
> This is going to flag this as an issue.  I suggest thinking about the
> security considerations.  There must be something which could go wrong.
>

Added one concern (citing CRIME attack).

>
> The basic idea is there.  It is a matter of having text to explain that.
>  I suggest avoiding sentences like 'Then, a block containing "llo"
> follows.' as it will create more work for the RFC Editor.

Which part of the sentence is bad for the RFC Editor?

>  The draft unfortunately has to be rewritten.  It is better not to make
> liberal use of RFC 2119 key words.  I suggest using them for
> interoperability.

Could you please point a sentence which is considered to be making liberal
use of the key worlds? I tried to remove inappropriate use but couldn't
find so many.

>  RFC 6709 discusses about design considerations for extensions.  It could
> be used as guidance.  I suggest looking at some other RFCs discussing about
> extensions to get an idea of the possible styles and then choosing one.
>
> Regards,
> -sm
>
>
>
>
Takeshi

---

From: SM <[email-masked]>
Date: Sat, 23 Mar 2013 00:23:33 -0700

Hi Takeshi,
At 05:13 12-03-2013, Takeshi Yoshino wrote:

>It's commonly used in RFC 6455. Maybe you meant that I should cite 
>RFC 6455 and define the term in this document. Done.

Yes.  What I meant was what the implementer would have to do as 
"fail" does not specify specific behavior, e.g. return a specific 
error condition.

>Not sure which is the most supported format...

It is a matter of personal preference. :-)  Pick something you find 
better and people can review and comment.

>Basically I feel like it's too much. But yes, it's good to show an 
>example of a diagram with the RSV1 is set once.

Ok.

>Sorry I couldn't get what you mean.

What I meant was not to get into all the details of how to 
implement.  It is better in my opinion to explain what is being 
sent.  Please let me know if I did not explain it well.

>Added one concern (citing CRIME attack).

Ok.

>Which part of the sentence is bad for the RFC Editor?

I don't really know how to explain this.  I would say that the 
sentence looks more like pseudo code instead of one written in English.

>Could you please point a sentence which is considered to be making 
>liberal use of the key worlds? I tried to remove inappropriate use 
>but couldn't find so many.

I'll comment using an example from draft-ietf-hybi-permessage-compression-07:

   'A server MAY attach the "c2s_no_context_takeover" extension parameter
    to disallow the client to use the LZ77 sliding window used to build
    frames for the last message the client sent to build frames for the
    next message to send.  The "c2s_no_context_takeover" extension
    parameter has no value.  Clients SHOULD be able to accept the
    "c2s_no_context_takeover" parameter.  A client that received this
    parameter MUST reset its LZ77 sliding window for sending to empty for
    each message.'

There are three RFC 2119 key words in the above, i.e. MAY, SHOULD, 
MUST.  The client "SHOULD be able to accept" is an assumption.  The 
"MUST reset" is what the client must do.   I could write it in terms 
of what "c2s_no_context_takeover" means and what the client must do 
when it receives the parameter.

Regards,
-sm