plugins/janus_sip.c: MESSAGE Authentication and Deliver Status Report #2786

thetechpanda · 2021-10-02T00:00:17Z

CHANGE: sets MESSAGE Call-ID if not defined in the message event
ADD: SIP Authentication for 401 and 407 reponse to Message
ADD: messagedelivery event to report delivery status back to the peer

CHANGE: sets MESSAGE Call-ID if not defined in the message event ADD: SIP Authentication for 401 and 407 reponse to Message ADD: messagedelivery event to report delivery status back to the peer

lminiero

Thanks! Added some notes inline.

plugins/janus_sip.c

lminiero · 2021-10-15T09:57:29Z

@thetechpanda any update on this? I'm planning to do a new release soon, so I was wondering if we should wait for this too, or leave it for the next update. Thanks!

thetechpanda · 2021-10-16T07:52:04Z

Hi Lorenzo, I’ve done the changes, including the change so that the helpers will receive the responses. Unfortunately I don’t have a testing rig anymore, my SIP service provider has disabled messages exchange in the latest major software release. I was planning to run Kamailio so that I could check the change was working as expected, but I am in a very busy schedule at work and leaves me little time to spend on personal projects. I don’t like to push code that I didn’t test, but if are ok with it, I believe is ready. Kind regards,

…

On Fri, 15 Oct 2021 at 8:57 pm, Lorenzo Miniero ***@***.***> wrote: @thetechpanda <https://github.com/thetechpanda> any update on this? I'm planning to do a new release soon, so I was wondering if we should wait for this too, or leave it for the next update. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3HVCDPTIZKXNWJBSE2DALUG73JJANCNFSM5FFWSMYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

lminiero · 2021-10-18T12:13:26Z

@thetechpanda if you made some changes feel free to push them, then, and we'll review if they're ok. In case there's still something to fix, no problem for me to keep this on hold until you can get back to it, and publish a new release without this for the moment.

…ginating session CHANGE removes redundant call_id and delivery status from deliveryrecepit (available through root call_id and code)

thetechpanda · 2021-10-21T21:24:16Z

I pushed the changes they should be ok. let me know if you need me to fix anything else. cheers!

thetechpanda · 2021-10-28T05:02:42Z

Hi @lminiero did you get a chance to look at my changes? Not being pushy I'm just very excited this could get actually get merged :)

lminiero · 2021-10-28T08:21:25Z

@thetechpanda not yet, sorry, I was busy with preparations for a presentation I had to make. I'll look at this before the weekend, thanks!

lminiero

Just checked the code, and I see some potentially serious issues that need to be addressed. Once they are, I think this should be tested before it can be merged. Thanks for taking the time to expand this effort!

plugins/janus_sip.c

* FIX: html div tag closed wrongly * ADD: demo sip message UI * FIX: plugins/janus_sip.c messagedelivery session handling * ADD: plugins/janus_sip.c messagedelivery documentation

thetechpanda · 2021-11-07T05:40:43Z

Hello,

I've rebuilt the test bench, using kamailio. I've tested using http and websocket transports and done multiple tests sending and receiving messages. I've debugged as extensively as I can, I've not seen any unexpected behaviour and I've carefully reviewed the locks, hope this time the patch is worthy or your time.

I've also added some documentation at the top of the plugin to cater for the new behaviour: optional call_id for message requests and messagedelivery event documentation.

I've noticed something, sending messages via Helpers works as expected and the messagedelivery event is correctly sent to the master or the helper. However inbound messages are always notified to the master session - I don't see an issue with it.

Let me know,

Kind Regards,

ycherniavskyi · 2021-11-07T11:26:10Z

plugins/janus_sip.c

+				const char *scheme = NULL;
+				const char *realm = NULL;
+				if(status == 401) {
+					/* Get scheme/realm from 401 error */
+					sip_www_authenticate_t const* www_auth = sip->sip_www_authenticate;
+					scheme = www_auth->au_scheme;
+					realm = msg_params_find(www_auth->au_params, "realm=");
+				} else {
+					/* Get scheme/realm from 407 error, proxy-auth */
+					sip_proxy_authenticate_t const* proxy_auth = sip->sip_proxy_authenticate;
+					scheme = proxy_auth->au_scheme;
+					realm = msg_params_find(proxy_auth->au_params, "realm=");
+				}
+				char authuser[100], secret[100];
+				memset(authuser, 0, sizeof(authuser));
+				memset(secret, 0, sizeof(secret));
+				if(session->helper) {
+					/* This is an helper session, we'll need the credentials from the master */
+					if(session->master == NULL) {
+						JANUS_LOG(LOG_WARN, "No master session for this helper, authentication will fail...\n");
+					} else {
+						session = session->master;
+					}
+				}
+				if(session->account.authuser && strchr(session->account.authuser, ':')) {
+					/* The authuser contains a colon: wrap it in quotes */
+					g_snprintf(authuser, sizeof(authuser), "\"%s\"", session->account.authuser);
+				} else {
+					g_snprintf(authuser, sizeof(authuser), "%s", session->account.authuser);
+				}
+				if(session->account.secret && strchr(session->account.secret, ':')) {
+					/* The secret contains a colon: wrap it in quotes */
+					g_snprintf(secret, sizeof(secret), "\"%s\"", session->account.secret);
+				} else {
+					g_snprintf(secret, sizeof(secret), "%s", session->account.secret);
+				}
+				char auth[256];
+				memset(auth, 0, sizeof(auth));
+				g_snprintf(auth, sizeof(auth), "%s%s:%s:%s:%s%s",
+					session->account.secret_type == janus_sip_secret_type_hashed ? "HA1+" : "",
+					scheme,
+					realm,
+					authuser,
+					session->account.secret_type == janus_sip_secret_type_hashed ? "HA1+" : "",
+					secret);
+				JANUS_LOG(LOG_VERB, "\t%s\n", auth);
+				/* Authenticate */
+				nua_authenticate(nh,
+					NUTAG_AUTH(auth),
+					TAG_END());


Just noticed that this bloc of code repeated the third time. Could we move it to a separate function?

I am implementing publish command and will repeat it a fourth time. For sure I could do it in my PR, just curious is it the right way of refactoring, or is there some other sense in repeating the same code several times?

It could be a function or could be before the main switch(event) in janus_sip_sofia_callback. Auth and ProxyAuth are not used outside of this function, IMHO it is better to split code into functions only if used elsewhere.

We do re-use the same auth code for different SIP responses, true. Anyway, this isn't something I'd do here: this is a change I'd do separately in another PR, devoted just to that, so I can look into this once we merge this effort.

lminiero · 2021-11-08T10:37:36Z

Hello,

I've rebuilt the test bench, using kamailio. I've tested using http and websocket transports and done multiple tests sending and receiving messages. I've debugged as extensively as I can, I've not seen any unexpected behaviour and I've carefully reviewed the locks, hope this time the patch is worthy or your time.

Contributions are always worthy of my time! ☺️
I'll try to make some tests, too, e.g., to check if there can be minor leaks 👍

I've also added some documentation at the top of the plugin to cater for the new behaviour: optional call_id for message requests and messagedelivery event documentation.

Ack, thx!

I've noticed something, sending messages via Helpers works as expected and the messagedelivery event is correctly sent to the master or the helper. However inbound messages are always notified to the master session - I don't see an issue with it.

I guess this depends on what kind of message this is. If this is an out-of-dialog MESSAGE, then yes, it will be delivered to the owner of SIP stack, so the master session: if it's an in-dialog MESSAGE, though, as part of an ongoing call, it should go to the session that's actually handling that call. I guess you tested the former? We'll probably also want to make some tests with in-dialog MESSAGE transactions, just to make sure nothing breaks there.

lminiero

Just checked your changes, and added some more notes inline 👍

html/siptest.html

html/siptest.js

plugins/janus_sip.c

* CHANGE: introduces sip_session_dereference * CHANGE: html/siptest.js doMessage() renames suffix to helperId * FIX: html/siptest.js helpers failing to make calls if they previously attempted to make a call to invalid SIP uri

html/siptest.js

thetechpanda · 2021-11-09T21:57:14Z

I've done the requested changes, I'm not too sure about the function name janus_sip_session_dereference as it is used only by the messageids hashtable.

I've tested in-dialog and out-of-dialog messages and seem to work as expected. I've also fixed an issue I was having with siptest.js where the helpers could not make the call after trying to dial an invalid SIP uri.

If you still feel we don't need the out-of-dialog message UI, let me know and I'll revert the changes, if you want me to change the ui I am more than happy to do further changes.

Apologies for the long time this is taking and thank you for taking the time to review my code.

lminiero · 2021-11-16T13:24:43Z

Apologies for the delay, but I've been quite busy last week. Your latest changes do look fine to me now! To address your notes:

I'm not too sure about the function name janus_sip_session_dereference as it is used only by the messageids hashtable.

That's fine: it's a good function to have in case we'll need it again for other hashtables or lists in the future.

If you still feel we don't need the out-of-dialog message UI, let me know and I'll revert the changes, if you want me to change the ui I am more than happy to do further changes.

Yeah, I personally don't think we need this new UI, which would complicate and make the UI even more confusing than it currently is (which is my fault!). For now, I think it's enough to have it documented in Doxygen that "message" can be used both in-dialog and out-of-dialog. We can always consider changes to the UI in separate PRs in the future, if it's needed.

Apologies for the long time this is taking and thank you for taking the time to review my code.

No problem at all, no one's in a hurry! I always appreciate contributions, and I know people are busy, so I never expect anyone to do things any faster than they can/want: that's how open source works 🙂

thetechpanda · 2021-11-21T01:00:53Z

For some reason VSCode kept changing the spacing for my changes after I'd save, hope you don't mind the two styling commits in the latest code push.

I've removed the out-of-dialog ui, anything else you'd like me to address?

lminiero · 2021-11-22T13:32:55Z

I think that the two </div> fixes are actually correct, and a typo on my end originally.
Looks good now, thanks again for the effort and the patience! Merging 👍

thetechpanda · 2021-11-22T21:34:38Z

yay!

…

On Tue, 23 Nov 2021 at 12:33 am, Lorenzo Miniero ***@***.***> wrote: Merged #2786 <#2786> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2786 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF3HVCAWVL2JRUOTKSJVX6LUNJBD3ANCNFSM5FFWSMYA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

thetechpanda added 2 commits October 2, 2021 09:44

SIP Messages

b5d0d5b

CHANGE: sets MESSAGE Call-ID if not defined in the message event ADD: SIP Authentication for 401 and 407 reponse to Message ADD: messagedelivery event to report delivery status back to the peer

FIX: removes unnecessary if as message_callid is always defined.

a6cdbd1

lminiero reviewed Oct 4, 2021

View reviewed changes

ADD support for messageids hashtable to report delivery status to ori…

6df44c9

…ginating session CHANGE removes redundant call_id and delivery status from deliveryrecepit (available through root call_id and code)

lminiero reviewed Oct 29, 2021

View reviewed changes

plugins/janus_sip.c Outdated Show resolved Hide resolved

plugins/janus_sip.c Show resolved Hide resolved

plugins/janus_sip.c Outdated Show resolved Hide resolved

plugins/janus_sip.c Outdated Show resolved Hide resolved

SIP Message update

52ec23d

* FIX: html div tag closed wrongly * ADD: demo sip message UI * FIX: plugins/janus_sip.c messagedelivery session handling * ADD: plugins/janus_sip.c messagedelivery documentation

ycherniavskyi reviewed Nov 7, 2021

View reviewed changes

lminiero reviewed Nov 8, 2021

View reviewed changes

html/siptest.html Outdated Show resolved Hide resolved

html/siptest.js Outdated Show resolved Hide resolved

html/siptest.js Outdated Show resolved Hide resolved

plugins/janus_sip.c Show resolved Hide resolved

plugins/janus_sip.c Outdated Show resolved Hide resolved

SIP Message update & siptest.js bugfix

d2ba18f

* CHANGE: introduces sip_session_dereference * CHANGE: html/siptest.js doMessage() renames suffix to helperId * FIX: html/siptest.js helpers failing to make calls if they previously attempted to make a call to invalid SIP uri

thetechpanda commented Nov 9, 2021

View reviewed changes

html/siptest.js Show resolved Hide resolved

thetechpanda requested a review from lminiero November 16, 2021 04:46

thetechpanda added 3 commits November 21, 2021 11:43

CHANGE: removes out-of-dialog message interface

ac9ee9d

FIX: fixes spacing to match original siptest.html

25d63d7

FIX: fixes spacing to match original siptest.html (2)

ea8cf03

lminiero merged commit d5e3d2e into meetecho:master Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins/janus_sip.c: MESSAGE Authentication and Deliver Status Report #2786

plugins/janus_sip.c: MESSAGE Authentication and Deliver Status Report #2786

thetechpanda commented Oct 2, 2021

lminiero left a comment

lminiero commented Oct 15, 2021

thetechpanda commented Oct 16, 2021 via email

lminiero commented Oct 18, 2021

thetechpanda commented Oct 21, 2021

thetechpanda commented Oct 28, 2021

lminiero commented Oct 28, 2021

lminiero left a comment

thetechpanda commented Nov 7, 2021

ycherniavskyi Nov 7, 2021

thetechpanda Nov 7, 2021

lminiero Nov 8, 2021

lminiero commented Nov 8, 2021

lminiero left a comment

thetechpanda commented Nov 9, 2021

lminiero commented Nov 16, 2021

thetechpanda commented Nov 21, 2021

lminiero commented Nov 22, 2021

thetechpanda commented Nov 22, 2021 via email

plugins/janus_sip.c: MESSAGE Authentication and Deliver Status Report #2786

plugins/janus_sip.c: MESSAGE Authentication and Deliver Status Report #2786

Conversation

thetechpanda commented Oct 2, 2021

lminiero left a comment

Choose a reason for hiding this comment

lminiero commented Oct 15, 2021

thetechpanda commented Oct 16, 2021 via email

lminiero commented Oct 18, 2021

thetechpanda commented Oct 21, 2021

thetechpanda commented Oct 28, 2021

lminiero commented Oct 28, 2021

lminiero left a comment

Choose a reason for hiding this comment

thetechpanda commented Nov 7, 2021

ycherniavskyi Nov 7, 2021

Choose a reason for hiding this comment

thetechpanda Nov 7, 2021

Choose a reason for hiding this comment

lminiero Nov 8, 2021

Choose a reason for hiding this comment

lminiero commented Nov 8, 2021

lminiero left a comment

Choose a reason for hiding this comment

thetechpanda commented Nov 9, 2021

lminiero commented Nov 16, 2021

thetechpanda commented Nov 21, 2021

lminiero commented Nov 22, 2021

thetechpanda commented Nov 22, 2021 via email