Page MenuHomePhabricator

Redefine mysql GRANTs for wikiadmin
Open, HighPublic

Description

Due to T157651: sql.php must not run LoadExtensionSchemaUpdates causing the drop of a table in production T249565: Wikidata's wb_items_per_site table has suddenly disappeared, creating DBQueryErrors on page views, one of the action items is to remove DROP and ALTER grant from the wikiadmin user.
This is what wikiadmin user currently have:

GRANT ALL PRIVILEGES ON `%wik%`.*

We should probably limit it to:

  • SELECT
  • UPDATE
  • INSERT
  • DELETE
  • PROCESS
  • REPLICATION CLIENT
  • CREATE
  • CREATE TEMPORARY TABLES
  • INDEX?

Anything else?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 8 2020, 6:22 AM

CREATE is already there but I want to emphasize that we need it for two reasons: 1- Sometimes devs, with coordination with DBAs, create tables in production. I have done it before several times. 2- During the outage, one of the reasons I was able to reduce the impact was that I had the CREATE right to create the table. Does this include creating indexes as well, right?

creating indexes as well, right

No, that would require ALTER rights. Creates are a non issue, that is why we let them be created by owners (e.g new extension or new wiki). ALTERS, however, are a source of both performance and replication issues. Maintenance will have to be adapted to create the indexes at the same time than the tables, not after.

hmm, my problem is that the command to create the table is like this:

CREATE TABLE IF NOT EXISTS /*_*/wb_items_per_site (
  ips_row_id                 BIGINT unsigned     NOT NULL PRIMARY KEY AUTO_INCREMENT, -- row ID
  ips_item_id                INT unsigned        NOT NULL, -- Id of the item
  ips_site_id                VARBINARY(32)       NOT NULL, -- Site identifier (global)
  ips_site_page              VARCHAR(310)        NOT NULL -- Prefixed title of the page
) /*$wgDBTableOptions*/;

CREATE UNIQUE INDEX /*i*/wb_ips_item_site_page ON /*_*/wb_items_per_site (ips_site_id, ips_site_page);
CREATE INDEX /*i*/wb_ips_item_id ON /*_*/wb_items_per_site (ips_item_id);

I create the table first then I add the indexes like that. I can't add the indexes as part of creating the table. Maybe I'm missing something obvious here.

Tgr added a comment.Apr 8 2020, 10:01 AM

This is the wrong direction to attack the problem from, IMO. ALTER is useful but also dangerous; there is no way to separate functionality into non-overlapping "safe" and "not needed" sets.

The real problem is that a dump script should never run with an admin role in the first place. There should probably be a separate wikireader and wikieditor role (at least the latter exists already IIRC), dump scripts and manual debugging by default should use the first (read only + EXPLAIN), purge etc. scripts and manual debugging when explicitly requested would use the second (read/write but no DDL) and the role for schema changes should only be accessible with a separate explicit flag.

I can see that ALTER might be useful sometimes and might require a longer discussion and some other deep changes (more different roles etc), I don't think we should be keeping DROP for any reason, right?

I can't add the indexes as part of creating the table

Why not? This is literally the definition live on the DB:

CREATE TABLE `wb_items_per_site` (
  `ips_row_id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `ips_item_id` int(10) unsigned NOT NULL,
  `ips_site_id` varbinary(32) NOT NULL,
  `ips_site_page` varbinary(310) NOT NULL,
  PRIMARY KEY (`ips_row_id`),
  UNIQUE KEY `wb_ips_item_site_page` (`ips_site_id`,`ips_site_page`),
  KEY `wb_ips_item_id` (`ips_item_id`)
)

If it cannot be done it is mediawiki's model fault, it is clearly possible on db, and more efficient/safer.

This is the wrong direction to attack the problem from

Removing wikiadmin grants, or stopping using that account and using 3 or 4 others without those grants are just a semantic difference :-D.

hoo added a subscriber: hoo.Apr 8 2020, 1:21 PM
Anomie added a comment.Apr 8 2020, 2:43 PM

Anything else?

Does CREATE cover CREATE TEMPORARY TABLES? If not, we should most likely include that one too.

There's also INDEX, see below.

While we don't currently use views in production as far as I know, we might add SHOW VIEW in case we ever do.

creating indexes as well, right

No, that would require ALTER rights. Creates are a non issue, that is why we let them be created by owners (e.g new extension or new wiki). ALTERS, however, are a source of both performance and replication issues. Maintenance will have to be adapted to create the indexes at the same time than the tables, not after.

I note https://mariadb.com/kb/en/grant/#table-privileges lists an "INDEX" privilege that allows CREATE INDEX.

MediaWiki's MySQL/MariaDB schemas generally avoid adding indexes in the CREATE TABLE statement, possibly because the same tables.sql is also munged for SQLite and SQLite supports "PRIMARY KEY" and "UNIQUE" but not plain "KEY".

mark added a subscriber: mark.Apr 8 2020, 2:53 PM

Anything else?

Does CREATE cover CREATE TEMPORARY TABLES? If not, we should most likely include that one too.

No, CREATE does not include temporary tables for that we need CREATE TEMPORARY TABLES. I will add it to the list, thank you.

There's also INDEX, see below.

While we don't currently use views in production as far as I know, we might add SHOW VIEW in case we ever do.

creating indexes as well, right

No, that would require ALTER rights. Creates are a non issue, that is why we let them be created by owners (e.g new extension or new wiki). ALTERS, however, are a source of both performance and replication issues. Maintenance will have to be adapted to create the indexes at the same time than the tables, not after.

I note https://mariadb.com/kb/en/grant/#table-privileges lists an "INDEX" privilege that allows CREATE INDEX.

Yes, INDEX grant would allow us to keep using the way we currently use via CREATE INDEX but not ALTER, it could be a solution for the specific case of needing to keep creating indexes on a separate way than including them on the table structure.
Note, that CREATE INDEX can still create replication issues, but it is of course a lot narrower than ALTER in terms of potential data loss (dropping a column etc) and available operations in general.

Reedy added a comment.EditedApr 8 2020, 4:20 PM

I can't add the indexes as part of creating the table

Why not? This is literally the definition live on the DB:

CREATE TABLE `wb_items_per_site` (
  `ips_row_id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `ips_item_id` int(10) unsigned NOT NULL,
  `ips_site_id` varbinary(32) NOT NULL,
  `ips_site_page` varbinary(310) NOT NULL,
  PRIMARY KEY (`ips_row_id`),
  UNIQUE KEY `wb_ips_item_site_page` (`ips_site_id`,`ips_site_page`),
  KEY `wb_ips_item_id` (`ips_item_id`)
)

If it cannot be done it is mediawiki's model fault, it is clearly possible on db, and more efficient/safer.

You don't need to move the PK from the first line to the seperate line.

AFAIK, the reason we tend to prefer the CREATE INDEX lines after the CREATE TABLE (rather than inline like that) is for sqlite compatibility, to save some duplication, as both work fine for mysql, but only one form work for sqlite.

But doing a manual transform between the two, when not under pressure, isn't difficult

Kormat added a subscriber: Kormat.Apr 30 2020, 11:00 AM
jcrespo added a parent task: Restricted Task.Aug 18 2020, 8:56 AM