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

What is wrong with this URL? #42

Open
2 tasks done
neilyoung opened this issue Nov 24, 2022 · 6 comments · May be fixed by #44
Open
2 tasks done

What is wrong with this URL? #42

neilyoung opened this issue Nov 24, 2022 · 6 comments · May be fixed by #44
Assignees
Labels
enhancement New feature or request

Comments

@neilyoung
Copy link
Contributor

Prerequisites

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

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:[email protected]: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:[email protected]: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:[email protected]: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:[email protected]: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
Copy link

Hello @neilyoung! 👋 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
Copy link
Contributor Author

The regexp does not cover the authentication case.

@neilyoung
Copy link
Contributor Author

neilyoung commented Nov 26, 2022

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
Copy link
Contributor Author

neilyoung commented Nov 26, 2022

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
Copy link
Member

j1elo commented Jun 12, 2023

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 :.

@j1elo j1elo reopened this Jun 12, 2023
@j1elo j1elo self-assigned this Jun 12, 2023
@j1elo j1elo added the enhancement New feature or request label Jun 12, 2023
@j1elo j1elo transferred this issue from Kurento/bugtracker Jun 12, 2023
@neilyoung
Copy link
Contributor Author

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

@j1elo j1elo linked a pull request Jun 13, 2023 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants