-
Notifications
You must be signed in to change notification settings - Fork 46
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
v2.1.3 Gremsy Vio camera payload fails to download camera definition due to "unsupported protocol" error #192
Comments
We build curl without ssl, I think it won't support https: https://github.com/mavlink/MAVSDK/blob/main/third_party/curl/CMakeLists.txt#L12 Would you have a way to use http instead? A weird thing in your case is that the camera definition is downloaded from the internet. Shouldn't it be served by the drone? Or is it somehow a simulator setup? |
I tried to add openssl with https support in mavlink/MAVSDK#2386 and I gave up trying to get it building for all platforms. If you want to waste a few days staring at CI playing with dependencies, feel free. I find it utterly frustrating. |
It's a real drone and payload. Nope, they for some reason decided it was a good idea to have the file fetched from Github every time; here's their documentation: https://docs.gremsy.com/payloads/vio/camera-setting-menu/camera-definition-file-download - pretty sure I have no way of changing this behavior...
Oh boy...sorry to hear about that. By any chance, were we able to get it to work on Android at least? |
Pretty sure Android was one of the broken ones: Is the file available with http though? In that case, you can just intercept (but not in Java) the message containing the URL and change Or what we could also try is to just parse |
Whoops it got resolved automatically because I mentioned it in the PR. Let me reopen it. |
Trying to backport it to v2.12 so that we can maybe release it before v3: mavlink/MAVSDK#2454 |
That's unfortunate. Thanks for the info!
Hmm... I don't think Github lets you download using HTTP anymore; here's what happens when I take that URL and do a wget with "HTTP" instead of "HTTPS":
@JonasVautherin Thanks! |
Can you try with mavsdk_server:2.1.4? Just pushed it to MavenCentral. It should support HTTPS |
@JonasVautherin - Thanks for the updated release! With 2.1.4 it does get a bit further, but runs into this issue,
|
Oh yeah, that's a good question: in order to properly leverage HTTPS, curl (openssl?) needs to have access to certificates. Usually they are found on the system, but I honestly don't know how we should do that with our static library 🤔. An easy way would be to disable the verification (see e.g. here). We could justify it by saying that downloading a wrong camera_definition.xml is not a security issue 😅. Ideally we would have a way to feed certificates to mavsdk_server though? Would you mind looking into libcurl to see if there is a way to give it certificates? Apparently the executable has some options for that (see here), but in our case we use libcurl (not the executable). The next question would be whether we can read system certificates on Android... I had not realized before, but it isn't as easy as just enabling https in curl 🙈. EDIT: here I see stuff like this:
So if we had read access to the system certificates on Android, we could maybe just use that? I checked on my device and I have certificates in |
Oh maybe we could just set CURLOPT_CAPATH to And it seems like this answer does it at build time!
|
@rayw-dronesense: would you be able to try this PR? mavlink/MAVSDK#2459 |
@JonasVautherin - Trying now... |
@JonasVautherin - I guess it makes it further now... but,
|
Gonna try doing a clean and rebuild for good measure. |
Does your device have /system/etc/security/cacerts? |
Yes it does,
|
Update: clean and rebuild didn't change anything. Still got |
Just for fun I tried disabling SSL verification per the above post...and that got it past the download definition no problem. But
|
Yeah okay, looks like the error is correct - it's getting an empty string for some reason.
The output is empty,
|
Got it. Also had to enable "Follow Location" to allow it to handle redirects,
Then it started to work,
So now it's just a matter of having the certs validate the proper way I guess |
Did you try with CURLOPT_SSL_VERIFYPEER=1 and CURLOPT_FOLLOWLOCATION=1? Maybe the error "SSL peer certificate or SSH remote key was not OK" happens on the empty string because the redirection was not followed? |
Just tried it. Same |
Just to be sure: are you sure that your Android device has up-to-date certificates? I.e. it's not an Android system from 6 years ago that never received updates? 😅 |
Actually, it is an Android system from 6 years ago - it's a Herelink controller running Android 7.1.2. Just to verify however, I did try loading up Chrome.apk on there and tried the Github page and it loaded fine there. |
Haha, yeah I was a bit suspicious of that 🙈😅
The problem here is that Chrome does not necessarily use the system certificates; I could imagine that they embed their own (which is good in your case because otherwise Chrome wouldn't work properly on the Herelink, I think). Would you be able to run the test on a more up-to-date Android system? It would be interesting to see if we have a bug in our solution or if the problem is Herelink's outdated certificates 🤔. Thanks a lot for all the testing, by the way! |
Unfortunately, I don't have a way to connect the drone to anything else 😢 |
I had initially posted earlier that running the logic on S23 independent of drone worked. However, that was too early to call, because I had forgotten to remove the "disable SSL verification" flag before running the test. Will re-test it with flag removed. Apologies for the confusion. |
Yeah, after removing the "disable verification" flag, running on S23 results in the same error as before,
Some contents from my original post:
I guess that means, this use case could be instrumentally tested.... 😉 |
@rayw-dronesense sorry, I'm not quite following. Are you saying we have the same problem on the S23, so this is not a Herelink problem as such? |
I was able to test the download logic specifically on the S23 without needing a drone and the same certificate related error occurs there. |
Just to be sure again: you tested on the S23 with both Since you have a testable setup, may I still ask you to try with an invalid path (say "/system/etc/security/cace") and a valid path to something that doesn't contain certificates (say "/system/etc/security/")? Just to see if we get a different error message. The error comes from https://github.com/curl/curl/blob/c948971e83f8673342de28691b4e7b6fd9bd670d/lib/strerror.c#L212 and is defined here:
|
Yes
Here's what happens with the "invalid path" case:
Here's what happens with the "doesn't contain certificates" case:
|
For science, tried changing the URL to "google.com" and here's what happened with that,
|
Hmm... so it's just not enough to build with "-DCURL_CA_PATH=/system/etc/security/cacerts"... |
Feels similar to this, but I don't know the situation with openssl 3 now... 😕 |
@rayw-dronesense: it feels like maybe using boringssl instead of openssl may help... I'm trying to build here: mavlink/MAVSDK#2460. Feel free to try it (assuming that the android build passes in the CI) 👍. EDIT: it does build for Android! Would you mind trying that? 🤞 |
@JonasVautherin - I pulled down the branch with the boringssl changes and did a clean build. Unfortunately, same error,
Using S23. |
I pushed up the test changes as well in case you guys wanted to try it on your end too: mavlink/MAVSDK#2462 It can be tested on any Android device the way it is hardcoded. |
Wait, that was with boringssl and with the curl path? I think we may need both mavlink/MAVSDK#2460 and mavlink/MAVSDK#2459? 🤔 Because it feels like "Problem with the SSL CA cert (path? access rights?)" was solved by 2459 (by building curl with the path to cacerts) |
I'm merging in the changes from mavlink/MAVSDK#2459 and trying again |
@JonasVautherin Great news! That worked on both the S23 and the Herelink controller
And also the logic when it kicks in during camera loading,
|
Exact diff can be found in the PR: https://github.com/mavlink/MAVSDK/pull/2462/files |
Oh that's great! Thanks a lot for testing all that 😁 |
For v3, I think it makes sense to update the dependencies, get https working properly, etc. For v2, I'm tempted to just release this hack: mavlink/MAVSDK#2471. Any chance you could test that PR @rayw-dronesense? |
@rayw-dronesense given my hack attempts didn't work, I'm going to give up on https for v2, and move on to v3 where we have Openssl/Boringssl working. |
The text was updated successfully, but these errors were encountered: