fix: Remove ICEServer proxying from client to server (#400)

This made testing simple, but enabled insecure behavior. This allows
the listener to fetch ICEServers from a remote location, which will
likely be coderd.

agent/agent_test.go

@@ -95,7 +95,7 @@ func TestAgent(t *testing.T) {
 func setup(t *testing.T) proto.DRPCPeerBrokerClient {
 	client, server := provisionersdk.TransportPipe()
 	closer := agent.New(func(ctx context.Context) (*peerbroker.Listener, error) {
-		return peerbroker.Listen(server, &peer.ConnOptions{
+		return peerbroker.Listen(server, nil, &peer.ConnOptions{
 			Logger: slogtest.Make(t, nil),
 		})
 	}, &agent.Options{

go.mod

@@ -37,6 +37,7 @@ require (
 	github.com/hashicorp/go-version v1.4.0
 	github.com/hashicorp/terraform-config-inspect v0.0.0-20211115214459-90acf1ca460f
 	github.com/hashicorp/terraform-exec v0.15.0
+	github.com/hashicorp/terraform-json v0.13.0
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.10.1
 	github.com/hashicorp/yamux v0.0.0-20211028200310-0bc27b27de87
 	github.com/justinas/nosurf v1.1.1
@@ -109,7 +110,6 @@ require (
 	github.com/hashicorp/hcl v1.0.0 // indirect
 	github.com/hashicorp/hcl/v2 v2.11.1 // indirect
 	github.com/hashicorp/logutils v1.0.0 // indirect
-	github.com/hashicorp/terraform-json v0.13.0 // indirect
 	github.com/hashicorp/terraform-plugin-go v0.5.0 // indirect
 	github.com/hashicorp/terraform-plugin-log v0.2.0 // indirect
 	github.com/hashicorp/terraform-registry-address v0.0.0-20210412075316-9b2996cce896 // indirect

peerbroker/dial.go

@@ -12,36 +12,6 @@ import (
 
 // Dial consumes the PeerBroker gRPC connection negotiation stream to produce a WebRTC peered connection.
 func Dial(stream proto.DRPCPeerBroker_NegotiateConnectionClient, iceServers []webrtc.ICEServer, opts *peer.ConnOptions) (*peer.Conn, error) {
-	// Convert WebRTC ICE servers to the protobuf type.
-	protoIceServers := make([]*proto.WebRTCICEServer, 0, len(iceServers))
-	for _, iceServer := range iceServers {
-		var credentialString string
-		if value, ok := iceServer.Credential.(string); ok {
-			credentialString = value
-		}
-		protoIceServers = append(protoIceServers, &proto.WebRTCICEServer{
-			Urls:           iceServer.URLs,
-			Username:       iceServer.Username,
-			Credential:     credentialString,
-			CredentialType: int32(iceServer.CredentialType),
-		})
-	}
-	if len(protoIceServers) > 0 {
-		// Send ICE servers to connect with.
-		// Client sends ICE servers so clients can determine the node
-		// servers will meet at. eg. us-west1.coder.com could be a TURN server.
-		err := stream.Send(&proto.NegotiateConnection_ClientToServer{
-			Message: &proto.NegotiateConnection_ClientToServer_Servers{
-				Servers: &proto.WebRTCICEServers{
-					Servers: protoIceServers,
-				},
-			},
-		})
-		if err != nil {
-			return nil, xerrors.Errorf("write ice servers: %w", err)
-		}
-	}
-
 	peerConn, err := peer.Client(iceServers, opts)
 	if err != nil {
 		return nil, xerrors.Errorf("create peer connection: %w", err)
@@ -54,9 +24,9 @@ func Dial(stream proto.DRPCPeerBroker_NegotiateConnectionClient, iceServers []we
 			case <-peerConn.Closed():
 				return
 			case sessionDescription := <-peerConn.LocalSessionDescription():
-				err = stream.Send(&proto.NegotiateConnection_ClientToServer{
-					Message: &proto.NegotiateConnection_ClientToServer_Offer{
-						Offer: &proto.WebRTCSessionDescription{
+				err = stream.Send(&proto.Exchange{
+					Message: &proto.Exchange_Sdp{
+						Sdp: &proto.WebRTCSessionDescription{
 							SdpType: int32(sessionDescription.Type),
 							Sdp:     sessionDescription.SDP,
 						},
@@ -67,8 +37,8 @@ func Dial(stream proto.DRPCPeerBroker_NegotiateConnectionClient, iceServers []we
 					return
 				}
 			case iceCandidate := <-peerConn.LocalCandidate():
-				err = stream.Send(&proto.NegotiateConnection_ClientToServer{
-					Message: &proto.NegotiateConnection_ClientToServer_IceCandidate{
+				err = stream.Send(&proto.Exchange{
+					Message: &proto.Exchange_IceCandidate{
 						IceCandidate: iceCandidate.Candidate,
 					},
 				})
@@ -89,10 +59,10 @@ func Dial(stream proto.DRPCPeerBroker_NegotiateConnectionClient, iceServers []we
 			}
 
 			switch {
-			case serverToClientMessage.GetAnswer() != nil:
+			case serverToClientMessage.GetSdp() != nil:
 				peerConn.SetRemoteSessionDescription(webrtc.SessionDescription{
-					Type: webrtc.SDPType(serverToClientMessage.GetAnswer().SdpType),
-					SDP:  serverToClientMessage.GetAnswer().Sdp,
+					Type: webrtc.SDPType(serverToClientMessage.GetSdp().SdpType),
+					SDP:  serverToClientMessage.GetSdp().Sdp,
 				})
 			case serverToClientMessage.GetIceCandidate() != "":
 				peerConn.AddRemoteCandidate(webrtc.ICECandidateInit{

peerbroker/dial_test.go

@@ -31,18 +31,26 @@ func TestDial(t *testing.T) {
 		defer client.Close()
 		defer server.Close()
 
-		listener, err := peerbroker.Listen(server, &peer.ConnOptions{
-			Logger: slogtest.Make(t, nil).Named("server").Leveled(slog.LevelDebug),
+		settingEngine := webrtc.SettingEngine{}
+		listener, err := peerbroker.Listen(server, func(ctx context.Context) ([]webrtc.ICEServer, error) {
+			return []webrtc.ICEServer{{
+				URLs: []string{"stun:stun.l.google.com:19302"},
+			}}, nil
+		}, &peer.ConnOptions{
+			Logger:        slogtest.Make(t, nil).Named("server").Leveled(slog.LevelDebug),
+			SettingEngine: settingEngine,
 		})
 		require.NoError(t, err)
 
 		api := proto.NewDRPCPeerBrokerClient(provisionersdk.Conn(client))
 		stream, err := api.NegotiateConnection(ctx)
 		require.NoError(t, err)
+
 		clientConn, err := peerbroker.Dial(stream, []webrtc.ICEServer{{
 			URLs: []string{"stun:stun.l.google.com:19302"},
 		}}, &peer.ConnOptions{
-			Logger: slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug),
+			Logger:        slogtest.Make(t, nil).Named("client").Leveled(slog.LevelDebug),
+			SettingEngine: settingEngine,
 		})
 		require.NoError(t, err)
 		defer clientConn.Close()

peerbroker/listen.go

@@ -17,12 +17,21 @@ import (
 	"github.com/coder/coder/peerbroker/proto"
 )
 
+// ICEServersFunc returns ICEServers when a new connection is requested.
+type ICEServersFunc func(ctx context.Context) ([]webrtc.ICEServer, error)
+
 // Listen consumes the transport as the server-side of the PeerBroker dRPC service.
 // The Accept function must be serviced, or new connections will hang.
-func Listen(connListener net.Listener, opts *peer.ConnOptions) (*Listener, error) {
+func Listen(connListener net.Listener, iceServersFunc ICEServersFunc, opts *peer.ConnOptions) (*Listener, error) {
+	if iceServersFunc == nil {
+		iceServersFunc = func(ctx context.Context) ([]webrtc.ICEServer, error) {
+			return []webrtc.ICEServer{}, nil
+		}
+	}
 	ctx, cancelFunc := context.WithCancel(context.Background())
 	listener := &Listener{
 		connectionChannel: make(chan *peer.Conn),
+		iceServersFunc:    iceServersFunc,
 
 		closeFunc: cancelFunc,
 		closed:    make(chan struct{}),
@@ -48,6 +57,7 @@ func Listen(connListener net.Listener, opts *peer.ConnOptions) (*Listener, error
 
 type Listener struct {
 	connectionChannel chan *peer.Conn
+	iceServersFunc    ICEServersFunc
 
 	closeFunc  context.CancelFunc
 	closed     chan struct{}
@@ -104,8 +114,12 @@ type peerBrokerService struct {
 
 // NegotiateConnection negotiates a WebRTC connection.
 func (b *peerBrokerService) NegotiateConnection(stream proto.DRPCPeerBroker_NegotiateConnectionStream) error {
+	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.
-	peerConn, err := peer.Server([]webrtc.ICEServer{}, b.connOptions)
+	peerConn, err := peer.Server(iceServers, b.connOptions)
 	if err != nil {
 		return xerrors.Errorf("create peer connection: %w", err)
 	}
@@ -121,9 +135,9 @@ func (b *peerBrokerService) NegotiateConnection(stream proto.DRPCPeerBroker_Nego
 			case <-peerConn.Closed():
 				return
 			case sessionDescription := <-peerConn.LocalSessionDescription():
-				err = stream.Send(&proto.NegotiateConnection_ServerToClient{
-					Message: &proto.NegotiateConnection_ServerToClient_Answer{
-						Answer: &proto.WebRTCSessionDescription{
+				err = stream.Send(&proto.Exchange{
+					Message: &proto.Exchange_Sdp{
+						Sdp: &proto.WebRTCSessionDescription{
 							SdpType: int32(sessionDescription.Type),
 							Sdp:     sessionDescription.SDP,
 						},
@@ -134,8 +148,8 @@ func (b *peerBrokerService) NegotiateConnection(stream proto.DRPCPeerBroker_Nego
 					return
 				}
 			case iceCandidate := <-peerConn.LocalCandidate():
-				err = stream.Send(&proto.NegotiateConnection_ServerToClient{
-					Message: &proto.NegotiateConnection_ServerToClient_IceCandidate{
+				err = stream.Send(&proto.Exchange{
+					Message: &proto.Exchange_IceCandidate{
 						IceCandidate: iceCandidate.Candidate,
 					},
 				})
@@ -156,28 +170,11 @@ func (b *peerBrokerService) NegotiateConnection(stream proto.DRPCPeerBroker_Nego
 		}
 
 		switch {
-		case clientToServerMessage.GetOffer() != nil:
+		case clientToServerMessage.GetSdp() != nil:
 			peerConn.SetRemoteSessionDescription(webrtc.SessionDescription{
-				Type: webrtc.SDPType(clientToServerMessage.GetOffer().SdpType),
-				SDP:  clientToServerMessage.GetOffer().Sdp,
+				Type: webrtc.SDPType(clientToServerMessage.GetSdp().SdpType),
+				SDP:  clientToServerMessage.GetSdp().Sdp,
 			})
-		case clientToServerMessage.GetServers() != nil:
-			// Convert protobuf ICE servers to the WebRTC type.
-			iceServers := make([]webrtc.ICEServer, 0, len(clientToServerMessage.GetServers().Servers))
-			for _, iceServer := range clientToServerMessage.GetServers().Servers {
-				iceServers = append(iceServers, webrtc.ICEServer{
-					URLs:           iceServer.Urls,
-					Username:       iceServer.Username,
-					Credential:     iceServer.Credential,
-					CredentialType: webrtc.ICECredentialType(iceServer.CredentialType),
-				})
-			}
-			err = peerConn.SetConfiguration(webrtc.Configuration{
-				ICEServers: iceServers,
-			})
-			if err != nil {
-				return peerConn.CloseWithError(xerrors.Errorf("set ice configuration: %w", err))
-			}
 		case clientToServerMessage.GetIceCandidate() != "":
 			peerConn.AddRemoteCandidate(webrtc.ICECandidateInit{
 				Candidate: clientToServerMessage.GetIceCandidate(),

peerbroker/listen_test.go

@@ -23,7 +23,7 @@ func TestListen(t *testing.T) {
 		defer client.Close()
 		defer server.Close()
 
-		listener, err := peerbroker.Listen(server, nil)
+		listener, err := peerbroker.Listen(server, nil, nil)
 		require.NoError(t, err)
 
 		api := proto.NewDRPCPeerBrokerClient(provisionersdk.Conn(client))
@@ -43,7 +43,7 @@ func TestListen(t *testing.T) {
 		defer client.Close()
 		defer server.Close()
 
-		listener, err := peerbroker.Listen(server, nil)
+		listener, err := peerbroker.Listen(server, nil, nil)
 		require.NoError(t, err)
 		go listener.Close()
 		_, err = listener.Accept()