[Last-Call] Re: [art] Artart last call review of draft-ietf-tsvwg-sctp-zero-checksum-09

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On 7. Jun 2024, at 18:46, Charles Eckel (eckelcu) <eckelcu@xxxxxxxxx> wrote:
> 
> One late comment (sorry), please see inline.
> 
>> On May 16, 2024, at 7:33 AM, Michael Tuexen <Michael.Tuexen@xxxxxxxxxxxxxxxxx> wrote:
>> 
>>> On 14. May 2024, at 02:54, Bernard Aboba <bernard.aboba@xxxxxxxxx> wrote:
>>> 
>>> Comments below.
>> Thanks a lot. Answers inline.
>> 
>> Best regards
>> Michael
>>> 
>>> On Tue, May 7, 2024 at 7:18 AM Michael Tuexen <Michael.Tuexen@xxxxxxxxxxxxxxxxx> wrote:
>>>> On 3. May 2024, at 07:23, Bernard Aboba via Datatracker <noreply@xxxxxxxx> wrote:
>>>> 
>>>> Reviewer: Bernard Aboba
>>>> Review result: Ready with Issues
>>>> 
>>>> Reviewer: Bernard Aboba
>>>> Document: draft-ietf-tsvwg-sctp-zero-checksum
>>>> Status: Ready with (minor) Issues
>>>> 
>>> Hi Bernard,
>>> 
>>> thank you very much for the review. See my comments in-line.
>>> 
>>> Best regards
>>> Michael
>>>> The document is well written and allowing a zero checksum for SCTP over DTLS makes sense.
>>>> In order to allow for other potential protection mechanisms, the document sets up an
>>>> IANA registry and associated documentation requirements. However, I am not sure
>>>> that the registration criteria are clear enough. 
>>>> 
>>>> At present the document does not talk about implementation status, although I believe it
>>>> has been implemented in Pion, dcSCTP and perhaps other WebRTC data channel implementations.
>>> It is also implemented in the FreeBSD implementation and therefore in usrsctp.
>>> There are also test cases available at:
>>> https://github.com/tsvwg/zero-checksum/tree/main/packetdrill-tests
>>> which have been used to test the FreeBSD implementation.
>>> I think all of this is covered in the shepherds write-up.
>>> Do you think it would be better to write the information up in a
>>> "Implementation Status Section"?
>>>> Have any issues arisen during implementation?
>>> The implementer of dcSCTP did not report any issues, he explicitly said that
>>> allowing sending packets with correct checksums where using a zero checksum
>>> would be allowed was simplifying the initial implementation.
>>> 
>>> The Pion stack implementer implemented a feature negotiation instead of
>>> feature declaration. Not sure why. I think the document is very clear
>>> about this. But this has been fixed in the Pion implementation now.
>>> 
>>> [BA] Thanks for the pointers and summary.  I don't think you need to add an implementation section, but you might want to note that the specification has been implemented and deployed widely.
>> I wouldn't say it is widely deployed yet, since, as far as I know, it
>> is not enabled in the Google stack yet, since they want to wait for the
>> IANA assignment.
>> I would prefer to keep the implementation status in the shepherd writeup
>> and not as part of the specification.
>> In the SCTP case, we usually have some implementation in an open source
>> stack.
> 
> You can use the Datatracker “Additional Resource” mechanism to point to the known implementations.
I guess, "one" can use it, but it is not "me". I, as an author/editor, can
not change any Additional Resources. Since I provided all the links to the
document shepherd, I leave it up to him to chose the place he prefers to
make the information available. Right now it is in the shepherd's writeup...

Best regards
Michael
> 
> Cheers,
> Charles
> 
>>>> 
>>>> NITs
>>>> ----
>>>> 
>>>> 3.  Alternate Error Detection Methods
>>>> 
>>>> SCTP uses a CRC32c checksum to provide some level of data integrity.
>>>> The CRC32c checksum is computed based on the SCTP common header and
>>>> the chunks contained in the packet.  In particular, the computation
>>>> of the CRC32c checksum does not involve a pseudo header for IPv4 or
>>>> IPv6 like the computation of the TCP checksum, as specified in
>>>> [RFC9293], or the UDP checksum, as specified in [RFC0768].
>>>> 
>>>> [BA] Would it be appropriate to advise against turning off the UDP checksum as well?
>>> I think using a zero checksum for UDP when using SCTP/DTLS/UDP/IP
>>> or SCTP/UDP/IP is independent of using the zero checksum for SCTP
>>> as discussed in this document. So I think there is no need to discuss
>>> it here.
>>> In case you are referring to SCTP/UDP/IP, this is discussed in
>>> RFC 6951 and this document does not change anything.
>>> The reference to the UDP and TCP checksum is only given, to
>>> stress the difference between the these and the SCTP checksum.
>>> The use of the pseudo-header for UDP and TCP.
>>> 
>>> [BA] Makes sense.
>>>> 
>>>> Alternate error detection methods have two requirements:
>>>> 
>>>> 1.  An alternate error detection method MUST provide an equal or
>>>>     better level of data integrity than the one provided by using the
>>>>     CRC32c checksum algorithm.  This MAY only apply to packets
>>>>     satisfying some method specific constraints.
>>>> 
>>>> [BA] I think you may need to define the meaning of "equal or better" more
>>>> exactly.  For example, that the alternative provides the same coverage
>>>> as the CRC32c checksum, with a lower probability of a false negative.
>>> Well, I intentionally used a vague term. The critical part in your
>>> definition is the "same coverage".
>>> The CRC32c covers the from the common header the port numbers and the
>>> verification tag and then all chunks.
>>> The authors of
>>> * https://www.ietf.org/archive/id/draft-ietf-tsvwg-dtls-over-sctp-bis-08.html
>>> * https://www.ietf.org/archive/id/draft-westerlund-tsvwg-sctp-crypto-chunk-02.html
>>> * https://www.ietf.org/archive/id/draft-westerlund-tsvwg-sctp-dtls-chunk-01.html
>>> suggest that you can also use an incorrect checksum of zero for
>>> all packets which have the AUTH, CRYPTO, or DTLS chunk as its first chunk.
>>> Then the coverage would only be over the chunks, not the fields contained
>>> in the common header.
>>> The protection would also work for these fields, since you would use
>>> these fields to lookup the association and check the verification
>>> tag. So if there would be an error in the port numbers, most likely
>>> the lookup will result in no association or one with different
>>> key material, such that the AUTH, CRYPTO, or DTLS chunk check would
>>> fail. If the transmission error is in the verification tag, the
>>> packet would be dropped anyway.
>>> 
>>> So if you prefer, we could say
>>> 
>>> An alternate error detection method MUST provide an equal or
>>> lower probability of false negatives than the one provided by using the
>>> CRC32c checksum algorithm.
>>> 
>>> Would that address your concern?
>>> 
>>> [BA] Yes.
>> Done.
>>>> 
>>>> 2.  Using an alternate error detection method MUST NOT result in a
>>>>     path failure for more than two retransmission timeouts (RTO) due
>>>>     to middleboxes on the path expecting correct CRC32c checksums.
>>>> 
>>>> [BA] This requirement depends on the behavior of middleboxes, so it's
>>>> not clear to me how adherence to the MUST NOT can be tested.
>>> My understanding is that an implementation must not fail a path for longer
>>> than 2 RTOs if the path starts to drop all packets with an incorrect
>>> checksum of zero at any time. The next sentence gives a hint how
>>> this could be done.
>>> You might want to combine this with some probing when you initially
>>> confirm the path. But since all of this is not needed for WebRTC,
>>> we wanted to keep this out of this document and put that into
>>> any upcoming document, where this is relevant.
>>> 
>>> [BA]  OK, that makes sense.
>>>> 
>>>> To fulfill the second requirement, alternate error detection methods
>>>> MAY use a heuristic to detect the existence of such middleboxes and
>>>> use correct CRC32c checksums on these affected paths.
>>>> 
>>>> [BA] The "MAY" here seems to be somewhat in conflict with the much
>>>> stronger MUST NOT, particularly since we are talking about middlebox
>>>> detection. At present the document only allocates a code point for
>>>> DTLS, to which this requirement doesn't apply.
>>> Would it be less confusing to write:
>>> 
>>> To fulfill the second requirement, alternate error detection methods
>>> could use a heuristic to detect the existence of such middleboxes and
>>> use correct CRC32c checksums on these affected paths.
>>> 
>>> [BA] Yes, that would help.
>> Done.
>>>> One example fulfilling the first requirement is using DTLS as the
>>>> lower layer of SCTP as specified in [RFC8261].  Another example is
>>>> using SCTP Authentication as specified in [RFC4895].  Of course, this
>>>> only applies to all SCTP packets having an AUTH chunk as its first
>>>> chunk.  However, using SCTP Authentication without any heuristic does
>>>> not fulfill the second requirement.  Since using DTLS as the lower
>>>> layer of SCTP as specified in [RFC8261] also fulfills the second
>>>> requirement, it can be used as an alternate error detection method
>>>> (see Section 6).
>>>> 
>>>> [BA] SCTP Authentication is not allocated a code point. So not sure
>>>> why this is mentioned as "another example" - is this just to indicate
>>>> why it is not acceptable (e.g. not meeting the second requirement)?
>>> Yes.
>>> Basically, SCTP/DTLS is an example fulfilling both requirements and
>>> not have any addition method specific constraints.
>>> The document is just using SCTP AUTH is an example fulfilling the first,
>>> but not the second requirement. It is also an example, which has a method
>>> specific constraint: "The AUTH chunk must be the first chunk".
>>> If you can suggest an improvement to the wording to make this clearer,
>>> I'm happy to take it.
>>> 
>>> [BA]  The text is clear as it is; was just unsure of why it was there.
>> OK.
>>>> 
>>>> 5.1.  Declaration of Feature Support
>>>> 
>>>> An endpoint willing to accept SCTP packets with an incorrect checksum
>>>> of zero MUST include the Zero Checksum Acceptable Chunk Parameter
>>>> indicating the alternate error detection method it is willing to use
>>>> in the INIT or INIT ACK chunk it sends.
>>>> 
>>>> An SCTP implementation MAY also require the upper layer to indicate
>>>> that it is fine to use a specific alternate error detection method
>>>> before including the corresponding Zero Checksum Acceptable Chunk
>>>> Parameter.
>>>> 
>>>> [BA] What if the alternate error detection method is not consistent
>>>> with what has been established? For example, SCTP over DTLS/UDP has
>>>> been established, but some other method (not yet allocated a code point)
>>>> is negotiated?
>>> Please note that there is no negotiation. Each side declared what it
>>> is willing to accept.
>>> Packets received with an incorrect checksum of zero are accepted if
>>> they fulfill the additional requirements of the error detection method
>>> announced. If that is not true they are dropped.
>>> 
>>> The text in 5.3 contains:
>>> 
>>> If an endpoint has sent the Zero Checksum Acceptable Chunk Parameter
>>> indicating the support of an alternate error detection method in an
>>> INIT or INIT ACK chunk, it MUST accept SCTP packets fulfilling the
>>> requirements of the announced alternate error detection method using
>>> an incorrect checksum value of zero in addition to SCTP packets
>>> containing the correct CRC32c checksum value for this association.
>>> 
>>> Should it be stated explicitly that if the is not true, packets are dropped?
>>> 
>>> [BA] Yes.
>> I added:
>> Otherwise, the endpoint <bcp14>MUST</bcp14> drop all SCTP packets with an
>> incorrect CRC32c checksum.</t>
>>>> 
>>>> Section 5.2
>>>> 
>>>> 4.  Alternate error detection methods might have some additional
>>>>     conditions requiring that the sender MUST include a correct
>>>>     CRC32c checksum in the packet.
>>>> 
>>>> [BA] The combination of "might" and MUST is an odd one. Is this normative language needed?
>>> An alternate error detection method can have additional requirements.
>>> Like in the SCTP AUTH case "the AUTH chunk must the first chunk". Not
>>> all packets can fulfill this requirement. In this case, the sender
>>> MUST use a correct CRC32c checksum.
>>> Whether an alternate error detection method has such additional requirements
>>> depends on the alternate error detection method.
>>> Would you prefer something like:
>>> 
>>> If an alternate error detection method has some method specific constraints,
>>> the sender MUST include a correct CRC32c checksum in all packets not fulfilling
>>> these method specific constraints.
>>> 
>>> [BA] Yes.
>> Done.
>>>> 
>>>> An SCTP end point MAY require that the upper layer allowed the use of
>>>> the alternate error detection method that was announced by the peer
>>>> before sending packets with an incorrect checksum of zero.
>>>> 
>>>> [BA] MAY? In the case of DTLS, the alternate error detection method
>>>> was setup prior to initiation of the SCTP association. So why would this
>>>> be optional?
>>> If an SCTP stack implements this MAY (as the FreeBSD stack does), it gives
>>> the upper layer control whether the method is needed or not. See the sentence
>>> 
>>> An implementation might only send packets with an incorrect checksum of zero,
>>> if the alternate error detection method announced by the peer is also enabled
>>> locally via this socket option.
>>> 
>>> in the Socket API section. For example, if there are any problems with
>>> using this feature, the upper layer can disable this.
>>> But it is a MAY. So if an SCTP implementation doesn't want to allow
>>> this level of control, it is fine.
>>> 
>>> [BA] OK.
>>>> 
>>>> Section 8 IANA Considerations
>>>> 
>>>> 2.  A reference to a specification describing:
>>>> 
>>>>     (a)  the alternate error detection method,
>>>> 
>>>>     (b)  why the alternate error detection method provides an equal
>>>>          or better level of data integrity protection than the one
>>>>          provided by using the CRC32c checksum,
>>>> 
>>>> [BA] It might help to sharpen the definition of "equal or better".
>>> This needs to be handled consistently with the above text. See discussion there.
>> I changed the text to
>> <li><t>why the alternate error detection method provides an equal or
>> lower probability of false negatives than the one provided by using the CRC32c
>> checksum,</t></li>
>>>> 
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> art mailing list
>>>> art@xxxxxxxx
>>>> https://www.ietf.org/mailman/listinfo/art
>>> 
>>> _______________________________________________
>>> art mailing list -- art@xxxxxxxx
>>> To unsubscribe send an email to art-leave@xxxxxxxx
>> 
>> _______________________________________________
>> art mailing list -- art@xxxxxxxx
>> To unsubscribe send an email to art-leave@xxxxxxxx
> 

-- 
last-call mailing list -- last-call@xxxxxxxx
To unsubscribe send an email to last-call-leave@xxxxxxxx




[Index of Archives]     [IETF Annoucements]     [IETF]     [IP Storage]     [Yosemite News]     [Linux SCTP]     [Linux Newbies]     [Mhonarc]     [Fedora Users]

  Powered by Linux