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
Added CURLPROTO_RTSP to redir_protocols #4750
Conversation
Actually that change was intentional for security reasons. You can enable RTSP on redirect by using libcurl's |
I understand that. Redirects in RTSP work different, not using 3xx. I have never used rtsp redirects, and it's not supported in curl. Apparently the same codepath handles both 3xx and 401? After the initial setting of redir_protocols was changed in version 7.66.0, the RTSP authentication (which is similar to HTTP Basic/Digest authentication) stopped working, findprotocol() in url.c returning CURLE_UNSUPPORTED_PROTOCOL To separate 3xx and 401 is a bit beyond me I am afraid. |
Ok but are you saying you have a HTTP URL and it's redirecting to an RTSP URL and that no longer works (expected) or you have an RTSP URL and it's redirecting to an RTSP URL (unexpected)? |
No, we are only discussing redirect because it seems that handling 3xx and 401 involves the same codepath The problem is that authentication doesn't work when RTSP isn't part of the redir_protocols. In order for curl to handle a normal Basic- or Digest auth it seems that the protocol must be part of the redir_protocols. Redirect in RTSP doesn't work with 3xx codes, it works different (REDIRECT response from server). Curl doesn't support it and I don't think it's needed |
Is there a public URL that we can use to reproduce this? |
It's more complicated than I thought... I made two streams available: rtsp://philea.ddns.net:50003/axis-media/media.amp -> Unauthenticated RTSP stream and adapted the examples/rtsp.c to support optional credentials. Turns out using the easy interface everything works. So the problem relates to either using the multi interface or my own code. I'll work on a multi-alternative for the rtsp example then. Please hang on for a few days... |
It's difficult to make sufficient time for it. I'll be back :) |
Someone fell over this bug on stackoverflow as well. |
Previously, it was only possible to set it to Windows Vista or XP by setting the option `ENABLE_INET_PTON` to `ON` resp. `OFF`. Use a new cache variable `CURL_TARGET_WINDOWS_VERSION` to be able to explicitly set the target Windows version. `ENABLE_INET_PTON` is ignored in this case. Ref: #1639 (comment) Ref: #4607 (comment) Closes #4815
It is superfluous and could even be misleading. Bug: https://curl.haxx.se/mail/archive-2020-01/0016.html Reported-by: Mike Norton Closes #4832
Fixes the bug where oauth_bearer gets deallocated when we re-use a connection. Closes #4824
follow-up from dea17b5 (one of these days I'll learn to check before I push)
Introduces CURLOPT_MAIL_RCPT_ALLLOWFAILS. Verified with the new tests 3002-3007 Closes #4816
- Copy CURLOPT_SSL_OPTIONS.3 description to CURLOPT_PROXY_SSL_OPTIONS.3. Prior to this change CURLSSLOPT_NO_PARTIALCHAIN was missing from the CURLOPT_PROXY_SSL_OPTIONS description.
Only ever used from within this file.
(and the corresponding unit test) Closes #4842
Hi, I am sorry for lack of follow up. Bit busy right now. Had hoped it was a simple thing to get approved. Will get back on it somewhere february |
As all the -I uses in CFLAGS at that point are for system headers and third party libraries this helps us remove/ignore warnings on those! Closes #5060
In bmake, if the directory is changed (with cd or anything else), bmake won't return to the "root directory" on the next command (in the same Makefile rule). This commit runs the cd command in a subshell so it would work in bmake. Closes #5073
This allows these test files to pass xmllint.
bumped to 7.69.2
Handle both absolute and relative control strings Removed some malloc's to simplify building with C++ compiler
…o rtsp_auth_fix
You need to rebase on top of master and then force-push to this branch again to make it possible for us to review this PR. Just note that adding RTSP as a redir protocol (as the title says) is not going to be okey. |
Daniel Stenberg schreef op 2020-03-21 00:31:
You need to rebase on top of master and then force-push to this branch
again to make it possible for us to review this PR. Just note that
adding RTSP as a redir protocol (as the title says) is not going to be
okey.
Hi Daniel.
Yes, I should do something.. actually decided this week I should just
set the option and be happy
it is not important enough. maybe I just add a note in the existing
example
Making an multi example is more work than I thought as I can't use my
C++ implementation, there's so many things
todo outside work and I have not been feeling well at all last few
weeks. It's a bit frustrating actually
I did work on another issue which is way more important for me, which is
that rtsp requires the socket and the rtp session (read: curl handle) to
keep their relation.
I tried an approach where to avoid the
curl_multi_remove_handle/curl_multi_add_handle pair, I made a
curl_multi_restart_handle which relaunches in-place the handle with
completed transfer. But it's difficult to get it right. And I am unsure
whether the strategy fits the 'zen' of the library, although I think a
small efficiency gain on persistent links to the same host can be
achieved. I sent a mail about it earlier this year to the list, which
was left without guidance unfortunately.
It's important for me as running multiple RTP-over-RTSP connections to
the same host two things go wrong
- some servers don't like messages for one session come in over
different sockets, disrespecting the RFC but well, it's gstreamer
- the payload of sessions can enter my code through the wrong handle.
I can fix it in the callback and maybe I will, because it is a lot
simpler. On the other hand it would make RTSP support in the lib really
mature, which would be a nice donation to the project
Greets,
Erik
|
I'm using easy interface to connect RTSP server, it works for me after I compiled curl-7.70.0 code base and turned on CURLOPT_FOLLOWLOCATION and set CURLOPT_REDIR_PROTOCOLS to CURLPROTO_RTSP in my application code. The RTSP server only allowed the digest authentication to connect, so curl issue another request to the same URL after 401 response. |
Sure, that's a work-around to get it working. However a 401 authentication handling is not a redirect so it shouldn't be necessary to set |
Ok, so how about this: Someone who suffers from this bug writes up a test case in the curl test suite that reproduces it as a first step! Then anyone (such as for example me) could take a stab and producing a fix that is more correct and not just a work-around? This PR doesn't move right now and I'm inclined to close it soon unless we can figure out how to drive it forward. |
Just stumbled upon this old PR as I started hitting this same problem. I remember reading about the workaround at the time, which I added to Janus (where we use libcurl to provide RTSP support) and that worked fine until recently. Today I noticed that RTSP authentication wasn't working anymore (no credential is ever put in the DESCRIBE messages), and digging around I noticed that if on Fedora I downgraded from 7.79.1 (the version currently available on Fedora 35) to 7.78.0 (the previous package) it started working again. As such, this seems to suggest that something in between those two tags broke the workaround, meaning that RTSP authentication is now apparently broken for good @bagder I see that at the time you asked for a test case. Writing a short client that performs the request should be fairly easy as I can extract it from the Janus codebase, so I can contribute that, but my guess is that wouldn't help much without a server to test against. I'm not aware of any publicly available RTSP server that requires authentication, and I suspect that would be needed if this auth functionality needs to be validated automatically as part of a test suite. The server I'm using for my tests is just a basic home RTSP camera I have on my desk: IIRC I also have some gstreamer-rtsp based code that implements a basic RTSP server with authentication as well, but I doubt that might be a viable option for the test suite as that would require importing the full gstreamer suite to build the server example. Please let me know what I can provide to help addressing this. |
RTSP in curl suffers from lack of attention and lack of people caring about it, which makes it fragile and easily a victim of regressions such as this. If we had (more, better, proper) RTSP tests maybe this bug wouldn't have happened. Maybe the best thing to do is start there? To make sure this problem can be reproduced in a curl test case. Then we can work on making the test case pass. |
I agree that a test to reproduce is the first step, but my point was that to actually reproduce this you need:
Without a server, or at least something that mimics one, the problem cannot be reproduced, since you'd never get to the authentication part. That's why I was asking how you think I should proceed in that regard: for instance, how are tests for HTTP or other protocols made in the libcurl test suite at the moment? Is the server simulated, or do the test applications expect a valid server up and running before they're run? I can embed a server as part of the test application, but that would likely require an additional dependency which I'm not sure would be the way to go. The alternative might be crafting a mockup of an RTSP server, but that sounds like a lot of work. |
@bagder just FYI, I wrote a self-contained test and made it available as a repo here: https://github.com/lminiero/rtsp-auth-test Edit: I updated the code to remove the GStreamer dependency. Now for the RTSP server we just spawn a super-dumb TCP socket and check we get what we expect. It only does Basic authentication which should be fine for the test. |
@lminiero Actually digest authentication got broken in libcurl with RTSP before basic authentication did, if I am not mistaken, so handling both later on would be amazing. I might be able to give a hand when I am available but my C/C++ skills are very rusty. |
@Ullaakut this was just a quick hack, so I stuck with Basic authentication since it was trivial to implement, and would provide feedback on whether that was fixed at least or not. That said, I had a look at the curl test suite and saw that there is indeed a test RTSP server in there already, so my dumb server is probably redundant. It's not clear to me how tests can be added to the test suite, though. From the little I could understand, there seems to be a templating mechanism where you specify which test server to launch, and what the client should do, but I haven't understood if it's given for granted that the Once I know more about how a test can be added to the test suite, I'll adapt the code I've written already and submit a PR. |
The test suite is documented in https://github.com/curl/curl/blob/master/tests/FILEFORMAT.md
|
See also for example test 1400, 1405, 1406, 1420, 567, 568, 569 ... All of them in https://github.com/curl/curl/tree/master/tests/data |
area: RTSP
The RTSP protocol got broken in 7.66.0 by changing the way of initialising redir_protocols in url.c. It would still connect to unauthenticated streams, but not to authenticating ones. This pull request fixes that.