← Back to team overview

maria-developers team mailing list archive

Re: MDEV-9659 Encryption plugin that uses AWS Key management service

 


Hi Serg,
thanks a lot ! I recommited , addressing most of the comments .


On 07.03.2016 19:20, Sergei Golubchik wrote:
Hi, Wlad!

On Mar 03,wlad@xxxxxxxxxxx  wrote:
revision-id: c1ed3c0267da2ece7664e9f876314979114b2186 (mariadb-10.1.11-26-gc1ed3c0)
parent(s): a62db8186e40a62366bc86f49a0195e9b687d86d
author: Vladislav Vaintroub
committer: Vladislav Vaintroub
timestamp: 2016-03-03 21:29:29 +0100
message:

MDEV-9659 Encryption plugin that uses AWS Key management service
Looks pretty good, thanks!
See comment below...

diff --git a/cmake/plugin.cmake b/cmake/plugin.cmake
index c24239c..361cb59 100644
--- a/cmake/plugin.cmake
+++ b/cmake/plugin.cmake
@@ -74,7 +74,9 @@ MACRO(MYSQL_ADD_PLUGIN)
        SET(compat "with${compat}")
      ENDIF()
- IF (compat STREQUAL ".")
+    IF (ARG_DISABLED)
+      SET(howtobuild NO)
+    ELSEIF (compat STREQUAL ".")
        SET(howtobuild DYNAMIC)
      ELSEIF (compat STREQUAL "with.")
        IF (NOT ARG_MODULE_ONLY)
@@ -122,7 +124,7 @@ MACRO(MYSQL_ADD_PLUGIN)
# Build either static library or module
    IF (PLUGIN_${plugin} MATCHES "(STATIC|AUTO|YES)" AND NOT ARG_MODULE_ONLY
-      AND NOT ARG_DISABLED AND NOT ARG_CLIENT)
+      AND NOT ARG_CLIENT)
IF(CMAKE_GENERATOR MATCHES "Makefiles|Ninja")
        # If there is a shared library from previous shared build,
@@ -178,8 +180,7 @@ MACRO(MYSQL_ADD_PLUGIN)
        SET (mysql_optional_plugins ${mysql_optional_plugins} PARENT_SCOPE)
      ENDIF()
    ELSEIF(PLUGIN_${plugin} MATCHES "(DYNAMIC|AUTO|YES)"
-         AND NOT ARG_STATIC_ONLY  AND NOT WITHOUT_DYNAMIC_PLUGINS
-         AND NOT ARG_DISABLED)
+         AND NOT ARG_STATIC_ONLY AND NOT WITHOUT_DYNAMIC_PLUGINS)
ADD_VERSION_INFO(${target} MODULE SOURCES)
      ADD_LIBRARY(${target} MODULE ${SOURCES})
a bit more of the explanation would've been nice. in the changeset
comment or - even better - pushing this change separately. you can
select what files to include in the commit, you know...

Ok. Made a separate commit for this
diff --git a/plugin/aws_key_management/CMakeLists.txt b/plugin/aws_key_management/CMakeLists.txt
new file mode 100644
index 0000000..ec9ee46
--- /dev/null
+++ b/plugin/aws_key_management/CMakeLists.txt
@@ -0,0 +1,144 @@
+# We build parts of AWS C++ SDK as CMake external project
+# The restrictions of the SDK (https://github.com/awslabs/aws-sdk-cpp/blob/master/README.md)
+# are
+
+# - OS : Windows,Linux or OSX
+# - C++11 compiler : VS2013+, gcc 4.7+, clang 3.3+
+# - libcurl development package needs to be present on Unixes
+#
+# Since we're using ExternalProject that reads from git repository
+# we also require GIT to be present on the build machine
+
+
+# Give message why the building this plugin is skipped (only if -DVERBOSE is defined)
+# and and bail out.
+MACRO(SKIP_AWS_PLUGIN msg)
+  IF(DEFINED VERBOSE)
VERBOSE - is it used somewhere else? Or you've invented it on the spot
for this plugin?
I understand that it can be used in other cmake files, but is it?
It is not used elsewhere. invented on the spot. It can be used elsewhere, if we decide to make it a common convention. It was basically just a tool debug the configuration errors, less chatty than some other CMakeLists.txt are using :)
I suppose I could use message_once in 10.2 instead.


+    MESSAGE(STATUS "Skip aws_key_management - ${msg}")
+	RETURN()
+  ENDIF()
+ENDMACRO()
Ok, as discussed on irc, I'd suggest to add here a check whether AWS SDK
is already installed. Like, CheckLibraryExists or CheckSymbolExists.
And if not - then try to download and compile it (which would require
new cmake, git, curl, etc).
Ok, I did that . It still requires curl, but no git.

+IF(CMAKE_VERSION VERSION_LESS "2.8.8")
+ SKIP_AWS_PLUGIN("CMake is too old")
+ENDIF()
+
+FIND_PACKAGE(Git)
+IF(NOT GIT_FOUND)
+ SKIP_AWS_PLUGIN("no GIT")
+ENDIF()
+
+INCLUDE(ExternalProject)
+IF (NOT(WIN32 OR APPLE  OR (CMAKE_SYSTEM_NAME MATCHES "Linux")))
+  SKIP_AWS_PLUGIN("OS unsupported by AWS SDK")
+ENDIF()
+
+IF(UNIX)
+  FIND_PACKAGE(CURL)
+  IF(NOT CURL_FOUND)
+    SKIP_AWS_PLUGIN("AWS C++ SDK requires libcurl development package")
+  ENDIF()
+  SET(PIC_FLAG -fPIC)
+ENDIF()
+
+SET(CXX11_FLAGS)
+SET(OLD_COMPILER_MSG "AWS SDK requires c++11 -capable compiler (minimal supported versions are g++ 4.7, clang 3.3, VS2103)")
+IF(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+  EXECUTE_PROCESS(COMMAND ${CMAKE_CXX_COMPILER} -dumpversion OUTPUT_VARIABLE GCC_VERSION)
eh? I thought cmake knows compiler versions... let me see
This one: CMAKE_<LANG>_COMPILER_VERSION
I did not change this code, I gave other options a fair try, but no avail.
1. CMAKE_<LANG>_COMPILER_VERSION one exists since CMake 2.8.10, and we use older versions on many machines. Can't really use that, if we stick to native cmakes that come with the distros. 2. gcc4.7 does not really work ((I on some reason thought it would , but rechecked), even if --std=c++11 . Level of C++11 standard in this compiler is still insufficient (missing std::is_trivially_destructible() at least), and also Amazon is using compile options not available with 4.7 (-Wno-<something>)

So I'm back at checking compiler version with conservative means.

+
+FIND_PROGRAM(PERL_EXECUTABLE perl)
+
+IF(PERL_EXECUTABLE)
+  # CMake minimimal version for building AWS SDK can be less than 3.1.2
+  # Downgrade this requirement.
+  SET(AWS_SDK_PATCH_COMMAND
+  PATCH_COMMAND ${PERL_EXECUTABLE} -i.bak -pe s/3.1.2/2.8/g  ${CMAKE_BINARY_DIR}/aws-sdk-cpp/CMakeLists.txt)
1. Without perl it'll require 3.1.2, right?
If we do not patch, it'll require 3.1.2
2. Couldn't you do that with cmake alone?
Possible, but would require a cmake scriptfile. I'm using our own "replace" utility - found use for that, for the first time :)


3. Is that safe at all - you pull the latest version from git, they can
    start using cmake-3.1.2 features any time.
Ok, I'm no longer use the latest version - I use the latest and the only one marked with a tag 0.9.6 . Using latest is not a good idea anyway, if we want a reproducible build.

+
+static Aws::KMS::KMSClient *client;
+
+/* Redirect AWS trace to error log */
+class  MySQLLogSystem : public Aws::Utils::Logging::FormattedLogSystem
+{
+public:
+
+  using Base = FormattedLogSystem;
+  MySQLLogSystem(LogLevel logLevel) :
+    Base(logLevel)
+  {
+  }
+  virtual LogLevel GetLogLevel(void) const override
+  {
+    return (LogLevel)log_level;
+  }
+  virtual ~MySQLLogSystem()
+  {
+  }
+
+protected:
+  virtual void ProcessFormattedStatement(Aws::String&& statement) override
+  {
+#ifdef _WIN32
+    /*
+      On Windows, we can't use C runtime functions to write to stdout,
+      because we compile with static C runtime, so plugin has a stdout
+      different from server. Thus we're using WriteFile().
+    */
+    DWORD nSize= (DWORD)statement.size();
+    DWORD nWritten;
+    const char *s= statement.c_str();
+    HANDLE h= GetStdHandle(STD_OUTPUT_HANDLE);
+
+    WriteFile(h, s, nSize, &nWritten, NULL);
+#else
+    printf("%s", statement.c_str());
printf? You have declared sql_print_information above, why wouldn't you
use it here?
The statement that is being printed is already formatted, it contains a longish prefix containing tag (like [INFO], [DEBUG], [WARNING]) a timestamp, a subcomponent name . sql_print_information would add duplicate timestamp, another INFO tag, and not really added value, but it will make it look weirder-


+#endif
+  }
+};
+
+/*
+  Read filenames from the current directory.
+  This is used in plugin initilization. We check for
+  plugin specific names aws-kms-key.<id>.<version>
+  to find out what keys/key versions are already there.
+*/
+static vector<string> read_dir()
+{
+  vector<string> v;
+  DBUG_ENTER("read_dir");
+#ifdef _WIN32
+  WIN32_FIND_DATA ffd;
+  HANDLE h= FindFirstFile("*", &ffd);
+  if (h == INVALID_HANDLE_VALUE)
+  {
+    DBUG_RETURN(v);
+  }
+  do
+  {
+    /* handle file possibly updates caches*/
+    v.push_back(ffd.cFileName);
+  } while (FindNextFile(h, &ffd));
+  FindClose(h);
+#else
+  DIR *dir= opendir(".");
+  if (!dir)
+  {
+    DBUG_RETURN(v);
+  }
+  struct dirent *dp;
+  while ((dp= readdir(dir)) != NULL)
+  {
+    v.push_back(dp->d_name);
+  }
+  closedir(dir);
+#endif
+  DBUG_RETURN(v);
ok. although you could've used my_dir and save yourself some troubles :)
Indeed, thanks for reminding!
+}
+
+
+/*
+  Plugin initialization.
+
+  Create KMS client and scan datadir to find out which keys and versions
+  are present.
+*/
+static int plugin_init(void *p)
+{
+  DBUG_ENTER("plugin_init");
+  client = new KMSClient();
+  if (!client)
+  {
+    sql_print_error("Can not initialize KMS client");
+    DBUG_RETURN(-1);
+  }
+  InitializeAWSLogging(Aws::MakeShared<MySQLLogSystem>("aws_key_management_plugin", (Aws::Utils::Logging::LogLevel) log_level));
+  pthread_mutex_init(&mtx, NULL);
+  vector<string> filenames= read_dir();
+  for (size_t i= 0; i < filenames.size(); i++)
+  {
+    KEY_INFO info;
+    if (extract_id_and_version(filenames[i].c_str(), &info.key_id, &info.key_version) == 0)
+    {
+      key_info_cache[KEY_ID_AND_VERSION(info.key_id, info.key_version)]= info;
+      latest_version_cache[info.key_id]= max(info.key_version, latest_version_cache[info.key_id]);
+    }
+  }
+  DBUG_RETURN(0);
+}
+
+
+
+static int plugin_deinit(void *p)
+{
+  DBUG_ENTER("plugin_deinit");
+  latest_version_cache.clear();
+  key_info_cache.clear();
+  pthread_mutex_destroy(&mtx);
+  delete client;
+  ShutdownAWSLogging();
+  DBUG_RETURN(0);
+}
+
+/*  Generate filename to store the ciphered key */
+static void format_keyfile_name(char *buf, size_t size, uint key_id, uint version)
+{
+  snprintf(buf, size, "aws-kms-key.%u.%u", key_id, version);
+}
+
+/* Extract key id and version from file name */
+static int extract_id_and_version(const char *name, uint *id, uint *ver)
+{
+  int len;
+  int n= sscanf(name, "aws-kms-key.%u.%u%n", id, ver, &len);
+  if (n == 2 && *id > 0 && *ver > 0 && len == (int)strlen(name))
+    return 0;
+  return 1;
+}
+
+/*
+  Decrypt key stored in aws-kms-key.<id>.<version>
+  Cache the decrypted key.
+*/
+static int load_key(KEY_INFO *info)
+{
+  int ret;
+  char path[256];
+  DBUG_ENTER("load_key");
+  DBUG_PRINT("enter", ("id=%u,ver=%u", info->key_id, info->key_version));
+  format_keyfile_name(path, sizeof(path), info->key_id, info->key_version);
+  ret= aws_decrypt_key(path, info);
+  if (ret)
+    info->load_failed= true;
+
+  latest_version_cache[info->key_id]= max(latest_version_cache[info->key_id], info->key_version);
+  key_info_cache[KEY_ID_AND_VERSION(info->key_id, info->key_version)]= *info;
+
+  if (!ret)
+  {
+    sql_print_information("AWS KMS plugin: loaded key %u, version %u, key length %u bit",
+      info->key_id, info->key_version,(uint)info->length*8);
+  }
+  else
+  {
+    sql_print_warning("AWS KMS plugin: key %u, version %u could not be decrypted",
+      info->key_id, info->key_version);
+  }
+  DBUG_RETURN(ret);
+}
+
+
+/*
+  Get latest version for the key.
+
+  If key is not decrypted yet, this function also decrypt the key
+  and error will be returned if decryption fails.
+
+  The reason for that is that InnoDB crashes
+   in case errors are returned by get_key(),
Shall we rather fix that in InnoDB?
Do you remember where it crashes?


When I found it first,Jan was in process of removing assertions, so I never filed a bug. I rechecked now, hoping everything is fixed, but it was not the case, I still run into an assertion.

I do "create table .. encrypted=YES". With invalid aws-kms-key.1.1 in datadir, purposefully invalid, to simulate AWS error

This gives:
a) get_latest_version  for key 1 returns 1,
b)attempt to retrieve version 1 for key  1 fails,
c) after b) then fil_crypt_buf() crashes here

https://github.com/MariaDB/server/blob/10.1/storage/xtradb/fil/fil0crypt.cc#L596

if (! ((rc == MY_AES_OK) && ((ulint) dstlen == srclen))) {

...
ut_error;


+
+  A new key will be created if it does not exist, provided there is
+  valid master_key_id.
+*/
+static unsigned int get_latest_key_version(unsigned int key_id)
+{
+  unsigned int ret;
+  DBUG_ENTER("get_latest_key_version");
+
+  pthread_mutex_lock(&mtx);
+  ret= get_latest_key_version_nolock(key_id);
why do you need get_latest_key_version_nolock if you never use it
directly?
No strong reason rreally. I do a lot of early returns from the nolock function. I could squeeze everything into one function, and do goto instead of returns, but I felt like using the helper function was easier.

+  pthread_mutex_unlock(&mtx);
+
+  DBUG_PRINT("info", ("key=%u,ret=%u", key_id, ret));
+  DBUG_RETURN(ret);
+}
+
+static unsigned int get_latest_key_version_nolock(unsigned int key_id)
+{
+  KEY_INFO info;
+  uint ver;
+  DBUG_ENTER("get_latest_key_version_nolock");
+  ver= latest_version_cache[key_id];
+  if (ver > 0)
+  {
+    info= key_info_cache[KEY_ID_AND_VERSION(key_id, ver)];
+  }
+  if (info.load_failed)
+  {
+    /* Decryption failed previously, don't retry */
+    DBUG_RETURN(ENCRYPTION_KEY_VERSION_INVALID);
+  }
+  else if (ver > 0)
+  {
+    /* Key encrypted already, return it*/
may be s/encrypted already/exists/ ?
Yes, this is the intention.


Thanks again,
Wlad


Follow ups

References