fix: Remove ICEServer proxying from client to server #400
Conversation
kylecarbs
force-pushed
the
iceproxy
branch
2 times, most recently
from
1ff282d
to
34ca05d
Compare
Codecov Report
@@ Coverage Diff @@
## main #400 +/- ##
==========================================
+ Coverage 66.10% 67.60% +1.49%
==========================================
Files 71 150 +79
Lines 773 8446 +7673
Branches 72 72
==========================================
+ Hits 511 5710 +5199
- Misses 248 2158 +1910
- Partials 14 578 +564
Continue to review full report at Codecov.
|
| iceServers, err := b.listener.iceServersFunc(stream.Context()) | ||
| if err != nil { | ||
| return xerrors.Errorf("get ice servers: %w", err) | ||
| } | ||
| // Start with no ICE servers. They can be sent by the client if provided. |
There was a problem hiding this comment.
Seems like this comment might need to be updated now - if I understand correctly, the client won't send ICE servers anymore.
| err := stream.Send(&proto.NegotiateConnection_ClientToServer{ | ||
| Message: &proto.NegotiateConnection_ClientToServer_Servers{ | ||
| Servers: &proto.WebRTCICEServers{ | ||
| Servers: protoIceServers, | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
It wasn't clear to me where the client is sending these up now?
EDIT: I should've read the PR description & title more closely, that explains why - it's insecure
There was a problem hiding this comment.
Was curious because it looks like the ice connection is failing in the tests now:
t.go:56: 2022-03-04 18:18:57.801 [DEBUG] (server) <conn.go:243> sending local candidate {"candidate": "candidate:3781755249 1 udp 1694498815 23.98.128.67 1986 typ srflx raddr 0.0.0.0 rport 49007"}
t.go:56: 2022-03-04 18:18:57.809 [DEBUG] (client) <conn.go:357> accepting candidate {"candidate": "candidate:3781755249 1 udp 1694498815 23.98.128.67 1986 typ srflx raddr 0.0.0.0 rport 49007"}
t.go:56: 2022-03-04 18:19:27.905 [DEBUG] (server) <conn.go:155> ice connection state updated {"state": "failed"}
https://github.com/coder/coder/runs/5426432416?check_suite_focus=true#step:7:130
And I suspect it must be related to this change 🤔
| listener, err := peerbroker.Listen(server, func(ctx context.Context) ([]webrtc.ICEServer, error) { | ||
| return []webrtc.ICEServer{{ | ||
| URLs: []string{"stun:stun.l.google.com:19302"}, | ||
| }}, nil |
There was a problem hiding this comment.
If I switch the ICEServersFunc discovery function to nil here - the tests seem to pass: https://github.com/coder/coder/runs/5426432416?check_suite_focus=true#step:7:130
I guess it indicates there may be a bug with how we're interacting with a 'real' STUN server?
There was a problem hiding this comment.
(or just removing the settingEngine.SetHostAcceptanceMinWait(time.Hour) line seems to make it pass too)
I had trouble finding docs on what the parameter was controlling - I found the code here https://github.com/pion/webrtc/blob/1765e9b913535f5e6aeacf91e7d4b75d1dbcdc9f/settingengine.go#L107 which sets https://github.com/pion/webrtc/blob/157220e800257ee4090f181e7edcca6435adb9f2/icegatherer.go#L102 - but wasn't clear to me how this enforces calling through to the STUN server.
This made testing simple, but enabled insecure behavior. This allows the listener to fetch ICEServers from a remote location, which will likely be coderd.
This made testing simple, but enabled insecure behavior. This allows
the listener to fetch ICEServers from a remote location, which will
likely be coderd.