Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lt_cred support for TURN REST format #252

Closed
wants to merge 0 commits into from
Closed

lt_cred support for TURN REST format #252

wants to merge 0 commits into from

Conversation

giavac
Copy link
Contributor

@giavac giavac commented Apr 8, 2022

Description

This adds support for the format timestamp:username used with the TURN REST API (https://tools.ietf.org/search/rfc5389#section-10.2).

@stv0g stv0g self-requested a review November 12, 2022 15:22
Copy link
Member

@stv0g stv0g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @giavac!

This is actually really useful for me as well :)
Thanks for the contribution!

@codecov
Copy link

codecov bot commented Nov 12, 2022

Codecov Report

Base: 68.05% // Head: 68.04% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (cd0ffb7) compared to base (775b003).
Patch coverage: 57.14% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
- Coverage   68.05%   68.04%   -0.01%     
==========================================
  Files          38       38              
  Lines        2432     2460      +28     
==========================================
+ Hits         1655     1674      +19     
- Misses        643      650       +7     
- Partials      134      136       +2     
Flag Coverage Δ
go 68.04% <57.14%> (-0.01%) ⬇️
wasm 45.91% <ø> (ø)

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

Impacted Files Coverage Δ
lt_cred.go 56.45% <57.14%> (+0.56%) ⬆️
client.go 71.76% <0.00%> (+0.77%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stv0g stv0g closed this Nov 12, 2022
@guusdk
Copy link

guusdk commented Dec 18, 2023

Hi! We're experiencing a challenge with Pion using REST API compatible (ephemeral) authentication. This may well be in our implementation, but just to make sure we're not chasing ghosts: Has the change mentioned in this PR made it into any release to date? The commit references are a bit tricky to follow through the Github UI.

@rg0now
Copy link
Contributor

rg0now commented Dec 18, 2023

Judging from the offending code in master, the default long-term auth handler currently does not seem to parse the timestamp:username pair. But I'm also somewhat lost here, as per the commit refs.

@giavac
Copy link
Contributor Author

giavac commented Dec 19, 2023

My changes were removed with the force-push mentioned in this PR. I'm not sure what happened, I might be missing something from the github flow.
I had a GenerateLonTermTURNRESTCredentials() and LongTermTURNRESTAuthHandler() functions to handle the long term credentials support, with an example.
If this is still needed I can try and create a new PR?
Otherwise you can retrieve the missing code from the diff of the forced push.

@guusdk
Copy link

guusdk commented Dec 19, 2023

The functionality is still desirable. As I'm not familiar with the code base (or even the programming language), I'd prefer you (instead of me) recreating the commit.

@rg0now
Copy link
Contributor

rg0now commented Dec 19, 2023

I agree: this is still a useful feature to have. We have implemented this functionality in STUNner, with the twist that we consider the first thing that parses as a valid unsigned integer as a UNIX timestamp, so we accept both timestamp:username and username:timestamp, and also a plain timestamp (without a user name) or even x:y:z:timestamp as well. This is completely upward compatible with the handler generated by the default NewLongTermAuthHandler, so you don't even need to introduce a separate LongTermTURNRESTAuthHandler.

Please submit as a new PR, I'd be happy to review.

@giavac
Copy link
Contributor Author

giavac commented Dec 20, 2023

Sounds good; it's been more than a year not using this code base so I'll need a little more time than just recreating the PR (also based on @rg0now 's information).

@giavac
Copy link
Contributor Author

giavac commented Feb 4, 2024

I've raised #370 - CI checks are pending but for the people interested in this feature please take a look and review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants