-
Notifications
You must be signed in to change notification settings - Fork 144
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
Update to OpenSSL 1.1.1g #692
Conversation
Switch from using Marcin's OpenSSL [1] to our own build of OpenSSL [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/krzyzanowskim/OpenSSL [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a binary-only framework. It will be downloaded from GitHub instead of building it from source. This is not much different from what the previous vendor did, but is more stable. Carthage builds use the static flavor of the framework. We have run into issues with dynamic framworks of OpenSSL when using Carthage, but static frameworks seems to do very good job: the resulting binaries are smaller, apps start a bit faster, and users are freed from the hassle of dealing with OpenSSL linkage to their app. Note that due to the way static linkage works, we will be exporting all OpenSSL symbols from ObjCThemis by default. In order to avoid conflicts, export only limited subset of symbols: Objective-C classes of ObjCThemis.
Switch from using Levi Groker's OpenSSL [1] to our own build [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/levigroker/GRKOpenSSLFramework [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a tricky pod, but for consumers like Themis it's just a pod. Introduce a separate subspec for the build with newer OpenSSL, and make it the default choice. We keep the old specs around in case someone needs them to share GRKOpenSSL or BoringSSL with other dependencies, as it is not possible to use CLOpenSSL simultaneously with them due to OpenSSL symbol conflicts. The new subspec has its oddities, but it's all (un)known magic that seems to be absolutely necessary to build Themis properly for iOS.
Well, of course it fails! I don't have mental capacity to deal with it today, or I'm afraid I'll need a new Mac and some painting to hang on the wall which would have taken the hit. |
My testing results: themis-cocoapods works on iOS sims and devices; themis-carthage shows error during installation. Details in slack. |
In Carthage builds, we need to export not only Objective-C classes from TS namespace but global functions as well. (Well, there is only one such function right now: TSGenerateSymmetricKey().)
Okay... One issue was in that I cherry-picked my old commit from the experimental branch which implemented exported symbols list, and that list did not include recently added |
Another issues that are around:
which are preceded by
which suggests that the OpenSSL was not compiled for some architectures?.. |
Previous Bitrise failure seems to have been caused by Bitrise using Xcode 10.3.x which cannot parse bitcode produce by my Xcode 11.2 used to build OpenSSL. Given than Xcode 11.6 is the current stable (= supported by Apple), I guess it's justified that we bump the requirements to Xcode 11.x. I have asked Bitrise to use Xcode 11.0.x on macOS 10.14 for builds from now on. Also, merge conflicts prevented other tasks on Bitrise from finishing successfully. I have synced this PR with master, pushed. This should (🤞) fix Bitrise. It also triggers GitHub Actions build. (No idea why changing |
It seems that Xcode 10 cannot handle bitcode data embedded into prebuilt OpenSSL frameworks we distribute which were built with Xcode 11.2. Make Xcode new minimum requirement for ObjCThemis and SwiftThemis. Xcode 11.0 is known to work.
Turns out that the spell we used before is not effective. sed does not understand "\s" so replace it with "[ ]" to match the leading spaces. Now the podspec should be correctly edited for linting and validate as if the current commit is going to be published. (The hack will be disabled for production branches where we are linting the spec as is.)
Fixed up the “version hack” used for CocoaPods spec linting. It turned out that the previous version of the hack (added in #610) was not effective and did not actuall update the spec file. Now it is properly updated with the current commit. This should make the last broken build on GitHub Actions go green. |
The last issue – Carthage failure about missing architectures – seems to be caused by indeed missing arm64e slice in OpenSSL binaries. The package will be updated shortly to contain it. |
Okay, the following PRs resolve issues found:
The |
Make sure that ObjCThemis is compiled for arm64e architecture as noted in Apple guidance [1]. This is important for the case when our users would like to test teeir apps with this architecture. Currently, Xcode does not build slices for this architecture by default, so Carthage builds will not include it. [1]: https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication
There is another (final final final) change, adding support for arm64e to Xcode projects built by Carthage. This ensures that Carthage will built this architecture even if Xcode does not select it by default (e.g., Xcode 11 does not). Otherwise missing architecture in dependencies may prevent apps from being tested with arm64e. (As noted above, we already build OpenSSL for arm64e as well.) |
Here we're waiting for me to test these changes on arm64e real device (iPhone XS) tomorrow, then we'll either merge this PR or decide what to do next. |
It seems that in some cases bitcode gets disabled in Xcode. Make sure that our project files keep it enabled at all times.
We have encountered some more compilation issues related to bitcode support in Carthage for arm64e. Trying to enable it as a last ditch effort to make it work. If it doesn't, we'll probably revert recent changes. |
It seems that ObjCThemis does not support arm64e very well, after all. Let's not mention it in the changelog. Maybe the situation will impove before 0.14 is out though.
Current state of things:
|
In any case, I think it's too early for us to claim full arm64e support in all use cases so I have removed those lines from the changelog (and resolved merge conflicts with master). This should the really final state of this PR 🤞 |
* Use cossacklabs/openssl-apple for Carthage Switch from using Marcin's OpenSSL [1] to our own build of OpenSSL [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/krzyzanowskim/OpenSSL [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a binary-only framework. It will be downloaded from GitHub instead of building it from source. This is not much different from what the previous vendor did, but is more stable. Carthage builds use the static flavor of the framework. We have run into issues with dynamic frameworks of OpenSSL when using Carthage, but static frameworks seems to do very good job: the resulting binaries are smaller, apps start a bit faster, and users are freed from the hassle of dealing with OpenSSL linkage to their app. Note that due to the way static linkage works, we will be exporting all OpenSSL symbols from ObjCThemis by default. In order to avoid conflicts, export only limited subset of symbols: Objective-C classes of ObjCThemis. * Use cossacklabs/openssl-apple for CocoaPods Switch from using Levi Groker's OpenSSL [1] to our own build [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/levigroker/GRKOpenSSLFramework [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a tricky pod, but for consumers like Themis it's just a pod. Introduce a separate subspec for the build with newer OpenSSL, and make it the default choice. We keep the old specs around in case someone needs them to share GRKOpenSSL or BoringSSL with other dependencies, as it is not possible to use CLOpenSSL simultaneously with them due to OpenSSL symbol conflicts. The new subspec has its oddities, but it's all (un)known magic that seems to be absolutely necessary to build Themis properly for iOS. * Note the update in CHANGELOG * Export global functions as well In Carthage builds, we need to export not only Objective-C classes from TS namespace but global functions as well. (Well, there is only one such function right now: TSGenerateSymmetricKey().) * Note Xcode 11 requirement in CHANGELOG It seems that Xcode 10 cannot handle bitcode data embedded into prebuilt OpenSSL frameworks we distribute which were built with Xcode 11.2. Make Xcode new minimum requirement for ObjCThemis and SwiftThemis. Xcode 11.0 is known to work. * Improve CocoaPods version hack for linting Turns out that the spell we used before is not effective. sed does not understand "\s" so replace it with "[ ]" to match the leading spaces. Now the podspec should be correctly edited for linting and validate as if the current commit is going to be published. (The hack will be disabled for production branches where we are linting the spec as is.) * Change comments to trigger CI (so high-tech) * Add arm64 to default architectures list Make sure that ObjCThemis is compiled for arm64e architecture as noted in Apple guidance [1]. This is important for the case when our users would like to test their apps with this architecture. Currently, Xcode does not build slices for this architecture by default, so Carthage builds will not include it. [1]: https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication * Explicitly enable bitcode for Carthage It seems that in some cases bitcode gets disabled in Xcode. Make sure that our project files keep it enabled at all times. * Do not claim arm64e support in changelog It seems that ObjCThemis does not support arm64e very well, after all. Let's not mention it in the changelog. Maybe the situation will improve before 0.14 is out though. (cherry picked from commit 6dacf9c)
* Avoid overflows on 32-bit systems (#677) * Avoid overflows in Secure Cell Themis Core C API works with buffer sizes expressed as "size_t" while in Go lengths are expressed as "int". Themis containers can typically contain up to 4 GB of data with internal length fields using "uint32_t". On typical 64-bit systems this does not cause overflows since uint32_t fits into both Go's int and C's size_t. However, on 32-bit system this can cause overflows. There, size_t is unsigned 32-bit value identical to uint32_t while int is 32-bit signed value, so the size may not fit into Go's size range. We can't do anything about that. On 32-bit systems the buffer sizes are typically limited to 2 GB anyway due to the way memory is distributed. However, if the overflow happens, Go will panic when trying to allocate (effectively) negatively-sized arrays. We should return an error instead. Add size checks before casting "C.size_t" into "int" and return an error if the size will overflow. Do this for all API, both new and old. Normally, Themis is not used to encrypt real 2+ GB messages, but this condition can easily happen if the data has been corrupted where the length field is stored. We don't want this to be a source of DOS attacks. * Reenable tests for corrupted data The panic condition has been originally detected by a couple of tests for Secure Cell's Token Protect mode which has the stars properly aligned for the issue to be visible. Now that the issue is fixed, we can enable these tests for 32-bit machines again. * Avoid overflows in Secure Compartor * Avoid overflows in key generation * Avoid overflows in Secure Message * Avoid overflows in Secure Session Just like Secure Cell, add more checks to other cryptosystems as well. Unfortunately, we have to duplicate the size check utility. GoThemis does not have a common utility module, and even if it did, it would not work due to the way CGo is implemented ("C.size_t" is a distinct type in different modules). (cherry picked from commit 8b83a71) * Publish source and Javadoc JARs (#679) Add tasks and artifacts for publishing JARs with source code and generated Javadocs. These are required for admission to Bintray's JCenter -- well-known public repository of open-source libraries. With this, the users will be able to import Themis without adding Cossack Labs repository. It's highly likely that they already have JCenter added for other dependencies: repositories { jcenter() } Since it's the same package, the dependency is specified as before: dependencies { implementation 'com.cossacklabs.com:themis:0.13.0' } Many Bothans died to bring us this information. No, really, Gradle documentation on this is less than stellar, and Groovy does not make it make it easier. *sigh* Android ecosystem vOv (cherry picked from commit 2344484) * Bump lodash from 4.17.15 to 4.17.19 (#680) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.15 to 4.17.19. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.15...4.17.19) Signed-off-by: dependabot[bot] <[email protected]> (cherry picked from commit 959e6d4) * Add missing OpenSSL includes (#684) * Add missing OpenSSL includes Add those files use BIGNUM API of OpenSSL but do not include relevant headers. Due to miraculous coincidence, this seems to somehow work for the OpenSSL versions we use, but only because either existing headers include this "bn.h" transitively, or because the compiler generates code that kinda works without function prototype being available. However, curiously enough, this breaks when building Themis for macOS with recent OpenSSL 1.1.1g but not with OpenSSL 1.0.2, or OpenSSL 1.1.1g on Linux. The issue manifests itself as missing "_BN_num_bytes" symbol. Indeed, there is no such symbol because this function is implemented as a macro via BN_num_bits(). However, because of the missing header, the compiler -- being C compiler -- decides that this must be a function "int BN_num_bytes()" and compiles it like a function call. Add the missing includes to define the necessary macros and prototype, resolving the issue with OpenSSL 1.1.1g. It must have stopped including <openssl/bn.h> transitively, revealing this issue. This is why you should always include and import stuff you use directly, not rely on transitive imports. P.S. A mystery for dessert: BoringSSL backend *includes* <openssl/bn.h>. * Treat warnings as errors in Xcode In order to prevent more silly issues in the future, tell Xcode to tell the compiler to treat all warnings as errors. That way the build should fail earlier, and the developers will be less likely to ignore warnings. * Fix implicit cast warnings Now that we treat warnings as errors, let's fix them. themis_auth_sym_kdf_context() accepts message length as "uint32_t" while it's callers use "size_t" to avoid early casts and temporary values. However, the message length has been checked earlier and will fit into "uint32_t", we can safely perform explicit casts here. * Suppress documentation warnings (temporarily) Some OpenSSL headers packaged with Marcin's OpenSSL that we use have borked documentation comments. This has been pointed out several times [1][2], but Marcin concluded this needs to be fixed upstream. [1]: krzyzanowskim/OpenSSL#79 [2]: krzyzanowskim/OpenSSL#41 Meanwhile, having those broken headers breaks the build if the warnings are treated as errors. Since we can't upgrade Marcin's OpenSSL due to other reasons (bitcode support), we have no hope to resolve this issue. For the time being, suppress the warnings about documentation comments. * Fix more implicit cast warnings There are more warnings actual only for 32-bit platforms. Some iOS targets are 32-bit, we should avoid warnings there as well. The themis_scell_auth_token_key_size() and themis_scell_auth_token_passphrase_size() functions compute the size of the autentication token from the header. They return uint64_t values to avoid overflows when working with corrupted input data on the decryption code path. However, they are also used on the encryption path where corruption is not possible. Normally, authentication tokens are small, they most definitely fit into uint32_t, and this is the type used in Secure Cell data format internally. It is not safe to assign arbitrary uint64_t to size_t on 32-bit platforms. However, in this case we are sure that auth tokenn length fits into uint32_t, which can be safely assigned to size_t. Note that we cast into uint32_t, not size_t. This is to still cause a warning on platforms with 16-bit size_t (not likely, but cleaner). (cherry picked from commit 1ca96de) * Update to OpenSSL 1.1.1g (#692) * Use cossacklabs/openssl-apple for Carthage Switch from using Marcin's OpenSSL [1] to our own build of OpenSSL [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/krzyzanowskim/OpenSSL [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a binary-only framework. It will be downloaded from GitHub instead of building it from source. This is not much different from what the previous vendor did, but is more stable. Carthage builds use the static flavor of the framework. We have run into issues with dynamic frameworks of OpenSSL when using Carthage, but static frameworks seems to do very good job: the resulting binaries are smaller, apps start a bit faster, and users are freed from the hassle of dealing with OpenSSL linkage to their app. Note that due to the way static linkage works, we will be exporting all OpenSSL symbols from ObjCThemis by default. In order to avoid conflicts, export only limited subset of symbols: Objective-C classes of ObjCThemis. * Use cossacklabs/openssl-apple for CocoaPods Switch from using Levi Groker's OpenSSL [1] to our own build [2] which provides newer OpenSSL 1.1.1g, bitcode, and other goodies. [1]: https://github.com/levigroker/GRKOpenSSLFramework [2]: https://github.com/cossacklabs/openssl-apple The new OpenSSL is distributed as a tricky pod, but for consumers like Themis it's just a pod. Introduce a separate subspec for the build with newer OpenSSL, and make it the default choice. We keep the old specs around in case someone needs them to share GRKOpenSSL or BoringSSL with other dependencies, as it is not possible to use CLOpenSSL simultaneously with them due to OpenSSL symbol conflicts. The new subspec has its oddities, but it's all (un)known magic that seems to be absolutely necessary to build Themis properly for iOS. * Note the update in CHANGELOG * Export global functions as well In Carthage builds, we need to export not only Objective-C classes from TS namespace but global functions as well. (Well, there is only one such function right now: TSGenerateSymmetricKey().) * Note Xcode 11 requirement in CHANGELOG It seems that Xcode 10 cannot handle bitcode data embedded into prebuilt OpenSSL frameworks we distribute which were built with Xcode 11.2. Make Xcode new minimum requirement for ObjCThemis and SwiftThemis. Xcode 11.0 is known to work. * Improve CocoaPods version hack for linting Turns out that the spell we used before is not effective. sed does not understand "\s" so replace it with "[ ]" to match the leading spaces. Now the podspec should be correctly edited for linting and validate as if the current commit is going to be published. (The hack will be disabled for production branches where we are linting the spec as is.) * Change comments to trigger CI (so high-tech) * Add arm64 to default architectures list Make sure that ObjCThemis is compiled for arm64e architecture as noted in Apple guidance [1]. This is important for the case when our users would like to test their apps with this architecture. Currently, Xcode does not build slices for this architecture by default, so Carthage builds will not include it. [1]: https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication * Explicitly enable bitcode for Carthage It seems that in some cases bitcode gets disabled in Xcode. Make sure that our project files keep it enabled at all times. * Do not claim arm64e support in changelog It seems that ObjCThemis does not support arm64e very well, after all. Let's not mention it in the changelog. Maybe the situation will improve before 0.14 is out though. (cherry picked from commit 6dacf9c) * Update CocoaPods repository (#673) After restoring CocoaPods from cache we also need to fetch the latest updates which were not cached yet. This is not noticeable most of the time, but otherwise it breaks builds right after a new version of Themis is released on CocoaPods and we can't fetch it because the repo is not updated yet. (cherry picked from commit 89304e5) * GoThemis README (#699) pkg.go.dev does not look at the repository README, it needs a file (not a symlink) near the "go.mod" file. Use a short README for Go, similar to the one used by RustThemis, to minimize maintenance effort. Note that pkg.go.dev cannot resolve relative links to files outside of the Go module directory ("gothemis") so we can't link to examples and license like that. Instead, direct links to the master branch of the repository are provided. It's not optimal, but a good compromise. (cherry picked from commit 5d9d5ae) * Themis 0.13.1 (#697) * Bump selected versions to 0.13.1 * Cut Themis 0.13.1 in changelog * Remove ObjCThemis.xcodeproj (#704) * Remove ObjCThemis.xcodeproj The idea behind building "objcthemis.framework" has been to unify import syntax between Carthage and CocoaPods. Unfortunately, it turned out to be a mistake. "objcthemis.framework" does not work without "themis.framework" being present alongside it because of how module resolution works. Despite "objcthemis.framework" providing the same "themis" module as "themis.framework", the compiler will look for a framework named "themis.framework" when resolving "import themis". Moreover, the original issue that "objcthemis.framework" has been called to rectify can be resolved more elegantly by importing the module: @import themis; which work well with "themis.framework" in both Carthage and CocoaPods. Since "objcthemis.framework" does not bring any value, remove it. Move all new things added to ObjCThemis.xcodeproj into Themis.xcodeproj (such as testing Swift 4 vs 5). Remove the import warning. Now Carthage will build only one framework: "themis.framework" from Themis.xcodeproj. I am sorry for the trouble and confusion of this fizzled migration. * Change "product name" to "themis" Make sure that Xcode targets produce "themis.framework", not "objcthemis.framework". * Recreate Xcode schemes It seems that some components stick the schemes after renaming. Recreate them to make sure that we're building "themis.framework" and there are no traces of the old Xcode project. * Bring back proxy umbrella header "themis.h" Since the framework is named "themis.framework", its umbrella header is expected to be called "themis.h". The actual umbrella header for ObjCThemis is "objcthemis.h" which we simply include. * Use alternative imports in unit tests One of the reasons for "objcthemis.framework" existence was to run ObjCThemis unit tests from Xcode. Initially, "themis.framework" has prevented that due to import issues, and "objcthemis.framework" has allowed #import <objcthemis/objcthemis.h> to work. Now that latter is gone, the unit-tests are broken again. However! It seems that using modular imports works for Xcode and Carthage (which uses Xcode project). The bad news here is that it *does not* work for CocoaPods, which still works only with the old form because CocoaPods does some special wicked magic with headers, putting them into the "objcthemis" directory. I do not have much time and willingness to deal with this stupidity anymore right now, so here's a compromise: Carthage uses its form, CocoaPods use their form, and you get this TODO to maybe get rid of this wart some time later. (cherry picked from commit 5522ace) * Themis 0.13.2 (#705) Hotfix for Carthage, removing dysfunctional ObjCThemis.xcodeproj. * Hotfix for Themis CocoaPods podspec: support Xcode12 (#719) * update podspec to fix builds on xcode12 * ok, use 0.13.2 as podspec version * set ios deployment target to 10.0 for cocoapods projects * update themis podspec for 0.13.3 tag Co-authored-by: Alexei Lozovsky <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
It's enough for us to be slaves to ye olde OpenSSL 1.0.2. Embrace the blessing of OpenSSL 1.1.1 which does not require users to register mutex locking callbacks to be thread safe, and brings other improvements (in particular, non-broken bitcode).
Unfortunately, the providers that we used are not very eager on upgrading to OpenSSL 1.1.1, especially the CocoaPods one. So I took a shot at packaging it myself. This PR switches from https://github.com/krzyzanowskim/OpenSSL and https://github.com/levigroker/GRKOpenSSLFramework to https://github.com/cossacklabs/openssl-apple
Carthage
The new OpenSSL is distributed as a binary-only framework. It will be downloaded from GitHub instead of building it from source. This is not much different from what the previous vendor did, but is more stable.
Carthage builds use the static flavor of the framework. We have run into issues with dynamic frameworks of OpenSSL when using Carthage, but static frameworks seems to do very good job: the resulting binaries are smaller, apps start a bit faster, and users are freed from the hassle of dealing with OpenSSL linkage to their app.
Note that due to the way static linkage works, we will be exporting all OpenSSL symbols from ObjCThemis by default. In order to avoid conflicts, export only limited subset of symbols: Objective-C classes of ObjCThemis.
For users: It is now not required to link and embed
openssl.framework
into your application. Onlyobjcthemis.framework
needs to be included.CocoaPods
The new OpenSSL is distributed as a tricky pod (which also downloads binaries from GitHub), but for consumers like Themis it's just a pod.
Introduce a separate subspec for the build with newer OpenSSL, and make it the default choice. We keep the old specs around in case someone needs them to share GRKOpenSSL or BoringSSL with other dependencies, as it is not possible to use CLOpenSSL simultaneously with them due to OpenSSL symbol conflicts.
The new subspec has its oddities, but it's all (un)known magic that seems to be absolutely necessary to build Themis properly for iOS.
Xcode update
Xcode 10.x is incompatible with bitcode provided by prebuilt OpenSSL frameworks. Therefore Xcode 11.0 is now the minimum required version for ObjCThemis and SwiftThemis.
Experimental arm64e support
ObjCThemis installed with Carthage now enables arm64e architecture. You can test your apps with it as well. (For CocoaPods you will have to add the architecture to the workspace as outlined in Apple documentation above.)
The support is still experimental and is know to fail on some Xcode versions.
Checklist
Example projects and code samples are up-to-date(should be updated after release)