Fix issue 19665 - UWP curl https require to define CURLOPT_CAINFO

[Chicopower]

Description

HTTPS connection to addons repository is not working on Matrix UWP-64 builds.

Motivation and context

Fix a bug stopper to eventually publish Kodi 19.x (Matrix) to Xbox platform.
Fix the issue #19665

How has this been tested?

Using my Xbox Series X in developer mode with Kodi 19.1 latest master branch code

What is the effect on users?

With this fix, Xbox and Windows 10 UWP users will now be able to download/update/install addons with Kodi 19.1

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[thexai]

That is great news. The best part is don't need to recompile curl / OpenSSL or bump versions.

For me it looks good and makes sense because this curl option CURLOPT_CAINFO is already used in Windows (desktop) to set a custom path for CA certs file through advanced settings.

I don't know the reason why we are using environment variables for this. In Windows UWP environment variables are very limited and tricky since processes cannot access environment variables of other processes or of the system. For the same reason in UWP I suppose that the full path does not work since the App cannot access the folders that are outside of where it is installed. On the other hand, the relative path does work.

However I leave the approval for @wsnipex and @Paxxi who know more about curl and maybe can clarify something about the reason to use environment variables.

EDIT:
I have tested it on Windows 10 UWP and it works but I have to change system/certs/cacert.pem with system\\certs\\cacert.pem

I think #include <xbmc/platform/Environment.h> is not needed.

[Paxxi]

Looks good. Thank you!

I don't know the history but I would guess that curl didn't have an option to set the ca bundle on some platform so the environment variable was the only way. Since it's mostly worked nobody has thought about it.

[wsnipex]

Please note that this overrides the advanced setting directly above and won't work for ffmpeg https connections(needs to set ca_file with av_opt_set ()) http://ffmpeg.org/doxygen/4.1/structTLSShared.html#a1a0a58119cfdd8b73bc43ace954e8496

Do you see a possibility to make the advancedsetting work?

[Paxxi]

Oh right, good catch @wsnipex
So this code should be moved above the advanced setting so the setting can override the defaults.

We should be able to do the same thing where we call av_opt_set I guess but I would have to look into how the code looks like today to be able to give a definitive answer.

[Paxxi]

Move line 683-686 before line 680 to allow the advanced setting to override the default value.

[wsnipex]

we also need some faculty to make sure the advanced setting points to a "usable" path. Does it really need to be relative or does it have to be in a specific dir that UWP apps can access(like on e.g. android)? What about path spec (\ vs /)

[thexai]

@Chicopower

On Windows directory separator is backslash character \ . The reason of use double backslash is that c++ use \ as scape sequence by default, then double \\ is interpreted as \.

Test with "system\\certs\\cacert.pem" on Xbox and should work same as on Windows 10 UWP and avoids possible incompatibility problems. Also test removing #include <xbmc/platform/Environment.h> line.

Some tips for git:

Now there are two commits in this branch.
You can remove latest one with command git reset --hard HEAD~1 (executed from the path where you have Kodi code).
Then edit code with Visual Studio and modify same commit checking box "Amend Last Commit" (assuming you are using git GUI)

Then push again checking the box "Force overwrite existing branch"

You can repeat this process as many times as you need but you should be careful not to do it while Jenkins is compiling the same PR (Jenkins does not like that and usually stops compiling the PR until it is invoked manually).

[Chicopower]

@Paxxi and @wsnipex Your recommended changes to keep advanced setting override are now done in this pull request. Make sense to keep ffmpeg https working! Thx for your help 👍

@thexai Thx for the git tip and your proposed improvement! I applied your recommendations. I can confirm your improvement (system\certs\cacert.pem) work on my Xbox 👍

General Notes for Curl UWP knowledge

TestCase 1:
If i set an invalid path to CURLOPT_CAINFO (example: S:\Program Files\WindowsApps\XBMCFoundation.KodiXbox_xxxversionxxx_xxxguidxxx\system\certs\cacertnotworking.pem), Curl will produce this error in Kodi logs:
DEBUG : Curl::Debug - TEXT: error setting certificate verify locations:
DEBUG : Curl::Debug - TEXT: CAfile: S:\Program Files\WindowsApps\XBMCFoundation.KodiXbox_xxxversionxxx_xxxguidxxx\system\certs\cacertnotworking.pem
DEBUG : Curl::Debug - TEXT: CApath: none

TestCase 2:
If i set a valid full path to CURLOPT_CAINFO (example: S:\Program Files\WindowsApps\XBMCFoundation.KodiXbox_xxxversionxxx_xxxguidxxx\system\certs\cacert.pem), Curl is working fine.

TestCase 3:
If i set a valid relative path to CURLOPT_CAINFO (example: system\certs\cacert.pem), Curl is working fine.

TestCase 4:
Current Kodi Curl version is working with this proposed UWP fix.
I tried using the latest Curl release (curl 7.77.0_2 statically linked for Windows 64 bit) and everything is fine for Kodi Xbox UWP.

Then, my last tests conclude that the relative path to cacert is not required for Curl running on UWP.
But, setting the CURLOPT_CAINFO with a valid cacert file path appear to be required on UWP.
I adjusted by code comment for this UWP fix :)

[thexai]

I have tried on my side with absolute path and it has not worked (Windows 10 UWP debug mode). The idea was to use CSpecialProtocol::TranslatePath("special://xbmc/system/certs/cacert.pem") to remove hardcoded path.

The path returned is correct: "T:\KODI\kodi-build-UWP\Debug\AppX\system\certs\cacert.pem" and file exist here but for some reason it doesn't work.

It also doesn't work in Release mode from C:\Program Files...

Perhaps the issue with environment variables is not that it does not work itself but that the path that is passed is not valid to be used in UWP (https://github.com/xbmc/xbmc/blob/master/xbmc/platform/win10/PlatformWin10.cpp#L25-L31). Although the current environment variable code in UWP doesn't inspire much confidence as it mixes ANSI and wide string system calls (https://github.com/xbmc/xbmc/blob/master/xbmc/platform/win10/Environment.cpp).

EDIT:
Also getenv (https://github.com/xbmc/xbmc/blob/master/xbmc/platform/win10/Environment.cpp#L39) can not be used on UWP apps: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-160

This API cannot be used in applications that execute in the Windows Runtime. For more information, see CRT functions not supported in Universal Windows Platform apps.

In any case, it seems then that the simplest and safest way for now is to use relative path (PR as it is now) as it works on Xbox debug/release mode and Windows 10 UWP-64 debug/release.

@Paxxi please check the PR with the latest changes to see if everything is correct now. I believe that your approval is necessary again to do things well.

[Paxxi]

Looks good to me.
The "hardcoded" path is the same wether we translate it using CSpecialProtocol or not so I don't see that as an issue.

We'll have to investigate the changes required to set the proper cert for ffmpeg as well but that's a separate PR, this solves a lot of issues so we should get this merged ASAP imo

[thexai]

@Chicopower, thank you very much for your contribution