Add PermissionHandler to ServerConfig #134

[rg0now]

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]

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

[codecov[bot]]

Codecov Report

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

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%)	:arrow_up:
wasm	45.55% <0.00%> (-0.36%)	:arrow_down:

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%)	:arrow_up:
server.go	59.05% <45.45%> (-0.78%)	:arrow_down:
internal/allocation/allocation_manager.go	57.57% <70.00%> (+1.01%)	:arrow_up:
server_config.go	33.33% <100.00%> (+3.92%)	:arrow_up:
client.go	68.65% <0.00%> (-2.34%)	:arrow_down:
internal/client/conn.go	61.86% <0.00%> (+1.01%)	:arrow_up:
internal/client/transaction.go	85.10% <0.00%> (+5.31%)	:arrow_up:

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

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

[stv0g]

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]

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]

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]

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]

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]

There is also another check which fails golangci-lint:

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

[rg0now]

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 https://github.com/pion/turn/blob/371fc35851d91a885a0621b295f8e9a3ed6af2d2/server.go#L68 to https://github.com/pion/turn/blob/371fc35851d91a885a0621b295f8e9a3ed6af2d2/server.go#L85 could go into a new function, say, runPacketConn, and the code that sets up the ListenerConfigs from https://github.com/pion/turn/blob/371fc35851d91a885a0621b295f8e9a3ed6af2d2/server.go#L90 to https://github.com/pion/turn/blob/371fc35851d91a885a0621b295f8e9a3ed6af2d2/server.go#L114 could be refactored into a function called runListener or similar. Would you be willing to take such a patch?

[rg0now]

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 https://github.com/l7mp/stunner/issues/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]

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.

[rg0now]

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]

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 :)