From daa0dc74c5bac1b704de38b65ea1a61e69b03ff1 Mon Sep 17 00:00:00 2001 From: Eric Daniels Date: Wed, 28 Feb 2024 10:11:18 -0500 Subject: [PATCH] Add some RFC based comments --- association.go | 37 ++++++++++++++++++++++++++++++------- association_test.go | 18 +++++++++--------- rtx_timer.go | 30 ++++++++++++++++++++++-------- 3 files changed, 61 insertions(+), 24 deletions(-) diff --git a/association.go b/association.go index ce049d36..68f0d0e5 100644 --- a/association.go +++ b/association.go @@ -137,6 +137,8 @@ func getAssociationStateString(a uint32) string { // // Note: No "CLOSED" state is illustrated since if a // association is "CLOSED" its TCB SHOULD be removed. +// Note: By nature of an Association being constructed with one net.Conn, +// it is not a multi-home supporting implementation of SCTP. type Association struct { bytesReceived uint64 bytesSent uint64 @@ -304,11 +306,17 @@ func createAssociation(config Config) *Association { tsn := globalMathRandomGenerator.Uint32() a := &Association{ - netConn: config.NetConn, - maxReceiveBufferSize: maxReceiveBufferSize, - maxMessageSize: maxMessageSize, + netConn: config.NetConn, + maxReceiveBufferSize: maxReceiveBufferSize, + maxMessageSize: maxMessageSize, + + // These two max values have us not need to follow + // 5.1.1 where this peer may be incapable of supporting + // the requested amount of outbound streams from the other + // peer. myMaxNumOutboundStreams: math.MaxUint16, myMaxNumInboundStreams: math.MaxUint16, + payloadQueue: newPayloadQueue(), inflightQueue: newPayloadQueue(), pendingQueue: newPendingQueue(), @@ -1123,7 +1131,10 @@ func (a *Association) handleInit(p *packet, i *chunkInit) ([]*packet, error) { return nil, fmt.Errorf("%w: %s", ErrHandleInitState, getAssociationStateString(state)) } - // Should we be setting any of these permanently until we've ACKed further? + // NOTE: Setting these prior to a reception of a COOKIE ECHO chunk containing + // our cookie is not compliant with https://www.rfc-editor.org/rfc/rfc9260#section-5.1-2.2.3. + // It makes us more vulnerable to resource attacks, albeit minimally so. + // https://www.rfc-editor.org/rfc/rfc9260#sec_handle_stream_parameters a.myMaxNumInboundStreams = min16(i.numInboundStreams, a.myMaxNumInboundStreams) a.myMaxNumOutboundStreams = min16(i.numOutboundStreams, a.myMaxNumOutboundStreams) a.peerVerificationTag = i.initiateTag @@ -1169,6 +1180,8 @@ func (a *Association) handleInit(p *packet, i *chunkInit) ([]*packet, error) { if a.myCookie == nil { var err error + // NOTE: This generation process is not compliant with + // 5.1.3. Generating State Cookie (https://www.rfc-editor.org/rfc/rfc4960#section-5.1.3) if a.myCookie, err = newRandomStateCookie(); err != nil { return nil, err } @@ -1308,6 +1321,8 @@ func (a *Association) handleCookieEcho(c *chunkCookieEcho) []*packet { return nil } + // RFC wise, these do not seem to belong here, but removing them + // causes TestCookieEchoRetransmission to break a.t1Init.stop() a.storedInit = nil @@ -1315,6 +1330,7 @@ func (a *Association) handleCookieEcho(c *chunkCookieEcho) []*packet { a.storedCookieEcho = nil a.setState(established) + // Note: This is a future place where the user could be notified (COMMUNICATION UP) a.handshakeCompletedCh <- nil } @@ -1343,6 +1359,7 @@ func (a *Association) handleCookieAck() { a.storedCookieEcho = nil a.setState(established) + // Note: This is a future place where the user could be notified (COMMUNICATION UP) a.handshakeCompletedCh <- nil } @@ -1356,9 +1373,9 @@ func (a *Association) handleData(d *chunkPayloadData) []*packet { if canPush { s := a.getOrCreateStream(d.streamIdentifier, true, PayloadTypeUnknown) if s == nil { - // silentely discard the data. (sender will retry on T3-rtx timeout) + // silently discard the data. (sender will retry on T3-rtx timeout) // see pion/sctp#30 - a.log.Debugf("discard %d", d.streamSequenceNumber) + a.log.Debugf("[%s] discard %d", a.name, d.streamSequenceNumber) return nil } @@ -2415,13 +2432,18 @@ func (a *Association) handleChunk(p *packet, c chunk) error { var err error if _, err = c.check(); err != nil { - a.log.Errorf("[ %s ] failed validating chunk: %s ", a.name, err) + a.log.Errorf("[%s] failed validating chunk: %s ", a.name, err) return nil } isAbort := false switch c := c.(type) { + // Note: We do not do the following for chunkInit, chunkInitAck, and chunkCookieEcho: + // If an endpoint receives an INIT, INIT ACK, or COOKIE ECHO chunk but decides not to establish the + // new association due to missing mandatory parameters in the received INIT or INIT ACK chunk, invalid + // parameter values, or lack of local resources, it SHOULD respond with an ABORT chunk. + case *chunkInit: packets, err = a.handleInit(p, c) @@ -2439,6 +2461,7 @@ func (a *Association) handleChunk(p *packet, c chunk) error { } a.log.Debugf("[%s] Error chunk, with following errors: %s", a.name, errStr) + // Note: chunkHeartbeatAck not handled? case *chunkHeartbeat: packets = a.handleHeartbeat(c) diff --git a/association_test.go b/association_test.go index 3361da27..e3a518e1 100644 --- a/association_test.go +++ b/association_test.go @@ -411,8 +411,8 @@ func establishSessionPair(br *test.Bridge, a0, a1 *Association, si uint16) (*Str } func TestAssocReliable(t *testing.T) { - // sbuf - small enogh not to be fragmented - // large enobh not to be bundled + // sbuf - small enough not to be fragmented + // large enough not to be bundled sbuf := make([]byte, 1000) for i := 0; i < len(sbuf); i++ { sbuf[i] = byte(i & 0xff) @@ -420,8 +420,8 @@ func TestAssocReliable(t *testing.T) { rand.Seed(time.Now().UnixNano()) rand.Shuffle(len(sbuf), func(i, j int) { sbuf[i], sbuf[j] = sbuf[j], sbuf[i] }) - // sbufL - large enogh to be fragmented into two chunks and each chunks are - // large enobh not to be bundled + // sbufL - large enough to be fragmented into two chunks and each chunks are + // large enough not to be bundled sbufL := make([]byte, 2000) for i := 0; i < len(sbufL); i++ { sbufL[i] = byte(i & 0xff) @@ -821,8 +821,8 @@ func TestAssocReliable(t *testing.T) { func TestAssocUnreliable(t *testing.T) { // sbuf1, sbuf2: - // large enogh to be fragmented into two chunks and each chunks are - // large enobh not to be bundled + // large enough to be fragmented into two chunks and each chunks are + // large enough not to be bundled sbuf1 := make([]byte, 2000) sbuf2 := make([]byte, 2000) for i := 0; i < len(sbuf1); i++ { @@ -836,8 +836,8 @@ func TestAssocUnreliable(t *testing.T) { rand.Seed(time.Now().UnixNano()) rand.Shuffle(len(sbuf2), func(i, j int) { sbuf2[i], sbuf2[j] = sbuf2[j], sbuf2[i] }) - // sbuf - small enogh not to be fragmented - // large enobh not to be bundled + // sbuf - small enough not to be fragmented + // large enough not to be bundled sbuf := make([]byte, 1000) for i := 0; i < len(sbuf); i++ { sbuf[i] = byte(i & 0xff) @@ -1752,7 +1752,7 @@ func TestAssocT3RtxTimer(t *testing.T) { } func TestAssocCongestionControl(t *testing.T) { - // sbuf - large enobh not to be bundled + // sbuf - large enough not to be bundled sbuf := make([]byte, 1000) for i := 0; i < len(sbuf); i++ { sbuf[i] = byte(i & 0xcc) diff --git a/rtx_timer.go b/rtx_timer.go index 823186c3..42848abc 100644 --- a/rtx_timer.go +++ b/rtx_timer.go @@ -10,14 +10,28 @@ import ( ) const ( - rtoInitial float64 = 1.0 * 1000 // msec - rtoMin float64 = 1.0 * 1000 // msec - defaultRTOMax float64 = 60.0 * 1000 // msec - rtoAlpha float64 = 0.125 - rtoBeta float64 = 0.25 - maxInitRetrans uint = 8 - pathMaxRetrans uint = 5 - noMaxRetrans uint = 0 + // RTO.Initial in msec + rtoInitial float64 = 1.0 * 1000 + + // RTO.Min in msec + rtoMin float64 = 1.0 * 1000 + + // RTO.Max in msec + defaultRTOMax float64 = 60.0 * 1000 + + // RTO.Alpha + rtoAlpha float64 = 0.125 + + // RTO.Beta + rtoBeta float64 = 0.25 + + // Max.Init.Retransmits: + maxInitRetrans uint = 8 + + // Path.Max.Retrans + pathMaxRetrans uint = 5 + + noMaxRetrans uint = 0 ) // rtoManager manages Rtx timeout values.