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

Share an auth session between multiple dialogs/regc #4262

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jwes
Copy link

@jwes jwes commented Jan 22, 2025

For Accounts that establish a lot of dialogs it may be beneficial to reuse a already established Authorization.
To achieve this, the account was extended to store a shared pjsip_auth_clt_sess and this is set on the relevant dialogs.

This patch adds PJSIP_SHARED_AUTH_SESSION to enable this behavior in the account and to extend the relevant api.

jwes added 2 commits January 21, 2025 09:23
To reduce the network load in scenarios where multiple SUBSCRIBE dialogs are used,
one can enable PJSIP_SHARED_AUTH_SESSION to allow setting a pjsip_auth_clt_sess for the dialogs.

This session will then be in charge of handling all auth requests.
when PJSIP_SHARED_AUTH_SESSION is enabled, the account will try to reuse authorization
headers.
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2025

CLA assistant check
All committers have signed the CLA.

return NULL;
}
if (sess->parent) {
pauth = sess->parent->cached_auth.next;
Copy link
Member

Choose a reason for hiding this comment

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

No iteration to lookup the matched realm & algorithm_type on parent?
(Or perhaps recursive call to self like in auth_find_cred())

@sauwming
Copy link
Member

I like the feature and I prefer a runtime option in pjsua_acc_config rather than a compile-time option (so you can get rid of all those annoying #ifdef).

But don't do any modifications yet, let's hear first what others think about this.

@sauwming sauwming self-requested a review January 23, 2025 07:58
@nanangizz
Copy link
Member

I like the feature and I prefer a runtime option in pjsua_acc_config rather than a compile-time option (so you can get rid of all those annoying #ifdef).

But don't do any modifications yet, let's hear first what others think about this.

Sounds like a good idea!

@jwes
Copy link
Author

jwes commented Jan 23, 2025

I like the feature and I prefer a runtime option in pjsua_acc_config rather than a compile-time option (so you can get rid of all those annoying #ifdef).

But don't do any modifications yet, let's hear first what others think about this.

yeah, that sounds good. I wasn't sure if that feature should be available without compile option, so i used a conservative approach

@jwes
Copy link
Author

jwes commented Jan 23, 2025

is there something i need to do regarding the failed checks?

@sauwming
Copy link
Member

I'm currently working on the fix on #4263.
Just waiting for the CI test there to complete and then I'll merge to master.

@sauwming
Copy link
Member

You can merge the master branch to fix the failed tests.

@jwes
Copy link
Author

jwes commented Jan 24, 2025

i just wanted to switch to an config parameter.
If i move it to the account config, it seems, that i need to set it up on acc_modify.

Is it safe to call pjsip_auth_ctl_deinit in acc_modify, since it releases a pool, that it did not create itself?

@sauwming
Copy link
Member

I don't understand the purpose of doing so, but I don't think you should call auth_clt_deinit() in acc_modify().
But anyway, you can proceed with what you think is best and we'll review it later.

jwes added 2 commits January 24, 2025 08:36
* use lock of parent instead of lock child lock.
* recusively call parent in auth_find_cred
Instead of an compile time option to share the authorization headers, the pjsua acc can be
configured to reuse the headers.
@nanangizz
Copy link
Member

Generally looks okay to me, perhaps a few touch-ups may improve it a bit, for example:

  • for efficiency, only parent auth sess needs lock, alternative approach from top of my head:
    • configurable via option param of auth_sess_init()
    • make a new API such as auth_sess_set_parent(), this way the lock can be created for parent only & app does not need to do the locking.
  • improve docs, e.g: May need to have PJSIP_AUTH_AUTO_SEND_NEXT, shouldn't it be required as it is the main purpose of this feature? As otherwise it will always wait for 401/407.
  • somehow lock/unlock return code check seems to reduce readibility a bit, unless expecting issues (e.g: race between destroy & lock/unlock), we don't usually check it :)
  • column width limit (80 chars).

If you want, we can do the above directly in the PR branch.

* improved comments
* line width
* create lock only in parent
* removed lock/unlock status tests
pjsip/include/pjsip-ua/sip_regc.h Outdated Show resolved Hide resolved
pjsip/include/pjsip-ua/sip_regc.h Outdated Show resolved Hide resolved
pjsip/include/pjsua-lib/pjsua.h Outdated Show resolved Hide resolved
pjsip/include/pjsua2/account.hpp Show resolved Hide resolved
jwes added 2 commits January 29, 2025 06:31
in pjsua2 it is now called useSharedAuth

also: improved some comments
@sauwming
Copy link
Member

The CI tests, despite being restarted several times, seem to consistently fail at the same test. Not sure if this is caused by this change, though

@jwes
Copy link
Author

jwes commented Jan 31, 2025

merged the master again, maybe that fixes something?

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.

4 participants