Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[uac_registrant] Match reply Contact with binding_URI by parameter. #2558

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[uac_registrant] Match reply Contact with binding_URI by parameter. #2558

wants to merge 1 commit into from

Conversation

vasilevalex
Copy link
Contributor

Added new integer module parameter 'match_contact' to choose, what
parts of URI from Contact header should match binding_URI when
OpenSIPS chooses expiration time for registration.

Summary
Solving #2557

Details
It simplifies using uac_registrant module in cluster environment, when there is one DB per cluster, and each host uses its' own IP-address for SIP.

Solution
For example, setting modparam("uac_registrant", "match_contact", 3) allows matching only by user, ignoring host. As binding_URI contains specific host, and every server sends REGISTER with its' own IP-address.

Compatibility
Default behaviour is not changed.

Closing issues
closes #2557

@vasilevalex vasilevalex force-pushed the reg_match_uri branch 2 times, most recently from 4e80c74 to 229eb20 Compare June 24, 2021 12:08
@stale
Copy link

stale bot commented Jul 21, 2021

Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days.

@stale stale bot added stale and removed stale labels Jul 21, 2021
@github-actions
Copy link

Any updates here? No progress has been made in the last 30 days, marking as stale.

@github-actions github-actions bot added the stale label Aug 21, 2021
@stale stale bot removed the stale label Aug 23, 2021
@github-actions
Copy link

Any updates here? No progress has been made in the last 30 days, marking as stale.

@github-actions github-actions bot added the stale label Sep 29, 2021
@stale stale bot removed the stale label Sep 29, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days.

@stale stale bot added stale and removed stale labels Jan 9, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

Any updates here? No progress has been made in the last 15 days, marking as stale. Will close this issue if no further updates are made in the next 30 days.

@stale stale bot added the stale label Apr 16, 2022
@bogdan-iancu bogdan-iancu self-assigned this Apr 18, 2022
@stale stale bot removed the stale label Apr 18, 2022
@stale stale bot removed the stale label Apr 18, 2022
@liviuchircu liviuchircu self-requested a review May 12, 2023 12:55
liviuchircu
liviuchircu requested changes May 12, 2023
View reviewed changes
Copy link
Member

@liviuchircu liviuchircu left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea behind this PR, as it brings a lot more flexibility to uac_registrant when trying to achieve interoperability with various UAS and their reply Contacts.

Still, I don't like that we are encouraging opensips.cfg code which uses these hardcore Contact matching algorithm bitmasks such as 3, 11, 19, 67, 273, 1374, etc., which increase the script reviewing difficulty, as well as forcing script developers to constantly consult the parser/parse_uri.h C program file, which I think should be avoided at all costs.

Suggestion to fix the above: add a "string CSV options" converter function in parser/parse_uri.h to reverse human-readable input such as "user|passwd|userparam|transport" into the same enum uri_match_flags as you are using now. The function will be called once at startup (during fixup phase) anyway, so it will come with zero runtime overhead, but with a big QoL improvement.

Finally, let's also add some documentation in the modules/uac_registrant/doc/uac_registrant_admin.xml file before we can merge this new feature (modparam).

@vasilevalex
[uac_registrant] New parameter to match reply Contact with binding_URI
819283c 
Added new string module parameter 'match_contact' to choose, what
parts of URI from Contact header should match binding_URI when
OpenSIPS chooses expiration time for registration.
@vasilevalex vasilevalex mentioned this pull request Dec 1, 2023
Open
@vasilevalex
Copy link
Contributor Author

@liviuchircu can you please review, when you have time?

@jes
Copy link
Contributor

jes commented Dec 5, 2023 •
edited

I just had a look at this and noticed a couple of potential issues:

  1. the "maybe we're lucky and straight-forward comparison succeeds" compares with strncasecmp which is not correct as detailed in uac_registrant: compare Contact URI case-insensitively #3263 (review)
  2. it allocates fixed-size buffers on the stack for char unescaped_a[UNESCAPED_BUF_LEN], unescaped_b[UNESCAPED_BUF_LEN], but then writes into them without any bounds-checking (EDIT: ah, I see there is bounds-checking in unescape_user)

@bogdan-iancu
Copy link
Member

@vasilevalex , could you detail/describe the scenario where the host part should be ignored (in a clustered setup) ?

@vasilevalex
Copy link
Contributor Author

Hi, @jes @bogdan-iancu. I described why we need in some cases to compare Contact header from 200 Ok only with partial match in #2557 .
I can add here. If database with credentials is used by several OpenSIPS server with clusterer setup. All the servers use the same "binding_URI". But all the servers have different IP addresses. So we distribute registrations through cluster. In 200 Ok we will receive different Contact URI, depending of the OpenSIPS server which sent the REGISTER. It means that we must also always change binding_URI in database to have Contact matching. And in case of failover of one of the servers we must not just move shared tag to another server, but also fix all the records in DB changing IP-address in the binding_URI. That's the case, when we want to compare URI from Contact header only partially, excluding host part.
P.S. @jes I'll try to check the issues you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liviuchircu liviuchircu Awaiting requested review from liviuchircu

Assignees

@bogdan-iancu bogdan-iancu

Labels
investigating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] uac_registrant match URI to Contact header partially
4 participants
@vasilevalex @jes @bogdan-iancu @liviuchircu