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