lt_cred support for TURN REST format #370

giavac · 2024-02-04T17:05:26Z

Description

This PR adds support for ephemeral credentials, as initially described in https://datatracker.ietf.org/doc/html/draft-uberti-behave-turn-rest-00 and widely used by https://github.com/coturn/coturn

giavac · 2024-02-05T15:27:32Z

I see the failed checks, I'll amend.

giavac · 2024-02-06T08:55:42Z

@rg0now I've pushed changes to satisfy some checks. I'm not sure all checks will pass - is https://github.com/pion/webrtc/wiki/Contributing up to date? I don't see all the referenced lint scripts in .goassets.

Please let me know if there's an up to date set of checks I can do locally. Thanks.

codecov · 2024-02-06T09:19:32Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (fb0ab51) 66.78% compared to head (7b2a1d4) 66.68%.

Files	Patch %	Lines
lt_cred.go	57.14%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   66.78%   66.68%   -0.10%     
==========================================
  Files          43       43              
  Lines        2860     2888      +28     
==========================================
+ Hits         1910     1926      +16     
- Misses        787      796       +9     
- Partials      163      166       +3

Flag	Coverage Δ
go	`66.68% <57.14%> (-0.10%)`	⬇️
wasm	`38.67% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rg0now

Just a minor final push and it's in!..:-)

examples/turn-server/lt-cred-turn-rest/main.go

lt_cred.go

rg0now · 2024-02-06T09:21:41Z

examples/turn-server/lt-cred-turn-rest/main.go

+		Realm: *realm,
+		// Set AuthHandler callback
+		// This is called everytime a user tries to authenticate with the TURN server
+		// Return the key for that user, or false when no user is found


Please remove useless comment. Also adding a short explanation might help people trying to adopt the feature.

rg0now · 2024-02-06T09:24:36Z

examples/turn-server/lt-cred-turn-rest/main.go

@@ -0,0 +1,72 @@
+// Package main implements a TURN server using


Please add copyright stanza as per the failing REUSE test. See examples/turn-server/lt-cred/main.go for an example.

rg0now · 2024-02-06T09:27:22Z

examples/turn-server/lt-cred-turn-rest/main.go

@@ -0,0 +1,72 @@
+// Package main implements a TURN server using
+// ephemeral credentials.


Please add a short description of what's going on here to examples/README.md. No need to go into details, just the basic usage like here. This can go right after the existing lt-creds description.

rg0now · 2024-02-06T09:34:02Z

go.mod

@@ -1,6 +1,6 @@
 module github.com/pion/turn/v3

-go 1.13
+go 1.19


I'm not sure we are allowed to upgrade the required Go version here. That would be a major break and may require a major release (not sure about this). Currently we are at 1.13 and my understanding is that there's nothing in your code that absolutely requires a higher version so I'd rather not bump the Go version here.

ps: not that I full understand the min Go version requirements across the entire pion project (some code like DTLS is still at 1.13 whole some like mediadevices require 1.19)...

rg0now · 2024-02-06T09:54:47Z

lt_cred.go

@@ -57,3 +67,31 @@ func NewLongTermAuthHandler(sharedSecret string, l logging.LeveledLogger) AuthHa
 		return GenerateAuthKey(username, realm, password), true
 	}
 }
+
+// LongTermTURNRESTAuthHandler returns a turn.AuthAuthHandler used with Long Term (or Time Windowed) Credentials.


Can you please add one line description of the main purpose of the REST API? I'm thinking of something along the following lines:

// LongTermTURNRESTAuthHandler returns a turn.AuthAuthHandler that can be used to authenticate // time-windowed TURN credentials generated by the TURN REST API described in // https://datatracker.ietf.org/doc/html/draft-uberti-behave-turn-rest-00 // // The supported format of is timestamp:username, where username is an arbitrary user id and the // timestamp specifies the expiry of the credential.

If you are at it: can you please also fix the URL in the doc of NewLongTermAuthHandler: the orignal URL https://tools.ietf.org/search/rfc5389#section-10.2 does not seem to work anymore for some reason, plus RFC 5389 is now obsoleted by RFC 8489, so we should use this URL instead: https://datatracker.ietf.org/doc/html/rfc8489#section-9.2.

rg0now

LGTM

rg0now · 2024-02-08T09:23:31Z

Dear @giavac, would you be so kind to collapse your commits into a single one? (Just reset --soft <branch-root> and then commit everything in one and force-push). It seems we will have to manually merge due to the inconclusive tests (I'm trying to ask around at Slack, still no info) and I don't want the squash-merge to overwrite authorship info in your commits.

giavac · 2024-02-08T10:43:40Z

Thanks @rg0now - it should be OK now?

giavac · 2024-02-09T11:45:12Z

@rg0now maybe I could try to increase the code coverage, if that's what's holding the last check?

rg0now · 2024-02-09T11:59:53Z

Unfortunately that won't fix this (codecov checks are not "required") but it would definitely help. The problem is the pending results for the Go 1.21 tests. Since I don't seem to receive a definite answer at Slack, I think we'll just merge manually anyway.

rg0now · 2024-02-09T14:34:09Z

Thanks a lot @giavac, merged

giavac · 2024-02-12T08:57:26Z

Thanks for the review and guidance @rg0now

giavac mentioned this pull request Feb 4, 2024

lt_cred support for TURN REST format #252

Closed

rg0now self-assigned this Feb 5, 2024

giavac force-pushed the master branch 2 times, most recently from 13764ec to 64db7d0 Compare February 6, 2024 08:46

rg0now requested changes Feb 6, 2024

View reviewed changes

rg0now approved these changes Feb 6, 2024

View reviewed changes

Add TURN REST format support

7b2a1d4

giavac force-pushed the master branch 2 times, most recently from 4bb736c to 7b2a1d4 Compare February 8, 2024 10:42

rg0now merged commit a9681a0 into pion:master Feb 9, 2024
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lt_cred support for TURN REST format #370

lt_cred support for TURN REST format #370

giavac commented Feb 4, 2024

giavac commented Feb 5, 2024

giavac commented Feb 6, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

rg0now left a comment

rg0now Feb 6, 2024

rg0now Feb 6, 2024

rg0now Feb 6, 2024

rg0now Feb 6, 2024 •

edited

Loading

rg0now Feb 6, 2024

rg0now left a comment

rg0now commented Feb 8, 2024

giavac commented Feb 8, 2024

giavac commented Feb 9, 2024

rg0now commented Feb 9, 2024

rg0now commented Feb 9, 2024

giavac commented Feb 12, 2024

		@@ -0,0 +1,72 @@
		// Package main implements a TURN server using

		@@ -0,0 +1,72 @@
		// Package main implements a TURN server using
		// ephemeral credentials.

lt_cred support for TURN REST format #370

lt_cred support for TURN REST format #370

Conversation

giavac commented Feb 4, 2024

Description

giavac commented Feb 5, 2024

giavac commented Feb 6, 2024

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

rg0now left a comment

Choose a reason for hiding this comment

rg0now Feb 6, 2024

Choose a reason for hiding this comment

rg0now Feb 6, 2024

Choose a reason for hiding this comment

rg0now Feb 6, 2024

Choose a reason for hiding this comment

rg0now Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

rg0now Feb 6, 2024

Choose a reason for hiding this comment

rg0now left a comment

Choose a reason for hiding this comment

rg0now commented Feb 8, 2024

giavac commented Feb 8, 2024

giavac commented Feb 9, 2024

rg0now commented Feb 9, 2024

rg0now commented Feb 9, 2024

giavac commented Feb 12, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

rg0now Feb 6, 2024 •

edited

Loading