search

pion / stun

A Go implementation of STUN
https://pion.ly/
MIT License
644 stars 95 forks source link

Rfc5780 #33

Closed cohosh closed 4 years ago

cohosh commented 4 years ago

Description

Adds support for RFC 5780. We'd really like this for Snowflake.

The first commit adds the actual support, the second commit adds a command that demonstrates how to use it. I'm not super concerned about getting the command added, just wanted to show an example of how it could work.

In particular, the command requires the flags for CHANGE-REQUEST be set manually. I could easily see adding a function that takes bools for these flags as arguments and constructs the attribute for you. But I figured I'd start with the bare minimum functionality we need.

Reference issue

Fixes #8

Sean-Der commented 4 years ago

Fantastic work @cohosh! I am 100% with merging this :)

The CLI tool is really cool, excited to try it out! I recently wanted something like this, excited I don't have to do any work :)

cohosh commented 4 years ago

Whoops sorry about the linting errors. I can patch these up :)

cohosh commented 4 years ago

Ooof alright. I'm not sure I'll be able to squash these last linting errors.

Sean-Der commented 4 years ago

@cohosh no worries! You can silence them, especially since they are examples maybe doesn't matter as much.

Sean-Der commented 4 years ago

Merged with 75c32927308cc44c4d10c6cc000ffd72679fb202 and 13a738c2102e887a6b290f7951e475048fd9bb4d

Thank you so much @cohosh! This is really fantastic, I want to share this wide! I think this could be a huge learning tool for people getting into WebRTC. Unfortunately I can't find any server that supports OtherAddress. I was able to find servers that support CHANGED_ADDRESS and SOURCE_ADDRESS and while these are deprecated I would be able to get this working!

What do you think of me migrating to those attributes? Or do you know a public server that supports OtherAddress

cohosh commented 4 years ago

Oh I found a few servers that support OTHER-ADDRESS. I started with the list here: https://gist.github.com/mondain/b0ec1cf5f60ae726202e#gistcomment-3238034 and wrote a script that tested them all and came up with ~12.

bixycler commented 4 years ago

I'm wondering why while the RFC5780 has been 10 years old, the support for it has just been added for weeks, and why the other 3 in 5 new attributes of RFC5780 are unsupported. Moreover, the MappedAddress should expose its methods addAs() and getAs() as AddToAs() and GetFromAs() so that the deprecated CHANGED-ADDRESS and SOURCE-ADDRESS (but still present in many servers) can be handled when needed by clients.

I'm forking the project to build a full-fledged NAT discoverer, and I'm wondering which way I should contribute back:

  1. Stick to the newest RFC(5780) with AddToAs() and GetFromAs() for optional handling of deprecated attributes.
  2. Support all RFC5780's attributes as well as deprecated ones like CHANGED-ADDRESS and SOURCE-ADDRESS with internal addAs() and getAs() only.

FYI, in 393 public STUN servers I've checked, there are only 145 ones supporting OTHER-ADDRESS & RESPONSE-ORIGIN from software coturn (RFC5780) while 248 ones response with CHANGED-ADDRESS, SOURCE-ADDRESS and XOR-MAPPED-ADDRESS from softwares Vovida.org, reTURNServer, etc. (RFC5389).

Sean-Der commented 4 years ago

Hey @bixycler

This library exists mostly to support https://github.com/pion/webrtc, so I only add things when needed! I would love your contributions :) The more we implement the better. Even if the code isn't being used, it is great for others to learn.

Add what you think is important! Since you are doing the work please just do what you want.

I am going to add you to the Pion repo so you can make a branch directly on this project. It is much easier to do development that way, and as long as the CI is green I would love to merge your changes :)

Also if you get a chance join Slack we have an active community and happy to talk anytime.

thanks!