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

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

Hi Gorry,

see my comments in-line. Sorry for the delay, I've also incorporated
comments received by Irene Ruengeler. 
I've submitted an updated version.

Please let me know if your comments have been addressed.

Best regards
Michael

On Sep 26, 2010, at 9:28 AM, Gorry Fairhurst wrote:

> Thanks for the new revision.
> 
> I have comments in-line on the responses where you asked
> for a few clarifications, which I have now tried to provide below.
> 
> Best wishes,
> 
> Gorry
> 
> On 25/09/2010 23:26, Michael Tüxen wrote:
>> 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:
>> 
> Good plan.
> 
> <snip>
>>> ---
>>> 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
> I think we can get numbers as soon as we have consensus to publish, that
> could during the WG process,  i.e. I'd especially support getting numbers as
> soon as we are ready to do a WGLC.
OK.
>> 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...
> I think the draft could simply say these are tentative assignments to be confirmed
> by IANA when published, in a RFC-ed note:-)
In the descriptions of the chunk and parameters we now use always the sentence:
The suggested value of this field for IANA is x.

I also added an RFC-ed note stating:
The suggested values for the chunk type and the chunk parameter types
are tentative and to be confirmed by IANA.
>>> ---
>>> 
>>> 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>
>> <snip>
>>> ---
>>> 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?
> OK
> <snip>
>> ---
>>> 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...
> I'd need to read, in general I prefer to use MUST when it
> refers to some action, or some format that must be used. So,
> 
> I'd have preferred something like:
> 
> The receiver must do the following;
> * It MUST do action A.
> * If p or q it MUST do action B.
> * A z format response MUST be sent and all option fields MUST be zero.
> 
> However, this is style.  I'd be happy to re-read the new version,
> and I know you have made many changes. The problem is when
> the list of things after the "following:" do not all have an
> RFC2119 requirement in them... e.g. E6 - which says something
> /is/ put into a field.
OK, I have changed the text to the style you prefer.
> 
> <snip>
>>> /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.
> I expect it will be fine, if consistently used.
OK, please double check.
>> /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.
> Then is the requirement that the Stream Number MUST be zero (?), indicating all streams?
You could also list all streams... So I use now SHOULD...
>>> /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?
> OK, the MUST is clear.
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.
> 
> - My problem here was a sentence with combinational logic /or/ /and/
> that also includes a MAY and a MUST NOT, and refers back to "it".
> - Perhaps two sentences would be clearer instead of the /and/.
> - Could you also refer to the requirement?
OK, changed to
The Outgoing SSN Reset Request Parameter MAY be put together with either
an Incoming SSN Reset Request Parameter or an Stream Reset Response Parameter
but not both. It MUST NOT be put together with any other parameter as
described in <xref target="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.
> IP MSL is an interesting topic in itself... but let's not be diverted down this
> conversation. My suggestion was state the time you required, and justify
> this as being the same as IP MSL.
OK, I changed it to
<t>Only one SSN/TSN Reset Request SHOULD be sent within 30 seconds,
which is considered a maximum segment lifetime, the IP MSL.</t>
>>> ----
>>> 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.
> - I didn't understand "filled"
OK, it reads now:
<t>The result of the processing of the incoming request according to
<xref target="resulttable"/> MUST be placed in the Result field of
the Stream Reset Response Parameter.</t>
> 
> 
> <snip>
>>> ---
>>> /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.
> - Say this rather than "filled".
Changed to
For other requests the Sender's next TSN field, which is optinal,
MUST NOT be used.</t>
and
For other requests the Receiver's next TSN field, which is optional,
MUST NOT be used.</t>	
>> 
>>> 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.
> See above IANA note. Documents should contain something to say this number is
> to be allocated or *confirmed* by IANA - at least as an RFC -Ed note :-)
OK, hopefully addressed above.
>> ----
>> 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.
> Sounds good to mention.
I added:
<t>The SCTP socket API as described in
<xref target="I-D.ietf-tsvwg-sctpsocket"/> exposes the sequence numbers of
received DATA chunks to the application. An application might expect them
to be monotonically increasing. When using the stream reset extension this
might no longer be true. Therefore the the application must enable this
extension explicitly before it is used.
>>> 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.
> I agree, let's state this!
Added:
<t>SCTP associations are protected against blind attackers by using
the the verification tags. This is still valid when the stream reset
extension is used. Therefore this extension does not add any additional
security risk to SCTP in relation to blind attackers.</t>
>>> 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.
> Again, good note.
Added:
 In addition, application must
subscribe explicitly to notifications related to the stream reset
extension before receiving them.</t>
>>> 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...
> Again, good note.
Added:
<t>When the both sequence numbers are reset, the maximum segement
liketime is used to avoid the wrap-around for the TSN.</t>
>>> ----
>>> 
>>> 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?
> It is indeed a mistake by me! /shoal/should/
Fixed.
>>> ---
>>> 
>>> 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.
> I don't think the SCTP world has a problem with this - but if we get a TCP reviewer, I'd like to just see a statement
> that SCTP Streams are uniquely defined, and that the TSNs give protection. The above discussions also help to
> identify what is already a part of SCTP and where there could be issues.
OK, I have added
<t>The socket API for SCTP defined in <xref target="I-D.ietf-tsvwg-sctpsocket"/>
exposes the sequence numbers used by SCTP for user message transfer.
Therefore, resetting them can be used by application writers. Please
note that the corresponding sequence number for TCP is not exposed
via the socket API for TCP.</t>

to the end of the introduction.
> 
> 
>