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

openssl/3.2.1: Patch for MD5 cert names on Android. #23562

Closed
wants to merge 2 commits into from

Conversation

sizeak
Copy link
Contributor

@sizeak sizeak commented Apr 15, 2024

Specify library name and version: openssl/3.2.1

Android ships CA Certificates with MD5 hash names which OpenSSL stopped supporting in v1.0.0 in favour of canonically encoded SHA1 names (according to openssl/openssl#13565 (comment)). This means that by default you can't use the certificates Android ships with OpenSSL to enable https support with libcurl etc. Instead you would have to include your own CA bundle with your app/library or do some work to copy & rehash the system certificates, neither of which are ideal.

There is a PR open (not mine) with OpenSSL which addresses this problem in a more generic way by allowing the library to fallback to MD5 when no file is found with SHA1, but this PR appears to be slightly contentious and it's not clear if/when this will be merged. Even if it is merged, it will not enable us to use OpenSSL easily on Android right now, with the currently released versions.

This PR provides a patch allowing the use of the older MD5 based names in favour of the SHA1 names when the FORCE_MD5_X509_NAME_HASHES preprocessor definition is detected. Combined with a new option in the recipe, this enables the build to be configured to work with the Android system CA certs without forcing this on anyone already building for Android and using other workarounds.

Hopefully if/when the linked PR is approved, this will no longer be needed, but until then it makes it much easier to get https working on Android with curl etc. (for anyone finding this from a search engine, you also need to point curl at the certs using something like curl_easy_setopt(handle, CURLOPT_CAPATH, "/system/etc/security/cacerts"); )


@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Sorry, the system is under maintenance and it doesn't accept builds right now.

Motivation: Updating Jenkins plugins

Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!

Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

See details:

Sorry, the system is under maintenance and it doesn't accept builds right now.

Motivation: Updating Jenkins plugins

Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!

Copy link
Contributor

🤖 Beep Boop! This pull request is making changes to 'recipes/openssl//'.

👋 @Hopobcn @Croydon you might be interested. 😉

@ghost
Copy link

ghost commented Apr 15, 2024

I detected other pull requests that are modifying openssl/3.x.x recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@jcar87
Copy link
Contributor

jcar87 commented Apr 15, 2024

There is a openssl/openssl#24002 (not mine) with OpenSSL which addresses this problem in a more generic way by allowing the library to fallback to MD5 when no file is found with SHA1, but this PR appears to be slightly contentious and it's not clear if/when this will be merged. Even if it is merged, it will not enable us to use OpenSSL easily on Android right now, with the currently released versions.

Hi @sizeak - thank you for taking the time to research this and propose a solution.

However, and considering the security implications of how OpenSSL is used, if the feature upstream is already contentious and not clear if it would be merged, then it's not good for us to consider accepting it in Conan Center either. The recipes in Conan Center are designed to be built from the source code as it is provided upstream.

Even if it is protected under a non-default option, this will not be checked by our CI. Moving forward we are making every effort that unverified changes, in particular unverified source code changes, make it into the recipes at all.

For this reason I'm afraid we cannot accept this PR. Happy to reconsider if the upstream OpenSSL maintainers consider a fix for this.

I'd strongly advise to present a compelling case to the OpenSSL maintainers, with the intended use case - so that they can evaluate what the best solution is.

Please be aware that with Conan you are able to host your own copy of the recipe, with your own custom changes, to satisfy your team's needs. Please let us know if you would require additional help on this topic.

@jcar87 jcar87 closed this Apr 15, 2024
@jcar87
Copy link
Contributor

jcar87 commented Apr 15, 2024

Added note, from here: https://stackoverflow.com/questions/26935662/openssl-1-0-2-to-read-md5-ca-certificates
Two solutions are proposed:

  • using your own CA bundle, in a way that it does not use the system ones
  • using boringssl - which is actually the library used by Android, IIRC

I'm not very familiar with boringssl, but you may be able to link curl against the system boringssl if this is something provided by the Android NDK

@sizeak
Copy link
Contributor Author

sizeak commented Apr 15, 2024

Yes thank you, I'm aware of those options. My intention with this PR was to make life easier for anyone else needing to do something similar and avoiding the need to host something custom; using https with curl from native code on Android doesn't seem to be a particularly niche use case.

You can't use the system boringssl, it's considered a private system library and since 7.0 the NDK blocks you from using those.

Apps should not use native libraries that are not included in the NDK because they may change or be removed between different versions of Android. The switch from OpenSSL to BoringSSL is an example of such a change.

I understand though that while this change is small and easily verifiable by reading the patch, that a blanket policy is a blanket policy and avoiding source changes makes sense given the recent spate of supply chain attacks.

Thank you for the quick response. We already host our internal Conan packages in an Artifactory instance so it shouldn't be too problematic to host a custom build of OpenSSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants