Add PermissionHandler to ServerConfig #134 by rg0now · Pull Request #267 · pion/turn

rg0now · 2022-06-09T22:15:34Z

This is another take on #134

This is another attempt to address #134, see an earlier attempt in #222

#222 introduces the DeniedPeerRange stanza into the ServerConfig to implement peer address blacklisting. This approach has a couple of issues:

implements only peer blacklists, but does not allow whiletelisting or filtering based on DNS, etc.
handles only the ChannelBindRequest codepath, but leaves the CreatePermission codepath (https://datatracker.ietf.org/doc/html/rfc8656#section-3.4) open
introduces a new package dependency on "inet.af/netaddr"

This patch takes a different approach: it allows the user to specify a PermissionHandler callback for each PacketConnConfig/ListenerConfig in the ServerConfig. Whenever a permission is about to be created via the associated PacketConn/Listener (either via a ChannelBindRequest or a CreatePermission), the PermissionHandler is called with the requested peer address and it can decide whether to accommodate the permission request (return boolean true) or deny it (return false). In the latter case, a "permission request administratively prohibited" error is returned to the client.

Also added tests and an example.

yawn · 2022-07-05T07:44:34Z

This looks like a great contribution! Anything anyone outside of the PR can do to help move it along?

codecov · 2022-12-17T09:29:03Z

Codecov Report

Base: 68.05% // Head: 68.08% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (d7a0c2f) compared to base (371fc35).
Patch coverage: 52.63% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   68.05%   68.08%   +0.03%     
==========================================
  Files          38       38              
  Lines        2432     2469      +37     
==========================================
+ Hits         1655     1681      +26     
- Misses        643      657      +14     
+ Partials      134      131       -3

Flag	Coverage Δ
go	`68.08% <52.63%> (+0.03%)`	⬆️
wasm	`45.55% <0.00%> (-0.36%)`	⬇️

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

Impacted Files	Coverage Δ
internal/server/turn.go	`59.25% <40.00%> (+1.22%)`	⬆️
server.go	`59.05% <45.45%> (-0.78%)`	⬇️
internal/allocation/allocation_manager.go	`57.57% <70.00%> (+1.01%)`	⬆️
server_config.go	`33.33% <100.00%> (+3.92%)`	⬆️
client.go	`68.65% <0.00%> (-2.34%)`	⬇️
internal/client/conn.go	`61.86% <0.00%> (+1.01%)`	⬆️
internal/client/transaction.go	`85.10% <0.00%> (+5.31%)`	⬆️

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

I did a review of the PR. To me it looks good overall.

From my side, we could merge it :)

server_test.go

internal/server/turn.go

stv0g · 2023-01-17T12:43:29Z

Thanks @rg0now for addressing my comments.

To get this merged, we need to rebase it on the latest master and fix the commit messages as they cause the CI to fail.

Do you have the capacity to do it? Or do you need some support?

rg0now · 2023-01-17T14:18:14Z

Done (hopefully): rebased on pion/turn master and fixed the failing linter warnings. Could you please approve the CI workflow again to see if anything went wrong?

stv0g · 2023-01-17T16:22:07Z

Thanks @rg0now,

The CI is running again. But the commit messages are still not conformant to the Pion CI checks:

The preceding commit message is invalid
it failed 'Separate subject from body with a blank line' of the following checks

* Separate subject from body with a blank line
* Limit the subject line to 50 characters
* Capitalize the subject line
* Do not end the subject line with a period
* Wrap the body at 72 characters

rg0now · 2023-01-17T16:45:31Z

This really baffles me: the corresponding commit message is only a single line (Fix linter warnings). Any ideas? Happy to redo the commit if you know how to work around this warning. Plus we fail both tests, with another set of curious errors:

Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3/dist/codecov' failed with exit code 255

stv0g · 2023-01-17T17:02:39Z

I think the Codecov error is only transient.

the corresponding commit message is only a single line (Fix linter warnings). Any ideas?

I agree. I think the check is also too strict. Usually, most people just repeat the first line separated by an empty line.
But this might also be a sign, that the commits are too fine grained. Maybe we could combine all these commits in a single one with a more detailed description in its message?

stv0g · 2023-01-17T17:06:00Z

There is also another check which fails golangci-lint:

Error: cognitive complexity 33 of func NewServer is high (> 30) (gocognit)

rg0now · 2023-01-17T19:00:41Z

But this might also be a sign, that the commits are too fine grained. Maybe we could combine all these commits in a single one with a more detailed description in its message?

Ohh, I thought you were going to just squash-merge the whole thing in a single commit. I redid the last commit as per your advice. If this still doesn't work I can go back and re-edit the entire commit history in another branch but I'm afraid that will take another PR. Wdyt?

Error: cognitive complexity 33 of func NewServer is high (> 30) (gocognit)

I tend to agree with this: NewServer was already rather complex in the first place. We've added
just 6 new lines of new fairly trivial code to handle the permissionHandlers.

One way to go about this is to separate out the two loop bodies into a separate function each. Namely, the code that sets up PacketConnConfigs and calls the readLoop from

turn/server.go

Line 68 in 371fc35

go func(i int, p PacketConnConfig) {

to

turn/server.go

Line 85 in 371fc35

s.readLoop(p.PacketConn, allocationManager)

could go into a new function, say, runPacketConn, and the code that sets up the ListenerConfigs from

turn/server.go

Line 90 in 371fc35

go func(i int, l ListenerConfig) {

to

turn/server.go

Line 114 in 371fc35

go s.readLoop(NewSTUNConn(conn), allocationManager)

could be refactored into a function called runListener or similar. Would you be willing to take such a patch?

rg0now · 2023-01-17T19:02:42Z

On a related note: the fact that there is only a single readLoop per each net.PacketConn limits the max performance of UDP TURN listeners quite substantially, since there is a single thread that handles all clients connecting via the listener (see l7mp/stunner#60). This is in contrast to net.Conns, which spawn per-client-connection threads. Would you consider a somewhat intrusive patch to remove this bottleneck?

stv0g · 2023-01-17T20:13:57Z

I agree with both of your comments. However, lets try to get the PermissionHandler merged first.

Could you add //nolint:gocognit comment above the NewServer() function to silence the linter?

Also please squash the commits into a single one.

Afterwards, we can address the two issues which you raised.

Another attempt to address pion#134, see an earlier attempt in pion#222. pion#222 introduces the DeniedPeerRange stanza into the ServerConfig to implement peer address blacklisting. This approach has a couple of issues: (1) it implements only peer blacklists, but does not allow whiletelisting or filtering based on DNS, etc.;(2) it handles only the ChannelBindRequest codepath, but leaves the CreatePermission codepath (https://datatracker.ietf.org/doc/html/rfc8656#section-3.4) open; and it introduces a new package dependency on "inet.af/netaddr". This patch takes a different approach: it allows the user to specify a PermissionHandler callback for each PacketConnConfig/ListenerConfig in the ServerConfig. Whenever a permission is about to be created via the associated PacketConn/Listener (either via a ChannelBindRequest or a CreatePermission), the PermissionHandler is called with the requested peer address and it can decide whether to accommodate the permission request (return boolean true) or deny it (return false). In the latter case, a "permission request administratively prohibited" error is returned to the client. Also added tests and an example.

rg0now · 2023-01-18T14:48:28Z

Could you add //nolint:gocognit comment above the NewServer() function to silence the linter?

Done. Fixed the linter warning on my side.

Also please squash the commits into a single one.

Done.

Afterwards, we can address the two issues which you raised.

Sure, Looking forward to hearing your opinion.

stv0g · 2023-01-18T19:09:01Z

Thanks a lot @rg0now 🥳 I merged your changes and created two new issues for the problems which you identified.
I am happy to review any PRs closing these issues :)

rg0now force-pushed the feature-permission-handler branch from 3d64e9b to 2cf8902 Compare July 6, 2022 08:47

rg0now force-pushed the feature-permission-handler branch from 2cf8902 to be50641 Compare December 1, 2022 16:44

stv0g approved these changes Jan 12, 2023

View reviewed changes

server_test.go Outdated Show resolved Hide resolved

internal/server/turn.go Show resolved Hide resolved

rg0now force-pushed the feature-permission-handler branch from c5dca04 to 6b9edbe Compare January 17, 2023 14:14

rg0now force-pushed the feature-permission-handler branch from 6b9edbe to 85adfa2 Compare January 17, 2023 18:38

rg0now force-pushed the feature-permission-handler branch from 85adfa2 to f35ce60 Compare January 18, 2023 14:41

rg0now force-pushed the feature-permission-handler branch from f35ce60 to d7a0c2f Compare January 18, 2023 14:44

stv0g mentioned this pull request Jan 18, 2023

Simplify NewServer() #286

Closed

stv0g merged commit 68e752b into pion:master Jan 18, 2023

stv0g mentioned this pull request Jan 18, 2023

Multi-threaded readLoop() for UDP TURN Listeners #287

Closed

stv0g mentioned this pull request Jan 18, 2023

Add PermissionHandler to ServerConfig #134

Closed

rg0now deleted the feature-permission-handler branch February 22, 2023 16:07

Conversation

rg0now commented Jun 9, 2022

This is another take on #134

This is another attempt to address #134, see an earlier attempt in #222

Uh oh!

yawn commented Jul 5, 2022

Uh oh!

codecov bot commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stv0g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stv0g commented Jan 17, 2023

Uh oh!

rg0now commented Jan 17, 2023

Uh oh!

stv0g commented Jan 17, 2023

Uh oh!

rg0now commented Jan 17, 2023

Uh oh!

stv0g commented Jan 17, 2023

Uh oh!

stv0g commented Jan 17, 2023

Uh oh!

rg0now commented Jan 17, 2023

Uh oh!

rg0now commented Jan 17, 2023

Uh oh!

stv0g commented Jan 17, 2023

Uh oh!

rg0now commented Jan 18, 2023

Uh oh!

stv0g commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 17, 2022 •

edited

Loading