Hi Chuck,
Summary:
From this email, so far I can say that I will not block progress. You
promised to fix the required changes and the requests are either
promised to fix or answered.
I am not fully satisfied with the reply though. IMHO the tests are not
as good as they could be. A few simple changes could improve them
remarkable. Please see below.
Charles Bell, 25.11.2009 20:38:
...
>> 3) Do we need privileges on mysql.* for RESTORE? (recurring)
>
> I do this intentionally. Short answer: I was required to do it this way
> in other works because it was argued that we do not know the specific
> privileges needed on mysql. I prefer to leave it as is for now until
> such time as we can pinpoint what the exact privileges are (note: my own
> explanation of the minimal set was rejected if you recall that discussion).
I am sorry that I cannot express myself clearly. And I'm sorry that I
still don't understand, how we work together in the team.
1. The issue is not that important. Push it if you think it is good work
as it is.
2. My main concern is that the different test cases use different grants
on mysql.*. I feel a personal discomfort with such inconsistencies. It
feels like the tester doesn't know, what he does.
3. I understand that we don't try to pinpoint what the exact privileges
are (I recall parts of that discussion). Hence I do not ask for limiting
privileges to certain tables in mysql. I am fine with mysql.*.
4. Since I see tests with successful non-elevated RESTORE that grant
ALL, other tests that grant SELECT, and at least one without any grants
on mysql.*, I thought it could be reasonable, not to grant anything on
mysql.*.
5. Even if we don't want to pinpoint what the exact privileges are, we
should not grant anything, if we know that we don't need anything.
6. There is a remarkable difference between trying fine-granular grants
or not to grant anything.
7. I'm human. I may be in error. If I misinterpret the test case that
successfully restores with no privileges on mysql.* and no elevation,
then I want to be told so. The question, I repeated a couple of times
was: Do we need privileges on mysql.* for RESTORE? It can be answered
with "yes" or "no".
8. It was my impression about our processes that a reviewer can ask such
question and the implementor tries to answer it. Now I get the
impression that the reviewer has to do such investigation himself. So I
wonder, how we work together in the team.
9. Here are the results of my investigation:
9.1 First try with no privileges on mysql.*:
--- mysql-test/suite/backup/r/backup_security_options.result
+++ mysql-test/suite/backup/r/backup_security_options.reject
@@ -265,27 +265,36 @@
# Test Case 6 : Show restore_elevation = OFF can succeed if privileges
# granted.
#
-REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
+REVOKE ALL PRIVILEGES, GRANT OPTION FROM 'bup_some_priv'@'localhost';
GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT
OPTION;
-GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
+GRANT SUPER, RELOAD ON *.* TO 'bup_some_priv'@'localhost';
FLUSH PRIVILEGES;
#
# Show grants for user.
#
SHOW GRANTS FOR 'bup_some_priv'@'localhost';
Grants for bup_some_priv@localhost
-GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost'
-GRANT SELECT ON `mysql`.* TO 'bup_some_priv'@'localhost'
+GRANT RELOAD, SUPER ON *.* TO 'bup_some_priv'@'localhost'
GRANT ALL PRIVILEGES ON `backup_test`.* TO 'bup_some_priv'@'localhost'
WITH GRANT OPTION
#
# Connect as user with only some privileges.
#
+#
+# Show variables.
+#
+SHOW VARIABLES LIKE 'restore_%';
+Variable_name Value
+restore_disables_events ON
+restore_elevation OFF
+restore_precheck OFF
#
# conn_some_priv: Attempting restore. Should succeed
#
RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
backup_id
#
+Warnings:
+# 1142 SELECT command denied to user
'bup_some_priv'@'localhost' for table 'event'
#
# Connect as root and prepare next test case.
So RESTORE succeeded with no elevation and no privileges on mysql.*
though internally some SELECT on mysql.event failed.
9.2 Second try with SELECT on mysql.*:
--- mysql-test/suite/backup/r/backup_security_options.result
+++ mysql-test/suite/backup/r/backup_security_options.reject
@@ -265,26 +265,35 @@
# Test Case 6 : Show restore_elevation = OFF can succeed if privileges
# granted.
#
-REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
+REVOKE ALL PRIVILEGES, GRANT OPTION FROM 'bup_some_priv'@'localhost';
GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT
OPTION;
-GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
+GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
+GRANT SUPER, RELOAD ON *.* TO 'bup_some_priv'@'localhost';
FLUSH PRIVILEGES;
#
# Show grants for user.
#
SHOW GRANTS FOR 'bup_some_priv'@'localhost';
Grants for bup_some_priv@localhost
-GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost'
+GRANT RELOAD, SUPER ON *.* TO 'bup_some_priv'@'localhost'
GRANT SELECT ON `mysql`.* TO 'bup_some_priv'@'localhost'
GRANT ALL PRIVILEGES ON `backup_test`.* TO 'bup_some_priv'@'localhost'
WITH GRANT OPTION
#
# Connect as user with only some privileges.
#
+#
+# Show variables.
+#
+SHOW VARIABLES LIKE 'restore_%';
+Variable_name Value
+restore_disables_events ON
+restore_elevation OFF
+restore_precheck OFF
#
# conn_some_priv: Attempting restore. Should succeed
#
RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
backup_id
#
#
# Connect as root and prepare next test case.
This seems to be perfect. Hence the answer is: "Yes, at least SELECT is
needed on mysql.*". So I would like to see all privilege settings for
RESTORE to include SELECT ON mysql.*, but not ALL.
10. See 1: The issue is not that important. Push it if you think it is
good work as it is.
...
>> 10) Create 'joe'.
>
> Will add (but not needed).
Ok, that's fine. But please accept my amazement. In your email "Re: bzr
commit into mysql-6.0-backup branch (charles.bell:2887)" of Tue, 03 Nov
2009 17:30:54 -0500, you wrote:
"
Ingo,
...
>> +CREATE USER 'bup_some_priv'@'localhost';
>> +CREATE USER 'joe'@'user';
>
> 4) What purpose serves 'joe'? I don't see a real use. I suggest to
> remove to reduce confusion.
The user 'joe'@'user' is given privileges in the include file
'basic_data' so that is why he is included here. It also adds grants to
the backup image.
"
I just would like to see a consistent use of 'joe'. But the issue is not
that important. Push it if you think it is good work as it is.
>
>> 11) Show, it fails in the middle. (recurring)
>
> I disagree with your assessment that it is not failing in the middle.
> Users are not the audience for these tests. However, I will attempt your
> suggestion and if it works I will include it or at least add it as a
> second step to the test case.
I agree that "users" are not the audience. But QA people should be seen
as part of the audience.
Again, in my email of Fri, 20 Nov 2009 14:10:49 +0100, I provided a
solution. Here it is in more detail:
--- mysql-test/suite/backup/r/backup_security_options.result
+++ mysql-test/suite/backup/r/backup_security_options.reject
@@ -174,34 +174,74 @@
# Test Case 3 : Ensure restore_precheck = OFF fails for not enough
# privileges (Restore will fail during the metadata
# execution stage).
#
-REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
-REVOKE SUPER ON *.* FROM 'bup_some_priv'@'localhost';
-GRANT RESTORE ON backup_test.* TO 'bup_some_priv'@'localhost';
+REVOKE ALL PRIVILEGES, GRANT OPTION FROM 'bup_some_priv'@'localhost';
+GRANT CREATE, DROP, RESTORE ON backup_test.* TO
'bup_some_priv'@'localhost';
+GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
FLUSH PRIVILEGES;
+#
+# Show list of all objects in the database.
+#
+SHOW FULL TABLES FROM backup_test;
+Tables_in_backup_test Table_type
+t1 BASE TABLE
+t2 BASE TABLE
+v1 VIEW
+SELECT event_name FROM INFORMATION_SCHEMA.EVENTS WHERE event_schema =
'backup_test';
+event_name
+e1
+SELECT routine_name FROM INFORMATION_SCHEMA.ROUTINES WHERE
routine_schema = 'backup_test';
+routine_name
+f1
+p1
+SELECT trigger_name FROM INFORMATION_SCHEMA.TRIGGERS WHERE
trigger_schema = 'backup_test';
+trigger_name
+trg
#
# Show grants for user.
#
SHOW GRANTS FOR 'bup_some_priv'@'localhost';
Grants for bup_some_priv@localhost
GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
-GRANT RESTORE ON `backup_test`.* TO 'bup_some_priv'@'localhost'
+GRANT SELECT ON `mysql`.* TO 'bup_some_priv'@'localhost'
+GRANT CREATE, DROP, RESTORE ON `backup_test`.* TO
'bup_some_priv'@'localhost'
#
# Connect as user with only some privileges.
#
+#
+# Show variables.
+#
+SHOW VARIABLES LIKE 'restore_%';
+Variable_name Value
+restore_disables_events ON
+restore_elevation OFF
+restore_precheck OFF
#
# conn_some_priv: Attempting restore. Should fail with
-# error ER_BACKUP_CANT_RESTORE_DB
+# error ER_BACKUP_CANT_RESTORE_SROUT
#
RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
-ERROR HY000: Could not restore database `backup_test`
+ERROR HY000: Could not restore stored routine `backup_test`.`p1`
SHOW ERRORS;
Level Code Message
Error # Access denied for user 'bup_some_priv'@'localhost' to
database 'backup_test'
-Error # Could not restore database `backup_test`
+Error # Could not restore stored routine `backup_test`.`p1`
#
-# Connect as root and prepare next test case.
+# Connect as root and show list of all objects in the database.
+#
+SHOW FULL TABLES FROM backup_test;
+Tables_in_backup_test Table_type
+t1 BASE TABLE
+t2 BASE TABLE
+SELECT event_name FROM INFORMATION_SCHEMA.EVENTS WHERE event_schema =
'backup_test';
+event_name
+SELECT routine_name FROM INFORMATION_SCHEMA.ROUTINES WHERE
routine_schema = 'backup_test';
+routine_name
+SELECT trigger_name FROM INFORMATION_SCHEMA.TRIGGERS WHERE
trigger_schema = 'backup_test';
+trigger_name
+#
+# Prepare next test case.
This is what I deem good work:
1. Start with REVOKE ALL PRIVILEGES, GRANT OPTION FROM ...
That is, remove all privileges, global and from all databases. Nothing
remains from former tests.
2. GRANT CREATE, DROP, RESTORE ON backup_test.*
This allows RESTORE to start with dropping the database and creating
tables, but it does not allow to create other things, e.g. routines.
3. Show the database objects before RESTORE.
4. (This was already done everywhere) Show resulting privileges.
5. Show relevant variable values.
6. Show the database objects after RESTORE. This clearly proves (even
for users) that RESTORE failed in the middle (or during the metadata
execution stage, if you prefer).
I am sorry that it came so far that I provided a full test. My aim as a
reviewer is to let the implementor provide algorithms as he likes them,
and just point at parts that I want to have changed. But I have the
impression that I'm not understood. I felt the need to show, how simple
a full implementation of my preferred test algorithm is, and how many
advantages it has over the existing test.
>
>> 14) Improve session variable testing.
>
> Ok, but I don't see the issue here. I set it as a session variable,
> check it, then change the global and check the session again. Perhaps
> the first check is superfluous, but in other places you seem to infer
> that I should be checking the values even though we know what they are -
> so which should I do -- check them even though they haven't changed or
> only check them when we know they've changed?
For a test of the variables themselves: Check them when we know they've
changed.
For RESTORE tests: Check (show) them before every restore after checking
(showing) the privileges and after connecting a new session. So that for
every RESTORE one has all information at one place and thus can check
the correctness more easily.
The reason for my comment (14) was that we know that restore_precheck
was ON. Then we set it to ON (no change), and then check it (though we
know it didn't change). So yes, the first check is superfluous (and also
the SET). And we do not have a check that we are able to change the
value. We have two tests where we SET the value of restore_precheck, but
are pleased about not seeing an error message. No check is done if the
value did indeed change. Well, that's fine as I trust the variables
code. But then it is really ugly to check the value after *not* changing it.
Again. It is not that important. If you think, it is a good test, push
it as it is.
>
>> 16) Better comment.
>
> Will fix.
>
>> 17) Declare variables in backup code. (recurring)
>
> Ok. Do you suggest a particular code module to put them?
No. But the following could work:
Alternative A) declare opt_backup_elevation in backup_info.h and
opt_restore_elevation in restore_info.h.
Alternative B) declare both in backup_kernel.h, which is included by
both headers.
Note that the *definition* of the variables in mysqld.cc is correct and
required by the command line options array.
>
>> 18) Please initialize [my_]bool variables with TRUE or FALSE.
>
> This was done to follow convention of other variables in that file.
> Since we are putting them in our own code then I will do as you say.
Not all existing code follows the coding guidelines. I have been taught
to use current coding style on new lines, even if adjacent lines follow
old coding style.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
