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

Added CURLPROTO_RTSP to redir_protocols #4750

Closed
wants to merge 320 commits into from
Closed

Added CURLPROTO_RTSP to redir_protocols #4750

wants to merge 320 commits into from

Conversation

janssen70
Copy link

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.

@jay
Copy link
Member

jay commented Dec 24, 2019

Actually that change was intentional for security reasons. You can enable RTSP on redirect by using libcurl's CURLOPT_REDIR_PROTOCOLS or the curl tool's --proto-redir. I am not familiar with RTSP though so I'll leave this open to gather feedback.

@jay jay added the RTSP label Dec 24, 2019
@janssen70
Copy link
Author

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.

@jay
Copy link
Member

jay commented Dec 24, 2019

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)?

@janssen70
Copy link
Author

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

@jay
Copy link
Member

jay commented Dec 28, 2019

Is there a public URL that we can use to reproduce this?

@janssen70
Copy link
Author

It's more complicated than I thought... I made two streams available:

rtsp://philea.ddns.net:50003/axis-media/media.amp -> Unauthenticated RTSP stream
rtsp://philea.ddns.net:50004/axis-media/media.amp -> Authenticated RTSP stream, credentials curl:curl

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

@janssen70
Copy link
Author

It's difficult to make sufficient time for it. I'll be back :)

@bagder
Copy link
Member

bagder commented Jan 17, 2020

Someone fell over this bug on stackoverflow as well.

MarcelRaad and others added 16 commits January 20, 2020 09:44
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
For now, no cert in the bundle actually sets a date there...

Co-Authored-by: Jay Satiro
Reported-by: Christian Heimes
Fixes #4834
Closes #4836
- 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.
Avoid "reparsing" the content and instead deliver more exactly what is
provided in the certificate and avoid truncating the data after 512
bytes as done previously. This no longer removes embedded newlines.

Fixes #4837
Reported-by: bnfp on github
Closes #4841
Only ever used from within this file.
(and the corresponding unit test)

Closes #4842
@janssen70
Copy link
Author

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

konimex and others added 7 commits March 11, 2020 10:15
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
@bagder
Copy link
Member

bagder commented Mar 20, 2020

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.

@Haffon
Copy link

Haffon commented May 6, 2020

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.

@bagder
Copy link
Member

bagder commented May 6, 2020

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 CURLOPT_REDIR_PROTOCOLS for that purpose. This is a bug, but setting this bit is not the correct fix!

@bagder
Copy link
Member

bagder commented May 8, 2020

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.

@lminiero
Copy link
Contributor

lminiero commented Jul 14, 2022

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 🙁 I tried having a look at the changes and diff but unfortunately my knowledge of the curl internals is very limited to say the least, so I wouldn't know where to start looking.

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

@bagder
Copy link
Member

bagder commented Jul 14, 2022

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.

@lminiero
Copy link
Contributor

I agree that a test to reproduce is the first step, but my point was that to actually reproduce this you need:

  1. a simple RTSP client based on libcurl
  2. a simple RTSP server that requires authentication

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.

@lminiero
Copy link
Contributor

lminiero commented Jul 15, 2022

@bagder just FYI, I wrote a self-contained test and made it available as a repo here: https://github.com/lminiero/rtsp-auth-test
It's very likely not what you're looking for, since GStreamer as a dependency for the RTSP server part would be overkill for the libcurl test suite, but I think it's a good starting point for something lighter. As it is, it can already be used to replicate the problem, as in my case it works as expected in 7.78.0 and fails in 7.79.1.

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.

@Ullaakut
Copy link

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

@lminiero
Copy link
Contributor

lminiero commented Jul 22, 2022

@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 curl executable will be used always, or if there are custom clients like the quick hack I wrote that will be built automatically. If it's the former, I honestly don't know if you can use the curl executable for RTSP links as well: I always assumed you needed to write the logic for that using curl as a library, but I may be wrong.

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.

@dfandrich
Copy link
Contributor

dfandrich commented Jul 23, 2022 via email

@bagder
Copy link
Member

bagder commented Jul 23, 2022

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

@bagder bagder removed the needs-info label Jul 23, 2022
@lminiero
Copy link
Contributor

lminiero commented Sep 6, 2022

@bagder thanks for the info, and apologies for the awfully late reply! (summer vacations in the middle)
I'll have a look at those examples and try to come up with one for RTSP auth too soon.

@lminiero
Copy link
Contributor

lminiero commented Sep 7, 2022

As promised, I worked on a test, that you can find linked above. Please let me know if that's indeed what you were looking for, or if the plan was for something different.

@janssen70
Copy link
Author

As it is, it can already be used to replicate the problem, as in my case it works as expected in 7.78.0 and fails in 7.79.1.

I upgrade my curl from 7.66 to 7.86 today. Bit late :(
RTSP Authentication failed but my problem was different. binary searching through versions I had no issue with 7.79.1 built from source. That one still Digest-authenticated without problem, but rtsp of course needs to be added to CURLOPT_REDIR_PROTOCOLS. Tested on Windows and Linux

I see a problem here:
There was a change from 7.82 to 7.83: in http.c Curl_allow_auth_to_host() was added, and it performs a stricter check than the previous inline version. When I make it equivalent to the check from 7.82, RTSP authenticates fine again.

In 7.86.0 that code is in lib/vauth/vauth.c and making the same change makes 7.86.0 successfully do Digest authenticate on RTSP. The problem is in:
(data->state.first_remote_port == conn->remote_port) &&
(data->state.first_remote_protocol == conn->handler->protocol)));

The root cause seems to me these state fields are not set by rtsp.c. I am clueless where to set them, and I shouldn't, I don't understand their purpose yet...

@bagder
Copy link
Member

bagder commented Oct 27, 2022

@janssen70 commenting on an old closed pull request is next to useless.

RTSP auth remains (semi-)broken because nobody works on fixing it. It "worked" previously because the code was using a "hack" that accidentally worked but when the redirect logic was fixed it stopped working. And we had no test cases that detected it so it took a while to realize.

#9449 adds a test that confirms the problem but there is no fix to my knowledge and I don't know about anyone working on it.

bagder added a commit that referenced this pull request Nov 8, 2022
Verified with test 3100

Fixes #4750
Closes #....
bagder added a commit that referenced this pull request Nov 9, 2022
Verified with test 3100

Fixes #4750
Closes #9870
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet