turn
turn copied to clipboard
Add a callback to handle allocation requests
First take at #324.
The idea is to make it possible to customize the behavior of the TURN server in order to admit/deny allocation requests (e.g., to implement user quotas) or redirect clients to an alternate server.
type AllocationHandler func(clientAddr net.Addr) (alternateServer net.Addr, errorCode stun.ErrorCode)
Currently the allocation handler is per-listener (i.e., per PacketConnConfig
and per ListenerConfig
) and it is called just before the server would create a new allocation.
The PR contains a minimalistic implementation plus a test to get a feeling of the new API.
Some observations:
- client API: we will need to find a way to return the alternate server IP from
client.Allocate()
, should the server respond with a "Try Alternate" error (maybe the returned error could have aStatusCode
and aParameter
of typeany
to return meaningful errors?) -- an alternative would be to automatically follow redirects in the client until we get to a usable server, but I guess this behavior should be made configurable, - handle the
ALTERNATE-DOMAIN
attribute: the current API allows to return an alternate server address, but no alternate domain (see here). - user quotas: the current API is not enough to implement user quotas, in that we can already prevent over the quota allocations from being made but, given that we are only called on allocation creation but not at deletion, we cannot know when an allocation goes away, effectively resulting in blocking all access for the user indefinitely.
My takeaway is that most probably we'll need two distinct APIs to handle user quotas and redirects:
- the allocation handler will be fine for redirects (but maybe we can change the name to
RedirectHandler
, plus we may also need to handle the alternate domain case) - user quotas need to track the lifecycle of allocations and that will require a new callback, say,
QuotaHandler
:
settingtype QuotaHandler func(clientAddr net.Addr, username string, new bool) (bool, error)
new=true
would mean it's a new allocation (otherwise we're deleting one), and returning (false
,nil
) would mean to deny access for the new allocation.
Codecov Report
Patch coverage: 67.44
% and project coverage change: +0.27
:tada:
Comparison is base (
f880e55
) 67.91% compared to head (70c67df
) 68.19%.
Additional details and impacted files
@@ Coverage Diff @@
## master #325 +/- ##
==========================================
+ Coverage 67.91% 68.19% +0.27%
==========================================
Files 39 39
Lines 2475 2509 +34
==========================================
+ Hits 1681 1711 +30
- Misses 662 666 +4
Partials 132 132
Flag | Coverage Δ | |
---|---|---|
go | 68.19% <67.44%> (+0.27%) |
:arrow_up: |
wasm | 44.09% <0.00%> (-0.71%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
server_config.go | 29.41% <ø> (-3.93%) |
:arrow_down: |
internal/server/turn.go | 59.58% <64.70%> (+0.32%) |
:arrow_up: |
internal/allocation/allocation_manager.go | 60.78% <66.66%> (+3.20%) |
:arrow_up: |
server.go | 57.25% <80.00%> (-0.68%) |
:arrow_down: |
... and 1 file with indirect coverage changes
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 in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Thanks @rg0now for taken a first shot at this :-)
There is not much I can add. I fully agree with your observations 👍
Regarding the redirect handler: couldnt we have two return values, for alternate-server and alternate-domain?
Sure, it's perfectly doable. Any thoughts on the client side API? How should we return the alt-server and alt-domain from client.Allocate
? Or should we just merge this in ASAP and find out client-side API later?
I'll update the redirect handler to return the alt-domain too ASAP.
Hi @rg0now,
Any thoughts on the client side API? Or should we just merge this in ASAP and find out client-side API later?
I think, I would merge the server side handlers first and care about the client side changes via another PR.
How should we return the alt-server and alt-domain from
client.Allocate
?
I think this would break the API which I would like to avoid. I think we could easily handle this by returning a custom error type:
type TryAlternateError struct {
AlternateServer net.Addr
AlternateDomain string
}
func main() {
...
conn, err := c.Allocate()
if err != nil {
var altErr *turn.TryAlternateError
if errors.As(err, &altErr) {
connectAlternate(altErr.AlternateServer, altErr.AlternateDomain)
}
}
}
On a second thought:
I think we should probably also simplify the signature of the allocation handler to:
type AllocationHandler func(clientAddr net.Addr) error
Then we can use the same TryAlternateError
from my previous comment.
I would also prefer to keep handlers closely related to the associated TURN methods.
E.g. I think a AllocateHandler
can cover more possible use cases than a RedirectHandler
or QuotaHandler
.
However, you have a valid point regarding the release of allocated resources..
Following my line, we might also need to add an AllocationExpiredHandler
which the user would need to hook for releasing resources. Same could be done for a PermissionsExpiredHandler
.
In my projects, I usually try to use interfaces for the implementation of handlers: But this is a larger, API-breaking change, which I am not sure we really need to do now:
type AllocationHandler interface {
Allocate(clientAddr net.Addr, username string) error
AllocationExpired(clientAddr net.Addr, username string) error
}
type PermissionHandler interface {
CreatePermission(clientAddr net.Addr, peerIP net.IP) error
PermissionExpired(clientAddr net.Addr, peerIP net.IP) error
}
type AuthenticationHandler interface {
Authenticate(username, realm string, srcAddr net.Addr) (key []byte, err error)
}
type ServerConfig struct {
Handler any // this handler can implement any of the previous interfaces
}
The advantage of tying the handlers to a struct would be, that the user can easily associate state to the handlers (like a quota database). Or implement handlers which implement subset of the aforementioned interfaces.
This makes a lot of sense. In Kubernetes we use API versions to handle breaking API changes: we release a new version (say, v1alpha2 on top of v1alpha1), an automatic converter that upgrades an API call from the old version to the new one, and then we deprecate the old API and stop supporting it after 2-3 releases. I really miss API versioning from Go (or any other programming language for that matter): it would make such API upgrades much simpler.
Anyway, here's the current plan, let me know if you're fine with it:
- add a new handler to
PacketConnConfig
andListenerConfig
types with the signature:type AllocationHandler func(clientAddr net.Addr) error
- insert a call to the allocation handler into
internal/server/turn.go:handleAllocateRequest
- add an example that demonstrates a TURN server that returns a redirect error for allocation requests
- worry about the client-side interface later.
As per the last point, I'm happy to implement the TryAlternateError
API too, let me know if I should do it now or put it into another PR.
Hi @rg0now,
I am missing just one thing: the AllocateHandler
s signature is not very feature proof. We cant handle additional attributes if we want to use the handler for other purposes (e.g. quota handling).
I am thinking about attributes like:
-
USERNAME
-
REQUESTED-TRANSPORT
- See: #311
- and: #315
-
REQUESTED-ADDRESS-FAMILY
-
ADDITIONAL-ADDRESS-FAMILY
- ...
What do you think about simply passing the stun.Message
as a second parameter?
I'm a bit reluctant to make users having to deal with super-low-level APIs like stun.Message
but I don't have a better idea right now. What if we create a struct with all the data the caller would ever need to make a decision (like the username, transport, etc.) and pass that to the callback?
As per the quota handler, we still have the problem of allowing the caller to track the lifetime of allocations from outside the server (the caller needs to know when allocations are going away). We could call a CreateAllocation
handler from server.handleAllocateRequest
to signal to the user that we are about to create an allocation (and this is perfectly fine because we have the stun.Message
and all the required info available at this point), but we would also need to call a matching DeleteAllocation
handler or similar from internal/allocation/allocation_manager.go:DeleteAllocation
where we do not have any stun.Message
available (say, because we are being called from a timeout), only the allocation itself (that is not quite a public API) and the FiveTuple
(which sort of is).
This would an API like the following (I'm not insisting on the names at all):
type AllocationHandler interface {
// CreateAllocation is called before the server making a new allocation. Return an error to prevent the allocation from being made.
CreateAllocation(clientAddr net.Addr, stun.Message) error
// AllocationDeleted is called by the server after deleting an allocation. This is useful to track the lifetime of allocations.
AllocationDeleted(fiveTuple *allocation.FiveTuple)
}
This lets only one option to the caller to implement user quotas: track all allocations per user by the 5-tuple. Would this be enough?
This lets only one option to the caller to implement user quotas: track all allocations per user by the 5-tuple. Would this be enough?
Yes, this is the same idea which I had in mind with the AllocationExpired
handler. But your proposed AllocationDeleted
handler makes more sense, as an allocation can also explicitly deleted.
The problem I see here is that we do not know the five-tuple at the time when CreateAllocation
handler is called.
Hence, the quota handler can not correlate the quota allocation (CreateAllocation
) to the release (AllocationDeleted
).
What if we create a struct with all the data the caller would ever need to make a decision (like the username, transport, etc.) and pass that to the callback?
I had the same idea. I think this would work as well as we can extend the structure in the future 👍
The problem I see here is that we do not know the five-tuple at the time when
CreateAllocation
handler is called. Hence, the quota handler can not correlate the quota allocation (CreateAllocation
) to the release (AllocationDeleted
).
Hmm, I really want to get this right so let's see what is that I don't understand.
Let's pass a struct like this to the CreateAllocation
handler:
type AllocationDescriptor struct {
Username stun.Username
FiveTuple allocation.FiveTuple
RequestedTransportProtocol allocation.Protocol
RequestedAddressFamily proto.RequestedAddressFamily
AdditionalAddressFamily proto.RequestedAddressFamily
}
The allocation handler interface would look like this:
type AllocationHandler interface {
// CreateAllocation is called before the server making a new allocation. Return an error to prevent the allocation from being made.
CreateAllocation(desc AllocationDescriptor) error
// AllocationDeleted is called by the server after deleting an allocation. This is useful to track the lifetime of allocations.
AllocationDeleted(fiveTuple *allocation.FiveTuple)
}
We would call the CreateAllocation
handler in internal/server/turn.go:handleAllocateRequest
. The structure of this longish function would look like this:
func handleAllocateRequest(r Request, m *stun.Message) error {
// integrity check
// create client-side 5-tuple
fiveTuple := &allocation.FiveTuple{
SrcAddr: r.SrcAddr,
DstAddr: r.Conn.LocalAddr(),
Protocol: allocation.UDP,
}
// check if allocation already exists
if alloc := r.AllocationManager.GetAllocation(fiveTuple); alloc != nil { ... }
// handle RequestedTrasnport, AttrDontFragment, ReservationToken and EvenPort
// call the allocation handler's CreateAllocation callback
desc := alloc.NewAllocationDescriptor()
err := r.AllocationManager.allocationHandler.CreateAllocation(desc)
if err != nil {
// return err to client
}
// create the allocation...
}
Another option here would be to call the CreateAllocation
handler from internal/allocation/allocation_manager.go:CreateAllocation
, this seems more "self-contained" but at this point we lost our stun.Message
so we have to pass the necessary AllocationDescriptor
fields (username, etc.) as arguments.
Finally, internal/allocation/allocation_manager.go:DeleteAllocation
would look like the below
func (m *Manager) DeleteAllocation(fiveTuple *FiveTuple) {
m.allocationHandler.AllocationDeleted(fiveTuple)
// delete and close allocation
}
Wdyt?
Hey @rg0now,
thanks for the detailed description :-) I think I got this wrong. I was under the impression that the five-tuple contains:
- Client IP+Port
- Relayed IP+Port
- Protocol
However, you seem to be correctly using the clients connection local address (from server perspective) instead of the relayed address.
I still need to understand why this works. This way, it seems like the client can not create multiple allocations using the same connection to the server as these would all use the same five-tuple?
I still need to understand why this works. This way, it seems like the client can not create multiple allocations using the same connection to the server as these would all use the same five-tuple?
Yes, this strikes me as odd too, but it seems that each turn.Client
can support only a single allocation (the client actually caches the relayedConn
). This seems like an assumption we don't quite document. If you want more allocations, create more Client
s, each with a separate net.PacketConn
and, hence, with a separate FiveTuple
.
The useful consequence of this assumption is that we can use the FiveTuple
to identify allocations, like in internal/allocation/allocation_manager.go:GetAllocation
:
func (m *Manager) GetAllocation(fiveTuple *FiveTuple) *Allocation {
m.lock.RLock()
defer m.lock.RUnlock()
return m.allocations[fiveTuple.Fingerprint()]
}
Maybe the reason of why you're finding this confusing now is that you're deep into the implementation of RFC6062 TCP allocations and the client model is so much different for TCP.
Okay, I've read the TURN RFC (again) 😁
It clearly says that the five-tuple is identifying the client-server connection and that a client may only have a single or dual allocation active (dual as in dual-stack when requesting an allocation with relayed addresses for multiple address familiies e.g. IPv4 + IPv6).
See: https://datatracker.ietf.org/doc/html/rfc8656#name-allocations-2
This now also resolves some of my confusion in regard to the TCP allocation PRs which we are currently working on 😃
So now, I can fully support your proposed implementation 👍
Just with one remark:
I would try to keep the handlers out of the AllocationManager
and just to Server
since I think it would be cleaner if the AllocationManager
does not need to care about the handlers. I would try to keep them just to the Server
types.
I still try to understand why somebody thought it was a good idea to have both a public Server
and internal Server
type -.- I believe, this makes a lot of stuff more difficult to understand. But thats a different topic :D