Hello Marc!
Here is my review for new version of changes to ACL subsystem that your
have introduced in your performance schema tree:
> --- mysql-next-mr/sql/sql_acl.cc 2009-11-05 18:18:34.000000000 +0300
> +++ mysql-trunk-perfschema/sql/sql_acl.cc 2009-11-09 16:04:05.000000000 +0300
> @@ -3884,6 +3884,7 @@ bool check_grant(THD *thd, ulong want_ac
> TABLE_LIST *table, *first_not_own_table= thd->lex->first_not_own_table();
> Security_context *sctx= thd->security_ctx;
> uint i;
> + ulong orig_want_access= want_access;
> DBUG_ENTER("check_grant");
> DBUG_ASSERT(number > 0);
>
> @@ -3917,6 +3918,25 @@ bool check_grant(THD *thd, ulong want_ac
> sctx = test(table->security_ctx) ?
> table->security_ctx : thd->security_ctx;
>
> + const ACL_internal_table_access *access;
> + access= get_cached_table_access(& table->grant.m_internal,
> + table->get_db_name(),
> + table->get_table_name());
> +
> + if (access)
> + {
> + switch(access->check(orig_want_access))
> + {
> + case ACL_INTERNAL_ACCESS_GRANTED:
> + continue;
Hm... Could you please add comment explaining why column-level privilege
checks will work properly in this case, even although we don't update
table->grant.privilege according to privileges granted (may be due to
fact that table->grant.want_privilege is 0 since GRANT_INFO is bzero'ed
when TABLE_LIST objects are constructed?).
Since in practice we don't have any ACL_internal_table_access
descendant that returns ACL_INTERNAL_ACCESS_GRANTED alternative
is to add DBUG_ASSERT(0) to this branch to emphasize that this
branch never works now.
> + case ACL_INTERNAL_ACCESS_DENIED:
> + goto err;
> + case ACL_INTERNAL_ACCESS_CHECK_GRANT:
> + break;
> + }
> + }
> +
> + want_access= orig_want_access;
> want_access&= ~sctx->master_access;
> if (!want_access)
> continue; // ok
>
> if (!(~table->grant.privilege & want_access) ||
> table->is_anonymous_derived_table() || table->schema_table)
> {
AFAIU in theory "table->schema_table" part of condition should be always
redundant since for I_S tables "!(~table->grant.privilege & want_access)"
part of condition should be always true. Unfortunately it is not so for
statements like SHOW COLUMNS since for them check_table_access() bypasses
call to check_access() and calls check_show_access() instead which does
not set "table->grant.privilege" properly. Please consider adjusting
check_show_access() to set table->grant.privilege to SELECT_ACL and
removing "table->schema_table" part of condition or at least add a
comment explaining why this part of condition is necessary.
(Another case when "table->grant.privilege" is not set properly is
when some global privilege is required in addition to simple SELECT_ACL,
but this case can and, IMO, should be handled by a change to
IS_internal_schema_access::check() method. See below.)
...
> @@ -6851,3 +6875,106 @@ bool check_routine_level_acl(THD *thd, c
> }
>
> #endif
> +
> +struct ACL_internal_schema_registry_entry
> +{
> + const LEX_STRING *m_name;
> + const ACL_internal_schema_access *m_access;
> +};
> +
> +/**
> + Internal schema registered.
> + Currently, this is only:
> + - performance_schema
> + - information_schema,
> + This can be reused later for:
> + - mysql
> +*/
> +static ACL_internal_schema_registry_entry registry_array[2];
> +static uint m_registry_array_size= 0;
> +
> +/**
> + Add an internal schema to the registry.
> + @param name the schema name
> + @param access the schema ACL specific rules
> +*/
> +void ACL_internal_schema_registry::register_schema
> + (const LEX_STRING *name, const ACL_internal_schema_access *access)
> +{
> + DBUG_ASSERT(m_registry_array_size < array_elements(registry_array));
> +
> + /* Not thread safe, and does not need to be. */
> + registry_array[m_registry_array_size].m_name= name;
> + registry_array[m_registry_array_size].m_access= access;
> + m_registry_array_size++;
> +}
> +
> +/**
> + Search per internal schema ACL by name.
> + @param name a schema name
> + @return per schema rules, or NULL
> +*/
> +const ACL_internal_schema_access *
> +ACL_internal_schema_registry::lookup(const char *name)
> +{
> + if (name == NULL)
> + return NULL;
Could you clarify why the above check is necessary ?
AFAICS this method is always called with non-NULL argument...
> +
> + uint i;
> + uint len= strlen(name);
> +
> + for (i= 0; i<m_registry_array_size; i++)
> + {
> + if ((registry_array[i].m_name->length == len) &&
> + (strncasecmp(registry_array[i].m_name->str, name, len) == 0))
> + return registry_array[i].m_access;
> + }
> + return NULL;
> +}
To compare system identifiers we usually use our own implementation of
strcasecmp() - my_strcasecmp() with system_charset_info as collation.
I think you should do it here as well.
Also AFAIK two strings which are equal according to my_strcasecmp()
still might have different lengths. So the above optimization which
compares string lengths first and then calls strncasecmp() is incorrect
in general case and should be removed (I don't think that accepting
and documenting trade-offs associated with such optimization and
adjusting other code, e.g., which checks if this table belongs to
INFORMATION_SCHEMA database, is a sensible option).
> +
> +/**
> + Get a cached internal schema access.
> + @param grant_internal_info the cache
> + @param schema_name the name of the internal schema
> +*/
> +const ACL_internal_schema_access *
> +get_cached_schema_access(GRANT_INTERNAL_INFO *grant_internal_info,
> + const char *schema_name)
> +{
> + if (grant_internal_info)
> + {
> + if (! grant_internal_info->m_schema_lookup_done)
> + {
> + grant_internal_info->m_schema_access=
> + ACL_internal_schema_registry::lookup(schema_name);
> + grant_internal_info->m_schema_lookup_done= true;
> + }
> + return grant_internal_info->m_schema_access;
> + }
> + return ACL_internal_schema_registry::lookup(schema_name);
> +}
> +
> +/**
> + Get a cached internal table access.
> + @param grant_internal_info the cache
> + @param schema_name the name of the internal schema
> + @param table_name the name of the internal table
> +*/
> +const ACL_internal_table_access *
> +get_cached_table_access(GRANT_INTERNAL_INFO *grant_internal_info,
> + const char *schema_name,
> + const char *table_name)
> +{
> + DBUG_ASSERT(grant_internal_info);
> + if (! grant_internal_info->m_table_lookup_done)
> + {
> + const ACL_internal_schema_access *schema_access;
> + schema_access= get_cached_schema_access(grant_internal_info, schema_name);
> + if (schema_access)
> + grant_internal_info->m_table_access= schema_access->lookup(table_name);
> + grant_internal_info->m_table_lookup_done= true;
> + }
> + return grant_internal_info->m_table_access;
> +}
In the above code please follow our coding style and use TRUE/FALSE
instead of true/false
(see http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines).
> +
> +
> --- mysql-next-mr/sql/sql_acl.h 2009-11-05 18:18:34.000000000 +0300
> +++ mysql-trunk-perfschema/sql/sql_acl.h 2009-11-09 16:03:58.000000000 +0300
> @@ -1,7 +1,7 @@
> #ifndef SQL_ACL_INCLUDED
> #define SQL_ACL_INCLUDED
>
> -/* Copyright (C) 2000-2006 MySQL AB
> +/* Copyright (C) 2000-2006 MySQL AB, 2008-2009 Sun Microsystems, Inc
>
May be it also makes sense to update copyright header in sql_acl.cc ?
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> @@ -286,4 +286,109 @@ bool has_any_table_level_privileges(THD
> #define check_grant_db(A,B) 0
> #define has_any_table_level_privileges(A,B,C) 0
> #endif
> +
> +/**
> + Result of an access check for an internal schema or table.
> + Internal ACL checks are always performed *before* using
> + the grant tables.
> + This mechanism enforces that the server implementation has full
> + control on its internal tables.
> + Depending on the internal check result, the server implementation
> + can choose to:
> + - always allow access,
> + - always deny access,
> + - delegate the decision to the database administrator,
> + by using the grant tables.
> +*/
> +enum ACL_internal_access_result
> +{
> + /** Access granted, do not use the grant tables. */
> + ACL_INTERNAL_ACCESS_GRANTED,
> + /** Access denied, do not use the grant tables. */
> + ACL_INTERNAL_ACCESS_DENIED,
> + /** No decision yet, use the grant tables. */
> + ACL_INTERNAL_ACCESS_CHECK_GRANT
> +};
> +
> +/**
> + Per internal table ACL access rules.
> + This class is an interface.
> + Per table(s) specific access rule should be implemented in a subclass.
> + @sa ACL_internal_schema_access
> +*/
> +class ACL_internal_table_access
> +{
> +public:
> + ACL_internal_table_access()
> + {}
> +
> + virtual ~ACL_internal_table_access()
> + {}
> +
> + /**
> + Check access to an internal table.
> + @param want_access the privileges requested
> + @return the access check result
> + */
> + virtual ACL_internal_access_result check(ulong want_access) const= 0;
> +};
> +
> +/**
> + Per internal schema ACL access rules.
> + This class is an interface.
> + Each per schema specific access rule should be implemented
> + in a different subclass, and registered.
> + Per schema access rules can control:
> + - every schema privileges on schema.*
> + - every table privileges on schema.table
> + @sa ACL_internal_schema_registry
> +*/
> +class ACL_internal_schema_access
> +{
> +public:
> + ACL_internal_schema_access()
> + {}
> +
> + virtual ~ACL_internal_schema_access()
> + {}
> +
> + /**
> + Check access to an internal schema.
> + @param want_access the privileges requested
> + @param [out] save_priv the privileges granted
> + @return the access check result
> + */
> + virtual ACL_internal_access_result check(ulong want_access,
> + ulong *save_priv) const= 0;
> +
> + /**
> + Search for per table ACL access rules by table name.
> + @param name the table name
> + @return per table access rules, or NULL
> + */
> + virtual const ACL_internal_table_access *lookup(const char *name) const= 0;
> +};
> +
> +/**
> + A registry for per internal schema ACL.
> + An 'internal schema' is a database schema maintained by the
> + server implementation, such as 'performance_schema' and 'INFORMATION_SCHEMA'.
> +*/
> +class ACL_internal_schema_registry
> +{
> +public:
> + static void register_schema(const LEX_STRING *name,
> + const ACL_internal_schema_access *access);
> + static const ACL_internal_schema_access *lookup(const char *name);
> +};
> +
> +const ACL_internal_schema_access *
> +get_cached_schema_access(GRANT_INTERNAL_INFO *grant_internal_info,
> + const char *schema_name);
> +
> +const ACL_internal_table_access *
> +get_cached_table_access(GRANT_INTERNAL_INFO *grant_internal_info,
> + const char *schema_name,
> + const char *table_name);
> +
> #endif /* SQL_ACL_INCLUDED */
> --- mysql-next-mr/sql/sql_parse.cc 2009-11-05 18:18:35.000000000 +0300
> +++ mysql-trunk-perfschema/sql/sql_parse.cc 2009-11-09 16:03:58.000000000 +0300
> @@ -1331,8 +1339,10 @@ bool dispatch_command(enum enum_server_c
> if (lower_case_table_names)
> my_casedn_str(files_charset_info, table_list.table_name);
>
> - if (check_access(thd,SELECT_ACL,table_list.db,&table_list.grant.privilege,
> - 0, 0, test(table_list.schema_table)))
> + if (check_access(thd, SELECT_ACL, table_list.db,
> + &table_list.grant.privilege,
> + &table_list.grant.m_internal,
> + 0, 0))
> break;
> if (check_grant(thd, SELECT_ACL, &table_list, TRUE, UINT_MAX, FALSE))
> break;
...
> @@ -2881,10 +2893,13 @@ end_with_restore_list:
> /* Must be set in the parser */
> DBUG_ASSERT(select_lex->db);
> if (check_access(thd, priv_needed, first_table->db,
> - &first_table->grant.privilege, 0, 0,
> - test(first_table->schema_table)) ||
> - check_access(thd,INSERT_ACL | CREATE_ACL,select_lex->db,&priv,0,0,
> - is_schema_db(select_lex->db))||
> + &first_table->grant.privilege,
> + &first_table->grant.m_internal,
> + 0, 0) ||
> + check_access(thd, INSERT_ACL | CREATE_ACL, select_lex->db,
> + &priv,
> + &first_table->grant.m_internal,
> + 0, 0) ||
> check_merge_table_access(thd, first_table->db,
> (TABLE_LIST *)
> create_info.merge_list.first))
AFAIU in the above code database name from "select_lex->db" might
differ from name in "first_table->db" in case of ALTER TABLE ... RENAME.
Therefore it is incorrect to use "first_table->grant.m_internal" cache
for the second check_access() call. I think you should use NULL instead.
...
> @@ -3012,41 +3030,50 @@ end_with_restore_list:
> }
> else
> {
> + bool got_one_priv= false;
> + ulong priv;
> ulong save_priv;
>
> + /* Loop through every privilege in the SHOW_CREATE_TABLE_ACLS set */
> + for (priv= 1; priv <= SHOW_CREATE_TABLE_ACLS; priv <<= 1)
> + {
> + if (priv & SHOW_CREATE_TABLE_ACLS)
> + {
> + save_priv= 0;
> + if (! check_access(thd, priv, first_table->db,
> + &save_priv,
> + &first_table->grant.m_internal,
> + FALSE,
> + TRUE /* do not print errors */))
> + {
> + /*
> + save_priv contains any privileges actually granted by check_access
> + (i.e. save_priv contains global (user- and database-level)
> + privileges).
> +
> + The fact that check_access() returned FALSE does not mean that
> + access is granted. We need to check if save_priv contains any
> + table-specific privilege.
> + */
> + if (save_priv & SHOW_CREATE_TABLE_ACLS)
> + {
> + /*
> + If any privilege in the SHOW_CREATE_TABLE_ACLS set is granted,
> + SHOW CREATE TABLE is allowed.
> + */
> + got_one_priv= true;
> + break;
> + }
> + }
> + }
> + }
> /*
> - If it is an INFORMATION_SCHEMA table, SELECT_ACL privilege is the
> - only privilege allowed. For any other privilege check_access()
> - reports an error. That's how internal implementation protects
> - INFORMATION_SCHEMA from updates.
> -
> - For ordinary tables any privilege from the SHOW_CREATE_TABLE_ACLS
> - set is sufficient.
> - */
> -
> - ulong check_privs= test(first_table->schema_table) ?
> - SELECT_ACL : SHOW_CREATE_TABLE_ACLS;
> -
> - if (check_access(thd, check_privs, first_table->db,
> - &save_priv, FALSE, FALSE,
> - test(first_table->schema_table)))
> - goto error;
> -
> - /*
> - save_priv contains any privileges actually granted by check_access
> - (i.e. save_priv contains global (user- and database-level)
> - privileges).
> -
> - The fact that check_access() returned FALSE does not mean that
> - access is granted. We need to check if save_priv contains any
> - table-specific privilege. If not, we need to check table-level
> - privileges.
> -
> + If none of the check_access calls returned a privilege,
> + we need to check table-level privileges.
> If there are no global privileges and no table-level privileges,
> access is denied.
> */
> -
> - if (!(save_priv & (SHOW_CREATE_TABLE_ACLS)) &&
> + if (!got_one_priv &&
> !has_any_table_level_privileges(thd, SHOW_CREATE_TABLE_ACLS,
> first_table))
> {
> my_error(ER_TABLEACCESS_DENIED_ERROR, MYF(0),
Please simple use existing check_some_access() function instead
(and drop has_any_table_level_privileges() function) :)
...
> @@ -5316,7 +5341,8 @@ bool check_one_table_access(THD *thd, ul
>
> bool
> check_access(THD *thd, ulong want_access, const char *db, ulong *save_priv,
> - bool dont_check_global_grants, bool no_errors, bool schema_db)
> + GRANT_INTERNAL_INFO *grant_internal_info,
> + bool dont_check_global_grants, bool no_errors)
> {
> Security_context *sctx= thd->security_ctx;
> ulong db_access;
Since you have changed signature of this function you need to update
comment describing it as well.
> @@ -5351,32 +5377,26 @@ check_access(THD *thd, ulong want_access
> DBUG_RETURN(TRUE); /* purecov: tested */
> }
>
> - if (schema_db)
> + if ((db != NULL) && (db != any_db))
> {
> - /*
> - We don't allow any simple privileges but SELECT_ACL or CREATE_VIEW_ACL
> - on the information_schema database.
> - */
> - want_access &= ~SELECT_ACL;
> - if (want_access & DB_ACLS)
> - {
> - if (!no_errors)
> - {
> - const char *db_name= db ? db : thd->db;
> - my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
> - sctx->priv_user, sctx->priv_host, db_name);
> + const ACL_internal_schema_access *access;
> + access= get_cached_schema_access(grant_internal_info, db);
> + if (access)
> + {
> + switch (access->check(want_access, save_priv))
> + {
> + case ACL_INTERNAL_ACCESS_GRANTED:
> + DBUG_RETURN(FALSE);
> + case ACL_INTERNAL_ACCESS_DENIED:
> + if (! no_errors)
> + {
> + my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
> + sctx->priv_user, sctx->priv_host, db);
> + }
> + DBUG_RETURN(TRUE);
> + case ACL_INTERNAL_ACCESS_CHECK_GRANT:
> + break;
> }
> - /*
> - Access denied;
> - [out] *save_privileges= 0
> - */
> - DBUG_RETURN(TRUE);
> - }
> - else
> - {
> - /* Access granted */
> - *save_priv= SELECT_ACL;
> - DBUG_RETURN(FALSE);
> }
> }
>
...
> @@ -5605,23 +5625,6 @@ check_table_access(THD *thd, ulong requi
> sctx= backup_ctx;
>
> /*
> - Always allow SELECT on schema tables. This is done by removing the
> - required SELECT_ACL privilege in the want_access parameter.
> - Disallow any other DDL or DML operation on any schema table.
> - */
> - if (tables->schema_table)
> - {
> - want_access &= ~SELECT_ACL;
> - if (want_access & DB_ACLS)
> - {
> - if (!no_errors)
> - my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
> - sctx->priv_user, sctx->priv_host,
> - INFORMATION_SCHEMA_NAME.str);
> - goto deny;
> - }
> - }
> - /*
> Register access for view underlying table.
> Remove SHOW_VIEW_ACL, because it will be checked during making view
> */
> @@ -5641,18 +5644,11 @@ check_table_access(THD *thd, ulong requi
> (int)tables->table->s->tmp_table))
> continue;
> thd->security_ctx= sctx;
> - if ((sctx->master_access & want_access) == want_access &&
> - thd->db)
> - tables->grant.privilege= want_access;
> - else if (tables->db && thd->db && strcmp(tables->db,
> thd->db) == 0)
> - {
> - if (check_access(thd, want_access, tables->get_db_name(),
> - &tables->grant.privilege, 0, no_errors,
> - test(tables->schema_table)))
> - goto deny; // Access denied
> - }
> - else if (check_access(thd, want_access, tables->get_db_name(),
> - &tables->grant.privilege, 0, no_errors, 0))
> +
> + if (check_access(thd, want_access, tables->get_db_name(),
> + &tables->grant.privilege,
> + &tables->grant.m_internal,
> + 0, no_errors))
> goto deny;
> }
I've spent some time trying to figure out if the above removal of
"if ((sctx->master_access & want_access) == want_access && thd->db)"
branch will have any negative side-effects.
I wasn't able to find any with current usage of check_table_access(),
but I would have worried much less about it if the following code
in check_access():
if ((sctx->master_access & want_access) == want_access)
{
/* get access for current db */
db_access= sctx->db_access;
/*
1. If we don't have a global SELECT privilege, we have to get the
database specific access rights to be able to handle queries of type
UPDATE t1 SET a=1 WHERE b > 0
2. Change db access if it isn't current db which is being addressed
*/
if (!(sctx->master_access & SELECT_ACL) &&
(db && (!thd->db || db_is_pattern || strcmp(db,thd->db))))
db_access=acl_get(sctx->host, sctx->ip, sctx->priv_user, db,
db_is_pattern);
/*
The effective privileges are the union of the global privileges
and the the intersection of db- and host-privileges.
*/
*save_priv=sctx->master_access | db_access;
DBUG_RETURN(FALSE);
}
was replaced with:
if ((sctx->master_access & want_access) == want_access)
{
/*
1. If we don't have a global SELECT privilege, we have to get the
database specific access rights to be able to handle queries of type
UPDATE t1 SET a=1 WHERE b > 0
2. Change db access if it isn't current db which is being addressed
*/
if (!(sctx->master_access & SELECT_ACL))
{
if (db && (!thd->db || db_is_pattern || strcmp(db, thd->db)))
db_access= acl_get(sctx->host, sctx->ip, sctx->priv_user, db,
db_is_pattern);
else
{
/* get access for current db */
db_access= sctx->db_access;
}
/*
The effective privileges are the union of the global privileges
and the the intersection of db- and host-privileges.
*/
*save_priv= sctx->master_access | db_access;
}
else
*save_priv= sctx->master_access;
DBUG_RETURN(FALSE);
}
As it should eliminate any chance that save_priv argument will be set
to value which will include privileges to some database other than
was passed to check_access() and it will have any negative effect.
So please consider such a change.
> thd->security_ctx= backup_ctx;
> @@ -5683,8 +5679,10 @@ check_routine_access(THD *thd, ulong wan
> */
> if ((thd->security_ctx->master_access & want_access) == want_access)
> tables->grant.privilege= want_access;
> - else if (check_access(thd,want_access,db,&tables->grant.privilege,
> - 0, no_errors, 0))
> + else if (check_access(thd, want_access, db,
> + &tables->grant.privilege,
> + &tables->grant.m_internal,
> + 0, no_errors))
> return TRUE;
>
> return check_grant_routine(thd, want_access, tables, is_proc, no_errors);
> @@ -5710,11 +5708,7 @@ bool check_some_routine_access(THD *thd,
> ulong save_priv;
> if (thd->security_ctx->master_access & SHOW_PROC_ACLS)
> return FALSE;
> - /*
> - There are no routines in information_schema db. So we can safely
> - pass zero to last paramter of check_access function
> - */
> - if (!check_access(thd, SHOW_PROC_ACLS, db, &save_priv, 0, 1, 0) ||
> + if (!check_access(thd, SHOW_PROC_ACLS, db, &save_priv, NULL, 0, 1) ||
> (save_priv & SHOW_PROC_ACLS))
> return FALSE;
> return check_routine_level_acl(thd, db, name, is_proc);
For two above functions could please add comments explaining why it
is OK to bypass calls to ACL_internal_schema_access::check() for if
there is enough privileges on global level, i.e. in
thd->security_ctx->master_access (AFAIU the main reason is that they
are used only for existing routines and there are no routines in I_S
and P_S databases).
...
> --- mysql-next-mr/sql/sql_show.cc 2009-11-05 18:18:36.000000000 +0300
> +++ mysql-trunk-perfschema/sql/sql_show.cc 2009-11-09 16:03:46.000000000 +0300
> @@ -7314,6 +7314,52 @@ bool show_create_trigger(THD *thd, const
> */
> }
>
> +class IS_internal_schema_access : public ACL_internal_schema_access
> +{
> +public:
> + IS_internal_schema_access()
> + {}
> +
> + ~IS_internal_schema_access()
> + {}
> +
> + ACL_internal_access_result check(ulong want_access,
> + ulong *save_priv) const;
> +
> + const ACL_internal_table_access *lookup(const char *name) const;
> +};
> +
> +ACL_internal_access_result
> +IS_internal_schema_access::check(ulong want_access,
> + ulong *save_priv) const
> +{
> + if (unlikely(want_access & ~(SELECT_ACL | FILE_ACL)))
> + return ACL_INTERNAL_ACCESS_DENIED;
> +
> + if ((want_access & ~SELECT_ACL) == 0)
> + {
> + /* Always grant SELECT for the information schema */
> + *save_priv= SELECT_ACL;
> + return ACL_INTERNAL_ACCESS_GRANTED;
> + }
> +
> + return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> +}
I think in this method it is better to use code more close to
code which originally handled I_S tables in check_access(),
i.e.:
ACL_internal_access_result
IS_internal_schema_access::check(ulong want_access,
ulong *save_priv) const
{
want_access &= ~SELECT_ACL;
/*
We don't allow any simple privileges but SELECT_ACL on
the information_schema database.
*/
if (unlikely(want_access & DB_ACLS))
return ACL_INTERNAL_ACCESS_DENIED;
/* Always grant SELECT for the information schema. */
*save_priv= SELECT_ACL;
return ACL_INTERNAL_ACCESS_GRANTED;
}
- It will solve the problem with proper setting of GRANT_INFO::privilege
member to correct value in cases when in addition to SELECT_ACL some
global privileges are also requested (e.g. FILE_ACL), which I have
mentioned above.
- Also in this case you won't have to explain in the comment why do
you pay special attention to FILE_ACL and disallow other global
privileges.
(There is no harm in returning ACL_INTERNAL_ACCESS_GRANTED in this case
even though we don't know if we have all necessary privileges, as in
check_access() this only means that one needs to call check_grant()
in order to determine if privileges are there, and the latter does
its own privilege checks).
> +
> +const ACL_internal_table_access *
> +IS_internal_schema_access::lookup(const char *name) const
> +{
> + return NULL;
> +}
> +
> +static IS_internal_schema_access is_internal_schema_access;
> +
> +void initialize_information_schema_acl()
> +{
> + ACL_internal_schema_registry::register_schema(&INFORMATION_SCHEMA_NAME,
> + &is_internal_schema_access);
> +}
> +
> /*
> Convert a string in character set in column character set format
> to utf8 character set if possible, the utf8 character set string
> --- /dev/null
> +++ mysql-trunk-perfschema/storage/perfschema/pfs_engine_table.cc
>
> /* Copyright (C) 2008-2009 Sun Microsystems, Inc
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; version 2 of the License.
>
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> GNU General Public License for more details.
>
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
>
> /**
> @file storage/perfschema/pfs_engine_table.cc
> Performance schema tables (implementation).
> */
>
> #include "mysql_priv.h"
> #include "pfs_engine_table.h"
>
> #include "table_events_waits.h"
> #include "table_setup_consumers.h"
> #include "table_setup_instruments.h"
> #include "table_setup_objects.h"
> #include "table_setup_timers.h"
> #include "table_performance_timers.h"
> #include "table_processlist.h"
> #include "table_events_waits_summary.h"
> #include "table_sync_instances.h"
> #include "table_file_instances.h"
> #include "table_file_summary.h"
>
> /* For show status */
> #include "pfs_column_values.h"
> #include "pfs_instr.h"
>
> /**
> @addtogroup Performance_schema_engine
> @{
> */
>
> static PFS_engine_table_share *all_shares[]=
> {
> &table_events_waits_current::m_share,
> &table_events_waits_history::m_share,
> &table_events_waits_history_long::m_share,
> &table_setup_consumers::m_share,
> &table_setup_instruments::m_share,
> &table_setup_objects::m_share,
> &table_setup_timers::m_share,
> &table_performance_timers::m_share,
> &table_processlist::m_share,
> &table_events_waits_summary_by_thread_by_event_name::m_share,
> &table_events_waits_summary_by_event_name::m_share,
> &table_events_waits_summary_by_instance::m_share,
> &table_file_summary_by_event_name::m_share,
> &table_file_summary_by_instance::m_share,
> &table_mutex_instances::m_share,
> &table_rwlock_instances::m_share,
> &table_cond_instances::m_share,
> &table_file_instances::m_share,
> NULL
> };
...
> static int compare_table_names(const char *name1, const char *name2)
> {
> if (lower_case_table_names)
> return strcasecmp(name1, name2);
> return strcmp(name1, name2);
> }
>
> /**
> Find a table share by name.
> @param name The table name
> @return table share
> */
> const PFS_engine_table_share*
> PFS_engine_table::find_engine_table_share(const char *name)
> {
> DBUG_ENTER("PFS_engine_table::find_table_share");
>
> PFS_engine_table_share **current;
> uint name_len= strlen(name);
>
> for (current= &all_shares[0]; (*current) != NULL; current++)
> {
> if ((name_len == (*current)->m_name.length) &&
> (compare_table_names(name, (*current)->m_name.str) == 0))
> DBUG_RETURN(*current);
> }
>
> DBUG_RETURN(NULL);
> }
AFAICS this method is used only for looking up share for the purpose of
privilege checks. But at this stage table names should be already
"normalized" (i.e. in lower case) so there is no need to do strcasecmp().
I think that compare_table_names() should be replaced with unconditional
strcmp() (or even with strncmp()). This will allow to avoid question
about what collation should be used for my_strcasecmp() and will
legitimize usage of compare-lengths-first optimization (see my comment
above about its applicability to strcasecmp()).
...
> class PFS_internal_schema_access : public ACL_internal_schema_access
> {
> public:
> PFS_internal_schema_access()
> {}
>
> ~PFS_internal_schema_access()
> {}
>
> ACL_internal_access_result check(ulong want_access,
> ulong *save_priv) const;
>
> const ACL_internal_table_access *lookup(const char *name) const;
> };
>
> ACL_internal_access_result
> PFS_internal_schema_access::check(ulong want_access,
> ulong *save_priv) const
> {
> const ulong always_forbidden= /* CREATE_ACL | DROP_ACL | */ REFERENCES_ACL
> | INDEX_ACL | ALTER_ACL | CREATE_TMP_ACL | EXECUTE_ACL
> | CREATE_VIEW_ACL | SHOW_VIEW_ACL | CREATE_PROC_ACL | ALTER_PROC_ACL
> | EVENT_ACL | TRIGGER_ACL ;
>
> if (unlikely(want_access & always_forbidden))
> return ACL_INTERNAL_ACCESS_DENIED;
>
> /*
> Proceed with regular grant tables,
> to give administrative control to the DBA.
> */
> return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> }
See comment about DROP_ACL and TRUNCATE below.
>
> const ACL_internal_table_access *
> PFS_internal_schema_access::lookup(const char *name) const
> {
> const PFS_engine_table_share* share;
> share= PFS_engine_table::find_engine_table_share(name);
> if (share)
> return share->m_acl;
> /*
> Do not return NULL, it would mean we are not interested
> in privilege checks for unknown tables.
> Instead, return an object that denies every actions,
> to prevent users for creating their own tables in the
> performance_schema database schema.
> */
> return &pfs_unknown_acl;
> }
>
> PFS_internal_schema_access pfs_internal_access;
>
> void initialize_performance_schema_acl(bool bootstrap)
> {
> /*
> ACL is always enforced, even if the performance schema
> is not enabled (the tables are still visible).
> */
> if (! bootstrap)
> {
> ACL_internal_schema_registry::register_schema(&PERFORMANCE_SCHEMA_str,
> &pfs_internal_access);
> }
> }
>
> PFS_readonly_acl pfs_readonly_acl;
>
> ACL_internal_access_result
> PFS_readonly_acl::check(ulong want_access) const
> {
> const ulong always_forbidden= INSERT_ACL | UPDATE_ACL | DELETE_ACL
> | /* CREATE_ACL | DROP_ACL | */ REFERENCES_ACL | INDEX_ACL | ALTER_ACL
> | CREATE_VIEW_ACL | SHOW_VIEW_ACL | TRIGGER_ACL | LOCK_TABLES_ACL;
>
> if (unlikely(want_access & always_forbidden))
> return ACL_INTERNAL_ACCESS_DENIED;
>
> return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> }
>
> PFS_truncatable_acl pfs_truncatable_acl;
>
> ACL_internal_access_result
> PFS_truncatable_acl::check(ulong want_access) const
> {
> const ulong always_forbidden= INSERT_ACL | UPDATE_ACL | DELETE_ACL
> | /* CREATE_ACL | DROP_ACL | */ REFERENCES_ACL | INDEX_ACL | ALTER_ACL
> | CREATE_VIEW_ACL | SHOW_VIEW_ACL | TRIGGER_ACL | LOCK_TABLES_ACL;
>
> if (unlikely(want_access & always_forbidden))
> return ACL_INTERNAL_ACCESS_DENIED;
>
> return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> }
Since TRUNCATE TABLE requires DROP privilege I guess you will never be
able to forbid DROP_ACL completely. So I guess it is better to remove
it from "always_forbidden" list (I guess the fact that is there means
that you plan to uncomment it in future, right?).
>
> PFS_updatable_acl pfs_updatable_acl;
>
> ACL_internal_access_result
> PFS_updatable_acl::check(ulong want_access) const
> {
> const ulong always_forbidden= INSERT_ACL | DELETE_ACL
> | /* CREATE_ACL | DROP_ACL | */ REFERENCES_ACL | INDEX_ACL | ALTER_ACL
> | CREATE_VIEW_ACL | SHOW_VIEW_ACL | TRIGGER_ACL;
>
> if (unlikely(want_access & always_forbidden))
> return ACL_INTERNAL_ACCESS_DENIED;
>
> return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> }
>
> PFS_editable_acl pfs_editable_acl;
>
> ACL_internal_access_result
> PFS_editable_acl::check(ulong want_access) const
> {
> const ulong always_forbidden= /* CREATE_ACL | DROP_ACL | */ REFERENCES_ACL
> | INDEX_ACL | ALTER_ACL | CREATE_VIEW_ACL | SHOW_VIEW_ACL | TRIGGER_ACL;
>
> if (unlikely(want_access & always_forbidden))
> return ACL_INTERNAL_ACCESS_DENIED;
>
> return ACL_INTERNAL_ACCESS_CHECK_GRANT;
> }
>
> PFS_unknown_acl pfs_unknown_acl;
>
> ACL_internal_access_result
> PFS_unknown_acl::check(ulong want_access) const
> {
> return ACL_INTERNAL_ACCESS_DENIED;
> }
>
...
Otherwise, I am OK with your patch and think that it can be pushed
after fixing/discussing the above issues.
--
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
