Reproducible as described

I have tried to convince that this is designed feature after request for a feature where no key rotation is done in background. In this contents unencrypted to encrypted is key rotation as well as encrypted to unencrypted is a key rotation. If unecrypted to encrypted or encrypted to unecrypted key rotation feature is needed it can't be done with no CPU usage because:

(1) At startup, we need to iterate all tables to find out what tables need this one time key rotation.
(2) Whenever user does set global innodb-encrypt-tables=[ON|OFF|FORCE]; we need to iterate all tables to find out tables requiring again one time key rotation.

Hi jplindst,

Thanks for the feedback. I understand your points. We should probably try to improve the documentation on encryption, so that implementation details like this that can affect users are clearer.

Should this be closed as "Not a Bug" and the implementation details added to the documentation (e.g. see MDEV-14157)?

Please do not close this yet, lets have a understanding the expectations from customer.

In this context there is following possibilities for enhancement:

• (a) innodb-encrypt-tables=[ON|FORCE}, innodb_encryption_rotate_key_age=0

• • Server will encrypt system tablespace but not other tablespaces (e.g. mysql.innodb_table_stats). Note that encrypting system tablespace will take time and CPU depending the size of system tablespace. Checking the need for encryption does not use much resources.

• (b) innodb-encrypt-tables=[OFF], innodb_encryption_rotate_key_age=0

• • Server will dencrypt system tablespace but not any other tablespaces. Again, it would require time and CPU depending the size of system tablespace if needed. Checking the need for encryption does not much need time or CPU.

• (c) Server startup with innodb-encrypt-tables=[ON|FORCE,OFF], innodb_encryption_rotate_key_age=0

• • Server will iterate all tablespaces and encrypt/decrypt those that are needed. This requires CPU depending the number of tables in database and time and CPU depending how many tables require encryption/decryption. However, this is done exactly once when server starts.

Question: what should happen when user issues set global innodb-encrypt-tables=[ON|OFF|FORCE];

How about this:

• when innodb_encrypt_tables is changed run-time, innodb starts a background job to check and en-/decrypt all tables, as necessary.
• system tablespace is always en-/decrypted last
• if on startup the state of the system tablespace does not match the value of innodb_encrypt_tables, the server starts a background job on startup as well.

To clarify, one can process the system tablespace first, but after the all tables are en-/decrypted InnoDB should record somewhere this fact, "all tables are en-/decrypted". If system tablespace is processed last, then it implicitly records this fact in its own state. If it's processed first, as now, it must be an explicit value written somewhere "ok, all tables are done"

TBoyd
By setting innodb-encryption-threads=1 and innodb-encryption-rotate-key-age=[large_number] will avoid block by block checking as only tablespace metadata in this case min_key_version is checked. Status of tablespace encryption can be queried from information schema. Also there is setting for throttling but that is used when we do block by block checking.

Workarounds:

(1) New databases: mysql_install_db configuration should set innodb-encryption-rotate-key-age != 0 with other encryption setup this will ensure that system tablespace is encrypted.
(2) Existing databases: You need to start database with innodb-encryption-rotate-key-age != 0 with other encryption setup and again your system tablespace is also encrypted. If future key rotations are not needed for existing tablespaces, examine result of query:

select NAME, ENCRYPTION_SCHEME, KEY_ROTATION_PAGE_NUMBER, KEY_ROTATION_MAX_PAGE_NUMBER, ROTATING_OR_FLUSHING FROM INFORMATION_SHCEMA. INNODB_TABLESPACES_ENCRYPTION;

And make sure that all tablespaces requiring encryption has marked so. After that shutdown server and you are safe to set innodb_encryption_rotate_key_age = 0 and start server.

We are planning to add syntax to encrypt any tablespace in MDEV-14610: Add syntax to manually encrypt InnoDB's system tablespace.

Global metadata on system tablespace has problem we would need to know it is there (as earlier releases did not have it) and maintenance of its value has challenges.

Normally we acquire a new key version for every create table so setting it as a large value it has impact how often block by block encryption happens.

Current plan:

• startup: as system tablespace page 0 (ibdata1) is always read at startup, we check does it match current encryption setup, if not we rotate to correct setup. In same time we check all those tablespaces where page 0 is read and rotate if needed. Rest of tablespaces are rotated if needed when page 0 is read.
• set global innodb_encrypt_tables=[ON,OFF,FORCE]; Now we check all tablespaces including the system tablespace where page 0 is read and rotate if needed, assumption here is that users would not do these operations too frequently (as it will cost you). Again tablespaces that are not yet opened and page 0 is not read currently are handled when they are opened.

Idea is to check only tablespaces whose page 0 is read whenever there is encryption state change and in startup. This requires some CPU time but we do not read page 0 from every tablespace. Naturally, tablespaces that are not much used remain on state they are maybe a long time. They can be encrypted/decrypted doing e.g. select. Users should use explicit encryption (create table t1 ... engine=innodb encrypted=YES) for those tables that require immediate encryption.

TBoyd Can you check from customer does this design fulfill their needs.

In current implementation (10.1.31) is there a way to decrypt the system tablespace?
Even restarting with innodb-encryption-rotate-key-age=0 leaves it encrypted, and that's the only encrypted table in the server, we really need a way to decrypt it so encryption plugin and keys can be removed from config.

See https://jira.mariadb.org/browse/MDEV-14610

Tried that but I don't see any automatic decrypt of innodb tablespace aka ibdata1. It stays encrypted.
I thought if no encrypted tables exist in server, ibdata1 should be automatically decrypted as well, but this doesn't seem to happen.

Ah, naturally as it is not implemented. If you want totally away from encryption you can try following config:

innodb-encryption-rotate-key-age=1

innodb-encryption-threads=4

innodb-encrypt-tables=OFF

Tried that and it does not work. So, can I assume there is no way out of encryption as of today?
I work in MariaDB's RDBA Team and we have a customer which we would like out of encryption.

As already noted by me this is either "works as designed" or feature request. There is clearly documented (if not then open a new bug to improve the documentation) way to work around this feature. This is not a critical bug.

See how to get system tablespace to encrypted/unencrypted state from blog https://mariadb.com/resources/blog/moving-mariadb-database-encrypted-and-unencrypted-states

I still think it's a regression bug, that needs to be fixed eventually

jplindst did not like the approach that I had been discussing with thiru: adding some persistent state to the DICT_HDR page in the InnoDB system tablespace to indicate whether all data files are encrypted or all are in cleartext, or there is a mix of them.

So, we discussed another approach that does not change any persistent data format, but instead changes the interfaces as follows (my proposal based on the discussion):

1. Introduce a Boolean parameter innodb_encrypt_tables_deferred that causes the encryption threads to only perform tasks on files that have already been accessed for some other reason (such as executing SQL, or purging history).
2. In information_schema, expand innodb_tablespaces_encryption with the columns is_encrypted BIT NULL, size INT UNSIGNED NULL (the latter is fil_space_t::size). Both columns would be NULL if the file has not been accessed yet. We might also include pages_processed INT UNSIGNED NULL for allowing progress monitoring after a file has been accessed. This would require backporting MDEV-11984 to 10.1.

ok to push by me

How does this another approach solve the original problem or tablespaces not being encrypted?

Basically patch removed the relation between innodb_encrypt_rotate_key_age and innodb_encrypt_tables variables. Introduced a variable called innodb_encrypt_tables_deferred to avoid i/o usage by rotation thread. Basically rotation threads avoids to
read page 0 from tablespace. It performs the encryption/decryption on files that are already accessed.

if innodb_encrypt_rotate_key_age = 0 then it avoids only re-encrypt the encrypted tablespace. It can encrypt/decrypt the tablespace
based on innodb_encrypt_tables variable.

It was agreed that we should persistently store the state of encryption in the InnoDB system tablespace. We can use the 32-bit field DICT_HDR_MIX_ID_LOW for that. Up until now, its value is always 10. We can repurpose this value to mean "some tables might be encrypted, some might not". Then, let us introduce the values 8 and 9 for "all tables are encrypted" and "all tables are unencrypted", respectively. If any other value is present in this field, we must refuse InnoDB startup.

Unfortunately, it seems to me that this solution will introduce a one-time first-startup penalty for all users who upgrade to the version with this fix present: When the field carries the old value, we must read the first page of every InnoDB data file in order to determine whether it is encrypted. This is because the data dictionary does not store information about whether encryption is used. That information is only stored in the first page of each data file itself. It is not stored in the InnoDB SYS_TABLES. Also the table option in .frm files is not reliable, because encryption can be by a global parameter. (And also reading all .frm files at startup could be slow.)

I reviewed the changes in bb-10.2-MDEV-14398 and requested a few minor changes. In the end, this should also be backported to 10.1.

Please submit the 10.1 version as well.

marko gave some review comments for mdev-14398 patch. Still working on the patch.

Completed the patch for encrypted list of tablespace. Cross verified whether we are doing false-positive case.
Ran the whole suite for it. It doesn't fail
Now working on the unencrypted tablspaces list in fil_system. Facing few assert failure. Working towards solving this assert failure.

I completed the patch for it. patch is in[ bb-10.2-MDEV-14398-branch|https://github.com/MariaDB/server/commits/bb-10.2-MDEV-14398-branch].
https://github.com/MariaDB/server/commits/bb-10.2-MDEV-14398-branch

Branch has 3 patches:
i) Creates encrypted and unencrypted list. Add the tablespace to the list whenever it is open as encrypted or decrypted.
ii) Store the global encryption status of all tablespace in dictionary header page. Read the global encryption status
during startup. Use the status variable to iterate through all tablespace for encryption.
iii) Make all i_s columns as NULL if the table is not loaded.

I did my review in a bit unconventional way, by pushing clean-up commits to the bb-10.4-MDEV-14398 branch.
The following tests fail both on buildbot and locally, and unrelated to my refactoring:

encryption.innodb_lotoftables innodb.innodb-wl5522-debug innodb_zip.wl5522_debug_zip

Please fix those, and check if you agree with my changes. Ultimately, after you have fixed the failures and we both are happy with the result, I’d squash the commits and push to 10.4.

I pushed one more change that avoids releasing and re-acquiring fil_system.mutex in low-level code, and tries to update the DICT_HDR page at the higher level where appropriate.

After that, I was thinking that the global encryption status should also change if we remove the last tablespace of its kind (encrypted or unencrypted). The following patch attempts to do that, but additional tests would start to fail if I actually enable the logic (see below):

diff --git a/storage/innobase/include/fil0fil.h b/storage/innobase/include/fil0fil.h

index fd9cc91d574..4015b39982c 100644

--- a/storage/innobase/include/fil0fil.h

+++ b/storage/innobase/include/fil0fil.h

@@ -644,12 +644,13 @@ struct fil_system_t {

   void close();

   /**

-    Set the encryption status of all tablespaces after a tablespace

-    has been added to unencrypted_spaces or encrypted_spaces.

+    Set the encryption status of all tablespaces after

+    unencrypted_spaces or encrypted_spaces has been changed.

     @param[in] encrypted	whether the tablespace is encrypted

+    @param[in] remove		whether the tablespace is being removed

     @return whether dict_hdr_crypt_status_update() must be called */

-  inline bool crypt_enlist(bool encrypted);

+  inline bool crypt_update(bool encrypted, bool remove);

 private:

   bool m_initialised;

diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc

index 68e684b69d4..3f07a37bc17 100644

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

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

@@ -5098,7 +5098,7 @@ fil_space_set_punch_hole(

 	node->space->punch_hole = val;

 }

-inline bool fil_system_t::crypt_enlist(bool encrypted)

+inline bool fil_system_t::crypt_update(bool encrypted, bool remove)

 {

 	ut_ad(is_initialised());

 	ut_ad(this == &fil_system);

@@ -5114,12 +5114,12 @@ inline bool fil_system_t::crypt_enlist(bool encrypted)

 	switch (status) {

 	case CRYPT_ENCRYPTED:

-		if (!encrypted) {

+		if (!remove && !encrypted) {

 			status = CRYPT_MIXED;

 		}

 		break;

 	case CRYPT_DECRYPTED:

-		if (encrypted) {

+		if (!remove && encrypted) {

 			status = CRYPT_MIXED;

 		}

 		break;

@@ -5158,12 +5158,12 @@ bool fil_space_t::crypt_enlist()

 	if (!crypt_data || !crypt_data->min_key_version) {

 		if (add_if_not_in_unencrypted_spaces()) {

 			remove_if_in_encrypted_spaces();

-			return fil_system.crypt_enlist(false);

+			return fil_system.crypt_update(false, false);

 		}

 	} else {

 		if (add_if_not_in_encrypted_spaces()) {

 			remove_if_in_unencrypted_spaces();

-			return fil_system.crypt_enlist(true);

+			return fil_system.crypt_update(true, false);

 		}

 	}

@@ -5178,9 +5178,18 @@ inline bool fil_space_t::crypt_delist()

 		return false;

 	}

-	if (remove_if_in_encrypted_spaces()

-	    || remove_if_in_unencrypted_spaces()) {

-		return true;

+	if (remove_if_in_encrypted_spaces()) {

+		ut_ad(!is_in_unencrypted_spaces());

+		ut_ad(true || fil_system.crypt_status

+		      != fil_system_t::CRYPT_DECRYPTED);

+		return true || fil_system.crypt_update(true, true);

+	}

+

+	if (remove_if_in_unencrypted_spaces()) {

+		ut_ad(!is_in_unencrypted_spaces());

+		ut_ad(true || fil_system.crypt_status

+		      != fil_system_t::CRYPT_ENCRYPTED);

+		return true || fil_system.crypt_update(false, true);

 	}

 	ut_ad(size == 0);

If you apply the patch and remove the 4 occurrences of true || at the end of the patch, then the following tests will fail (in addition to the 3 already failing tests):

encryption.encrypt_and_grep encryption.innochecksum encryption.innodb-bad-key-change encryption.innodb-bad-key-change2 encryption.innodb-compressed-blob encryption.innodb-key-rotation-disable encryption.innodb-missing-key encryption.innodb-page_encryption encryption.innodb-page_encryption_compression
encryption.innodb-page_encryption_log_encryption encryption.innodb_encrypt_key_rotation_age encryption.innodb_encrypt_log encryption.innodb_encryption_discard_import encryption.innodb_onlinealter_encryption encryption.innodb_page_encryption_key_change mariabackup.xb_fulltext_encrypted

thiru, please investigate and fix this.

Also, while working on the patch, I noticed that MLOG_FILE_CREATE2 is not being written (and flushed) ahead of the file creation, but behind it. That would be fixed in the course of MDEV-18128 or MDEV-18518.

I see that at some point we added a check, so that you aren't able to change innodb_encrypt_tables if innodb_encryption_rotate_key_age=0 is set. For example:

MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=0;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=ON;

ERROR 1231 (42000): Variable 'innodb_encrypt_tables' can't be set to the value of 'ON'

MariaDB [(none)]> SHOW WARNINGS;

+---------+------+-------------------------------------------------------------------------------------------------+

| Level   | Code | Message                                                                                         |

+---------+------+-------------------------------------------------------------------------------------------------+

| Warning |  138 | InnoDB: cannot enable encryption, innodb_encryption_rotate_key_age=0 i.e. key rotation disabled |

| Error   | 1231 | Variable 'innodb_encrypt_tables' can't be set to the value of 'ON'                              |

+---------+------+-------------------------------------------------------------------------------------------------+

2 rows in set (0.000 sec)

MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=1;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=ON;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=0;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;

ERROR 1231 (42000): Variable 'innodb_encrypt_tables' can't be set to the value of 'OFF'

MariaDB [(none)]> SHOW WARNINGS;

+---------+------+--------------------------------------------------------------------------------------------------+

| Level   | Code | Message                                                                                          |

+---------+------+--------------------------------------------------------------------------------------------------+

| Warning |  138 | InnoDB: cannot disable encryption, innodb_encryption_rotate_key_age=0 i.e. key rotation disabled |

| Error   | 1231 | Variable 'innodb_encrypt_tables' can't be set to the value of 'OFF'                              |

+---------+------+--------------------------------------------------------------------------------------------------+

2 rows in set (0.000 sec)

MariaDB [(none)]> SET GLOBAL innodb_encryption_rotate_key_age=1;

Query OK, 0 rows affected (0.00 sec)

MariaDB [(none)]> SET GLOBAL innodb_encrypt_tables=OFF;

Query OK, 0 rows affected (0.00 sec)

If the patch for this issue makes it so that background encryption/decryption operations are no longer internally considered key rotation operations, then I think that the patch should also remove this restriction. A lot of users like to disable key rotations because the key rotation checks constantly use CPU in the background. These key rotation checks even waste CPU resources when file_key_management plugin is used, which doesn't support key rotations at all. However, I think this would be a non-issue if MDEV-14180 were implemented.

The code has not been changed since my previous comment. The full code changes against 10.4 can be viewed with the following command:

git diff f4f8dd69aa373d7418a4148ac9eaf20e55da5a4d..472d07c5aa7f76fb16aedc4fe759d7619b95cf0d

If I remember correctly, this change is not distinguishing tablespaces based on the ENCRYPTION parameter. My current understanding of the design is that tables that were explicitly created or altered with ENCRYPTION=OFF or ENCRYPTION=ON should not be affected by the innodb_encrypt_tables parameter. Therefore, when keeping track of "mixed encryption status", only ENCRYPTION=DEFAULT tables should be considered.

There appears to be a less intrusive alternative that would avoid changing the format of the InnoDB system tablespace and thus be more suitable for implementation in GA releases of MariaDB. Instead, we would extend the update trigger for SET GLOBAL innodb_encrypt_tables in innodb_encrypt_tables_update() or fil_crypt_set_encrypt_tables() as follows:

1. Read the first page of all .ibd files that has not been read yet, to determine the encryption status.
2. If needed, read also the .frm files, and be sure to cache the results in fil_space_t::crypt_data or similar. In MariaDB 10.2 and later, the function open_purge_table() can be used for this purpose.
3. Update some lists in fil_system so that the encryption threads will not do any busy work, but will be able to apply or remove encryption even if innodb_encryption_rotate_key_age=0.

The observable effects would be as follows:

-- trigger the removal of encryption of all ENCRYPTION=DEFAULT tables

SET GLOBAL innodb_encrypt_tables=OFF;

-- trigger the encryption of all unencrypted ENCRYPTION=DEFAULT tables

SET GLOBAL innodb_encrypt_tables=ON;

-- trigger the encryption of all unencrypted ENCRYPTION=DEFAULT tables, and prevent further creation of ENCRYPTION=OFF tables

SET GLOBAL innodb_encrypt_tables=FORCE;

I agree with GeoffMontee that the check that was added to innodb_encrypt_tables_validate() in MDEV-11738/MDEV-11581 should be removed as part of this fix.

Good work! I pushed an amended version for testing.

Can we remove fil_system_t::crypt_status and related code? It was mainly needed for writing a persistent global encryption state, in the 10.4 based earlier version of the patch.

I pushed the removal of fil_system_t::crypt_status and its related code. Let's hope for the testing to give good results.