maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Julius!

On Nov 27, Julius Goryavsky wrote:
> revision-id: 387839e44cc (mariadb-10.4.10-179-g387839e44cc)
> parent(s): 8115a02283d
> author: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> committer: Julius Goryavsky <julius.goryavsky@xxxxxxxxxxx>
> timestamp: 2019-11-15 13:37:35 +0100
> message:
> 
> Plugin for Hashicorp Vault Key Management System
> 

> 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..6de1d9d1f3a
> --- /dev/null
> +++ b/plugin/hashicorp_key_management/hashicorp_key_management_plugin.cc
> @@ -0,0 +1,330 @@
> +/* 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 <mysql/plugin.h>

This is redundant, because plugin_encryption.h includes plugin.h
But it does not harm either, so if you want to include it explictly
for clarity - sure, do it

> +#include <mysql/plugin_encryption.h>
> +#include <mysqld_error.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <string>
> +#include <sstream>
> +#include <curl/curl.h>
> +
> +static char* vault_url;
> +static char* token;
> +static char* vault_ca;
> +
> +static MYSQL_SYSVAR_STR(vault_ca, vault_ca,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "Path to Certificate Authority (CA) bundle (is a file "
> +  "that contains root and intermediate certificates)",
> +  NULL, NULL, "");
> +
> +static MYSQL_SYSVAR_STR(vault_url, vault_url,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY,
> +  "HTTP[s] URL that is used to connect to the Hashicorp Vault server",
> +  NULL, NULL, "https://127.0.0.1:8200/v1/secret";);
> +
> +static MYSQL_SYSVAR_STR(token, token,
> +  PLUGIN_VAR_RQCMDARG | PLUGIN_VAR_READONLY | PLUGIN_VAR_NOSYSVAR,
> +  "Authentication token that passed to the Hashicorp Vault "
> +  "in the request header",
> +  NULL, NULL, "");
> +
> +static struct st_mysql_sys_var* settings[] = {
> +  MYSQL_SYSVAR(vault_url),
> +  MYSQL_SYSVAR(token),
> +  MYSQL_SYSVAR(vault_ca),
> +  NULL
> +};
> +
> +static std::string get_error_from_curl (CURLcode curl_code, char *curl_errbuf)
> +{
> +  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;
> +}
> +
> +#define timeout 300
> +
> +static bool setup_curl_session (CURL *curl, char *curl_errbuf,
> +                                curl_slist **list_ptr,
> +                                std::ostringstream &read_data_stream)
> +{
> +  struct curl_slist *list = NULL;
> +  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();

Seems redundant, the stream was just created

> +  curl_errbuf[0] = '\0';
> +  if ((list= curl_slist_append(list, token_header.c_str())) == NULL ||
> +      (list= curl_slist_append(list, "Content-Type: application/json")) == NULL ||

Why do you specify Content-Type header if you aren't sending any content?
Seems redundant.

> +      (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 ||

Do you need to cast here?
Seems redundant.

> +      (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 ||

This is redundant, CURLOPT_SSL_VERIFYPEER is 1 by default,
CURLOPT_SSL_VERIFYHOST is 2 by default.
But again, if you want to be explicit for the sake of clarity - feel free to

> +      (vault_ca != NULL && strlen(vault_ca) != 0 && (curl_res= curl_easy_setopt(curl, CURLOPT_CAINFO, vault_ca)) != CURLE_OK) ||

How can vault_ca == NULL here?
I think it cannot, the check is redundant too.

> +      (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)

Why?

> +  {
> +    *list_ptr = list;
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +                   get_error_from_curl(curl_res, curl_errbuf).c_str(),
> +                   ME_ERROR_LOG_ONLY);
> +    return true;
> +  }
> +  *list_ptr = list;
> +  return false;
> +}
> +
> +static bool curl_run (char * url, const char * * response)
> +{
> +  char curl_errbuf [CURL_ERROR_SIZE];
> +  struct curl_slist *list = NULL;
> +  std::ostringstream read_data_stream;
> +  CURLcode curl_res = CURLE_OK;
> +  long http_code = 0;
> +  CURL *curl = curl_easy_init();
> +  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_errbuf, &list, read_data_stream) ||

your setup_curl_session doesn't make much sense now. It could've been useful if one
could prepare the session once and then reuse it.
but the way you do it now, it's just draws a rather arbitrary line
where some one of the curl setup happens inside setup_curl_session function
and the rest in the caller.

better remove setup_curl_session and put that big if() here.

> +      (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)
> +  {
> +    if (list != NULL)
> +    {
> +      curl_slist_free_all(list);
> +    }
> +    my_printf_error(ER_UNKNOWN_ERROR,
> +                   get_error_from_curl(curl_res, curl_errbuf).c_str(),
> +                   ME_ERROR_LOG_ONLY);
> +    return true;
> +  }
> +  if (list != NULL)
> +  {
> +    curl_slist_free_all(list);
> +  }
> +  if (http_code == 404)
> +  {
> +    *response = NULL;
> +    return false;
> +  }
> +  *response = read_data_stream.str().c_str();

Really? Who owns this string? Will read_data_stream free it automatically
when it gets destructed?

> +  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/%u", vault_url, 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);

Why do you need to do that? create std::string, copy/reallocate it?
What's the point? Just do atol(ver)

> +}
> +
> +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 *)malloc(
> +    length_of_memory_needed_for_decode * sizeof(char));
> +  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);
> +    free(data);
> +    return ENCRYPTION_KEY_BUFFER_TOO_SMALL;
> +  }
> +  if (*buflen < (unsigned) decoded_length) {
> +    *buflen= decoded_length;
> +     free(data);
> +    return ENCRYPTION_KEY_BUFFER_TOO_SMALL;
> +  }
> +  *buflen= decoded_length;
> +  memcpy(dstbuf, data, decoded_length);
> +  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/%u?%u", vault_url, key_id, key_version);
> +  else
> +    snprintf(url, sizeof(url), "%s/%u", vault_url, 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;
> +    }
> +#ifndef NDEBUG
> +    /*
> +      An internal check that is needed only for debugging the plugin
> +      operation - in order to ensure that we get from the Hashicorp Vault
> +      server exactly the version of the key that is needed:
> +    */
> +    std::string ver_str = std::string(ver, ver_len);

same redundant std::string

> +    if (strtoul(ver_str.c_str(), NULL, 10) != key_version)
> +      return ENCRYPTION_KEY_VERSION_INVALID;
> +#endif
> +  }
> +  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);

Why? I don't see that in hashicorp docs that the key is base64-encoded

> +}
> +
> +struct st_mariadb_encryption hashicorp_key_management_plugin= {
> +  MariaDB_ENCRYPTION_INTERFACE_VERSION,
> +  get_latest_version,
> +  get_key_from_vault,
> +  0, 0, 0, 0, 0
> +};
> +
> +static int hashicorp_key_management_plugin_init(void *p)
> +{
> +  curl_global_init(CURL_GLOBAL_ALL);
> +  return 0;
> +}
> +
> +static int hashicorp_key_management_plugin_deinit(void *p)
> +{
> +  curl_global_cleanup();
> +  return 0;
> +}
> +
> +/*
> +  Plugin library descriptor
> +*/
> +maria_declare_plugin(hashicorp_key_management)
> +{
> +  MariaDB_ENCRYPTION_PLUGIN,
> +  &hashicorp_key_management_plugin,
> +  "hashicorp_key_management",
> +  "MariaDB Corporation",
> +  "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_ALPHA
> +}
> +maria_declare_plugin_end;

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