Fix handling of retransmitted cookie-echo #130

enobufs · 2020-07-06T02:13:22Z

Relates to pion/webrtc#1270

Description

Please see this comment(pion/webrtc#1270 (comment)) for details.

What's in this PR

Fixed the issue by sending COOKIE-ACK on the receipt of COOKIE-ECHO even in the Established state.
Added a reproducible test using vnet, successfully confirmed the problem, and the fix.

codecov · 2020-07-06T06:53:07Z

Codecov Report

Merging #130 into master will increase coverage by 0.10%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   78.71%   78.82%   +0.10%     
==========================================
  Files          43       43              
  Lines        2795     2800       +5     
==========================================
+ Hits         2200     2207       +7     
+ Misses        457      455       -2     
  Partials      138      138

Flag	Coverage Δ
#go	`78.82% <64.28%> (+0.10%)`	⬆️
#wasm	`69.96% <64.28%> (+0.23%)`	⬆️

Impacted Files	Coverage Δ
association.go	`82.63% <64.28%> (-0.11%)`	⬇️
chunkheader.go	`82.14% <0.00%> (+7.14%)`	⬆️
chunk_cookie_ack.go	`69.23% <0.00%> (+15.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd77f5a...ee0cd3c. Read the comment docs.

enobufs · 2020-07-14T20:04:15Z

@Sean-Der This is a fix for a fundamental error in handling delayed cookie-echo received in the "established" state (which would happen over lossy connection). If you could review this at your earliest convenience, that would really be appreciated.

association.go

Relates to pion/webrtc#1270

This is related to #62 in which we removed the use of immediate ack (I) bit. This test became unstable as we no longer use immediate ack. Relates to pion/webrtc#1270

at-wat · 2020-07-15T05:53:18Z

$ bash .github/lint-commit-message.sh
Update association.go

Accepted the change.

Co-authored-by: Atsushi Watanabe <atsushi.w@ieee.org>

-------------------------------------------------
The preceding commit message is invalid
it failed 'Separate subject from body with a blank line' of the following checks

* Separate subject from body with a blank line
* Limit the subject line to 50 characters
* Capitalize the subject line
* Do not end the subject line with a period
* Wrap the body at 72 characters

Commit message from GitHub web UI uses CR+LF which is currently not supported by our commit message lint script. pion/.goassets#6

git commit --amend and remove CR will fix the error.

Relates to pion/webrtc#1270

at-wat · 2020-07-15T07:13:10Z

association.go

 		return nil
-	}
+	case established:


Current implementation seems returning cookie-ack whenever cookie-echo is received after established.
https://tools.ietf.org/html/rfc4960#section-5.2.4 seems to specify several conditions/actions and cookie-echo should be silently discarded in some case.

@at-wat Yes, good finding. The current implementation does not follow the RFC for building a cookie. It is currently a random value that prevents us from validating the echoed cookie as specified in the RFC.

I am hoping to address this issue in #74 sometime soon, encoding TCB information into the cookie, then validate as the spec says. (no effect in WebRTC usage however, if we provide Listen/Dial semantics, yes it is desired.)

This pull-request tries to address missing state transitions from COOKIE-ECHOED to ESTABLISHED because the peer in the ESTABLISHED state does not send COOKIE-ACK, a different issue. We are seeing the connection stalled in the real world because of this.

I am hoping to address this issue in #74 sometime soon, encoding TCB information into the cookie, then validate as the spec says.

Great!
Maybe it's better to notify the current status of the implementation in README. If users try to use the library as a full sctp implementation, simplified implementation may cause some security problems. (as it's over DTLS in WebRTC, we don't have to care.)

Hey @at-wat You just made me realize we at least needed to check if the echoed cookie was identical to the original one. Thanks!

Let me add these lines in the "established" case.

if !bytes.Equal(a.myCookie.cookie, c.cookie) { return nil }

When the cookie does not match, it will silently be discarded.

@at-wat a quick question. Use used 'break' inside the switch statement. Golang does not require break but is that something you'd recommend for readability or any other reasons? (just out of my curiosity...)

I don't have strong reason, but I just feel it's more clear to tell that it's not fallthrough. (I mainly used C/C++ before starting Go)

@at-wat I have just added the cookie comparison to the established case.

I hear you. I write C/C++ too. ;) but I am trying to think in Go. I removed it in the last commit, hope you wouldn't mind...

It's 100% fine to me!

at-wat

LGTM!

Relates to pion/webrtc#1270

enobufs self-assigned this Jul 6, 2020

enobufs requested a review from Sean-Der July 6, 2020 07:32

enobufs force-pushed the fix-cookie-echo-handling branch from 0357fc7 to 892b525 Compare July 6, 2020 09:38

enobufs force-pushed the fix-cookie-echo-handling branch from 892b525 to 9119023 Compare July 13, 2020 17:08

at-wat reviewed Jul 15, 2020

View reviewed changes

association.go Outdated Show resolved Hide resolved

enobufs added 3 commits July 14, 2020 22:35

Fix handling of retransmitted cookie-echo

356ad03

Relates to pion/webrtc#1270

Fix test failing with timeout too short

3e6a389

Relates to pion/webrtc#1270

Remove a test that no longer works

b910e4a

This is related to #62 in which we removed the use of immediate ack (I) bit. This test became unstable as we no longer use immediate ack. Relates to pion/webrtc#1270

enobufs force-pushed the fix-cookie-echo-handling branch from ae7ec58 to ae15c0e Compare July 15, 2020 05:36

Switch to switch statement for readability

e9599dc

Relates to pion/webrtc#1270

enobufs force-pushed the fix-cookie-echo-handling branch from ae15c0e to e9599dc Compare July 15, 2020 06:46

at-wat reviewed Jul 15, 2020

View reviewed changes

at-wat approved these changes Jul 15, 2020

View reviewed changes

Validate cookie even in the established state

ee0cd3c

Relates to pion/webrtc#1270

enobufs merged commit 248c545 into master Jul 15, 2020

enobufs deleted the fix-cookie-echo-handling branch July 15, 2020 08:44

enobufs mentioned this pull request Jul 16, 2020

datachannel hangs with packet loss pion/webrtc#1270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of retransmitted cookie-echo #130

Fix handling of retransmitted cookie-echo #130

enobufs commented Jul 6, 2020

codecov bot commented Jul 6, 2020 •

edited

Loading

enobufs commented Jul 14, 2020 •

edited

Loading

at-wat commented Jul 15, 2020

at-wat Jul 15, 2020

enobufs Jul 15, 2020 •

edited

Loading

at-wat Jul 15, 2020

enobufs Jul 15, 2020

enobufs Jul 15, 2020

at-wat Jul 15, 2020

enobufs Jul 15, 2020

enobufs Jul 15, 2020

at-wat Jul 15, 2020

at-wat left a comment

Fix handling of retransmitted cookie-echo #130

Fix handling of retransmitted cookie-echo #130

Conversation

enobufs commented Jul 6, 2020

Description

What's in this PR

codecov bot commented Jul 6, 2020 • edited Loading

Codecov Report

enobufs commented Jul 14, 2020 • edited Loading

at-wat commented Jul 15, 2020

Choose a reason for hiding this comment

enobufs Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

at-wat left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2020 •

edited

Loading

enobufs commented Jul 14, 2020 •

edited

Loading

enobufs Jul 15, 2020 •

edited

Loading