PS-4556: Implement innodb redo log encryption #2439

dutow · 2018-06-28T14:53:41Z

This is based on WL#9290 in MySQL 8.0, while also adding a keyring based rotated key encryption.

The encryption setting supports two options:

master_key, which works like the redo log encryption in MySQL 8.0
keyring_key, which encrypts each new redo log tablespace with the latest percona_redo key from the keyring.

Related commits in upstream 8.0:

laurynas-biveinis

Please reference backported commits

laurynas-biveinis · 2018-06-29T06:30:35Z

client/mysqltest.cc

+  Remove the directory <dir_name>
+*/
+
+void do_force_rmdir(struct st_command *command, DYNAMIC_STRING *ds_dirname)


laurynas-biveinis · 2018-06-29T06:32:58Z

mysql-test/suite/innodb/t/log_encrypt_1.test

+
+#Enable redo log encryption, should report error in server log, since keyring is not loaded.
+SET GLOBAL innodb_redo_log_encrypt = ON;
+--sleep 1


Needs proper synchronization

laurynas-biveinis · 2018-06-29T06:33:20Z

mysql-test/suite/innodb/t/log_encrypt_1.test

+
+#Enable redo log encryption
+SET GLOBAL innodb_redo_log_encrypt = ON;
+--sleep 1


laurynas-biveinis · 2018-06-29T06:33:44Z

mysql-test/suite/innodb/t/log_encrypt_2.test

+call mtr.add_suppression("You need to use --log-bin to make --binlog-format work");
+--enable_query_log
+
+--sleep 2


laurynas-biveinis · 2018-06-29T06:33:55Z

mysql-test/suite/innodb/t/log_encrypt_2.test

+
+DROP TABLE t1;
+
+# Restart the server with keyring loaded


No custom restart sequences, use includes

laurynas-biveinis · 2018-06-29T06:50:52Z

storage/innobase/os/os0file.cc

+
+		ut_ad(static_cast<ulint>(elen) == main_len);
+
+		/* Copy the remain bytes. */


comment not addressed

laurynas-biveinis · 2018-06-29T06:51:20Z

storage/innobase/os/os0file.cc

+	byte*		ptr = src;
+	dberr_t		ret;
+
+	/* Do nothing if it's not a log request. */


assert is not a "do nothing"

laurynas-biveinis · 2018-06-29T06:51:29Z

storage/innobase/os/os0file.cc

+
+	/* Encrypt the log blocks one by one. */
+	while (ptr != src + src_len) {
+    //ut_a((m_type == NONE) == !is_encrypted_log(ptr));


laurynas-biveinis · 2018-06-29T06:51:39Z

storage/innobase/os/os0file.cc

+			log_block_get_hdr_no(ptr));
+		ut_print_buf_hex(std::cerr, ptr, OS_FILE_LOG_BLOCK_SIZE);
+		fprintf(stderr, "\n");
+	ut_print_buf(stderr, m_key, 32);


Indentation

laurynas-biveinis · 2018-06-29T06:51:53Z

storage/innobase/os/os0file.cc

@@ -9496,6 +10250,8 @@ Encryption::decrypt(

 	fprintf(stderr, "Decrypting page:%lu.%lu len:%lu\n",
 		space_id, page_no, src_len);
+	//ut_print_buf(stderr, m_key, 32);


robgolebiowski · 2018-06-29T11:28:28Z

CMakeLists.txt

@@ -13,6 +13,8 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA

+ADD_DEFINITIONS(-DUNIV_ENCRYPT_DEBUG)


Remove before merging

robgolebiowski · 2018-06-29T11:56:06Z

mysql-test/suite/innodb/t/log_encrypt_1.test

@@ -0,0 +1,134 @@
+# WL#9290 InnoDB: Support transparent tablespace data encryption for redo log


These tests will need to be made a bit more "abstract" so it could be run with different keyrings - keyring_file, keyring_vault and the future keyrings. Have a look at suite/innodb/table_encrypt_1.test (version for keyring_file) and at keyring_vault/tests/table_encrypt_1.test (version for keyring_vault).

robgolebiowski · 2018-06-29T12:03:23Z

mysql-test/suite/innodb/t/log_encrypt_1.test

+# WL#9290 InnoDB: Support transparent tablespace data encryption for redo log
+# This test case will test basic redo log encryption support features.
+
+--source include/no_valgrind_without_big.inc


What I am missing in those tests are actual server crashes that will check if the innodb checkpoint has not been reached. Please have a look at test from MariaDB repo: innodb-redo-badkey.test and innodb-redo-nokey.test.

robgolebiowski · 2018-06-29T12:32:16Z

storage/innobase/fil/fil0fil.cc

@@ -7380,8 +7388,6 @@ fil_set_encryption(
 		return(DB_NOT_FOUND);
 	}

-	ut_ad(algorithm != Encryption::NONE);


Why this was moved to the bottom of the function ? Seems like unnecessary change.

robgolebiowski · 2018-06-29T12:40:32Z

storage/innobase/fsp/fsp0fsp.cc

+		master_key_id = 0;
+		master_key = static_cast<byte*>(ut_zalloc_nokey(
+		                                ENCRYPTION_KEY_LEN));
+		memcpy(master_key, ENCRYPTION_DEFAULT_MASTER_KEY,


How does it work - in case of boot we are using predefined master key ? why ? This means we could not encrypt this at all ...

robgolebiowski · 2018-06-29T12:58:41Z

storage/innobase/fsp/fsp0fsp.cc

@@ -211,6 +211,21 @@ fsp_flags_to_dict_tf(

 	return(flags);
 }
+
+bool
+fsp_is_undo_tablespace(uint32 space_id)


unused function ?

robgolebiowski · 2018-06-29T14:16:48Z

storage/innobase/os/os0file.cc

+		if (master_key == NULL) {
+			return(false);
+		}
+	}


I do not see information on key version. Please have a look into my email "Supporting different key versions in redo log". Since we are porting this from MySQL, we need to add support for key versioning.

dutow · 2018-09-27T06:26:23Z

PR updated.

Major changes:

Changed the variable from ON/OFF to MASTER_KEY/ROTATED_KEY/OFF
Rotated key encryption uses a key named "percona_redo". Compared to mariadb, key version is per file instead of per block, and the key version is saved in the redo log file header. (This works without changing the log format; the other doesn't)
Generalized the tests with include files
Added more tests (tests which also tests undo log encryption in 8.0, but I removed the undo specific parts from them)
Increased the data size in some tests (so they do detect some issues instead of passing even with a partially broken encryption)

There are a few formatting issues left / I left a numeric magic constant in the code which should be a define, and maybe a few assertions should be correct error messages instead (they would be quite complex to reach in a real use case, but still) - I'll fix those together with the next round of review comments.

laurynas-biveinis

Spurious submodule updates
Empty commit message, backported commit(s) not referenced

laurynas-biveinis · 2018-10-03T10:04:23Z

mysql-test/include/log_encrypt_2.inc

+DROP TABLE t1;
+
+# Restart the server with keyring loaded
+--exec echo "wait" > $MYSQLTEST_VARDIR/tmp/mysqld.1.expect


Use include files, never a hand-coded restart sequence

laurynas-biveinis · 2018-10-03T10:04:39Z

mysql-test/include/log_encrypt_1.inc

+call mtr.add_suppression("\\[ERROR\\] InnoDB: Can't set redo log tablespace to be encrypted.");
+--enable_query_log
+
+#Enable redo log encryption, should report error in server log, since keyring is not loaded.


Space after #

laurynas-biveinis · 2018-10-03T10:04:59Z

mysql-test/include/log_encrypt_2.inc

+--source include/have_innodb.inc
+--source include/not_embedded.inc
+
+#Suppress following messages from myslqd log


Space after #, "mysqld", but the whole comment looks redundant

mysql-test/include/log_encrypt_2.inc

laurynas-biveinis · 2018-10-03T10:05:38Z

mysql-test/include/log_encrypt_2.inc

+INSERT INTO t1 select * from t1;
+
+# Restart to confirm the encryption info can be retrieved properly.
+--source include/kill_mysqld.inc


kill_and_restart_mysqld.inc

laurynas-biveinis · 2018-10-03T13:12:29Z

storage/innobase/os/os0file.cc

+	byte*			src_ptr,
+	byte*			dst_ptr)
+{
+	ulint		len = 0;


Declare locals at first use, const when possible

laurynas-biveinis · 2018-10-03T13:20:59Z

storage/innobase/os/os0file.cc

+	byte*			src,
+	byte*			dst)
+{
+	ulint		data_len;


Declare locals at first use, const when possible

laurynas-biveinis · 2018-10-03T13:21:09Z

storage/innobase/os/os0file.cc

+	remain_len = data_len - main_len;
+
+	ptr += LOG_BLOCK_HDR_SIZE;
+	switch(m_type) {


Space after switch

laurynas-biveinis · 2018-10-03T13:21:31Z

storage/innobase/os/os0file.cc

+
+		ut_ad(static_cast<ulint>(elen) == main_len);
+
+		/* Copy the remain bytes. */


comment not addressed

laurynas-biveinis · 2018-10-03T13:22:21Z

storage/innobase/srv/srv0start.cc

@@ -460,6 +460,26 @@ create_log_files(
 	ut_a(fil_validate());
 	ut_a(log_space != NULL);

+      /* Once the redo log is set to be encrypted,
+       initialize encryption information. */


robgolebiowski · 2018-10-03T15:50:40Z

Possible values for the variable should be : MASTER_KEY/KEYRING_KEY

satya-bodapati · 2018-10-03T18:48:43Z

One suggestion about keyring loading and suppression messages in test. There is no need for suppression message (unless explicitly caused by test).

The warnings come from non availablity of keyring plugin at startup. I have modified MTR to provide a way to 'early-load' plugin. See mysql-test/suite/innodb/t/percona_sys_tablespace_encrypt-master.opt

$KEYRING_PLUGIN_OPT
$KEYRING_PLUGIN_EARLY_LOAD
--keyring_file_data=$MYSQL_TMP_DIR/mysecret_keyring

You don't see a single suppression in the mysql-test/suite/innodb/t/percona_sys_tablespace_encrypt.test. This is because of KEYRING_PLUGIN_EARLY_LOAD changes to MTR framework

Another benefit of this KEYRING_PLUGIN_EARLY_LOAD is that all subsequent restarts automatically do early-plugin-load.

I know that Oracle tests have these suppressions but we can get rid most of them with the above suggested changes.

dutow · 2018-10-05T11:44:51Z

Updated.

Fixed test / source code comments
Changed the asserts to print out actual error messages - those should only happen if something in the keyring plugin is broken, but this way even that reports an error.
Renamed ROTATED_KEY to KEYRING_KEY
Removed warning supressions from most tests. I couldn't remove it from all, as some tests do generate those errors/warnings to test them.

laurynas-biveinis

Please see the comments.
After the PR is merged, please figure out how compatible redo log encryption with redo log reading in changed page tracking - if not, please file a JIRA issue

laurynas-biveinis · 2018-10-19T13:19:34Z

mysql-test/include/log_encrypt_2.inc

+let $innodb_file_per_table = `SELECT @@innodb_file_per_table`;
+
+SET GLOBAL innodb_file_per_table = 1;
+SELECT @@innodb_file_per_table;


Two very redundant lines, better to use assert.inc or nothing. OK to fix after the merge or never.

laurynas-biveinis · 2018-10-19T13:20:35Z

mysql-test/include/log_encrypt_3.inc

+--source include/restart_mysqld_no_echo.inc
+
+SET GLOBAL innodb_redo_log_encrypt = 0;
+SELECT @@global.innodb_redo_log_encrypt;


Not sure what these SELECTs past SET are supposed to test (OK to fix post-merge or never)

laurynas-biveinis · 2018-10-19T13:24:53Z

storage/innobase/fil/fil0fil.cc

@@ -7512,7 +7519,9 @@ fil_encryption_rotate()
 	for (space = UT_LIST_GET_FIRST(fil_system->space_list);
 	     space != NULL; ) {
 		/* Skip unencypted tablespaces. */
-		if (srv_is_undo_tablespace(space->id)
+		/* Encrypted redo log tablespaces is handled in function


"tablespaces are"
Fix only if you have to re-push and re-test otherwise

laurynas-biveinis · 2018-10-19T13:25:25Z

storage/innobase/fsp/fsp0fsp.cc

@@ -985,6 +1015,7 @@ fsp_header_fill_encryption_info(
 	mach_write_to_4(ptr, crc);

 	my_free(master_key);
+


Spurious diff

laurynas-biveinis · 2018-10-19T13:26:19Z

storage/innobase/include/log0log.h

@@ -139,7 +139,12 @@ Closes the log.
 @return lsn */
 lsn_t
 log_close(void);
-/*===========*/
+
+/************************************************************//**


/** Enables redo ... */
fix only if you have to re-push/re-test the PR otherwise

laurynas-biveinis · 2018-10-19T13:26:50Z

storage/innobase/include/log0log.h

+@param[in]	val		value to set */
+UNIV_INLINE
+void
+log_block_set_encrypt_bit(


log_block_set_encrypt_bit(byte* log_block, bool val);
Fix only if you have to re-push/re-test otherwise

laurynas-biveinis · 2018-10-19T13:42:17Z

storage/innobase/log/log0log.cc

+			char *redo_key_type = NULL;
+			byte *rkey = NULL;
+			unsigned char *rkey2 = NULL;
+			if(my_key_fetch(PERCONA_REDO_KEY_NAME, &redo_key_type, NULL,


Still space after if, but only fix if you have to re-push/re-test otherwise

laurynas-biveinis · 2018-10-19T13:44:05Z

storage/innobase/os/os0file.cc

@@ -9061,7 +9178,7 @@ Encryption::create_master_key(byte** master_key)

 	if (ret || *master_key == NULL) {
 		ib::error() << "Encryption can't find master key, please check"
-				" the keyring plugin is loaded.";
+			       " the keyring plugin is loaded.";


Spurious diffs, but only fix if you have to re-push

laurynas-biveinis · 2018-10-19T13:44:52Z

storage/innobase/os/os0file.cc

+		if (m_key_id == 0) {
+			/* When m_key_id is 0, which means it's the
+			default master key for bootstrap. */
+			master_key = static_cast<byte*>(ut_zalloc_nokey(


So what is your opinion about considering using my_ allocation?
(by now only change something if you have to re-push/re-test otherwise)

dutow · 2018-10-22T04:47:22Z

I fixed the formatting only comments, as I forgot to add the rerecorded result of perfschema.show_sanity. Will

I also found out, that the sleeps were likely there to "fix" CI issues, as without those, jenkins release jobs report test failures: https://ps.cd.percona.com/job/percona-server-5.7-param/89/
I can't reproduce this exact failure locally even with a high parallel repeated test of my new tests :( (assertion failure in a release build, uh), but I do see some failures under heavy loads locally - it just needed a lot more parallel processes than I originally used for testing. I'll try to do some more tests for this later today, but the same can happen with 8.0 & without the sleeps.

dutow · 2018-10-22T05:34:28Z

I fixed the formatting only comments, as I forgot to add the rerecorded result of perfschema.show_sanity. Will

I also found out, that the sleeps were likely there to "fix" CI issues, as without those, jenkins release jobs report test failures: https://ps.cd.percona.com/job/percona-server-5.7-param/89/
I can't reproduce this exact failure locally even with a high parallel repeated test of my new tests :( (assertion failure in a release build, uh), but I do see some failures under heavy loads locally - it just needed a lot more parallel processes than I originally used for testing. I'll try to do some more tests for this later today, but the same can happen with 8.0 & without the sleeps.

laurynas-biveinis · 2018-10-22T08:02:03Z

Sleep presence should not block trunk merge in this case, although we will must fix them there afterwards

dutow · 2018-10-24T11:09:04Z

Last update, with disabled tests: https://ps.cd.percona.com/job/percona-server-5.7-param/97/#showFailuresLink

No failure which occurs on every configuration, except for percona_bug1289599 which seems unrelated.

This PR implements Encryption threads for encryption/re-encryption/decryption of innodb tablespaces. It is based on MariaDB implementation ported from Google's patch. With this WL user can: 1) Change default KEYRING encryption key 2) Create a new table with KEYRING 3) Encrypt offline already existing tables with KEYRING (with alter) 4) Encrypt/re-encrypt online already existing tables with KEYRING (with innodb_online_encryption, innodb_online_encryption_rotate_key_age variables and innodb_online_encryption_threads) – including tables already encrypted with Master Key encryption. 5) Re-encrypt online already encrypted tables with newer version of encryption key (with variables innodb_online_encryption variable, innodb_online_encryption_threads, innodb_online_encryption_rotate_key_age). This WL also fixed the following bugs reported to MariaDB: (MDEV-17231) Encryption threads keep re-encrypting/encrypting corrupted pages (MDEV-17235) Data dictonary is not updated with encryption flags (MDEV-17234) In memory crypt_data is updated before page0 is updated (MDEV-17233) Page 0 is updated more than once when encryption completes (MDEV-17230) encryption_key_id from alter is ignored by encryption threads (MDEV-17229) Encryption threads ignore innodb_default_encryption_key_id

This is based on WL#9290 in MySQL 8.0, while also adding a keyring based rotated key encryption. The encryption setting supports two options: * master_key, which works like the redo log encryption in MySQL 8.0 * keyring_key, which encrypts each new redo log tablespace with the latest percona_redo key from the keyring. Related commits in upstream 8.0: * 1a6dd57 * 8facbb0

dutow force-pushed the ps-5.7-redo-log-encryption branch 3 times, most recently from b666973 to bae1cb0 Compare June 28, 2018 15:08

laurynas-biveinis suggested changes Jun 29, 2018

View reviewed changes

robgolebiowski suggested changes Jun 29, 2018

View reviewed changes

robgolebiowski assigned dutow Jun 29, 2018

dutow force-pushed the ps-5.7-redo-log-encryption branch from bae1cb0 to 07cbc21 Compare September 5, 2018 07:03

dutow force-pushed the ps-5.7-redo-log-encryption branch 3 times, most recently from f1a9fef to a0df22e Compare September 27, 2018 06:18

laurynas-biveinis suggested changes Oct 3, 2018

View reviewed changes

dutow force-pushed the ps-5.7-redo-log-encryption branch 2 times, most recently from 03f3794 to b56b453 Compare October 5, 2018 11:44

dutow force-pushed the ps-5.7-redo-log-encryption branch from b56b453 to dcf5003 Compare October 5, 2018 11:45

dutow force-pushed the ps-5.7-redo-log-encryption branch 9 times, most recently from 5cb8041 to 184eae1 Compare October 18, 2018 11:55

laurynas-biveinis approved these changes Oct 19, 2018

View reviewed changes

dutow force-pushed the ps-5.7-redo-log-encryption branch 2 times, most recently from 5b972c9 to 522222d Compare October 22, 2018 04:46

dutow force-pushed the ps-5.7-redo-log-encryption branch 4 times, most recently from 8f95e9d to f756c78 Compare October 24, 2018 05:46

dutow force-pushed the ps-5.7-redo-log-encryption branch 2 times, most recently from 22d210c to 35c1a8a Compare October 30, 2018 05:06

Robert Golebiowski and others added 2 commits October 30, 2018 18:57

dutow force-pushed the ps-5.7-redo-log-encryption branch from 35c1a8a to 243e2d8 Compare October 30, 2018 18:40

dutow merged commit a705dd5 into percona:5.7 Oct 31, 2018

dutow deleted the ps-5.7-redo-log-encryption branch October 31, 2018 07:54


		ut_ad(static_cast<ulint>(elen) == main_len);

		/* Copy the remain bytes. */

		@@ -0,0 +1,134 @@
		# WL#9290 InnoDB: Support transparent tablespace data encryption for redo log

		@@ -985,6 +1015,7 @@ fsp_header_fill_encryption_info(
		mach_write_to_4(ptr, crc);

		my_free(master_key);

PS-4556: Implement innodb redo log encryption #2439

PS-4556: Implement innodb redo log encryption #2439

Conversation

dutow commented Jun 28, 2018 • edited Loading

laurynas-biveinis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutow commented Sep 27, 2018

laurynas-biveinis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robgolebiowski commented Oct 3, 2018

satya-bodapati commented Oct 3, 2018

dutow commented Oct 5, 2018

laurynas-biveinis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutow commented Oct 22, 2018

dutow commented Oct 22, 2018

laurynas-biveinis commented Oct 22, 2018

dutow commented Oct 24, 2018

dutow commented Jun 28, 2018 •

edited

Loading