maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Julius!

First, please combine your two commits in one.

There's no historical value in keeping the first overcomplicated
implementation in the tree.

See other comments below:

On Nov 23, Julius Goryavsky wrote:
> revision-id: c23c5671a60 (mariadb-10.4.10-180-gc23c5671a60)
> parent(s): 387839e44cc
> author: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> committer: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> timestamp: 2019-11-22 18:38:39 +0100
> message:
> 
> Simplified version
> 
> diff --git a/plugin/hashicorp_key_management/CMakeLists.txt b/plugin/hashicorp_key_management/CMakeLists.txt
> new file mode 100644
> index 00000000000..95ff2434976
> --- /dev/null
> +++ b/plugin/hashicorp_key_management/CMakeLists.txt
> @@ -0,0 +1,20 @@
> +SET(HASHICORP_KEY_MANAGEMENT_PLUGIN_SOURCES
> +  hashicorp_key_management_plugin.cc)
> +
> +IF (WITH_CURL)
> +  INCLUDE(FindCURL)
> +  IF(CURL_FOUND)
> +  ELSE()
> +    # Can't build plugin
> +    RETURN()
> +  ENDIF()
> +ELSE()
> +  # Can't build plugin
> +  RETURN()
> +ENDIF()

Better just

  INCLUDE(FindCURL)
  IF(NOT CURL_FOUND)
    RETURN()
  ENDIF()

No need to check for -DWITH_CURL=ON,
it should be enough to specify -DHASHICORP_KEY_MANAGEMENT_PLUGIN=ON
if one wants a plugin, the plugin should implicitly look for curl.

> +
> +INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/sql ${CURL_INCLUDE_DIR})
> +
> +MYSQL_ADD_PLUGIN(HASHICORP_KEY_MANAGEMENT
> +  ${HASHICORP_KEY_MANAGEMENT_PLUGIN_SOURCES}

I wouldn't introduce HASHICORP_KEY_MANAGEMENT_PLUGIN_SOURCES variable
just for one file, but do as you like

> +  LINK_LIBRARIES ${CURL_LIBRARIES})
> diff --git a/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc
> new file mode 100644
> index 00000000000..3dc5214323b
> --- /dev/null
> +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc
> @@ -0,0 +1,405 @@
> +/* Copyright (C) 2019 MariaDB Corporation
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; version 2 of the License.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335  USA */
> +
> +#include <my_global.h>
> +#include <typelib.h>
> +#include "sql_error.h"

these three headers shouldn't be included, only use plugin services, do
not include server headers at random.

> +#include <mysqld_error.h>
> +#include <mysql/plugin_encryption.h>
> +#include <string.h>
> +#include <string>
> +#include <sstream>
> +#include <curl/curl.h>
> +
> +static char* vault_url;
> +static char* secret_mount_point;
> +static char* token;
> +static char* vault_ca;
> +
> +static unsigned long encryption_algorithm;
> +
> +static const char *encryption_algorithm_names[]=
> +{
> +  "aes_cbc",
> +#ifdef HAVE_EncryptAes128Ctr
> +  "aes_ctr",
> +#endif
> +  0
> +};

This is what file plugin does, I don't think you need to copy it here.
Just use the default algorithm.

> +
> +static TYPELIB encryption_algorithm_typelib=
> +{
> +  array_elements(encryption_algorithm_names)-1,"",
> +  encryption_algorithm_names, NULL
> +};
> +
> +static MYSQL_SYSVAR_STR(vault_ca, vault_ca,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Path to Certificate Authority (CA) bundle",
> +  NULL, NULL, "");
> +
> +static MYSQL_SYSVAR_STR(vault_url, vault_url,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Hashicorp Vault URL",
> +  NULL, NULL, "");
> +
> +static MYSQL_SYSVAR_STR(secret_mount_point, secret_mount_point,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Secret mount point",
> +  NULL, NULL, "");

I don't see a point in splitting the url in two parts that are _always_
used together. Please, remove the "secret_mount_point" variable, the
user will specify it as a part of the url.

> +
> +static MYSQL_SYSVAR_STR(token, token,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Authentication token",
> +  NULL, NULL, "");

Here, perhaps, I'd specify PLUGIN_VAR_NOSYSVAR flag. To hide the token
from SHOW VARIABLES, because the token is all one needs to retrieve
encryption keys.

> +
> +#ifdef HAVE_EncryptAes128Ctr
> +#define recommendation	", aes_ctr is the recommended one"
> +#else
> +#define recommendation	""
> +#endif
> +static MYSQL_SYSVAR_ENUM(encryption_algorithm, encryption_algorithm,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Encryption algorithm to use" recommendation ".",
> +  NULL, NULL, 0, &encryption_algorithm_typelib);
> +
> +static struct st_mysql_sys_var* settings[] = {
> +  MYSQL_SYSVAR(vault_url),
> +  MYSQL_SYSVAR(secret_mount_point),
> +  MYSQL_SYSVAR(token),
> +  MYSQL_SYSVAR(vault_ca),
> +  MYSQL_SYSVAR(encryption_algorithm),
> +  NULL
> +};
> +
> +static char curl_errbuf [CURL_ERROR_SIZE];
> +
> +static std::string get_error_from_curl (CURLcode curl_code)
> +{
> +  size_t len = strlen(curl_errbuf);
> +  std::ostringstream stream;
> +  if (curl_code != CURLE_OK)
> +  {
> +    stream << "CURL returned this error code: " << curl_code;
> +    stream << " with error message : ";
> +    if (len)
> +      stream << curl_errbuf;
> +    else
> +      stream << curl_easy_strerror(curl_code);
> +  }
> +  return stream.str();
> +}
> +
> +#define max_response_size 65536
> +
> +static size_t write_response_memory(void *contents, size_t size, size_t nmemb, void *userp)
> +{
> +  size_t realsize = size * nmemb;
> +  if (size != 0 && realsize / size != nmemb)
> +    return 0; // overflow
> +  std::ostringstream *read_data = static_cast<std::ostringstream*>(userp);
> +  size_t ss_pos = read_data->tellp();
> +  read_data->seekp(0, std::ios::end);
> +  size_t number_of_read_bytes = read_data->tellp();
> +  read_data->seekp(ss_pos);
> +  if (number_of_read_bytes + realsize > max_response_size)
> +    return 0; // response size limit exceeded
> +  read_data->write(static_cast<char*>(contents), realsize);
> +  if (!read_data->good())
> +    return 0;
> +  return realsize;
> +}
> +
> +std::ostringstream read_data_stream;
> +
> +#define timeout 300
> +
> +static struct curl_slist *list = NULL;
> +
> +static bool setup_curl_session (CURL * curl)
> +{
> +  std::string token_header = std::string("X-Vault-Token:") + std::string(token);
> +  CURLcode curl_res = CURLE_OK;
> +  read_data_stream.str("");
> +  read_data_stream.clear();
> +  curl_errbuf[0] = '\0';
> +  if (list != NULL)
> +  {
> +    curl_slist_free_all(list);
> +    list = NULL;
> +  }

1. why do you recreate the list? isn't it always the same?
2. there's no protection against concurrent access. this probably won't
   work at all when many threads will try to request keys in parallel.

note, I don't think you need a mutex protection here, better populate
the list once in the plugin initializer and then use it.

> +  if ((list= curl_slist_append(list, token_header.c_str())) == NULL ||
> +      (list= curl_slist_append(list, "Content-Type: application/json")) ==
> +	  NULL ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, curl_errbuf)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION,
> +				  write_response_memory)) != CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_WRITEDATA,
> +				  static_cast<void *>(&read_data_stream))) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2L)) !=
> +	  CURLE_OK ||
> +      (vault_ca != NULL && strlen(vault_ca) != 0 &&
> +       (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO, vault_ca)) !=
> +	   CURLE_OK) ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_USE_SSL, CURLUSESSL_ALL)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, timeout)) !=
> +	  CURLE_OK ||
> +      (curl_res= curl_easy_setopt(curl, CURLOPT_TIMEOUT, timeout)) !=
> +	  CURLE_OK ||
> +      (curl_res = curl_easy_setopt(curl, CURLOPT_HTTP_VERSION,
> +				   (long)CURL_HTTP_VERSION_1_1)) !=
> +	  CURLE_OK)
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +		    get_error_from_curl(curl_res).c_str(),
> +		    ME_ERROR_LOG_ONLY);
> +    return true;
> +  }
> +  return false;
> +}
> +
> +static bool curl_run (char * url, const char * * response)
> +{
> +  CURLcode curl_res = CURLE_OK;
> +  long http_code = 0;
> +  CURL *curl = curl_easy_init();

the manual for curl_easy_init says

       If you did not already call curl_global_init(3), curl_easy_init(3) does
       it  automatically.   This  may be lethal in multi-threaded cases, since
       curl_global_init(3) is not thread-safe, and it may result  in  resource
       problems because there is no corresponding cleanup.

       You  are  strongly  advised  to  not allow this automatic behaviour, by
       calling curl_global_init(3) yourself properly.  See the description  in
       libcurl(3) of global environment requirements for details of how to use
       this function.

> +  if (curl == NULL)
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +		    "Cannot initialize curl session",
> +		    ME_ERROR_LOG_ONLY);
> +    return true;
> +  }
> +  if (setup_curl_session(curl) ||
> +      (curl_res = curl_easy_setopt(curl, CURLOPT_URL, url)) != CURLE_OK ||
> +      (curl_res = curl_easy_perform(curl)) != CURLE_OK ||
> +      (curl_res = curl_easy_getinfo (curl, CURLINFO_RESPONSE_CODE, &http_code)) != CURLE_OK)
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +		    get_error_from_curl(curl_res).c_str(),
> +		    ME_ERROR_LOG_ONLY);
> +    return true;
> +  }
> +  if (http_code == 404)
> +  {
> +    *response = NULL;
> +    return false;
> +  }
> +  *response = read_data_stream.str().c_str();
> +  return http_code < 200 || http_code >= 300;
> +}
> +
> +static unsigned int get_latest_version (uint key_id)
> +{
> +  char url[2048];
> +  const char *response;
> +  const char *js, *ver;
> +  int js_len, ver_len;
> +  size_t response_len;
> +  snprintf(url, sizeof(url), "%s/v1/%s/%u",
> +           vault_url, secret_mount_point, key_id);
> +  if (curl_run(url, &response)) {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +		    "Unable to get key data",
> +		    0);
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  }
> +  response_len = strlen(response);
> +  if (json_get_object_key(response, response + response_len,
> +                          "metadata", &js, &js_len) != JSV_OBJECT)
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  if (json_get_object_key(js, js+js_len,"version", &ver, &ver_len) != JSV_NUMBER)
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  std::string ver_str = std::string(ver, ver_len);
> +  return strtoul(ver_str.c_str(), NULL, 10);
> +}
> +
> +static int decode_data (const char *src, int src_len,
> +                        unsigned char* dstbuf, unsigned *buflen)
> +{
> +  int length_of_memory_needed_for_decode =
> +    my_base64_needed_decoded_length(src_len);
> +  char* data= (char *)my_malloc(
> +    length_of_memory_needed_for_decode * sizeof(char), MYF(0));
> +  int decoded_length=
> +    my_base64_decode(src, src_len, data, NULL, 0);
> +  if (decoded_length <= 0)
> +  {
> +    memset(data, 0, length_of_memory_needed_for_decode);
> +    my_free(data);
> +    return ENCRYPTION_KEY_BUFFER_TOO_SMALL;
> +  }
> +  if (*buflen < (unsigned) decoded_length) {
> +    *buflen= decoded_length;
> +     my_free(data);

you cannot use my_malloc and my_free here, the plugin won't link.
use malloc/free instead.

> +    return ENCRYPTION_KEY_BUFFER_TOO_SMALL;
> +  }
> +  *buflen= decoded_length;
> +  memcpy(dstbuf, data, decoded_length);
> +  my_free(data);
> +  return 0;
> +}
> +
> +static unsigned int get_key_from_vault (unsigned int key_id,
> +                                        unsigned int key_version,
> +                                        unsigned char* dstbuf,
> +                                        unsigned int *buflen)
> +{
> +  char url[2048];
> +  const char *response;
> +  const char *js, *ver, *key;
> +  int js_len, key_len, ver_len;
> +  size_t response_len;
> +  if (key_version != ENCRYPTION_KEY_VERSION_INVALID)
> +    snprintf(url, sizeof(url), "%s/v1/%s/%u?%u",
> +             vault_url, secret_mount_point, key_id, key_version);
> +  else
> +    snprintf(url, sizeof(url), "%s/v1/%s/%u",
> +             vault_url, secret_mount_point, key_id);
> +  if (curl_run(url, &response))
> +  {
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +		    "Unable to get key data",
> +		    0);
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  }
> +  response_len = strlen(response);
> +  if (key_version != ENCRYPTION_KEY_VERSION_INVALID)
> +  {
> +    if (json_get_object_key(response, response + response_len,
> +                            "metadata", &js, &js_len) != JSV_OBJECT)
> +      return ENCRYPTION_KEY_VERSION_INVALID;
> +    if (json_get_object_key(js, js+js_len,"version", &ver, &ver_len) != JSV_NUMBER)
> +      return ENCRYPTION_KEY_VERSION_INVALID;
> +    std::string ver_str = std::string(ver, ver_len);
> +    if (strtoul(ver_str.c_str(), NULL, 10) != key_version)
> +      return ENCRYPTION_KEY_VERSION_INVALID;

Can this happen? You've requested a specific version, can Hashicorp
return a _different_ version? That'd be strange.

> +  }
> +  if (json_get_object_key(response, response + response_len,
> +                          "data", &js, &js_len) != JSV_OBJECT)
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  if (json_get_object_key(js, js + js_len, "data", &key, &key_len) != JSV_STRING)
> +    return ENCRYPTION_KEY_VERSION_INVALID;
> +  return decode_data(key, key_len, dstbuf, buflen);
> +}
> +
> +// let's simplify the condition below
> +#ifndef HAVE_EncryptAes128Gcm
> +#define MY_AES_GCM MY_AES_CTR
> +#ifndef HAVE_EncryptAes128Ctr
> +#define MY_AES_CTR MY_AES_CBC
> +#endif
> +#endif
> +
> +static inline enum my_aes_mode mode(int flags)
> +{
> +  /*
> +    If encryption_algorithm is AES_CTR then
> +      if no-padding, use AES_CTR
> +      else use AES_GCM (like CTR but appends a "checksum" block)
> +    else
> +      use AES_CBC
> +  */
> +  if (encryption_algorithm)
> +    if (flags & ENCRYPTION_FLAG_NOPAD)
> +      return MY_AES_CTR;
> +    else
> +      return MY_AES_GCM;
> +  else
> +    return MY_AES_CBC;
> +}
> +
> +static int ctx_init(void *ctx, const unsigned char* key, unsigned int klen,
> +		    const unsigned char* iv, unsigned int ivlen, int flags,
> +		    unsigned int key_id, unsigned int key_version)
> +{
> +  return my_aes_crypt_init(ctx, mode(flags), flags, key, klen, iv, ivlen);
> +}
> +
> +static int ctx_update(void *ctx, const unsigned char *src, unsigned int slen,
> +  unsigned char *dst, unsigned int *dlen)
> +{
> +  return my_aes_crypt_update(ctx, src, slen, dst, dlen);
> +}
> +
> +
> +static int ctx_finish(void *ctx, unsigned char *dst, unsigned int *dlen)
> +{
> +  return my_aes_crypt_finish(ctx, dst, dlen);
> +}
> +
> +static unsigned int get_length(unsigned int slen, unsigned int key_id,
> +			       unsigned int key_version)
> +{
> +  return my_aes_get_size(mode(0), slen);
> +}
> +
> +static uint ctx_size(uint, uint)
> +{
> +  return my_aes_ctx_size(mode(0));
> +}
> +
> +struct st_mariadb_encryption hashicorp_key_management_plugin= {
> +  MariaDB_ENCRYPTION_INTERFACE_VERSION,
> +  get_latest_version,
> +  get_key_from_vault,
> +  ctx_size,
> +  ctx_init,
> +  ctx_update,
> +  ctx_finish,
> +  get_length

you've seen my template, I've intentionally did not define any functions
beyond get_latest_version and get_key_from_vault. You don't need to
either, just put 0, don't copy this code from the file plugin.

> +};
> +
> +static int hashicorp_key_management_plugin_init(void *p)
> +{
> +  return 0;
> +}

if you don't need to initialize the plugin - don't create an initializer
at all. what's the point in a dummy one?

> +
> +static int hashicorp_key_management_plugin_deinit(void *p)
> +{
> +  read_data_stream.str("");
> +  read_data_stream.clear();
> +  return 0;
> +}
> +
> +/*
> +  Plugin library descriptor
> +*/
> +maria_declare_plugin(hashicorp_key_management)
> +{
> +  MariaDB_ENCRYPTION_PLUGIN,
> +  &hashicorp_key_management_plugin,
> +  "hashicorp_key_management",
> +  "MariaDB Corporation",

you don't want to have your name here? :)

> +  "HashiCorp Vault key management plugin",
> +  PLUGIN_LICENSE_GPL,
> +  hashicorp_key_management_plugin_init,
> +  hashicorp_key_management_plugin_deinit,
> +  0x0100 /* 1.0 */,
> +  NULL, /* status variables */
> +  settings,
> +  "1.0",
> +  MariaDB_PLUGIN_MATURITY_STABLE
> +}
> +maria_declare_plugin_end;

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx