Support latest 3GPP TS 33102 #4414
Conversation
The length of the authentication key (K) has changed since the implementation of Digest Authentication. Issue: pjsip#4413
pjsip/src/pjsua-lib/pjsua_acc.c
Outdated
| if (status != PJ_SUCCESS) { | ||
| pjsua_perror(THIS_FILE, | ||
| "Cannot set credentials for registration", | ||
| status); | ||
| } |
There was a problem hiding this comment.
Thanks for the report & the fix.
Rather OOT, while here, perhaps failure in setting credentials (partial/complete) should cause pjsua_regc_init() to fail (or returning error) as well so the app will be aware of any potential problems earlier?
Also, there are some other function calls around here without error checks, I guess better fix them as well :)
If you want, we can do it.
There was a problem hiding this comment.
I have also thought about this (.e.g. by introducing a "goto on_error_return").
One other topic: pjsip_regc_set_credentials() fails, if one "AuthCredInfo" "out of N" fails. This is because an array instead of the memory pool is used. It would be better to just don't use the erroneous "AuthCredInfo". But I am not sure whether it is worth to do.
I you do the changes, it will be cool.
There was a problem hiding this comment.
I have also thought about this (.e.g. by introducing a "goto on_error_return").
Done. This introduces behavioral changes, but I guess it is necessary because skipping failures may introduce issues, e.g: unexpected behaviors, debugging difficulties.
One other topic:
pjsip_regc_set_credentials()fails, if one "AuthCredInfo" "out of N" fails. This is because an array instead of the memory pool is used. It would be better to just don't use the erroneous "AuthCredInfo". But I am not sure whether it is worth to do.
Ignoring erroneous credential information may lead to unexpected behavior. For example, if the app prefers to use AKA credentials over MD5, but as AKA is silently skipped due to an error, the app may not achieve its preferred security level. Therefore, I think allowing pjsip_regc_set_credentials() to fail seems to be the correct approach.
There was a problem hiding this comment.
Thank you! Looks good to me.
| @@ -2728,10 +2728,7 @@ static pj_status_t pjsua_regc_init(int acc_id) | |||
| pjsua_perror(THIS_FILE, "Unable to generate suitable Contact header" | |||
| " for registration", | |||
| status); | |||
| destroy_regc(acc, PJ_TRUE); | |||
| pj_pool_release(pool); | |||
| acc->regc = NULL; | |||
There was a problem hiding this comment.
This is not moved to the on_return. Will it have side-effects?
There was a problem hiding this comment.
destroy_regc() also resets the acc->regc.
The length of the authentication key (K) has changed since the implementation of Digest Authentication.
Issue: #4413