Sip module - get sip password from external url #3480
Conversation
|
Thanks for your contribution, @dvillaume! Please make sure you sign our CLA, as it's a required step before we can merge this. |
There was a problem hiding this comment.
Hi David, thanks for your contribution! I've added some generic notes inline.
On a general note, I'm not 100% sure whether this could be a good idea or not. This seems to assume that a backend component has access to the credentials of users, which sounds like it's "bad"? When would such a setup be considered acceptable? It also seems to auto-default to using this for plain text secrets, when a different approach might be better (but I'm not a SIP expert, so I'm not familiar about what alternatives could exist here). There doesn't seem to be any way to provide some form of authentication, since all it relies on is a path to use for lookups: I guess that in controlled environments this is probably ok, but shouldn't this account for the possibility that it might not be the case? I don't know if there are standard ways of retrieving credentials, e.g., for provisioning purposes, that may make sense here.
I'll leave it to other users of the SIP plugin to discuss whether or not such a feature could be helpful to them as well.
| @@ -687,6 +689,7 @@ | |||
|
|
|||
| #include <arpa/inet.h> | |||
| #include <net/if.h> | |||
| #include <curl/curl.h> | |||
There was a problem hiding this comment.
libcurl is an optional dependency in Janus. A few plugins can use it, but always in an optional way. Please see how the Streaming plugin only conditionally uses it, for instance (in that case for RTSP support). I'm not going to make it a mandatory requirement just for this. Of course, this means that all references and usages of this functionality will need to be made conditional as well (including error messages when trying to use them when curl isn't available).
| @@ -873,6 +878,10 @@ static struct janus_json_parameter sipmessage_parameters[] = { | |||
| {"headers", JSON_OBJECT, 0}, | |||
| {"call_id", JANUS_JSON_STRING, 0} | |||
| }; | |||
| struct curl_response_buffer { | |||
| char *response; | |||
| size_t size; | |||
There was a problem hiding this comment.
General note: it looks like a lot of your code uses spaces for indentation. Please use tabs everywhere (here and in all your other changes), since that's the project code style.
| json_t *authuser = json_object_get(root, "authuser"); | ||
| if(!secret && !ha1_secret) { | ||
| JANUS_LOG(LOG_ERR, "Missing element (secret or ha1_secret)\n"); | ||
|
|
There was a problem hiding this comment.
Please remove this unneeded empty line.
| JANUS_LOG(LOG_ERR, "Missing element (secret or ha1_secret)\n"); | ||
|
|
||
| if(!secret && !ha1_secret && (!get_sip_passwd_from_url || !web_session_id)) { | ||
| JANUS_LOG(LOG_ERR, "Missing element (secret or ha1_secret, or web_session_id (in get_sip_passwd_from_url mode) )\n"); |
There was a problem hiding this comment.
There's an extra bracket in web_session_id (in... that's not needed.
| goto error; | ||
| } | ||
| if(secret && ha1_secret) { | ||
| JANUS_LOG(LOG_ERR, "Conflicting elements specified (secret and ha1_secret)\n"); | ||
| if( (secret && ha1_secret) || (secret && web_session_id) || (ha1_secret && web_session_id)) { |
There was a problem hiding this comment.
Please don't add spaces in checks like if( ( (code style).
| /* Build the URL */ | ||
| size_t url_len = strlen(sip_passwd_url) + 1 + strlen(web_session_id_str) + 1; | ||
| char *url = malloc(url_len); | ||
| if(url == NULL) { |
There was a problem hiding this comment.
This whole check is unneeded: we don't do memory checks like this because, once they occur, the server will crash anyway.
| g_snprintf(error_cause, 512, "Out of memory"); | ||
| goto error; | ||
| } | ||
| snprintf(url, url_len, "%s/%s", sip_passwd_url, web_session_id_str); |
There was a problem hiding this comment.
Again, please use glib utilities like we do in other parts of the code: this should be g_snprintf.
| curl_easy_setopt(curl_handle, CURLOPT_URL, url); | ||
| curl_easy_setopt(curl_handle, CURLOPT_WRITEFUNCTION, curl_write_cb); | ||
| curl_easy_setopt(curl_handle, CURLOPT_WRITEDATA, (void *)&chunk); | ||
| curl_easy_setopt(curl_handle, CURLOPT_USERAGENT, "Janus SIP Plugin/0.0.9"); |
There was a problem hiding this comment.
I wouldn't hardcode this, since we have defines like JANUS_SIP_VERSION_STRING and JANUS_SIP_NAME.
| curl_easy_setopt(curl_handle, CURLOPT_WRITEFUNCTION, curl_write_cb); | ||
| curl_easy_setopt(curl_handle, CURLOPT_WRITEDATA, (void *)&chunk); | ||
| curl_easy_setopt(curl_handle, CURLOPT_USERAGENT, "Janus SIP Plugin/0.0.9"); | ||
| curl_easy_setopt(curl_handle, CURLOPT_FOLLOWLOCATION, 1L); |
There was a problem hiding this comment.
This is missing properties like CURLOPT_TIMEOUT and CURLOPT_CONNECTTIMEOUT. Considering this is a blocking wait, this could end up waiting forever. You can refer to some properties we add in the Streaming plugin for RTSP (e.g., CURLOPT_NOSIGNAL or CURLOPT_NOPROGRESS).
| } | ||
|
|
There was a problem hiding this comment.
You're adding a lot of empty lines, in the previous block too. Please don't do that.
.github/workflows/janus-ci.yml
Outdated
| @@ -63,6 +63,7 @@ jobs: | |||
| libvorbis-dev | |||
| ninja-build | |||
| openssl | |||
| libcurl4-openssl-dev | |||
There was a problem hiding this comment.
Please don't do that. Our CI workflow already has flags to conditionally add libcurl or not. As I explained in my inline notes, you have to make support for libcurl conditional in your code as well, which will prevent the CI error you got now.
dvillaume
force-pushed
the
get_sip_passwd_from_url
branch
from
9e455a7
to
6f34e7c
Compare
Hello,
We wanted to avoid end user to see SIP password in flows.
The idea here would be to retrieve a web_session_id from webrtc client and to get the associated sip_password from http request (accessible only from Janus).
hope this could be helpfull for other people and avoid SIP credentials leak.
Tell me if there is anything i can enhance as my C skills are pretty low.