Update API for SIP. Allow setting outbound number.

[dennwc]

• Add update API for SIP Trunks and Dispatch Rules.
• Allow setting custom outbound number in CreateSIPParticipant.
• Relax Inbound Trunk validation: allow setting zero numbers for wildcard Trunk.
• Fix typo in SIPUri: port should be an integer.

[changeset-bot]

🦋 Changeset detected

Latest commit: d6cfe3c

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR

Some errors occurred when validating the changesets config:
The package or glob expression "github.com/livekit/protocol" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

[biglittlebigben]

Can you comment on the motivation to allow wildcard trunks?

[biglittlebigben]

This change is moving to a different API pattern to what we've used in other APIs (outside SIP at least). Can you describe the motivation?

[dennwc]

If you mean creation of objects by providing the full proto - the main motivation is simplicity.

Using separate fields in Create*Request and the actual object requires updating the code to pass these fields from one struct to the other with no additional benefit.

At least in SIP, all objects are config-like and are saved as-is. There's no additional transformation done on them, thus I think there's no point in having two separate messages.

[biglittlebigben]

The motivation for separate Create*Request messages has been that some of the fields in the *Info objects are usually not user settable (IDs, status, state, etc). As the Info objects become larger, it can become non trivial to understand which field can be set and which can't.

I'm not necessarily against this change, but it seems like we are facing our customers and ourselves through an API change to a version that also has drawbacks. Do we have a good reason to do it? At the very least, I'd feel more conformable with this change if we can convince ourselves that we'll never add "state" related fields to the info (ie, "created_d/updated_at/dispatch_list/participant_list")

[dennwc]

OK, I understand now why you are concerned about this change.

Unfortunately, it's a bit too late to fix it - inbound and outbound trunk APIs were already implemented this way. They assume all fields are static. So this change was only to make dispatch rules consistent with newer trunk API (and more maintainable).

I think we can guarantee that this API version will never contain state, timestamps or any other dynamic fields.

Instead, I'd propose to define a new API that properly separates dynamic and static fields into separate sub-messages, when needed. I have a draft of it already locally, but it's only an experiment for now.

So to make it specific, I'll add a comment here stating that only static fields are allowed in this message.

[davidzhao]

I think requiring all of the fields will also make it difficult to only update a specific attribute.. For example, if I'm only looking to update the numbers field, it seems weird to have to include all of the other attributes as well.

I think it's too late to change the Create* APIs, which also doesn't suffer from this issue, we should decide what we want to do for Update before moving forward

[dennwc]

@davidzhao please see discussion in livekit/livekit#3141 (comment)

[dennwc]

@biglittlebigben Re: wildcard trunks, initially they were allowed in OSS, but disallowed in Cloud. But actually, Cloud doesn't need to know about inbound numbers upfront either. So they will now work consistently in both LK versions.

[biglittlebigben]

@biglittlebigben Re: wildcard trunks, initially they were allowed in OSS, but disallowed in Cloud. But actually, Cloud doesn't need to know about inbound numbers upfront either. So they will now work consistently in both LK versions.

Do we have a mechanism anywhere (in cloud) that prevents creating trunks that will allow any call in from anywhere?

[dennwc]

The only mechanism as this Validate method. Now we started using it in CLI and it prevents creating inbound trunks for OSS as well.

But otherwise, Cloud relies on SIP URI to tell which project the call belongs to. It doesn't need the number for a successful call dispatch.

[biglittlebigben]

The only mechanism as this Validate method. Now we started using it in CLI and it prevents creating inbound trunks for OSS as well.

But otherwise, Cloud relies on SIP URI to tell which project the call belongs to. It doesn't need the number for a successful call dispatch.

If I understand the code above properly, Validate on the trunk makes sure the headers are valid, and that the rule is not nil on DispatchRuleInfo.

Do we have a way to prevent customers to get surprise bills, or spam calls because they created an inbound trunk with no access control at all?

[dennwc]

First, for the context, we are talking about more sophisticated spam. Regular spam bots do not know the expected format for the LK SIP URI, so they will be filtered out immediately. The ones that remain have knowledge about LK's URI format. And I assume they know the correct project ID.

With that knowledge, number filtering won't do much - an attacker can fake it as well. So you are right, in the long term, we may want to require either CIDR filter or authentication on the trunks. But that would need to be a different protocol change with a proper notice.

[biglittlebigben]

We have heard of users who think got caught by such an abuse scenario. My experience with doing a lot of spam bot/infra abuse mitigation is that as soon as a bot maker notices you exist, they will cater the requests to the weaknesses of your access control.

One can argue that this Validate function is not the right place to do such validation, and we should have cloud specific, stricter enforcement, as OSS users may have a good reason for wildcard trunks/dispatches on private networks. I however don't think we can ship a relaxation of these rules on cloud.

[dennwc]

OK, then I propose the following: require that either of 3 fields is set: numbers, auth_* or CIDR. This is a step toward better trunk security.

We can then send a notice that going forward, we will require one of two fields: auth_* or CIDR. Number filter doesn't give meaningful protection.

[biglittlebigben]

OK, then I propose the following: require that either of 3 fields is set: numbers, auth_* or CIDR. This is a step toward better trunk security.

We can then send a notice that going forward, we will require one of two fields: auth_* or CIDR. Number filter doesn't give meaningful protection.

Sure! Folks who really want a wildcard trunk can use a 0.0.0.0/0 CIDR, but at least they opted into the trouble they're getting themselves into.

[dennwc]

Done, added trunk validation as discussed.

[biglittlebigben]

This is fine with me, but please check with @davidzhao about his last comment.

[dennwc]

Added a separate oneof message for per-field update, plus the helpers to apply these updates. PTAL.

[biglittlebigben]

This PR still has both add/remove/set semantics, both at the top level (oneof for the payload type) and at the individual fields add_/remove_/set_ fields. So far, the pattern has been to only have add_/remove_ semantics for updates. The set semantics can be convenient, but I'm not sure they warrant the extra API complexity. Curious about what other folks think.

[davidzhao]

hmm I think this will make the API more complicated.. nested seems harder to use to me.

[dennwc]

You mean the dispatch rule nesting? It mirrors new trunk API in that it reuses the SIPDispatchRuleInfo message. It's easier to manage that way. CLI will hide the nesting.

[davidzhao]

yeah, I'm talking about the nesting. I think CLI could hide it, just thinking through for SDK integrations. while this is more consistent, the SDK update cycle is a bit painful. Your call though on the path forward.

[dennwc]

This still needs a bit more work on the Cloud side. I split parts unrelated to update API into #890 and #891 to not block them.