turn icon indicating copy to clipboard operation
turn copied to clipboard

RFC6062 implementation

Open seankhliao opened this issue 4 years ago • 4 comments

I've read #118 and the associated branch rfc-6062-client and decided starting from scratch would be a better for me.

Currently on branch rfc6062 (sorry for confusing naming) is a partially done modification of the client. Creating outbound requests Connect works as well as well as associating data connections ConnectionBind. Tested manually with coturn.

Changes

internal/proto:

  • add ProtoTCP

turn:

  • add ProtoTCP and ProtoUDP
  • add ConnectionID type to main package
  • add ClientConfig.TransportProtocol
  • add Client.transportProtocol
  • add Client.nonce
  • add Client.Connect()
  • add Client.ConnectionBind()

Questions

  • what kind of tests to add?
  • successful authentication method should probably be stored (need to be reused)?
  • Client.Allocate returns a net.PacketConn, not sure how that fits into TCP?
  • the server will send ConnectionAttempt indications to the client for receiving connections, how should this be exposed to users? (also needs CreatePermission)
  • handling refreshes

example client

package main

import (
	"fmt"
	"log"
	"net"

	"github.com/pion/turn/v2"
)

func main() {
	// Dial TURN Server
	turnServerAddr := "1.2.3.4:3478"
	conn, err := net.Dial("tcp", turnServerAddr)
	if err != nil {
		panic(err)
	}

	// Start a new TURN Client and wrap our net.Conn in a STUNConn
	// This allows us to simulate datagram based communication over a net.Conn
	cfg := &turn.ClientConfig{
		STUNServerAddr: turnServerAddr,
		TURNServerAddr: turnServerAddr,
		Conn:           turn.NewSTUNConn(conn),
		Username:       "user",
		Password:       "pass",
		TransportProtocol:       turn.ProtoTCP,
	}

	client, err := turn.NewClient(cfg)
	if err != nil {
		panic(err)
	}
	defer client.Close()

	// Start listening on the conn provided.
	err = client.Listen()
	if err != nil {
		panic(err)
	}

       // not sure what to do with relayConn
	relayConn, err := client.Allocate()
	if err != nil {
		panic(err)
	}
	defer func() {
		if closeErr := relayConn.Close(); closeErr != nil {
			panic(closeErr)
		}
	}()
	log.Printf("relayed-address=%s", relayConn.LocalAddr().String())

        // peer address to connect to
	ta, _ := net.ResolveTCPAddr("tcp4", "145.100.104.117:8888")

        // attempt connection, get connection id
	cid, err := client.Connect(ta)
	if err != nil {
		panic(err)
	}

        // open new data connection to turn server
	dconn, err := net.Dial("tcp", turnServerAddr)
	if err != nil {
		panic(err)
	}

        // associate connection with connection id
	err = client.ConnectionBind(dconn, cid)
	if err != nil {
		panic(err)
	}

       // use connection
	_, err = dconn.Write([]byte("hello world"))
	if err != nil {
		panic(err)
	}

	buf := make([]byte, 1024)
	n, err := dconn.Read(buf)
	if err != nil {
		panic(err)
	}
	fmt.Println(string(buf[:n]))
}

seankhliao avatar Jun 12 '20 11:06 seankhliao

Fantastic work @seankhliao!

what kind of tests to add?

I don't have any hard rules, but if you can get ~80% coverage that would be great! I usually like doing E2E tests, and then more fine grained tests for Marshal/Unmarshal.

successful authentication method should probably be stored (need to be reused)?

Yea I noticed we do this wrong. I would cut an issue and solve in another PR. If your PR is really big it is hard for me to merge :( I have a lot of people get annoyed, but I need to read all the code because I might end up bug fixing/maintaining it.

Client.Allocate returns a net.PacketConn, not sure how that fits into TCP?

Maybe an AllocateTCP? Then you can return net.Conn, I don't feel strongly any way. You are doing the work so want you to be proud of the work you are doing. I am happy to give advice, but I have full faith in w/e you choose.

the server will send ConnectionAttempt indications to the client for receiving connections, how should this be exposed to users? (also needs CreatePermission)

handling refreshes

I think this can both be kept internal. The existing UDP client does this so it should be ok!

Sean-Der avatar Jun 13 '20 08:06 Sean-Der

I think the ConnectionAttempt thing needs to be exposed to the user in some way, in UDP everything comes in on a single socket, but in TCP you need to open/dial a new connection for every incoming one. Maybe have AllocateTCP take a handler as a argument?

seankhliao avatar Jun 13 '20 08:06 seankhliao

A update:

The current branch has both a server / client that should work now (tested manually with coturn / coturn_uclient). Problems:

  • reusing the same port for incoming/outgoing conns with TCP requires some extra syscalls, not sure how portable it is (it need golang.org/x/sys/unix
  • related to the previous point, haven't wrapped my head around vnet
  • api changes:
    • the incoming connection handler is now a config option, not sure how ergonomic it is for users
    • maybe ConnectionID can be in pion/stun so there doesn't have to be import cycles/duplicate declarations
  • no tests

seankhliao avatar Jul 03 '20 14:07 seankhliao

crap sorry I missed this @seankhliao! Holy crap that is amazing progress!

  • I am not sure about portability issues, but it would probably be a blocker :( I will have to see what you are doing to be sure
  • vnet is really only for UDP right now, so you can ignore!
  • API changes are fine with me! We can just do a /v3 and do w/e you need. I really want to merge your work.

Really sorry I was slow on this. If you open a PR we should start merging this.

Sean-Der avatar Jul 25 '20 06:07 Sean-Der

I am closing this issue in favour of #118 and the two new PRs #311 & #315

stv0g avatar Apr 01 '23 11:04 stv0g