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

[Gorry Fairhurst <[e-mail]>]

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

<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.
> /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?
>> /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.
>> / 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?
>>
>> - 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.
>> ----
>> 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"


<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".
>
>> 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 :-)
> ----
> 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 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!
>> 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.
>> 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.
>> ----
>>
>> 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/
>> ---
>>
>> 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.