Openssl 1.1 #507

andred · 2017-04-20T13:19:16Z

I can't push to #362, so this is an updated version.

Compared to #362, this fixes a few issues detected so far:

code involving EVP_CIPHER_CTX_copy() has been fixed
EVP_CIPHER_CTX_init() / EVP_CIPHER_CTX_reset() / OpenSSLCipher::Reset() usages have been simplified in the OpenSSL 1.1.x case
class Sha256HMACOpenSSLImpl allocates OpenSSL objects in its constructor now as requested
this supports LibreSSL again

The above OpenSSL header declares a function 'HMAC' which when included causes various compilation issues due to redefined types. Be explicit about our HMAC namespace, we avoid those and will be able to include above header. Signed-off-by: André Draszik <git@andred.net>

OpenSSL v1.1 made various types opaque - change the existing code to prepare for that to make it clearer what the actual OpenSSL v1.1 changes entail. This is the first step, dealing with EVP_CIPHER_CTX Signed-off-by: André Draszik <git@andred.net>

OpenSSL v1.1 made various types opaque - change the existing code to prepare for that to make it clearer what the actual OpenSSL v1.1 changes entail. This is the second step, dealing with HMAC_CTX Signed-off-by: André Draszik <git@andred.net>

OpenSSL v1.1: * made various types opaque and as such added various new allocators / destructors for those types * completely removed the old locking functions as they are now unneeded Change the code to employ the relevant changes in case we're compiling against OpenSSL >= v1.1.0 rc3 Signed-off-by: André Draszik <git@andred.net>

While LibreSSL strives to be API compatible with OpenSSL, it does so against the version when it was forked, 1.0.1g LibreSSL defines its own LIBRESSL_VERSION_NUMBER but at the same time it defines OPENSSL_VERSION_NUMBER to an insane value, which causes issues now. By detecting this case and setting a more reasonable version number, we can compile against LibreSSL again. Signed-off-by: André Draszik <git@andred.net>

singku · 2017-04-27T19:31:17Z

aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h

 #include <atomic>
 #include <mutex>

+#if defined(LIBRESSL_VERSION_NUMBER) && (OPENSSL_VERSION_NUMBER == 0x20000000L)


Even though in the Libressl code they said they never change the OPENSSL_VERSION_NUMBER from 0x20000000L, what would happen if they updated openssl version used, for say, from 1.0 to 1.1. It's could be more complicated situation.

I don't think it makes sense to speculate what LibreSSL might or might not do in the future. If they change the API, code using LibreSSL will have to follow. This is no different behaviour than before this PR - with the current code-base, if LibreSSL decided to change the API tomorrow, things would break.

The approach suggested here is what I've seen several other open-source projects taking.
It avoids additional clutter, as it redefines the version number to correctly reflect the API that LibreSSL actually implements.

The alternative would be to implement proper configuration checks using CMake, to detect features / APIs, rather than clutter the code with ifdefs. I don't really care about LibreSSL, so if you want I can drop this commit. It looked like a low-hanging fruit.

singku · 2017-04-27T21:24:20Z

aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h


+#if defined(LIBRESSL_VERSION_NUMBER) && (OPENSSL_VERSION_NUMBER == 0x20000000L)
+#undef OPENSSL_VERSION_NUMBER
+#define OPENSSL_VERSION_NUMBER 0x1000107fL


How is this version number comes?

As per the commit message:

...
While LibreSSL strives to be API compatible with OpenSSL,
it does so against the version when it was forked, 1.0.1g
...

singku · 2017-04-27T21:32:56Z

aws-cpp-sdk-core/include/aws/core/utils/crypto/openssl/CryptoImpl.h


-                EVP_CIPHER_CTX m_ctx;
+                EVP_CIPHER_CTX *m_ctx;
+#if OPENSSL_VERSION_NUMBER < 0x10100003L


This is not good to appear in multiple locations.

I agree. Do you have a better suggestion? What about two subclasses, where each of them implements specifics for each OpenSSL version. I am not using C++ much day-to-day, so there might be better options?

singku · 2017-04-27T21:35:47Z

aws-cpp-sdk-core/source/utils/crypto/openssl/CryptoImpl.cpp

-                toMove.m_ctx.engine = nullptr;
+                Init();
+                EVP_CIPHER_CTX_copy (m_ctx, toMove.m_ctx);



In original impl, there are several fields set up to nullptr, and I checked the code of EVP_CIPHER_CTX_copy, it doesn't cleanup in arg, I guess we should call EVP_CIPHER_CTX_cleanup(toMove.m_ctx) here.

Well spotted. EVP_CIPHER_CTX_cleanup() has been removed in OpenSSL 1.1 and instead EVP_CIPHER_CTX_reset() is to be used, so this adds another ifdef,

I'll do that.

singku · 2017-04-27T21:55:10Z

Why don't we just switch to pointer impl if OPENSSL made the type opaque?

singku · 2017-04-27T22:57:05Z

aws-cpp-sdk-core/source/utils/crypto/openssl/CryptoImpl.cpp

-                m_ctx.engine = nullptr;
+#if OPENSSL_VERSION_NUMBER < 0x10100003L
+                EVP_CIPHER_CTX_cleanup(m_ctx);
+                m_ctx = nullptr;


After openssl 1.1 m_ctx will be acquired from heap, so here we should free it right?

I think this is OK. The code paths are slightly different (I think it's a bit ugly and hard to follow, actually). The idea was to be able to reuse the same EVP_CIPHER_CTX throughout the lifetime of a class OpenSSLCipher rather then freeing and re-allocating to achieve a more static memory allocation modelling, comparable to the OpenSSL 1.0 implementation.

Cleanup() is called from the destructor and from ::Reset(). For OpenSSL 1.1:

Cleanup() itself doesn't do anything to the EVP_CIPHER_CTX

the destructor does Cleanup(), followed by EVP_CIPHER_CTX_free()

::Reset() does Cleanup(), followed by Init(), which itself does EVP_CIPHER_CTX_reset()
hence we achieve re-use of the handle, rather than free() + malloc()

For OpenSSL 1.0:

Cleanup() itself invalidates the static handle as per your comment

the destructor does Cleanup()

::Reset() does Cleanup(), followed by Init(), which itself reinitialises the static handle

singku · 2017-04-28T08:40:17Z

I have pulled this PR to our local branch, after reviewing and making proper changes, we will push out to this repo, probably in early next week.

andred · 2017-04-28T10:36:02Z

Why don't we just switch to pointer impl if OPENSSL made the type opaque?

OpenSSL 1.0 doesn't have APIs for allocation: #362 (comment)

singku · 2017-05-03T02:06:38Z

We have merged this pull request and done several changes. Check out the latest code in master branch.

andred and others added 5 commits April 26, 2017 17:09

singku self-assigned this Apr 27, 2017

singku reviewed Apr 27, 2017

View reviewed changes

singku mentioned this pull request Apr 27, 2017

Fix #361 adding conditions to differentiate OpenSSL v1.0.2 and v1.1.x in CryptoImpl #362

Closed

singku reviewed Apr 27, 2017

View reviewed changes

singku closed this May 3, 2017

andred deleted the openssl_1.1 branch May 11, 2017 10:26

Openssl 1.1 #507

Openssl 1.1 #507

Uh oh!

Conversation

andred commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andred Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singku commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singku commented Apr 28, 2017

Uh oh!

andred commented Apr 28, 2017

Uh oh!

singku commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andred commented Apr 20, 2017 •

edited

Loading

andred Apr 28, 2017 •

edited

Loading

singku commented May 3, 2017 •

edited

Loading