November 2019 - developers - lists.mariadb.org

Re: [Maria-developers] ef2519fee4e: MDEV-16546 System versioning setting to allow history modification
by Aleksey Midenkov 08 Jun '21

08 Jun '21

Hello, Sergei! On Fri, May 3, 2019 at 8:43 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On May 03, Aleksey Midenkov wrote: > > revision-id: ef2519fee4e (versioning-1.0.5-17-gef2519fee4e) > > parent(s): 56145be2951 > > author: Aleksey Midenkov <midenok(a)gmail.com> > > committer: Aleksey Midenkov <midenok(a)gmail.com> > > timestamp: 2018-06-28 13:42:09 +0300 > > message: > > > > MDEV-16546 System versioning setting to allow history modification > > > > 1. Add server variable system_versioning_modify_history which will > > allow to set values for row_start, row_end in DML operations. > > > > 2. If secure_timestamp is YES or REPLICATION, > > system_versioning_modify_history does not have effect. If > > secure_timestamp is SUPER, system_versioning_modify_history requires > > special privilege (same as for setting current timestamp). > > I thought more about this idea. We don't really want to have the history > editable, do we? Well, I'm thinking about rollback table data to specific point in time. That could be a useful feature. > But it's needed for replication, to keep the master and > slave identical. That's what secure_timestamp is for. > > The idea was that this new variable, system_versioning_modify_history, > will be just a convenience feature, it will not allow history editing > any more than one can do without it. > > But now I suspect that even with secure_timestamp=NO one cannot truly > edit history. One can only insert new rows with arbitrary timestamps. > For example, to insert a row with row_start=1000 and row_end=2000, one > needs to do (if secure_timestamp=NO): > > set timestamp=1000; > insert row; > set timestamp=2000; > delete row; > > But I don't see how one can update or delete a history row with > secure_timestamp=NO. > > Now, with a SUPER privilege and secure_timestamp=NO or SUPER, one can > use the BINLOG command and truly edit the history arbitrarily, by faking > row events. I don't really get it why this is so important: since there is some limitation by configuration and privilege, we are just fine. Everything can be changed at filesystem level after all. > > The conclusion, I believe, is that system_versioning_modify_history > should allow INSERTs when secure_timestamp=NO, and it should allow > UPDATE/DELETE only for a SUPER user when secure_timestamp=NO or SUPER. I don't see a reason to argue on that. The only thing that is not clear, why we don't allow INSERTs when secure_timestamp=SUPER? > > The second thing I don't like at all, is when a table is created like > > CREATE TABLE t1 (a int) WITH SYSTEM VERSIONING > > with row_start/row_end implicit. You don't have it in the test, but > anyway one should be able to load history into such a table, while the > table does not have row_start and row_end columns. From the user point > of view these columns don't exist, they're pseudo-columns, like ROWID. > They just cannot be insertable-into, conceptually. But a user will want > to restore the history, right? I don't have a solution for this yet :( > Any ideas? We don't have to follow the conception if it doesn't help us. Since we have physical row_start/row_end, we don't have to pretend they don't exist. Who will win from that? > > See below a couple of minor comments about the patch itself. > > ... These are going to be fixed. > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 15

Re: [Maria-developers] bd98ef106c8: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
by Sergei Golubchik 19 Feb '20

19 Feb '20

Hi, Aleksey! 1. It should be possible to enable/disable auto-creation. For example, CREATE TABLE ... PARTITION BY SYSTEM_TIME ... PARTITIONS AUTO; this solves few problems at once: * a user explicitly tells when auto-creation should work * you don't need to worry "if this name is already occupied" * you can define that these partitions can never overflow (for INTERVAL) * if you know that AUTO partitions never overflow, you can keep the old behavior for ALTER TABLE ADD PARTITION. 2. Separate thread is an interesting solution. Nicely avoids lots of problems. Still: * it's asynchronous, all tests have to take it into account * it's racy. "low probability" or not, with our number of users they'll hit it and rightfully will report it as a bug * if LOCK TABLE prevents it, partitions can overflow But I think these problems are easier to solve than those we'll face if auto-creation will happen in the main connection thread. So, let's keep your approach with a thread. But instead of going through the parser and mysql_execute_command, create a function that takes a TABLE or a TABLE_SHARE. And adds a partition there. It'll fix the "racy" part. This function can be called from a new thread. As for the LOCK TABLES - if you're under LOCK TABLES, you can simply call that function (to add a partition) directly at the end of the main INSERT/UPDATE/DELETE statement. It'll solve the last problem, LOCK TABLES won't prevent auto-creation. A couple of style comments: 1. create a one-liner make_partition_name (or something) with: sprintf(move_ptr, "p%u", i) and use it both in create_default_partition_names() and in your new code. Just to make sure partition names are always generated uniformly by the same function and if we'll want to change it to "pp%u" it should be done only in one place. 2. don't overload operators, use, say, bool greater_than(size_t seconds) But overall it's pretty good. A nice idea with a separate thread. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Nov 07, Aleksey Midenkov wrote: > revision-id: bd98ef106c8 (mariadb-10.4.7-55-gbd98ef106c8) > parent(s): 6d1fa01e8f1 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:01:14 +0300 > message: > > MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT > > When there are E empty partitions left, auto-create N new empty > partitions. > > This scheme must not allow partition overflow. I.e. E-fill time must > not exceed N-creation time. This means that low values for INTERVAL > and LIMIT must not be allowed for auto-creation. In case when overflow > is detected there is no need to do anything special: a warning will be > issued and the user will run manual rebuild to redistribute records > correctly. This is important because automatic ADD must be done fast, > without forced rebuild, by the obvious reason. > > Initial version implements hard-coded values of 1 for E and N. As well > as auto-creation threshold MinInterval = 1 hour, MinLimit = 1000. > > The name for newly added partition will be first chosen as "pX", where > X is partition number and "p" is hard-coded name prefix. If this name > is already occupied, the X will be incremented until the resulting > name will be free to use. > > Auto-creation mechanism is applied to every table having LIMIT or > INTERVAL clause. Note that there is no much sense in specifying > explicit partition list in this case and this is covered by > MDEV-19903. The syntax to explicitly turn it on/off as well as > user-defined values for E, N and name prefix is subject for further > discussion and feature requests. > > ALTER TABLE ADD PARTITION is now always fast. If there some history > partition overflow occurs manual ALTER TABLE REBUILD PARTITION is > needed.

2 5

Re: [Maria-developers] 6ac19d09c66: MDEV-16978 Application-time periods: WITHOUT OVERLAPS
by Sergei Golubchik 17 Dec '19

17 Dec '19

Hi, Nikita! On Nov 15, Nikita Malyavin wrote: > revision-id: 6ac19d09c66 (mariadb-10.4.4-280-g6ac19d09c66) > parent(s): 67ddb6507d5 > author: Nikita Malyavin <nikitamalyavin(a)gmail.com> > committer: Nikita Malyavin <nikitamalyavin(a)gmail.com> > timestamp: 2019-08-20 17:56:07 +1000 > message: > > MDEV-16978 Application-time periods: WITHOUT OVERLAPS > > * The overlaps check is implemented on a handler level per row > command. It creates a separate cursor (actually, another handler > instance) and caches it inside the original handler, when > ha_update_row or ha_insert_row is issued. Cursor closes on unlocking > the handler. > > * Containing the same key in index means unique constraint violation > even in usual terms. So we fetch left and right neighbours and check > that they have same key prefix, excluding from the key only the period > part. If it doesnt match, then there's no such neighbour, and the > check passes. Otherwise, we check if this neighbour intersects with > the considered key. > > * the check does introduce new error and fails with ER_DUPP_KEY error. > This might break REPLACE workflow and should be fixed separately > > diff --git a/sql/handler.cc b/sql/handler.cc > index 94cffd69b75..560f6316602 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6913,6 +6928,130 @@ void handler::set_lock_type(enum thr_lock_type lock) > table->reginfo.lock_type= lock; > } > > +int handler::ha_check_overlaps(const uchar *old_data, const uchar* new_data) > +{ > + DBUG_ASSERT(new_data); > + if (!table_share->period.unique_keys) > + return 0; > + if (table->versioned() && !table->vers_end_field()->is_max()) > + return 0; > + > + bool is_update= old_data != NULL; > + if (!check_overlaps_buffer) > + check_overlaps_buffer= (uchar*)alloc_root(&table_share->mem_root, > + table_share->max_unique_length > + + table_share->reclength); > + auto *record_buffer= check_overlaps_buffer + table_share->max_unique_length; > + auto *handler= this; > + if (handler->inited != NONE) > + { > + if (!check_overlaps_handler) > + { > + check_overlaps_handler= clone(table_share->normalized_path.str, > + &table_share->mem_root); wouldn't it be simpler and cheaper to use HA_EXTRA_REMEMBER_POS and HA_EXTRA_RESTORE_POS, as we've discussed? > + int error= -1; > + if (check_overlaps_handler != NULL) > + error= check_overlaps_handler->ha_external_lock(table->in_use, F_RDLCK); > + if (error) > + return error; > + } > + handler= check_overlaps_handler; > + } why "if (handler->inited != NONE)" ? What happens if it is NONE? > + > + for (uint key_nr= 0; key_nr < table_share->keys; key_nr++) > + { > + const KEY *key_info= table->key_info + key_nr; > + const uint key_parts= key_info->user_defined_key_parts; > + if (!key_info->without_overlaps) > + continue; > + > + key_copy(check_overlaps_buffer, new_data, key_info, 0); > + if (is_update) > + { > + bool key_used= false; > + for (uint k= 0; k < key_parts && !key_used; k++) > + key_used= bitmap_is_set(table->write_set, > + key_info->key_part[k].fieldnr - 1); > + if (!key_used) > + continue; > + } Good. > + > + int error= handler->ha_index_init(key_nr, 0); > + if (error) > + return error; > + > + for (int run= 0; run < 2; run++) > + { > + if (run == 0) > + { > + error = handler->ha_index_read_map(record_buffer, > + check_overlaps_buffer, > + key_part_map((1 << key_parts) - 1), > + HA_READ_KEY_OR_PREV); > + if (error == HA_ERR_KEY_NOT_FOUND) > + continue; > + } > + else > + { > + error = handler->ha_index_next(record_buffer); > + if (error == HA_ERR_END_OF_FILE) > + continue; > + } > + > + if (error) > + { > + handler->ha_index_end(); > + return error; > + } I found this "for (int run= 0; run < 2; run++)" rather confusing. Particularly as you have an if() to have different code for the first and the second runs. it would be clearer to unroll the loop and move the common code (period comparison) into a function. Like handler->ha_index_read_map(...); compare_periods(); handler->ha_index_next(); compare_periods(); But it can be done better. You search for the key with the same value and a period start <= than the period start of new row. And then you have to check two rows for overlaps. If you'll search for a key with the period start <= than the _period end_ of the new row, then you'll only need to check one row (may be one more, if updating). Also, why do you store both period ends in the index? It seems like it'd be enough to store only one end - only the start or only the end. Both ends help if you use keyreads, but you don't. On the second thought, perhaps, you should use keyreads, there's no need to fetch the whole row for this overlap check. On the yet another thought it might not work with HA_EXTRA_REMEMBER_POS. > + > + /* In case of update it could appear that the nearest neighbour is > + * a record we are updating. It means, that there are no overlaps > + * from this side. */ > + if (is_update && memcmp(old_data + table->s->null_bytes, > + record_buffer + table->s->null_bytes, > + table->s->reclength - table->s->null_bytes) == 0) > + { > + continue; > + } I'd rather compare row positions here. > + > + uint period_key_part_nr= key_parts - 2; > + int cmp_res= 0; > + for (uint part_nr= 0; !cmp_res && part_nr < period_key_part_nr; part_nr++) > + { > + Field *f= key_info->key_part[part_nr].field; > + cmp_res= f->cmp(f->ptr_in_record(new_data), > + f->ptr_in_record(record_buffer)); > + } > + if (cmp_res) > + continue; /* key is different => no overlaps */ > + > + int period_cmp[2][2]= {/* l1 > l2, l1 > r2, r1 > l2, r1 > r2 */}; > + for (int i= 0; i < 2; i++) > + { > + for (int j= 0; j < 2; j++) > + { > + Field *lhs= key_info->key_part[period_key_part_nr + i].field; > + Field *rhs= key_info->key_part[period_key_part_nr + j].field; > + > + period_cmp[i][j]= lhs->cmp(lhs->ptr_in_record(new_data), > + rhs->ptr_in_record(record_buffer)); > + } > + } this can be simplified too if you'll change the code as I suggested above to do only one index read - you'll only need one comparison, instead of four. > + > + if ((period_cmp[0][0] <= 0 && period_cmp[1][0] > 0) > + || (period_cmp[0][0] >= 0 && period_cmp[0][1] < 0)) > + { > + handler->ha_index_end(); > + return HA_ERR_FOUND_DUPP_KEY; > + } > + } > + error= handler->ha_index_end(); > + if (error) > + return error; > + } > + return 0; > +} > + > #ifdef WITH_WSREP > /** > @details > diff --git a/sql/handler.h b/sql/handler.h > index 63d0bf2215c..71debf9bab7 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -3237,6 +3243,8 @@ class handler :public Sql_alloc > DBUG_ASSERT(cached_table_flags < (HA_LAST_TABLE_FLAG << 1)); > return cached_table_flags; > } > + /** PRIMARY KEY WITHOUT OVERLAPS check is done globally */ what do you mean "globally"? > + int ha_check_overlaps(const uchar *old_data, const uchar* new_data); > /** > These functions represent the public interface to *users* of the > handler class, hence they are *not* virtual. For the inheritance > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 9bb1d98152b..5aba86003c6 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -4549,25 +4552,57 @@ static bool vers_prepare_keys(THD *thd, HA_CREATE_INFO *create_info, > if (key->type != Key::PRIMARY && key->type != Key::UNIQUE) > continue; > > - Key_part_spec *key_part= NULL; > - List_iterator<Key_part_spec> part_it(key->columns); > - while ((key_part=part_it++)) > + if (create_info->versioned()) > { > - if (!my_strcasecmp(system_charset_info, > - row_start_field, > - key_part->field_name.str) || > - > - !my_strcasecmp(system_charset_info, > - row_end_field, > - key_part->field_name.str)) > - break; > + Key_part_spec *key_part=NULL; > + List_iterator<Key_part_spec> part_it(key->columns); > + while ((key_part=part_it++)) > + { > + if (row_start_field.streq(key_part->field_name) || > + row_end_field.streq(key_part->field_name)) > + break; > + } > + if (!key_part) > + key->columns.push_back(new Key_part_spec(&row_end_field, 0)); > } > - if (key_part) > - continue; // Key already contains Sys_start or Sys_end > + } > > - Key_part_spec *key_part_sys_end_col= > - new (thd->mem_root) Key_part_spec(&create_info->vers_info.as_row.end, 0); > - key->columns.push_back(key_part_sys_end_col); > + key_it.rewind(); > + while ((key=key_it++)) > + { > + if (key->without_overlaps) > + { > + if (key->type != Key::PRIMARY && key->type != Key::UNIQUE) > + { > + my_error(ER_PERIOD_WITHOUT_OVERLAPS_NON_UNIQUE, MYF(0), key->period.str); > + return true; > + } > + if (!create_info->period_info.is_set() > + || !key->period.streq(create_info->period_info.name)) > + { > + my_error(ER_PERIOD_NOT_FOUND, MYF(0), key->period.str); > + return true; > + } > + if (thd->work_part_info) > + { > + my_error(ER_PERIOD_WITHOUT_OVERLAPS_PARTITIONED, MYF(0)); why? > + return true; > + } > + const auto &period_start= create_info->period_info.period.start; > + const auto &period_end= create_info->period_info.period.end; > + List_iterator<Key_part_spec> part_it(key->columns); > + while (Key_part_spec *key_part= part_it++) > + { > + if (period_start.streq(key_part->field_name) > + || period_end.streq(key_part->field_name)) > + { > + my_error(ER_KEY_CONTAINS_PERIOD_FIELDS, MYF(0), key->name.str); > + return true; > + } > + } > + key->columns.push_back(new Key_part_spec(&period_start, 0)); > + key->columns.push_back(new Key_part_spec(&period_end, 0)); > + } > } > > return false; > diff --git a/sql/table.h b/sql/table.h > index 2b866159fe0..6511960488e 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -1730,10 +1731,18 @@ class IS_table_read_plan; > > /** number of bytes used by field positional indexes in frm */ > constexpr uint frm_fieldno_size= 2; > +/** number of bytes used by key position number in frm */ > +constexpr uint frm_keyno_size= 2; > static inline uint16 read_frm_fieldno(const uchar *data) > { return uint2korr(data); } > -static inline void store_frm_fieldno(const uchar *data, uint16 fieldno) > +static inline void store_frm_fieldno(uchar *data, uint16 fieldno) > { int2store(data, fieldno); } > +static inline uint16 read_frm_keyno(const uchar *data) > +{ return uint2korr(data); } > +static inline void store_frm_keyno(uchar *data, uint16 fieldno) > +{ int2store(data, fieldno); } > +static inline size_t frm_ident_stored_size(size_t len) > +{ return len + (len > 255 ? 3 : 1); } this is exactly extra2_str_size() function. Don't duplicate it, reuse. (and rename, if you'd like a more generic name) > > class select_unit; > class TMP_TABLE_PARAM; > diff --git a/sql/unireg.cc b/sql/unireg.cc > index 7130b3e5d8a..1e95242786e 100644 > --- a/sql/unireg.cc > +++ b/sql/unireg.cc > @@ -485,6 +486,16 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING &table, > store_frm_fieldno(pos, get_fieldno_by_name(create_info, create_fields, > create_info->period_info.period.end)); > pos+= frm_fieldno_size; > + store_frm_keyno(pos, create_info->period_info.unique_keys); > + pos+= frm_keyno_size; > + for (uint key= 0; key < keys; key++) > + { > + if (key_info[key].without_overlaps) > + { > + store_frm_keyno(pos, key); > + pos+= frm_keyno_size; > + } > + } This will make 10.5 frms look corrupted in 10.4. Better use a new EXTRA2 flag for that. With a number > EXTRA2_ENGINE_IMPORTANT. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 20

[Maria-developers] MariaDB for CentOS 8
by Thomas Stephen Lee 12 Dec '19

12 Dec '19

Hi, Please release MariaDB 10.4 for CentOS 8. https://downloads.mariadb.org/mariadb/repositories/ does not show CentOS 8. thanks -- Thomas Stephen Lee

3 5

Re: [Maria-developers] 387839e44cc: Plugin for Hashicorp Vault Key Management System
by Sergei Golubchik 28 Nov '19

28 Nov '19

Hi, Julius! On Nov 28, Julius Goryavsky wrote: > Hi, Sergey, > > many thanks for the detailed review, I fixed all the flaws and left only > what raises questions or where I have explanations: > > >> + 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. > > I have already come across servers that respond with the wrong content-type > if the input request had a different content-type or if the input request > does not contain a content-type at all. Fortunately, the Hashicorp server > works without setting the correct content-type in the input, but may be > more reliable to set it explicitly? I don't understand. Content-Type in the request header does not mean that you *accept* application/json, it means you are *sending* application/json. And you are not, you are sending no body at all, just the header. > >> + (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. > > I don’t know, we have a guarantee that if this option is not set, then the > corresponding variable will always point to an empty zero-terminated > string, but will never just be NULL instead? (This is the path to the file > with the CA bundle. In principle, a Hashicorp Vault can work without it, it > is needed only if there is no root or intermediate certificates on the > other side.) Yes, I believe vault_ca can never be NULL. The default value is "", so it'll be either that or a string specified on the command line. > > > + 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) > > Does json_get_object_key return a new zero-terminated string? atol cannot > be applied to a fragment with a length (without trailing zero), as far as I > know. No, it doesn't return a zero-terminated string. It returns a pointer into a valid json string. So you can be sure that ver[ver_len] is a non-digit character (it'll be '\n' or, may be, '}'). Which is enough for atol(). > >> + 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 > > But after all, as far as I understand at this level, the key for us is > arbitrary binary data? What if it contains all kinds of quotation marks, > commas, or, UTF-8 escape codes and all kinds of unprintable characters like > a zero? I thought that it should be base64-endoded if it is binary data, so > that we can pass it through JSON encoding in the form of text that will not > cause a failure in the JSON parser. I believe that with the proper escaping json can handle any binary data just fine. With your base64 requirement you basically mean that one cannot use 3rd party tools to create/manage keys, unless the tool has an option to store the keys base64-encoded. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 387839e44cc: Plugin for Hashicorp Vault Key Management System
by Sergei Golubchik 27 Nov '19

27 Nov '19

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(a)mariadb.com> > committer: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > 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(a)mariadb.org

1 0

Re: [Maria-developers] c23c5671a60: Simplified version
by Sergei Golubchik 23 Nov '19

23 Nov '19

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(a)mariadb.com> > committer: Julius Goryavsky <julius.goryavsky(a)mariadb.com> > 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(a)mariadb.org

1 0

Re: [Maria-developers] systemd changes in Debian Sid
by Faustin Lammler 18 Nov '19

18 Nov '19

Hi Otto! Otto Kekäläinen <otto(a)debian.org>, 11/11/2019 – 15:06:24 (+0100): > Hello Faustin! > > I have been updating the MariaDB 10.3 packaging in Debian to compat level > 12, and that includes some changes to how the systemd services are > installed. Do you have any comments? > > https://salsa.debian.org/mariadb-team/mariadb-10.3/commits/bugfix/systemd-r… See my comment on the d14de1c0 WIP commit. > Are there some systemd issues regarding 10.3 that you think we should put > in while at this? Yes and it is related to all the extra work that is needed to support both mysql and mariadb systemctl commands (see my comment and Ondrej commit referenced in it). IMO we should definitively remove aliases from systemd unit files (also suggested by M. Biebl #932289) but I did not have time to get things to move on (https://github.com/MariaDB/server/pull/1172 or MDEV-15526) and if possible I believe that we should try to upstream it first. So for the moment I have no other changes to suggest in salsa. Faustin

1 0

Re: [Maria-developers] systemd changes in Debian Sid
by Faustin Lammler 12 Nov '19

12 Nov '19

Hi Otto! I did not had time to look at this for now. I believe I will have a bit of time before the week-end and surely next week. Faustin Otto Kekäläinen <otto(a)debian.org>, 11/11/2019 – 15:06:24 (+0100): > Hello Faustin! > > I have been updating the MariaDB 10.3 packaging in Debian to compat level > 12, and that includes some changes to how the systemd services are > installed. Do you have any comments? > > https://salsa.debian.org/mariadb-team/mariadb-10.3/commits/bugfix/systemd-r… > > Are there some systemd issues regarding 10.3 that you think we should put > in while at this? > > - Otto

1 0

Re: [Maria-developers] c790763e1f0: MDEV-19903 Setup default partitions for system versioning
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Aleksey! Looks good! Ok to push On Nov 06, Aleksey Midenkov wrote: > revision-id: c790763e1f0 (mariadb-10.4.7-51-gc790763e1f0) > parent(s): f10c9ac941e > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:00:33 +0300 > message: > > MDEV-19903 Setup default partitions for system versioning > > Implement syntax like: > > create table t1 (x int) with system versioning partition by system_time; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4de59fa0f92: MENT-360: ASAN heap-use-after-free in strmake_root / Query_arena::strmake
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Oleksandr! On Nov 06, Oleksandr Byelkin wrote: > revision-id: 4de59fa0f92 (mariadb-10.2.27-129-g4de59fa0f92) > parent(s): 4b21495343d > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2019-10-18 15:14:20 +0200 > message: > > MENT-360: ASAN heap-use-after-free in strmake_root / Query_arena::strmake > > Do not allow thd->query point to freed memory, use safe method to set query. > > --- > mysql-test/r/processlist.result | 32 +++++++++++++++++++++++ > mysql-test/t/processlist.test | 58 +++++++++++++++++++++++++++++++++++++++++ > sql/sql_prepare.cc | 7 ++++- > 3 files changed, 96 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/processlist.result b/mysql-test/r/processlist.result > index ab518d961ef..066154feaf1 100644 > --- a/mysql-test/r/processlist.result > +++ b/mysql-test/r/processlist.result > @@ -43,3 +43,35 @@ Message Incorrect string value: '\xF0\x9F\x98\x8Eyy...' for column `information_ > # > # End of 10.1 tests > # > +# > +# MENT-360: ASAN heap-use-after-free in strmake_root / > +# Query_arena::strmake > +# > +CREATE PROCEDURE pr1() > +BEGIN > +DECLARE CONTINUE HANDLER FOR 1146 SET @a = 1; > +DECLARE CONTINUE HANDLER FOR 1243 SET @a = 1; > +LOOP > +PREPARE stmt FROM "CREATE TEMPORARY TABLE ps AS SELECT * FROM non_existing_table"; > +EXECUTE stmt; > +END LOOP; > +END $ > +CREATE PROCEDURE pr2() > +BEGIN > +DECLARE CONTINUE HANDLER FOR 1094 SET @a = 1; > +LOOP > +SHOW EXPLAIN FOR 1; > +END LOOP; > +END $ > +connect con1,localhost,root,,; > +CALL pr1(); > +connect con2,localhost,root,,; > +CALL pr2(); > +connection default; > +KILL 6; > +KILL 7; ^^^ you forgot replace_result also, mysqltest was designed for deterministic test cases, it'd be better to rewrite this test to be deterministic, if possible. > +DROP PROCEDURE pr1; > +DROP PROCEDURE pr2; > +# > +# end of Enterprise tests > +# > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 525f09d611b..d0043979135 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -2885,6 +2885,11 @@ void mysql_sql_stmt_prepare(THD *thd) > > if (stmt->prepare(query.str, (uint) query.length)) > { > + /* > + stmt->prepare() sets thd->query_string, so we have to reset it beck, > + before it will point to uninitialised memory > + */ > + thd->set_query(orig_query); > /* Statement map deletes the statement on erase */ > thd->stmt_map.erase(stmt); > } > @@ -2902,7 +2907,7 @@ void mysql_sql_stmt_prepare(THD *thd) > But here we should restore the original query so it's mentioned in > logs properly. > */ > - thd->set_query_inner(orig_query); > + thd->set_query(orig_query); > DBUG_VOID_RETURN; > } may be better to have set_query() only once, like in bool res=stmt->prepare(query.str, (uint) query.length); thd->set_query(orig_query); if (res) ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] aeb3432849e: MDEV-20076: SHOW GRANTS does not quote role names properly
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Oleksandr! On Nov 06, Oleksandr Byelkin wrote: > revision-id: aeb3432849e (mariadb-10.3.19-2-gaeb3432849e) > parent(s): 5d3bd2b75b5 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2019-11-06 12:35:19 +0100 > message: > > MDEV-20076: SHOW GRANTS does not quote role names properly > > Quotes added to output. > > +# > +# MDEV-20076: SHOW GRANTS does not quote role names properly > +# > +create role 'role-1'; > +create user 'user-1'@'localhost'; > +grant select on mysql.user to 'role-1'; > +GRANT 'role-1' TO 'user-1'@'localhost'; > +show grants for 'role-1'; > +Grants for role-1 > +GRANT USAGE ON *.* TO 'role-1' > +GRANT SELECT ON `mysql`.`user` TO 'role-1' > +show grants for 'user-1'@'localhost'; > +Grants for user-1@localhost > +GRANT 'role-1' TO 'user-1'@'localhost' > +GRANT USAGE ON *.* TO 'user-1'@'localhost' > +drop role 'role-1'; > +drop user 'user-1'@'localhost'; Please, add a test for names that contains a single quote. Like create role `role'1`; create user `user'1`@localhost; and the rest as in your test above. Or, if you want it to look a bit less artificial create role `rock'n'roll`; create user `O'Brien`@localhost; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0