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

Fixed Service-Route update upon registration #3823

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Fixed Service-Route update upon registration #3823

merged 2 commits into from
Jan 10, 2024

Conversation

sauwming
Copy link
Member

@sauwming sauwming commented Jan 8, 2024

To fix #3790.

According to the RFC (https://datatracker.ietf.org/doc/html/rfc3608.html#section-6.1):

If
   the UA refreshes the registration, the stored value of the Service-
   Route is updated according to the Service-Route header field of the
   latest 200 class response.  If there is no Service-Route header field
   in the response, the UA clears any service route for that address-
   of-record previously stored by the UA.  If the re-registration
   request is refused or if an existing registration expires and the UA
   chooses not to re-register, the UA SHOULD discard any stored service
   route for that address-of-record.

There are a couple issues with the current implementation:

  • We do not clear the service route if there is no Service-Route header found in the response.
  • We do not clear the service route upon registration failure.

Thanks to @andreas-wehrmann for the report and investigation.

@sauwming sauwming self-assigned this Jan 8, 2024
@sauwming sauwming added this to the release-2.15 milestone Jan 8, 2024
@sauwming sauwming closed this Jan 8, 2024
@sauwming sauwming reopened this Jan 9, 2024
@sauwming sauwming changed the title Always update Service-Route header upon successful registration Fixed Service-Route update upon registration Jan 9, 2024
@sauwming
Copy link
Member Author

sauwming commented Jan 9, 2024

Attached is the SIPP xml for testing the issue:
uas-register.xml.txt

Test flow:
Test 1:

1. REGISTER-200 OK with Service Route header
2. Confirm that outgoing call contains that header
3. Re-REGISTER-200 OK without any Service Route header
4. Confirm that outgoing call now contains no Route header

Test 2:

1. REGISTER-200 OK with Service Route header
2. Confirm that outgoing call contains that header
3. Re-REGISTER-failed
4. Confirm that outgoing call now contains no Route header

@andreas-wehrmann
Copy link
Contributor

andreas-wehrmann commented Jan 9, 2024

Though I haven't tested it, I believe there may be another issue with pjsua_acc_modify().
From the looks of it, it seems to assume that the route set consists always of the global outbound proxy list followed by the account specific proxy list (if any).

See here (global outbound proxies first):

pjsip_route_hdr *r = acc->route_set.next;

and here (account proxies always last):
pjsip_route_hdr *r = acc->route_set.prev;

It doesn't seem to account for any additional routes injected via Service-Route
in which case it may be best to simply discard any headers learned via Service-Route
when modifying an account before attempting to modify the route set?

@sauwming
Copy link
Member Author

sauwming commented Jan 9, 2024

I believe we don't need to clear the learnt Service-Route headers during account modification, except in rare cases such as modifying the registrar perhaps?

@andreas-wehrmann
Copy link
Contributor

update_service_route() appends the headers to the route set of an account.
So the route set is made up like this: global proxies + account proxies + service route.

My worry is, that the code here:

pjsip_route_hdr *r = acc->route_set.prev;

removes the service route headers (if there are any) when it actually wants to remove the proxies of the account.
Or am I not understanding this code correctly?

@sauwming
Copy link
Member Author

sauwming commented Jan 9, 2024

You're right. I understand the problem now.

@andreas-wehrmann
Copy link
Contributor

andreas-wehrmann commented Jan 9, 2024

Ok, so the questions remains what to do when?

  • Forget all Service-Routes on account modify unconditionally and let it relearn?
  • Forget Service-Routes only when there is a change to the global/account proxy list?
  • Forget Service-Routes only when there is change to either proxy list and/or the registrar (what about the Contact header and possibly other fields?)

In my naiveté I tend towards the first since it seems to be the option which is least error prone
but I am not sure...

@sauwming
Copy link
Member Author

sauwming commented Jan 9, 2024

I'm currently leaning towards the third option (only change in either proxy list or the registrar).
And I will probably open a new PR since it's a different issue (albeit related to this one).

@sauwming sauwming merged commit 19c018a into master Jan 10, 2024
35 checks passed
@sauwming sauwming deleted the reg-route branch January 10, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PJSUA][ACC] Service-Route header not cleared
4 participants