Add callback for TURN authentication success

[renandincer]

Description

Creates a new callback for users to be notified by successful TURN authentication. Together with the authHandler function, this can help determine the ratio of successful vs unsuccessful authentication attemps per username. This information then can be used to implement methods to prevent attacks to guess credentials similar to fail2ban.

There's been some discussion around implementing various usage quotas and limits within this package. An auth success callback can be used to implement some of these given that a succesful source IP address can be reliably linked to usernames. Together with maximum allocation time limit and permission timeouts per RFC, allocation refesh requests (requires auth) and configurable channel bind timeouts, fivetuple -> username usage can be reliably tracked outside of this package.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.46%. Comparing base (b44d85a) to head (2404127).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #402      +/-   ##
==========================================
+ Coverage   68.15%   68.46%   +0.30%     
==========================================
  Files          43       43              
  Lines        2352     2356       +4     
==========================================
+ Hits         1603     1613      +10     
+ Misses        582      579       -3     
+ Partials      167      164       -3     

Flag	Coverage Δ
go	68.46% <100.00%> (+0.30%)	⬆️
wasm	27.84% <0.00%> (-0.05%)	⬇️

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.

[renandincer]

(forced pushed to resolve the linter issue)

[rg0now]

There have been lots of former attempts to add callback support to the authentication handler, see #2, #324 and the unmerged PR #325. At this point my minimal requirement for merging a new callback interface would be to handle user quoatas as per point (10) in the RFC (this means we need a callback for notifying the caller both on successful allocation creation events as well as when existing allocations are being deleted), and possibly redirects. Care to update the PR to handle at least the first case (i.e., add a callback for allocation deletions) and provide a sample code for implementing user quotas on top of your callbacks?

Unfortunately, there has been no progress on this front so maybe we should merge this as is. However, this PR involves a user-visible API change so we need a conformation from @Sean-Der (he needs to roll a new release github.com/pion/turn/v4 at the very least).

[Sean-Der]

Great call out @rg0now it would be great if full life cycle could be tracked!

@rg0now since this isn't a breaking change feel free to merge/change however you see fit! I also am in support of you starting a /v4 at anytime you see fit. You have done an exceptional job (better then me!) with maintaining pion/turn :)

Tell me what I can do to support you. I think you are a better advocate/understand what 'next-gen TURN' looks like.

[rg0now]

Dear @renandincer, would you be willing to work together towards merging this?

Some issues we'd need to discuss:

• You mention that the intention of the PR is to ensure that "fivetuple -> username usage can be reliably tracked". Can we handle this with the AuthHandler callback? I mean, the application provides the AuthHandler so they may also provide the fivetuple-tracking functionality as well in the AuthHandler, no?
• Currently the AuthSuccess callback is called every time (1) an allocation is created or refreshed, or a permission or a channel is created. Is this the intended semantics?
• Shouldn't we rather put the callback site into the CreateAllocation function of the allocation.Manager? This may be perhaps semantically correct, in turn we lose the username and perhaps the realm at this point (but it may not be difficult to store these in turn.Request and pass in the full turn.Request into AllocationManager.CreateAllocation instead of only r.Conn). In other words, would you be better off with a AllocationCreated callback rather than an AuthSuccess callback?
• Are you interested in handling user quotas? Would you be willing to add a AllocationDeleted callback too?

p.s.: care to do a quick call sometime? interested in your use case here...

[renandincer]

Hey @rg0now, yes I reviewed those PRs, and wanted to keep the scope of this as small as possible due to the discussion there - keeping the ALTERNATE-DOMAIN style use cases out of it.

You mention that the intention of the PR is to ensure that "fivetuple -> username usage can be reliably tracked". Can we handle this with the AuthHandler callback? I mean, the application provides the AuthHandler so they may also provide the fivetuple-tracking functionality as well in the AuthHandler, no?

AuthHandler will be called regardless of authentication success. This prevents a reliable mapping between fivetuple to usernames so that's why a callback only for success is desirable.

Currently the AuthSuccess callback is called every time (1) an allocation is created or refreshed, or a permission or a channel is created. Is this the intended semantics?

This is the intended semantics. It keeps 1-1 mapping with AuthHandler, so it's simple and doesn't cause any breaking changes while achieving the two goals: (1) create a way to detect failed vs successful requests per srcAddr, and (2) create a way map fiveTuple to username.

Shouldn't we rather put the callback site into the CreateAllocation function of the allocation.Manager? This may be perhaps semantically correct, in turn we lose the username and perhaps the realm at this point (but it may not be difficult to store these in turn.Request and pass in the full turn.Request into AllocationManager.CreateAllocation instead of only r.Conn). In other words, would you be better off with a AllocationCreated callback rather then an AuthSuccess callback?

I do fully agree with you. I think what you described is a superior way of doing this. Callback for AllocationCreated and AllocationDeleted along with username and fivetuple in the function signature like so:

AllocationCreated(username string, fiveTuple FiveTuple)
AllocationDeleted(username string, fiveTuple FiveTuple)

Somewhat related, getting the AllocatePacketConn function signature to accept username and fiveTuple will (or a way to access the net.Conn assigned by AllocatePacketConn when allocation is created) is also desirable to be able to implement rate limiting style functions on a per-username basis).

Are you interested in handling user quotas? Would you be willing to add a AllocationDeleted callback too?

RFC does not have a explicit way of deleting a allocation or permission so timeouts are enough for keeping track of usage accurately along with this auth success callback function.

I'm willing to discuss other implementations, contributing AllocationDeleted/AllocatedCreated/etc.

Yep. Let's get on a call and discuss :)

[rg0now]

wanted to keep the scope of this as small as possible due to the discussion there - keeping the ALTERNATE-DOMAIN style use cases out of it.

Full ack.

AuthHandler will be called regardless of authentication success. This prevents a reliable mapping between fivetuple to usernames so that's why a callback only for success is desirable.
This is the intended semantics. It keeps 1-1 mapping with AuthHandler, so it's simple and doesn't cause any breaking changes while achieving the two goals: (1) create a way to detect failed vs successful requests per srcAddr, and (2) create a way map fiveTuple to username.

I see. It seems worth supporting this use case.

Somewhat related, getting the AllocatePacketConn function signature to accept username and fiveTuple will (or a way to access the net.Conn assigned by AllocatePacketConn when allocation is created) is also desirable to be able to implement rate limiting style functions on a per-username basis).

I think the way user quotas could be supported is through the AuthHandler: when a user has filled their quota we start rejecting new allocations from the AuthHandler where we have enough context to do so. But this workflow is somewhat unintuitive, that's why we should provide an example for supporting user quotas in the server. Wdyt?

RFC does not have a explicit way of deleting a allocation or permission so timeouts are enough for keeping track of usage accurately along with this auth success callback function.

I'm not sure I get this. We (the user) need to be notified when an allocation goes away, otherwise we cannot keep track of user quotas.

I'm willing to discuss other implementations, contributing AllocationDeleted/AllocatedCreated/etc.

Here is a final API I have in mind:

type EventContext struct {
	Username, Realm  string   // add to allocation.Allocation
	Network          string   // from allocation.FiveTuple
	SrcAddr, DstAddr net.Addr // from allocation.FiveTuple
}

type EventHandlers struct {
	AuthSuccess       func(context EventContext)                       // called from server.authenticateRequest
	AuthFailure       func(context EventContext)                       // called from server.authenticateRequest
	AllocationCreated func(context EventContext, requestedPort int)    // called from allocation.CreateAllocation
	AllocationRemoved func(context EventContext)                       // called from allocation.DeleteAllocation
	PermissionCreated func(context EventContext, ip net.IP)            // called from allocation.AddPermission
	PermissionRemoved func(context EventContext, ip net.IP)            // called from allocation.RemovePermission
	ChannelCreated    func(context EventContext, channelNumber uint16) // called from alloction.AddChannelBind (this also calles PermissionCreated)
	ChannelRemoved    func(context EventContext, channelNumber uint16) // called from alloction.RemoveChannelBind (this also calles PermissionRemoved)
}

This is deliberately not an interface so people will not have to implement all event handlers, just the ones they are interested in. Of course we don't have to implement all this: the first two callbacks will do for now (this will already handle your use case) and then we can work together to gradually implement the full API. Wdyt?

Let's chat! Wrote an email!...

[rg0now]

Somewhat related, getting the AllocatePacketConn function signature to accept username and fiveTuple will (or a way to access the net.Conn assigned by AllocatePacketConn when allocation is created) is also desirable to be able to implement rate limiting style functions on a per-username basis).

OK, now I think I get it: the goal is not admission control, it's rate limitation. That will indeed need user context in AllocatePacketConn or anywhere inside the created net.Conn.

Problem is: any way to solve this seems to require a breaking change. My suggestion would be to get the lifecycle event handlers straight and once we're happy with it only then break the API.

[renandincer]

We (the user) need to be notified when an allocation goes away, otherwise we cannot keep track of user quotas.

With the knowledge of:

• Successful authentication
• Username
• IP:Port tuple for the source
• Various timeouts in TURN

you can keep track of a map between username and src ip:port tuple, which can drive user quotas.

The authSuccess function is called for every:

• Allocate Request (maximum allocation time 1 hour per RFC recommendation)
• Allocation Refresh Request - any extension to 1 hour RFC)
• Channel Bind Request (configurable timeout - 10 minutes by default)
• Create Permission Request (permission timeout 5 minutes per RFC requirement)

Since one can't send or receive packets without channel bind or permission or authenticated packet, you can assume ip:port tuple to username mapping to be 10 minutes by default in this situation.

Obviously this is a unintuitive way of implementing this.

[renandincer]

I think the way user quotas could be supported is through the AuthHandler: when a user has filled their quota we start rejecting new allocations from the AuthHandler where we have enough context to do so. But this workflow is somewhat unintuitive, that's why we should provide an example for supporting user quotas in the server. Wdyt?

I think example is a good idea in general whatever we implement. :)

Here is a final API I have in mind:

I like this API, but I would like to sleep on it a little. I am not sure if ChannelCreated/ChannelRemoved callbacks are very useful. Do you think there is a good use for them?

[renandincer]

Problem is: any way to solve this seems to require a breaking change. My suggestion would be to get the lifecycle event handlers straight and once we're happy with it only then break the API.

One option could be to provide an API to get a pointer to the net.Conn given username (or something from the allocation)

[rg0now]

I like this API, but I would like to sleep on it a little. I am not sure if ChannelCreated/ChannelRemoved callbacks are very useful. Do you think there is a good use for them?

One use case that I have in mind is making eBPF acceleration less intrusive (see #360). The current PR hooks directly into AddChannelBind and DeleteChannelBind to create/remove the eBPF offload mappings for the channel. Should we have the lifecycle event handlers, we could implement the eBPF acceleration completely outside the base pion/turn package without having to distribute the eBPF binary object files with the source code (something nobody seems to like for understandable reasons).

[renandincer]

To close the loop here for those watching, we had a very good call.

I agree that a interface similar to what @rg0now suggested is the way forward. This way, in the future, extra information from the allocation can be exported with relative ease without having to make breaking changes to the API.

I will close this PR and follow up with a implementation in coming months unless somebody else takes on before then :)

[rg0now]

Hi @renandincer any progress on this? I plan to take a stride at an implementation but I don't want to do duplicate work...