12 Mar
2016
12 Mar
'16
12:38 a.m.
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@mariadb.com 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