What is wrong with this URL?

neilyoung commented 2 years ago

Prerequisites

These are MANDATORY, otherwise the issue will be automatically closed.

[x] I agree to fill this issue template.
[x] I have read the Troubleshooting Guide and Support Instructions.

Issue description

Trying to make curlhttpsink run with an authenticated URL following the suggestion here: https://doc-kurento.readthedocs.io/en/latest/_static/client-javadoc/org/kurento/client/RecorderEndpoint.html

I'm attempting to use this URL:

http://username:password@127.0.0.1:8080/api/record/abcde/1

It is rejected by the regex in kms_is_valid_uri

gboolean
kms_is_valid_uri (const gchar * url)
{
  gboolean ret;
  GRegex *regex;

  regex = g_regex_new ("^(?:((?:https?):)\\/\\/)([^:\\/\\s]+)(?::(\\d*))?(?:\\/"
      "([^\\s?#]+)?([?][^?#]*)?(#.*)?)?$", 0, 0, NULL);
  ret = g_regex_match (regex, url, G_REGEX_MATCH_ANCHORED, NULL);
  g_regex_unref (regex);

  return ret;
}

(which is really wild).

This ends up in an error trace in KMS

0:19:37.674259307 106823 0xaaaac503e6a0 DEBUG                GST_URI gsturi.c:644:gst_element_make_from_uri: type:1, uri:http://username:password@127.0.0.1:8080/api/record/abcde/1, elementname:(null)
0:19:37.675713097 106823 0xaaaac503e6a0 DEBUG                GST_URI gsturi.c:650:gst_element_make_from_uri: No sink for URI 'http://username:password@127.0.0.1:8080/api/record/abcde/1'
0:19:37.675870513 106823 0xaaaac503e6a0 ERROR         basemediamuxer kmsbasemediamuxer.c:147:kms_base_media_muxer_get_sink_fallback:<KmsAVMuxer@0xffff3c006680> URL not valid
0:19:37.675883930 106823 0xaaaac503e6a0 ERROR         basemediamuxer kmsbasemediamuxer.c:228:kms_base_media_muxer_get_sink:<KmsAVMuxer@0xffff3c006680> No URI handler implemented for 'http'
0:19:37.675888263 106823 0xaaaac503e6a0 ERROR         basemediamuxer kmsbasemediamuxer.c:252:kms_base_media_muxer_create_sink_impl:<KmsAVMuxer@0xffff3c006680> No available sink for uri http://username:password@127.0.0.1:8080/api/record/abcde/1

Context

Authentication not working. Unauthenticated URL works

How to reproduce?

Just do it :)

Expected & current behavior

Should pass, since it complies to the pattern

(Optional) Possible solution

Info about your environment

About Kurento Media Server

github-actions[bot] commented 2 years ago

Hello @neilyoung! :wave: we're sorry you found a bug... so first of all, thank you very much for reporting it.

To know about progress, check in Triage. All issues are considered Backlog Candidates until work priorities align and the issue is selected for development. It will then become part of our official Backlog.

neilyoung commented 2 years ago

The regexp does not cover the authentication case.

neilyoung commented 2 years ago

Too bad. I patched out the erroneous test in kms_is_valid_uri. URI with credentials is accepted now, but authentication does not work. Even though my server rejects a request w/o credentials with 401, the request is neither repeated nor it contains the authentication header in the first call.

Strange. Close to version 7 and nobody noticed that?

neilyoung commented 2 years ago

Finally: curlhttpsink does obviously not support URL auth ala "http://username:password@..." in "location". There are two properties "user" and "passwd" which would have to be set. I can't see, how RecorderEndpoint would support that.

j1elo commented 1 year ago

Hi @neilyoung , are you building Kurento 7 from sources? This looked like an easy enough addition and today I had a couple hours to look into it, so I made this patch that adds support for it:

recorder-auth.patch.txt

it should be able to apply cleanly to Kurento 7 sources; something like:

cd ~/work/kurento/
git apply ~/downloads/recorder-auth.patch.txt

What it does is use the GstUri class of GStreamer, to extract the username:password parts and apply them separately to the user and passwd properties of curlhttpsink.

Right now, these fields are just extracted, but still kept as part of the URL. So I hope the curlhttpsink is just ignoring that part, and doesn't cause an access error. Otherwise, maybe it would be also needed to actually remove those fields from the URL, after setting them as properties to the curlhttpsink object.

Also note an important limitation of GstUri as of GStreamer 1.16 (but fixed in newer versions which we don't support yet): it doesn't support user and pass with special characters. With current implementation, these are split at the first : found, so long story short, the username cannot contain a :.

neilyoung commented 1 year ago

Thanks for dealing with this. Meanwhile I'm using another approach. Are you still interested to know, how I build KMS 7? I'm having Ansible scripts for this, but I'm sure I can derive a 5 liner, which runs from command line

Kurento / kurento