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.

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

Rework contact generation, add validation & enforce contact_user even for requests taking Contact from server_address #3317

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

Conversation

vladpaiu
Copy link
Member

@vladpaiu vladpaiu commented Feb 26, 2024

Summary
Rework contact generation, add validation & enforce contact_user even for requests taking Contact from server_address

Details
Due to server_address potentially containing PVARs, validate the server_address before usage
Enforce contact_user modparam even for requests that generate the Contact from server_address

Thanks to Voicenter for reporting it & helping troubleshoot.

If contact_user is set, propagate FROM username to Contact even for requests taking Contact from server_address
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 Mar 28, 2024
@github-actions github-actions bot closed this Apr 6, 2024
@vladpaiu vladpaiu reopened this Apr 6, 2024
@stale stale bot removed the stale label Apr 6, 2024
@bogdan-iancu bogdan-iancu self-assigned this Apr 10, 2024
Copy link
Member

@bogdan-iancu bogdan-iancu left a comment

Choose a reason for hiding this comment

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

There are couple of things not right here :

  • parsing the generated URI (based on server_address_pve) is an unnecessary burn - as script writer you need to be sure you configuration is correct and not to expect OpenSIPS to parse it (for each message) just to be sure you are right - eventually you can do a dummy test at startup, to see if the pve generates a valid URI
  • the replacement of the username part (if contact_user is on) in the generated URI (based on server_address_pve) does not look right - you do not actually replace the username part, but build a completely new URI based on the username and socket - so if my server_address_pve was pushing some URI params or anything else, it will be lost. Normally you should have here a sctrict username replacement into the local_contact parsed URI - this may be simply done with 3 memcpy's .

@vladpaiu
Copy link
Member Author

There are couple of things not right here :

  • parsing the generated URI (based on server_address_pve) is an unnecessary burn - as script writer you need to be sure you configuration is correct and not to expect OpenSIPS to parse it (for each message) just to be sure you are right - eventually you can do a dummy test at startup, to see if the pve generates a valid URI

Don't agree with the above ... OpenSIPS should validate the server address upon building, in order to avoid sending broken messages ( due to the script writer's fault, indeed ). Can't be validated at startup, since it might contain pvars which can be only evaluated in the SIP message context.

  • the replacement of the username part (if contact_user is on) in the generated URI (based on server_address_pve) does not look right - you do not actually replace the username part, but build a completely new URI based on the username and socket - so if my server_address_pve was pushing some URI params or anything else, it will be lost. Normally you should have here a sctrict username replacement into the local_contact parsed URI - this may be simply done with 3 memcpy's .

This is indeed an issue - my use case didn't have uri params so I lost track of this. Needs correcting.

@bogdan-iancu
Copy link
Member

There are couple of things not right here :

  • parsing the generated URI (based on server_address_pve) is an unnecessary burn - as script writer you need to be sure you configuration is correct and not to expect OpenSIPS to parse it (for each message) just to be sure you are right - eventually you can do a dummy test at startup, to see if the pve generates a valid URI

Don't agree with the above ... OpenSIPS should validate the server address upon building, in order to avoid sending broken messages ( due to the script writer's fault, indeed ). Can't be validated at startup, since it might contain pvars which can be only evaluated in the SIP message context.

yes, the contact must be correct, but opensips should not continuously check if the configured data is correct or not. Like we do not check if the received AVP (in registrar) is a valid info, and so one. It is the duty of the script writer to be sure the configured data is correct.

  • the replacement of the username part (if contact_user is on) in the generated URI (based on server_address_pve) does not look right - you do not actually replace the username part, but build a completely new URI based on the username and socket - so if my server_address_pve was pushing some URI params or anything else, it will be lost. Normally you should have here a sctrict username replacement into the local_contact parsed URI - this may be simply done with 3 memcpy's .

This is indeed an issue - my use case didn't have uri params so I lost track of this. Needs correcting.

LEt me know if you plan to change this, we have only 2 days left to the release

@vladpaiu
Copy link
Member Author

vladpaiu commented May 8, 2024

did not have time to prioritize this, since it's labeled as a bug, maybe we can hotfix & backport later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants