From 9882c917c94ced977421d937a178a1a1aac33e2a Mon Sep 17 00:00:00 2001 From: Brian Rosmaita Date: Thu, 25 May 2017 08:47:11 -0400 Subject: [PATCH 1/1] DO NOT GIT-REVIEW! This must be reviewed on the bug report. Change-Id: I97747479f5e6af1a04ddf1761b8edb9c738f12eb --- .../approved/glance/prevent-premature-deletion.rst | 455 ++++++++++++++++++++ 1 files changed, 455 insertions(+), 0 deletions(-) create mode 100644 specs/pike/approved/glance/prevent-premature-deletion.rst diff --git a/specs/pike/approved/glance/prevent-premature-deletion.rst b/specs/pike/approved/glance/prevent-premature-deletion.rst new file mode 100644 index 0000000..7eb23f6 --- /dev/null +++ b/specs/pike/approved/glance/prevent-premature-deletion.rst @@ -0,0 +1,455 @@ +.. + This work is licensed under a Creative Commons Attribution 3.0 Unported + License. + + http://creativecommons.org/licenses/by/3.0/legalcode + +======================================== +Prevent premature deletion of image data +======================================== + +https://blueprints.launchpad.net/glance/+spec/prevent-premature-deletion + +In a non-default configuration, it is possible for image data to be +deleted even though an image record containing the location of that +data still exists. + + +Problem description +=================== + +Suppose that glance is deployed in a non-default configuration with +``show_multiple_locations = True`` but with the following (default) policy +settings: + +- ``get_location: ""`` +- ``set_location: ""`` +- ``delete_location: ""`` + +Assume that the backing store for Glance is not a read-only store. In +such a configuration, it is possible for user U to determine the +location of image data from image I (which U may or may not own), +create a new image J (which U does own), and set the location of image +I's data as a location for image J's data. If U deletes J, the image +data for I is removed from the glance store, thereby making image I +useless. + +To be clear on the players here: + +* operator: sets the configuration as described above +* end user: can perform the actions described, given that configuration + +Note that for read-only stores, for example, the http store, it makes +sense that there could be multiple image records owned by various end +users that all refer to the same image location. + +Proposed change +=============== + +Implement reference counting for image locations, and only allow +deletion when the counter reaches zero. + +This needs to be done in Glance (as opposed to in glance_store) +because of the way Glance currently uses multiple stores. There is +only one default store which is "active" as the Glance store, and +hence if we put location checking in the glance_store driver, if store +A is active, it wouldn't know how to verify a location of the store B +scheme. + +The change would be accomplished as follows. + +1. Add a ``to_be_deleted`` table to the database. This is a single-column + table that stores an image location URI (type: text). + +2. Add a ``locations_count`` table. It would contain the following columns: + + * ``hash`` - type: fixed length char (primary key) + * ``count`` - type: int (default value: 1) + * ``location`` - type: text + + This table would not support soft deletions. It would contain a database + trigger whose behavior is described below. This trigger would be fired + when a change on the ``count`` column occurs. + +The new tables would be used as follows. + +1. When an image location is set: + + 1. Normalize the URI. After this change, Glance will store only + normalized URIs (this will have to be pointed out in release + notes). This primarily affects URIs for the Ceph store, which + have a long form and a short form that refer to the same location + in Ceph. Glance would use only the short form. For example, + + * long form: ``rbd://647f7ae8-648a-44f5-83ad-f7bd2299274e/images/2e4b6dca-9700-4715-b81d-4463cd7038de/snap`` + * short form: ``rbd://2e4b6dca-9700-4715-b81d-4463cd7038de`` + + Normalization will also have to occur for swift URIs, because + anyone with access to the glance_store source code (i.e., + everyone) can figure out that images are stored as "Large + Objects" in swift, that is, as a series of segments bound by a + manifest. Thus it could be possible for a user to create an + image in Glance, set the location of the image data as the third + segment of some swift Large Object, and then delete the image, + thereby removing the third segment, and rendering the image data + stored as a Large Object useless. In the case of swift + normalization, if a swift image location is of the form of a + segment as created by Glance, the set image operation should + fail. + + Note that if an administrator has uploaded images directly to + the swift backend using a non-Glance naming scheme, we're not + going to be able to detect it. On the other hand, there is no + reason for an operator using the swift backend to expose image + locations, so hopefully this situation has not occurred. + + 2. See if the location is in the ``to_be_deleted`` table. This + table should be small, so the lookup should not affect + performance. + + If the location is in the ``to_be_deleted`` table: + + fail the set-location operation with a suitable error + indicating that the data at that location is inaccessible. + + 3. Otherwise, hash the location using a suitable algorithm + (probably sha-512, which appears to be faster than sha-256). + The key thing here is that we want a fixed-length string that + can serve as a primary key in order to force a uniqueness + constraint on the column. + + 4. Insert (hash, location) into the ``locations_count`` table. + + If the insertion fails due to a uniqueness constraint violation: + + increment the count on the table using the "incrementing + sequence" behavior of the database in order to prevent race + conditions. + +2. When an image location is deleted: + + 1. Hash the location URI. + + 2. Decrement the count in the ``locations_count`` table using the + "decrementing sequence" behavior of the database. + + 3. The previous step would invoke a database trigger that would behave + as follows: + + 1. Lock the ``locations_count`` table. + + 2. If ``count`` == 0: + + 1. Insert the ``location`` value into the ``to_be_deleted`` + table. + + 2. Remove the row from the ``locations_count`` table. + + 3. Release the lock on the ``locations_count`` table. + + 4. Check the ``to_be_deleted`` table. If the ``url`` of the + location is in the table, then attempt to delete the data from + the backend. Remove the row from the ``to_be_deleted`` table + **after** a successful delete. (The row needs to remain in the + table in order to prevent another user from setting this location + on a new image.) + + (Note that this gives Glance a way to track failed deletes. A + follow up spec could propose that we add a feature to the + scrubber to clean out "dark data" as indicated by a non-empty + ``to_be_deleted`` table. Glance could be enhanced to check this + table on startup and log a message to the operator if there's + some dark data. But this is beyond the scope of the current + spec.) + +3. When an image location is accessed: there is no change from the + current behavior. (Just mentioned here for completeness.) + + +Alternatives +------------ + +Some alternatives: + +1. Disallow multiple image records from referring to the same image data. + + We'd have to make an exception for read-only stores because, for + example, the whole point of the http store is that image data can + be stored off-site and be consumed by multiple end users. + + Additionally, there are some legitimate use cases for such usage. + For example, the same image data could be included in multiple + autoscale groups by having multiple image records having distinct + image properties appropriate to different groups. + + This could be addressed by requiring end users to upload the same + image data multiple times, but that seems inefficient and + non-user-friendly. + +2. Allow multiple image records to refer to the same image data, but + only allow the true owner to delete image data. + + It may be impossible to determine the "true" owner. For example, + suppose user One has uploaded image data directly to some store + with the intent of creating an image in Glance and setting the + location. User Two is shoulder-surfing and creates an image in + Glance and sets the location before user One has a chance to do it. + As far as Glance would be concerned, user Two would be the owner of + the image data, although in fact it is user One who is the true + owner. + +3. Modify image locations so that they have an additional field + indicating whether the location was set by Glance (in the normal + image-upload workflow) or set directly by a user. On image + deletion, Glance would only attempt to delete image data for a + location that Glance itself had created. + + This would break a common workflow for migration to a new Glance + backing store: (a) data is copied directly from backend to backend, + (b) the new location of the data is set as a second location, and + (c) after testing has been completed, (d) the original location is + removed from the image record. The "official" image location is no + longer a Glance-created image location, and hence Glance would + never delete the image data. + + This isn't a fatal objection, but it does indicate that this + strategy would require more thought, planning, and operator input + than is allowable for fixing a bug with security implications that + has been sitting too long as it is. We should make the fix + described in this spec first, and then revisit these alternatives. + +Data model impact +----------------- + +While there is no change to the data model per se, we are proposing to +implement this change with a new database table. + +* What new data objects and/or database schema changes is this going to + require? + + This change proposes the addition of a ``locations_count`` table. + +* Glance is committed to zero-downtime database migrations and has + adopted an `E-M-C migration strategy`_ to achieve this. Address the + following in sufficient detail to make it clear that the intended + database change will be achieved. + +.. _E-M-C migration strategy: https://docs.openstack.org/developer/glance/database_migrations.html + + - Will this change require database triggers? If yes, describe them. + + This change will require a permanent database trigger (that is, + the trigger isn't being used solely for the migration). + + The trigger will behave as follows: + + When a decrement operation occurs in a cell in the ``count`` + column of the ``location_count`` table (or, when an update is made + to that column): + + 1. Lock the ``location_count`` table (or if possible, that row). + + 2. If the ``count`` in the cell == 0: + + 1. Insert the ``location`` value into the ``to_be_deleted`` + table. + + 2. Remove the row from the ``locations_count`` table. + + 3. Release the lock on the ``locations_count`` table. + + - Explain what your expand migrations will look like. + + The expand migration will create the new tables and define the database + trigger. + + - Explain what your data migrations will look like. + + The ``locations_count`` table will have to be populated as follows: + + 1. The operator disables image deletion and image location + deletion on all Glance nodes by modifying the the + ``delete_image`` and ``delete_image_location`` policies and + issuing a SIGHUP, causing Glance to reload its configuration + files. Deletions must be disallowed on all Glance nodes during + the data migration, and it cannot ever be allowed again on the + Ocata nodes. + + The operator should probably disable ``set_image_location`` on + the Ocata release Glance nodes near the end of the migration, + and maybe route all image-create requests to Pike release + Glance nodes, which will know how to populate the + ``image_locations`` and ``location_count`` tables correctly. + (This would allow Nova image snapshots to occur during most of + the migration.) This could be done for a brief window after + most of the data has been migrated so that the script can go + back and process any locations added by the Ocata nodes during + the data migration. + + 2. A script crawls the ``image_locations`` table. + + 1. For each non-deleted location: normalize the location URI in + the `url` column of the `image_locations` table. If the + normalized URI is different from the original, replace the + original with the normalized version. (What to do about the + swift-segment case? You can't normalize, so maybe log it and + soft-delete the row? It will make the location disappear, + but that's probably what we want.) + + 2. Hash the normalized URI and insert (hash, location) into the + ``location_count`` table as describe above for the + set-location case (that is, if the insert fails, increment + the count using the database's "increment sequence" + behavior). + + - Explain what your contract migrations will look like. + + The contract migration will be empty. + + - Finally, do these changes have the potential to interfere with the + database migrations for other specs that have been approved for this + cycle? + + There are two Pike specs with database implications: + + 1. The Glare-ectomy: this removes the artifacts tables, it has no + impact on the images tables. + + 2. The Image Import refactor: will probably have some impact on + the current image-related tables. However, note that this + proposal does not modify the current image-related tables in + any way, so it doesn't appear that there will be a conflict. + +* How will the initial set of new data objects be generated? For example if you + need to take into account existing images, or modify other existing data, + describe how that will work. + + This is described in the 'data migrations' section above. We should + probably create a copy of the `image_locations` before the data + migration occurs just in case things go sideways. + +REST API impact +--------------- + +None + +Security impact +--------------- + +This is a security improvement. Note, however, that it does not +address a data leakage issue, namely, that for some backing stores, a +user U can access another user K's image data by creating an image and +setting K's image data location as a location on U's image. Fixing +the data leakage problem is beyond the scope of this spec, plus, if U +has the URI of K's data, U can probably simply download the data from +the backing store, thereby bypassing Glance entirely. + +Notifications impact +-------------------- + +None + +Other end user impact +--------------------- + +None + +Performance Impact +------------------ + +* Calls which result in database queries can have a profound impact on + performance when called in critical sections of the code. + + The scheme outlined in this proposal should result in a fairly quick + lookup upon image location creation and deletion, so it shouldn't affect + these operations very much. + +* Will the change include any locking, and if so what considerations are there + on holding the lock? + + The lock is being held for a very short time. + +Other deployer impact +--------------------- + +Discuss things that will affect how you deploy and configure OpenStack +that have not already been mentioned, such as: + +* Is this a change that takes immediate effect after its merged, or is it + something that has to be explicitly enabled? + + An operator will have to conduct the migration explicitly, disabling + various operations as discussed above. + +Another possible impact on deployers depends on how they do their +billing for images. While this scheme will prevent data from being +deleted prematurely, it may also cause some users to pay for image +data after they've deleted their image, as would happen if some other +user has created another image whose location refers to the first +user's image data. + +Developer impact +---------------- + +Developers should understand what the new image location deletion +workflow is and how it uses database triggers. + +Implementation +============== + +Assignee(s) +----------- + +Primary assignee: + +* unknown + +Other contributors: + +* unknown + +Work Items +---------- + +1. write the database E-M-C code + +2. code changes to Glance + +3. documentation as described above + +Dependencies +============ + +None + + +Testing +======= + +The current tests for the image-delete operation should prevent regressions. + +Tests will be added for the particular scenario described in this spec. + + +Documentation Impact +==================== + +No impact to end-user documentation. + +Release notes should address the following points: + +* This change fixes bug https://bugs.launchpad.net/glance/+bug/1546507 + +* From Pike onward, Glance stores normalized URLs in the `url` for a + location. Thus, this value may differ from what was specified by the + user. (At this point, this only impacts Ceph.) + +* The database migration is a bit complex and may be too lengthy for + release notes. If so, add a page to the Glance operator's docs + about this migration. + + +References +========== + +* https://bugs.launchpad.net/glance/+bug/1546507 -- 1.7.1