Re: [Maria-developers] [Commits] ce29dfac72e: MDEV-18601: Can't create table with ENCRYPTED=DEFAULT when innodb_default_encryption_key_id!=1
Greetings from MariaDB Openworks in New York City.
I just discussed this with Monty. His idea is that "don’t care"
attributes should be ignored, or at most a warning could be issued for
them. This allows ALTER TABLE from one storage engine to another while
preserving attributes that only matter for some storage engines.
Based on that basic principle, we should ignore the encryption_key_id
attribute (whether or not it was explicitly specified by the user, or
inherited from the session variable innodb_default_encryption_key_id).
We could issue a warning if innodb_default_encryption_key_id is
assigned to an invalid value, or an invalid encryption_key_id is being
implicitly or explicitly passed to CREATE TABLE or ALTER TABLE.
On Thu, Feb 21, 2019 at 2:53 PM jan
diff --git a/mysql-test/suite/encryption/r/innodb-encryption-alter.result b/mysql-test/suite/encryption/r/innodb-encryption-alter.result index 5245d1da7d0..038e2a2fd47 100644 --- a/mysql-test/suite/encryption/r/innodb-encryption-alter.result +++ b/mysql-test/suite/encryption/r/innodb-encryption-alter.result @@ -3,24 +3,18 @@ SET GLOBAL innodb_file_per_table = ON; SET GLOBAL innodb_encrypt_tables = ON; SET GLOBAL innodb_encryption_threads = 4; CREATE TABLE t1 (pk INT PRIMARY KEY AUTO_INCREMENT, c VARCHAR(256)) ENGINE=INNODB ENCRYPTED=NO ENCRYPTION_KEY_ID=4; -Warnings: -Warning 140 InnoDB: Ignored ENCRYPTION_KEY_ID 4 when encryption is disabled DROP TABLE t1; set innodb_default_encryption_key_id = 99; +Warnings: +Warning 1210 InnoDB: Ignored innodb_default_encryption_key_id=99 as it is not available in the key file. Using default=1.
I think that the warning during CREATE TABLE was appropriate and should not be removed. A warning for the SET is appropriate, but I do not like the "Using default=1" part. I think that we should keep the invalid attribute, and throw an error or warning during CREATE TABLE as appropriate.
CREATE TABLE t1 (pk INT PRIMARY KEY AUTO_INCREMENT, c VARCHAR(256)) ENGINE=INNODB; -ERROR HY000: Can't create table `test`.`t1` (errno: 140 "Wrong create options") SHOW WARNINGS; Level Code Message -Warning 140 InnoDB: ENCRYPTION_KEY_ID 99 not available -Error 1005 Can't create table `test`.`t1` (errno: 140 "Wrong create options") -Warning 1030 Got error 140 "Wrong create options" from storage engine InnoDB
I believe that this CREATE TABLE should continue to be disallowed, because innodb_encrypt_tables=ON and innodb_default_encryption_key_id=9 should imply ENCRYPTED=YES and ENCRYPTION_KEY_ID=99, and the key 99 is not available. Even if you think that it should be allowed (I find it hard to agree with that), I believe that we should at least be issuing warnings. [snip]
SET GLOBAL innodb_encrypt_tables=OFF; CREATE TABLE t1 (a int not null primary key) engine=innodb; ALTER TABLE t1 ENCRYPTION_KEY_ID=4; -ERROR HY000: Table storage engine 'InnoDB' does not support the create option 'ENCRYPTION_KEY_ID' -SHOW WARNINGS; -Level Code Message -Warning 140 InnoDB: innodb_encrypt_tables=OFF only allows ENCRYPTION_KEY_ID=1 -Error 1478 Table storage engine 'InnoDB' does not support the create option 'ENCRYPTION_KEY_ID'
Here, it is correct that we should not throw a warning, because innodb_encrypt_tables=OFF causes the encryption_key_id to not matter. But we could issue a warning if ENCRYPTION_KEY_ID is not available. I am going to push a revised patch to the bb-10.1-marko branch and ask for your review. With best regards, Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation
Hi, Marko! On Feb 27, Marko Mäkelä wrote:
Greetings from MariaDB Openworks in New York City.
I just discussed this with Monty. His idea is that "don’t care" attributes should be ignored, or at most a warning could be issued for them. This allows ALTER TABLE from one storage engine to another while preserving attributes that only matter for some storage engines.
This behavior is configurable. We've got many complaints about this "ignored" behavior. Currently unknown (not "invalid values" - only completely unknown) attributes are ignored only if IGNORE_BAD_TABLE_OPTIONS sql-mode is enabled.
Based on that basic principle, we should ignore the encryption_key_id attribute (whether or not it was explicitly specified by the user, or inherited from the session variable innodb_default_encryption_key_id).
encryption_key_id is not "don't care", it's something that a user expects to be used after innodb_encrypt_tables=ON. This was the whole reason of MDEV-17230 in the first place. Do you suggest to un-fix MDEV-17230 and ignore the user specified encryption key id unless it can be used used right away? It's possible, and it'll fix MDEV-18601. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei,
On Thu, Feb 28, 2019 at 5:08 PM Sergei Golubchik
Hi, Marko!
On Feb 27, Marko Mäkelä wrote:
Greetings from MariaDB Openworks in New York City.
I just discussed this with Monty. His idea is that "don’t care" attributes should be ignored, or at most a warning could be issued for them. This allows ALTER TABLE from one storage engine to another while preserving attributes that only matter for some storage engines.
This behavior is configurable. We've got many complaints about this "ignored" behavior. Currently unknown (not "invalid values" - only completely unknown) attributes are ignored only if IGNORE_BAD_TABLE_OPTIONS sql-mode is enabled.
Based on that basic principle, we should ignore the encryption_key_id attribute (whether or not it was explicitly specified by the user, or inherited from the session variable innodb_default_encryption_key_id).
encryption_key_id is not "don't care", it's something that a user expects to be used after innodb_encrypt_tables=ON. This was the whole reason of MDEV-17230 in the first place.
Do you suggest to un-fix MDEV-17230 and ignore the user specified encryption key id unless it can be used used right away? It's possible, and it'll fix MDEV-18601.
I believe that our best option is to basically turn into a warning the error that was introduced in MDEV-17230. I did that and some other cleanup in my proposed patch at https://github.com/MariaDB/server/commit/e39d6e0c53bccaf0c6b2a0341f8deb420f5... which I plan to push to 10.1 if it passes the tests. In MariaDB Server 10.2 or later versions we would be able to actually fix the root cause of what was reported in MDEV-17230, by opening the .frm file from within InnoDB when encryption is about to be enabled. If you have something against this plan, please let me know. Marko
participants (2)
-
Marko Mäkelä
-
Sergei Golubchik