> 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. > > > > 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 -- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
