Remove internal AES implementation and replace it with use of libcrypto by TomasKorbar · Pull Request #114 · awesomized/libmemcached

TomasKorbar · 2021-06-25T10:58:45Z

Hi,
As we are planning to replace original libmemcached in Fedora with awesomized libmemcached
i want to propose replacement of internal AES implementation with use of openssl.
In my opinion it is safer and more maintainable to use cryptographic library.
All tests are passing.
Feel free to propose any change.

remicollet · 2021-06-25T12:54:14Z

@TomasKorbar this break build with old openssl (such as 1.02k in RHEL-7)

Proposal

diff -up ./src/libhashkit/aes.cc.old ./src/libhashkit/aes.cc
--- ./src/libhashkit/aes.cc.old	2021-06-25 14:53:38.891064258 +0200
+++ ./src/libhashkit/aes.cc	2021-06-25 14:53:45.840043166 +0200
@@ -37,10 +37,10 @@ bool aes_initialize(const unsigned char
     return false;
   }
 
-  if (EVP_CIPHER_CTX_init(encryption_context) != 1 ||
-      EVP_EncryptInit_ex(encryption_context, EVP_aes_256_cbc(), NULL, key,
+  EVP_CIPHER_CTX_init(encryption_context);
+  EVP_CIPHER_CTX_init(decryption_context);
+  if (EVP_EncryptInit_ex(encryption_context, EVP_aes_256_cbc(), NULL, key,
                          aes_iv) != 1 ||
-      EVP_CIPHER_CTX_init(decryption_context) != 1 ||
       EVP_DecryptInit_ex(decryption_context, EVP_aes_256_cbc(), NULL, key,
                          aes_iv) != 1) {
     return false;

in Openssl 1.0

void EVP_CIPHER_CTX_init(EVP_CIPHER_CTX *a);

In Openssl 1.1

#  define EVP_CIPHER_CTX_init(c)      EVP_CIPHER_CTX_reset(c)
int EVP_CIPHER_CTX_reset(EVP_CIPHER_CTX *c);

m6w6

Thank you!

Please see the inline comments.

m6w6 · 2021-06-28T07:22:38Z

CMakeLists.txt

+if(NOT CRYPTO_LIB)
+  message(FATAL_ERROR "crypto library not found")
+endif()
+


This should be optional, and the build should use the bundled implementation when libcrypto was not found.

It should use the cmake module FindOpenSSL and lbhashkit should link against OpenSSL::Crypto if found.

It should be located in src/libhashkit/CMakeLists.txt

m6w6 · 2021-06-28T07:23:54Z

include/libhashkit-1.0/hashkit.h

 #include <libhashkit-1.0/string.h>

+#include <openssl/evp.h>
+


Should be #ifdef'd for optional support

m6w6 · 2021-06-28T07:24:46Z

include/libhashkit-1.0/hashkit.h

+  bool use_encryption;
+  EVP_CIPHER_CTX *encryption_context;
+  EVP_CIPHER_CTX *decryption_context;
 };


This breaks the ABI, so either allocate a private struct and reuse _key, or this may only be targeted at v2.0

m6w6 · 2021-06-28T07:29:53Z

include/libhashkit-1.0/hashkit.h


 HASHKIT_API
-bool hashkit_key(hashkit_st *, const char *key, const size_t key_length);
+bool hashkit_initialize_encryption(hashkit_st *kit, const char *key, const size_t key_length);


We're way too far into the stability game, so changing the API for better names is not a good idea.

m6w6 · 2021-06-28T07:30:04Z

src/libhashkit/CMakeLists.txt

        $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>
        $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/include>
        $<INSTALL_INTERFACE:include>)
+target_link_libraries(libhashkit PUBLIC -lcrypto)


See the FindOpenSSL and OpenSSL::Crypto comment above.

m6w6 · 2021-06-28T07:32:07Z

src/libmemcached/is.h

 /* These are private */
 #define memcached_is_allocated(__object)        ((__object)->options.is_allocated)
-#define memcached_is_encrypted(__object)        ((__object)->hashkit._key)
+#define memcached_is_encrypted(__object)        ((__object)->hashkit.use_encryption)


I think this should be (!!(__object)->hashkit._key) or alike.

m6w6 · 2021-06-28T07:33:59Z

src/libhashkit/aes.cc

+      EVP_CIPHER_CTX_init(decryption_context) != 1 ||
+      EVP_DecryptInit_ex(decryption_context, EVP_aes_256_cbc(), NULL, key,
+                         aes_iv) != 1) {
+    return false;


See comment from Remi: #114 (comment)

TomasKorbar · 2021-07-02T08:40:49Z

Hi @m6w6 ,
I updated pull request according to your review. What do you think about it now?

TomasKorbar · 2021-07-09T06:17:38Z

@m6w6 Any update on this?

m6w6 · 2021-07-09T06:28:50Z

Yes, the update is, that I'm currently camping and there have been thunderstorms tonight. 😉

TomasKorbar · 2021-07-09T09:21:49Z

@m6w6 No pressure. Just wanted to keep this alive :) Enjoy your camping and i hope that the storms did not cause you any trouble.

m6w6 · 2021-07-13T07:33:47Z

Merged, with a couple of fixes and simplifications, thank you!

m6w6 requested changes Jun 28, 2021

View reviewed changes

TomasKorbar force-pushed the v1.x branch from ed20c3e to 6193f8f Compare July 2, 2021 08:39

Add possibility to use libcrypto for encryption

2aab181

TomasKorbar force-pushed the v1.x branch from 6193f8f to 2aab181 Compare July 2, 2021 08:42

m6w6 merged commit 2aab181 into awesomized:v1.x Jul 13, 2021

Conversation

TomasKorbar commented Jun 25, 2021

Uh oh!

remicollet commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m6w6 left a comment

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

m6w6 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

TomasKorbar commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomasKorbar commented Jul 9, 2021

Uh oh!

m6w6 commented Jul 9, 2021

Uh oh!

TomasKorbar commented Jul 9, 2021

Uh oh!

m6w6 commented Jul 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

remicollet commented Jun 25, 2021 •

edited

Loading

TomasKorbar commented Jul 2, 2021 •

edited

Loading