Re: Some comments on draft-ietf-tsvwg-sctp-strrst-05.txt

[Michael Tüxen <[e-mail]>]

Hi Gorry,

thank you very much for the comments. See my comments in-line.

I tried to address your points. To simplify the discussion
I have submitted a version which includes the changes based
on the comments from you and Tom.

Most likely you still have some comments (I might have not
addressed all of them appropriately), but I wanted to have
a 'known base' for future discussion. That is why I submitted
the new rev. It is not, because I do not want to do more changes.

Please go through my comments based on the new rev and let me
know what still need to be addressed.

Best regards
Michael

On Sep 8, 2010, at 11:06 AM, Gorry Fairhurst wrote:

> 
> I've read draft-ietf-tsvwg-sctp-strrst-05.txt, since this document has been suggested as nearing completion. I have some comments: In this version of the draft, I was unable to  check the details of the specification, because I didn't find the current language sufficeintly tight.
> 
> This is therefore a non-chair review, intended to help preparation of the next revision of this document. I'd expect the WG to comment on the protocol details. I also plan to review the next version.
> 
> Gorry (not wearing a chair hat this time!)
> 
> ----
> 
> The document speaks of/maximum segment lifetime/ - is this the IP-level MSL or something more specific?
> (MSL is a rather imprecise concept in the current IETF).
Yes, it is the IP level MSL. That is why we used that term.
> 
> ---
> 
> Introduction needs to be clearer - please check English and that this is clear about what the document describes and how this may be used.
I've taken the suggested modifications from Tom. I hope this make the text clear enough.
If not, please let me know.
> 
> ---
> Section 3 (NiT)
> - This section should assume IANA makes the assignment and therefore simply say
> "the chunk type assigned by IANA for a STREAM RESET"
> 
> or "the value XXTBDXX"
> - with a note to IANA to replace the string with the assigned type.
Hmm. We really want IANA to use this value. It is used in FreeBSD, Wireshark, INET
and possibly in other implementations. All IANA related text is written such that
concrete numbers are suggested. We've done this earlier and had the same comments.
The problem is that we can;t get the numbers assigned before the RFC comes out
but we have (at least most of the time) released code before the RFC comes out.

So I would suggest to leave it as it is. If the WG thinks this is not acceptable,
we can take out the concrete numbers, wait for IANA making its choices and will
have interoperability problems...
> ---
> 
> sect 3, point 8 - is the error value unambiguous? can you say what the code is, or refer to the section number in an RFC that defines appropriate values?
I make the text more explicit:
<t>If a sender transmits an unsupported combination, the receiver SHOULD
send an ERROR chunk with a Protocol Violation cause as defined in
section 3.3.10.13 of <xref target="RFC4960"/>).</t>
> ---
> Section 4.1
> 
>  Stream Reset Request Sequence Number: 4 bytes (unsigned integer)
> /It is increased by 1./
> - when? Is this once for each request? - please specify.
I changed it to
"It is increased by 1 whenever sending a new Stream Reset Request parameter."
This is explained in detail in the procedures section.
> 
> Stream Number N: 2 bytes (unsigned integer)
> /     This optional field, if included, is used to indicates specific
>       streams that are to be reset./
> - Can this field be present several times in the same request?
I have replace "Stream Number N" by "Stream Number 1..N" to make
clear (a in the figure) that you can have N Stream Numbers.
> - How does the receiver know this?
From the Parameter Length... I changed that text to
This field holds the length in bytes of the parameter;
the value MUST be 16 + 2 * N.
> ---
> Section 4.2
> 
> - Same two questions as in section 4.1 above.
Same answer. Only the length is 8 + 2 * N.
> ---
> Section 4.2
> 
>  Stream Reset Request Sequence Number: 4 bytes (unsigned integer)
> /It is increased by 1./
> - when? Is this once for each request?
Same as above.
> ---
> Section 4.5
> /This field holds the length of the parameter, a fixed value of 12
>       MUST be found in this field./
> Suggest:
> /This field holds the length of the parameter, this
>  MUST be a fixed value of 12./
Based on suggested text by Tom it reads now:
This field holds the length in bytes of the parameter; the value MUST be 12.
OK?
> 
> When is Stream Reset Request Sequence Number increased (as before).
Same answer as before...
> 
> /Number of new streams: 2 bytes (unsigned integer)  This value holds
>       the number of additional outgoing streams the sender would like
>       added to the association./
> - /would like added/? or /requests to be added/?
I took "requests to be added"...
> 
> 
> /   This parameter can appear in a STREAM RESET chunk.  This parameter/
>                    ^^^
> - is this /MAY/?
Applied.
> ---
> Section 5.1.1 para 2
> 
> If the chunk type of the STREAM_RESET chunk
>    does NOT appear in the
>         ^^^
> - This does not seem to be a correct use of a keyword, could you please
> check?
It is just a 'not'...
> 
> /with an appropriate error indicating the peer SCTP stack does not
>    support the stream reset extension./
> - What is the appropriate error code?
The text says:
... any request by the application for such service SHOULD
be responded to with an appropriate error...
Therefore this is an API thing and the "appropriate error code"
is API dependent. So I think we can not be more specific.
> ---
> Section 5.1.2
> 
> /The following steps MUST be followed:/
> - This does not seem an appropriate use of MUST.
Why not? What are you suggesting to use instead.
We use similar wording for example in RFC 4960, page 76,
first sentence on that page. This does not mean that
the wording is used appropriately...
> 
> /it is unknown as to if/
> - it is not known whether
Already reported by Tom.
> 
> /will accept or deny it/
> - is the word /deny/ or /reject/ or /not accept/ ?
Reconsidering this: I think we use deny almost everywhere. I'll replace some
occurrences of reject by deny, also undoing a change suggested by Tom.

If you think deny is wrong, we can change that (in all places), but this
is exposed in the API. So we would need these constants, too.
> 
> /A3:  If this Outgoing SSN Reset Request Parameter is sent in response
>         to an Incoming SSN Request Parameter, the Stream Numbers between
>         the Outgoing and the Incoming parameters MUST match.  /
> 
> - I don't understand the MUST.
> - What happens if they do not match? I suspect the wording needs to be
> changed to say, if this xxx is received, the receiver MUST , otherwise
> it MUST do yyy.
A3 and A5 are somewhat duplications of each other. I have removed A3.
> 
> A.5
> /no Stream Numbers MUST be
>         put into the Outgoing SSN Reset Request Parameter/
> - This does not seem a requirement,please use lower case /must/.
No, it is the way to say All streams. 
> 
> /If the
>         sender wants only some outgoing streams to be reset these Stream
>         Numbers MUST be filled in the Outgoing SSN Reset Request
>         Parameter./
> - perhaps should be something like:
> /If the
>         sender requests only some outgoing streams to be reset the
>         set of Stream are identified in the Outgoing SSN Reset Request
>         Parameter./
The text reads after Toms suggestions:
If the sender requests only some outgoing streams to be reset these Stream 
Numbers MUST be placed in the Outgoing SSN Reset Request Parameter.
OK?
> 
> / It MAY be put together with an Incoming SSN Reset
>         Request Parameter or an Stream Reset Response Parameter and MUST
>         NOT be put together with any other parameter./
> - I am not sure exactly what this means.
These are the limitations of putting two parameters in the chunk.
> 
> 
> - Please check section 5.1.3, and 5.1.4, for similar cases.

> 
> -----
> 
> /  There SHOULD NOT be sent more than one SSN/TSN Reset Request within a
>    maximum segment lifetime./
> - where is the maximum segment lifetime defined, if this is the IP MSL,
> then I suggest an explicit time should be declared. This could be the
> MSL value, is this seems appropriate.
What is the explicit MSL value? We actually mean the IP MSL. I have
added some clarification.
> 
> ----
> 5.1.5
> /   D2:  The result of the processing of the incoming request is filled/
> - I'm not sure what this means, is it the resulting value, or return code or what?
It is one of the values from table 3.
> ---
> / D3:  If the incoming request is a SSN/TSN reset requests, /
>                                                         ^
> - delete /s/
Done.
> ---
> /is filled with the next TSN the sender
>        of this Stream Reset Response Parameter will assign/
> - perhaps clearer as /the next TSN value to be assigned by the sender of this Stream Reset Response Parameter/
Done.
> ---
> /the Sender's next TSN field is not filled./
> - SHOULD be zero? - or unchanged or what?
It is an optional field. So you do not fill it, it is not there. That is why it is optional.
> ---
> D4
> - This procedure seems under-specified.
> /for this is the highest TSN it has seen plus some delta. /
> - I get the idea, but this is not much help to an implementor, can
> we use some SHOULD and MUSTs to guide this?
> - How big is delta - or how do you choose?
Thanks for catching this. I added:
The value should be the smallest TSN not acknowledged by the receiver
of the request plus 2^31.
> ---
> 5.1.6
> /it is able to send to/
> suggest:
> /to which it is able to send/
Done.
> ---
> / Note that new streams are added adjacent to
>   the previous steams with no gaps/
> - no gaps? - in what space, please clarify - does this mean streams are numbered consecutively?
The next sentence gives an example. Streams are identified by stream identifiers starting from
0 to N-1, where N is the number of streams. There cannot be gaps.
> ---
> /   Any new stream MUST number its first message to be stream sequence 0./
> -/Any/A/
> - I'm not sure what this sentence is about, is this the SCTP sender action for a new stream, what is being numbered?
Each ordered message in a stream has a stream sequence number (SSN). The meaning of the sentence
is that the SSN of the first ordered message is 0.
I changed it to:
A new stream MUST use for its first ordered message the stream sequence number 0.
> ---
> 5.2.1
> /   Upon reception of a Stream Reset Chunk each parameter within it
>   should be processed.  If some parameters have to be sent back, they
>   MUST all be put into one STREAM RESET chunk./
> - The language here needs to be a protocol spec, not a narrative. Please use some SHOULD and MUSTs to specify this.
Done.
> ---
> /the same STREAM RESET chunk has to be sent back in response as
>   earlier./
> - The language here needs to be a protocol spec, not a narrative. Please use some SHOULD and MUSTs to specify this.
Done.
> ---
> /   formed.  If for whatever reason the endpoint does NOT wish to process/
>                                                      ^^^
> - I think this should be lower case, it does not seem like a protocol action.
Correct.
> ---
> 5.2.2.  Receiver Side Procedures for the Outgoing SSN Reset Request
>        Parameter
> 
>   In the case that the endpoint is willing to perform a stream reset
>   the following steps SHOULD be followed:
>                       ^^^^^^^
changed to MUST.
> - This SHOULD conflicts with /MUST/ later in the same section. Either the actions are enquired or recommended, please refine use of RFC 2119 keywords in this section.
> 
> - This also applies 5.2.3, and 5.2.4!
Solve similar.
> - 5.2.6 has the opposite problem, it requires a recommended procedure.
I changed the API stuff to lowercase wording.
> - It may be best to omit the keyword in the introduction and ensure each protocol action is clearly called-out as recommended, optional, or required using MUST, SHOULD or MAY.
I hope I made it at least consistent.
> ---
> E1: /NOT/not/
Done.
> ---
> E2:
> /If the Stream Reset Timer is running for the Stream Reset
>        Request Sequence Number indicated in the Stream Reset Response
>        Sequence Number field, mark the Stream Reset Request Sequence
>        Number as acknowledged.  If all Stream Reset Request Sequence
>        Numbers the Stream Reset Timer is running for are acknowledged,
>        stop the Stream Reset Timer./
> - This and following sections need to be more concrete and use RFC 2119 language.
Fixed.
> ---
> 5.2.5
> - what is the action if not successful?
... then he does not change anything.
> ---
> 5.3
> /Various Examples of the Stream Reset procedures/
> - omit Various?
Done.
> ----
> - please include one sentence explaining the diagram, saying what E-Z is, and saying that time progresses in a downwards direction.
Added:
<t>The following message flows between an Endpoint A and an
Endpoint Z illustrate the described procedures. The time
progresses in downward direction.</t>
> ---
> 6
> - I'd have changed /SHOULD/should/ - it is not a specific protocol requirement. You may even prefer to say /needs to be extended/ ?
Yes, "needs to be extended" is what we want.
> Overall:
> ---
> - I have a question on section 6:
> - What is the intended status of this API change?
I've added the sentence:
<t>Please note that this section is informational only.</t>
This is what we want, since the Socket API is also informational. Therefore the
reference to the Socket API ID can continue to be an informational one, right?
> 
> /The suggested value of this field for IANA is/
I do not understand this comment. Section 6 requires no action from IANA and does not
contain this sentence.
> 
> ----
> 7
> 
> Please further consider the risks and countermeasures.
> 
> It appears to me there is no ambiguity at the application in the re-use of a stream, can you add text to explain why this does not result in applications problems by receiving the numbered data twice
The only problem I see is that an application is written under the assumption that the SSNs are monotonically
increasing. When they run on top of a stack supporting stream reset and the peer resets a stream they
might not be able to handle it. That is why we have specified in the API that the application must explicitly
allow the processing the stream reset requests. So for an old applications all requests would be denied.
> 
> I see the point on the abort… but if I understand, this method can also be used to create extra state for "rogue" flows, and therefore this is a vulnerability that may need to be countered, and is different to aborting a connection.
The point is that an on path attacker can always do anything. An off-path attacker has to guess
the verification tag. This does not depend on the use of the extension described in this ID.
> 
> Reseting a stream "breaks" the connection, but can it leave state? How do the end points detect this?
It does not leave state, applications can subscribe to corresponding notifications. But this is an API issue.
> 
> etc.
> 
> - I'm not suggesting these are show-stoppers, but they could be points that may require a robust implementation to take precautions. Does the "maximum segment lifetime" mitigate this?
The MSL is to avoid multiple TSNs wrap arounds within an MSL...
> 
> ----
> 
> 8.1,8.2 - /The value MUST be from/IANA shoal assign this value from/
Applied. Do you really mean 'shoal' or is it a typo?
> ---
> 
> References
> - Normative reference needed to Chunk Types ID.
Good point. Done.
> 
> - The current text will require a NORMATIVE reference to the API, since this is a standards-track document that updates this. This could be relaxed if the API were explicitly stated as an "informative" section or an informative annexe. Either way though, this looks like it may need APPs area review, in the same way that the API will need this - so there is a logical dependency at least for getting the reviews completed.
Hopefully addressed above. 
> 
> ---
> 
> The document, I suggest could benefit from a little explanation of WHY this is OK for SCTP and would be difficult for TCP. This understanding may be clear to the SCTP community, but I do not think it is necessarily clear to a TSV reviewer or the IETF in general. I discussed this with Randal informally - things like "how do you reset a stream sequence number during and avoid problems with older mis-sequenced in-flight segments?" (i.e. explain how this works in SCTP); "how do you avoid blind replay attacks (i.e. again explain how SCTP has a way to protect this);
Hmm, not sure what you want here. TCP has no streams, so it does not have stream sequence numbers exposed
to the application. TCP numbers bytes, but the sequence numbers are not exposed. So I'm not sure how 
procedures defined in this ID could be 'translated to TCP'.

SCTP uses verification tags to protect against bind attacks. This has nothing to do with using
the extension defined in this ID or not.

So if you could describe more detailed what kind of explanation you want (and where), I can write up
something.
> 
> ---
>