mbedtls
mbedtls copied to clipboard
Mbed-TLS
Update Windows Crypto APIs
Description
This is an update on PR #730 to address issues building with mingw and Visual Studio when using cmake generated files.
To quote the original PR.
CryptGenRandom and lstrlenW are not permitted in Windows Store apps, meaning apps that use mbedTLS can't ship in the Windows Store. Instead, use BCryptGenRandom and wcslen, respectively, which are permitted.
Also make sure conversions between size_t, ULONG, and int are always done safely; on a 64-bit platform, these types are differnt sizes.
Also, removed the obsolete Visual Studio 6 data files which are no longer used.
Status
READY
Requires Backporting
NO
Migrations
NO
Todos
- [X] Tests
- [] Documentation
- [X] Changelog updated
- [] Backported
We should document the minimum mingw version in README. If not, at least on the website (update the KB article https://tls.mbed.org/kb/compiling-and-building/compiling-mbedtls-in-mingw).
@gilles-peskine-arm - We already have an open task in JIRA (IOTSSL-1042) to update the mingw documentation.
Review comments from @gilles-peskine-arm addressed. Please review my responses and respond or approve.
Thanks!
Updated with a comment on the codepageCP_ACP.
@gilles-peskine-arm / @Patater - please re-review and approve.
Testing done (in addition to CI checks): Built benchmark.exe with Makefile and mingw. Ran benchmark.exe on wine; it worked.
The provided solution files for x64 fail to build. This is either a regression introduced by this PR or because the PR is based on old versions of those files. If the latter, please consider rebasing on a more recent point in history to get our recent fixes to those solution files.
@Patater - You're right. It was a regression introduced by the PR, which is now fixed. The PR is now passing the Windows CI and builds for Win32 and x64 with Visual Studio 2015 on my development machine.
@gilles-peskine-arm / @Patater - please review and approve as you see fit.
Please note I've also removed the obsolete Visual Studio 6 project files.
@gilles-peskine-arm b862f4c is basically a fixup to "Replace Windows APIs that are banned in Windows Store apps" (e241eb0). "Replace Windows APIs that are banned in Windows Store apps" introduces a regression by incorrectly re-adding mbedTLS.lib to the vcproj files, which we had removed to fix bug #1347. The commit history would be simpler and easier to understand if b862f4c were applied as a fixup to e241eb0, but that makes reviewing with GitHub more difficult.
Thanks @Patater . Rather than change an old commit, which will make the history inaccurate and makes it harder to diagnose problems later, I'd rather have a fix-up commit that says ”do <such-and-such>. This reverts the incorrect change made in commit 1234 which caused <problem>.”
The problems in b862f4c were introduced when I rebased the original PR against the current development branch, and I inadvertently introduced mbedtls.lib into the project files which reverted a change introduced in PR #1384.
Rather than layer on more commits to fix the original botched rebase, I rebased the original PR again, and have added back the other commits required to address the other issues and review feedback again.
The PR is now passing the Windows CI with the exception of the known issues and intermittent problems, and also issue #1430, which is causing some Release builds to fail.
Given I've rebased the PR again, this PR needs a review from beginning to end again.
LGTM, except there seems to be a minor inconsistency with the casing of the variable names: len_as_ulong and lengthAsInt.
This PR now passes Windows cross build on Linux after updating CI slave to newer version. https://jenkins-internal.mbed.com/job/mbedtls-release/358/
I've rebased this PR, and kicked off a CI job on buildbot, and a release job on Jenkins.
@Patater / @gilles-peskine-arm - Could you please re-review and approve this PR? Or if you're unavailable, please let me know so I can find substitutes. Thanks.
@sbutcher-arm Given my limited bandwidth and my lack of familiarity with Windows, please find someone else for this PR.
Failed Travis CI due to stale Visual Studio project files, found by tests/scripts/check-generated-files.sh. I'll refresh them and update the PR.
Fixed the stale project files and the merge error. Everything should now pass the CI.
Release testing job is running at https://jenkins-internal.mbed.com/view/mbed-tls/job/mbedtls-release/508/
@k-stachowiak / @Patater / @gilles-peskine-arm - I've fixed the style issue @k-stachowiak identified. Could you please re-review?
Scheduled a Windows release job as build #552.
Sadly, the PR failed all.sh release job:
Extract from the log:
[all_sh] !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
[all_sh] FAILED: 5
[all_sh] build: create and build yotta module: tests/scripts/yotta-build.sh -> 1
[all_sh] build: Windows cross build - mingw64, make (Link Library): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 lib programs -> 2
[all_sh] build: Windows cross build - mingw64, make (Link Library): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror WINDOWS_BUILD=1 tests -> 2
[all_sh] build: Windows cross build - mingw64, make (DLL): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 SHARED=1 lib programs -> 2
[all_sh] build: Windows cross build - mingw64, make (DLL): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 SHARED=1 tests -> 2
[all_sh] !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
@sbutcher-arm Maybe the new API isn't supported by the version of Mingw on the CI? IIRC at some point in our Windows crypto API saga there were proposals that didn't build with the toolchain version in Ubuntu 16.04 but did build with the toolchain version in Ubuntu 18.04.
I think you're right @gilles-peskine-arm, and the issue is with the mingw version on the Jenkins CI. We're trying to get hold of the dockerfile and update it.
cc: @dgreen-arm
This PR passes with the correct mingw version, so I'm going accept the PR as-is, and fix the Jenkins CI as quickly as I can independently. In the meantime, all.sh will continue to fail on Jenkins CI until we fix it, for the mingw tests.
cc: @dgreen-arm, @Patater
This has failed on Visual Studio builds for Release and x64. Looking at the PR, it looks like the configuration for Release|x64 wasn't updated like the others were.
Build: #770
Now passing on Jenkins CI for 'Release|x64'.
@Patater / @k-stachowiak - Can you please review the last commit, and approve, so I can merge this? Thanks.
This PR failed in my attempts to merge it causing the CI release job to fail. I've rebased the PR (will need re-approval - sorry), and set off a new CI release job #894.
Demoting to 'needs: work'.
The CI job #894 all.sh failed after the rebase with the following errors:
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
FAILED: 4
build: Windows cross build - mingw64, make (Link Library): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 lib programs -> 2
build: Windows cross build - mingw64, make (Link Library): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror WINDOWS_BUILD=1 tests -> 2
build: Windows cross build - mingw64, make (DLL): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 SHARED=1 lib programs -> 2
build: Windows cross build - mingw64, make (DLL): command make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS=-Werror -Wall -Wextra WINDOWS_BUILD=1 SHARED=1 tests -> 2
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
This passed before the rebase.
I've tested with the Docker image in the CI, and the issue is the version of mingw is too old, being i686-w64-mingw32-gcc (GCC) 5.3.1 20160211.
This is too old, and even Ubuntu 17.10 currently has a package version of i686-w64-mingw32-gcc (GCC) 6.3.0 20170415.
This issue is with the CI and needs to be updated to merge this PR.
i'm sad this PR hasn't been merged yet, but since it hasn't, i thought i'd point out an alternative that causes less changed files. the change to bcrypt api in entropy_poll.c required many changes in the makefiles, project files, cmake files, etc. instead of all these explicit reference to link against bcrypt.lib, adding this one line:
#pragma comment(lib,"bcrypt.lib")
into entropy_poll.c (eg just after #include <bcrypt.h>) causes the dependency on bcrypt.lib to be noted inside entropy_poll.obj, and therefore it is pulled in by the linker automatically.
Is there any chance that this change ends up in a release any time soon? What is blocking it? Can I do something to help out?
I'll pick this PR up again, rebase it and resubmit it. Hopefully we can get it in this time.
Note: there was a PR on the crypto side that may or may not contain changes that need to be picked up here now: https://github.com/ARMmbed/mbed-crypto/pull/341
Any news on this? I'm trying to build a project that uses mbedtls for UWP.
For the record, we've been using that PR downstream in Godot Engine for a few years for our UWP build. Here's a diff of this PR compatible with mbedTLS 2.28.1: https://github.com/godotengine/godot/blob/master/thirdparty/mbedtls/patches/1453.diff
Our UWP port doesn't see a lot of use though so I can't attest for how well this solves the issue, and whether mbedTLS works properly on UWP.
