As far as I can see, there is no need to do key rotation if innodb-encryption-rotate-key-age=1 and latest key version is 1 (which is always the case with file_key_management plugin).

There is a need to encrypt existing tables, but I suppose MDEV-11738 already solves that.

This is not as easy as it sounds. To find out does table need to be encrypted or not when innodb-encryption-rotate-key-age=1 requires that we read at least page 0 from table tablespace. That is required to be done for every tablespace i.e. for big db's that could mean we are reading 10k-100k pages. Secondly, InnoDB does not really know what encryption plugin we are using, thus this would require a change to encryption plugin interface e.g. encryption_supports_key_rotation() function that we could call. Do not really know if you can say key_version = 1 ==> file_key_management_plugin is used?

I mean, normally, if innodb-encryption-rotate-key-age=1, innodb needs to scan the tablespace to see if any of the pages is encrypted with the old key.

But "old" means "more than innodb-encryption-rotate-key-age behind the current key version".

So if the current key version is 1, there is no need to scan the tablespace.

This optimization does not require changing defaults and does not require extending the encryption API. InnoDB does not need to know what encryption plugin is used either.

Generally if current key version is less or equal than innodb-encryption-rotate-key-age, there is no need to scan. But innodb-encryption-rotate-key-age=key_version=1 is the most important practical use case.

This is not a fully ready version and intentionally contains only xtradb implementation. I commit this for feedback as it uses quite radical idea but in my understanding there is not very good alternatives. Using fil_system list has a problem that already processed tablespaces can't be removed causing periodic check also for tablespaces not needing any operation anymore.

https://github.com/MariaDB/server/commit/1a29767cd9a78ac396c4f1b4ae2f570cc3289c1f

I sent a few comments. Here are some more generic comments:

The title of the commit message is truncated, and there are some grammatical errors, such as writing "unnecessary" instead of "unnecessarily".

Apparently, the call to encryption_can_rotate() will determine if key rotation is supported. I would suggest to use a global variable for this instead, to be set when the encryption plugin is initialized. That should not require any changes to the encryption plugin function interface.

I am confused by the commit message. Where exactly does the following take place:

At startup, add all those tablespaces to key rotation list that need state change (i.e. unencrypted => encrypted or encrypted => uncenrypted). This is needed for ENCRYPTED=DEFAULT tables.

We do not want to read the first page of each data file at startup, to determine encryption status. Users would for sure be hurt by that. See MDEV-18733 for the latest fix in this area.

I feel that there is some overlap between your changes and MDEV-14398. Could you try to minimize the changes? I do not think that we need to change the function interface of encryption plugins.

Idea is not to read the first page of each data file at startup, nothing is changed on that area. Whenever a new database is used e.g. in use db1; InnoDB will open all tables on that database and on fil_node_open_file we do read page 0 and there I added also insert that tablespace to key rotation list.

Can you explain how how we can avoid change the function interface of encryption plugins ? Actually we could avoid it with innodb_encryption_rotate_key_age=0 but that does not solve the default configuration case.

Sorry, my comment was truncated. I edited it to clarify how exactly we can avoid changing the function interface.

I do not think that executing use db1 will invoke anything in InnoDB. The mysql client can execute additional statements behind the scenes unless it is invoked as mysql --no-auto-rehash.

Branch: bb-10.1-MDEV-14180

TODO:

• Also add system tablespace on key rotation list when it is created or opened after restart if it requires state change.
• In my opinion there is no need to protect srv_encrypt_tables with fil_system or other mutex, concurrent global variable changes are already protected
• Tests are not updated
• Make sure that tablespaces are added to key rotation list only when they are first time accessed i.e. on create and open.

origin/bb-10.6-MDEV-14180_1 703d69c2a89c57905c47bd9708f6356bbeb9028d 2021-03-03T17:27:31+05:30
worked well in RQG testing.

Patch is in bb-10.6-MDEV-14180_1

thiru, what about this change to your patch:

--- a/storage/innobase/fil/fil0crypt.cc

+++ b/storage/innobase/fil/fil0crypt.cc

@@ -116,8 +116,6 @@ void fil_space_crypt_init()

 {

   pthread_cond_init(&fil_crypt_throttle_sleep_cond, nullptr);

   mysql_mutex_init(0, &crypt_stat_mutex, nullptr);

-  if (srv_operation == SRV_OPERATION_NORMAL)

-    srv_encrypt_rotate = (encryption_get_no_rotation() == 1);

   memset(&crypt_stat, 0, sizeof crypt_stat);

 }

@@ -141,6 +139,8 @@ fil_space_crypt_t::key_get_latest_version(void)

                key_version = encryption_key_get_latest_version(key_id);

                srv_stats.n_key_requests.inc();

                key_found = key_version;

+               if (key_version > srv_fil_crypt_rotate_key_age)

+                       srv_encrypt_rotate= true;

        }

        return key_version;

and removing all changes outside of storage/innobase ?

Patch is in bb-10.6-MDEV-14180_2

Thank you. I posted some review comments. I would like to know how you tested this with both types of encryption plugins (with regard to supporting multiple key versions).