Cinder encrypted vol connection info include full nova internal class name

Bug #1639293 reported by Daniel Berrange on 2016-11-04
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Undecided
Unassigned
os-brick
Undecided
Unassigned
tempest
Undecided
Lee Yarwood

Bug Description

When making an API call to Cinder to get the volume encryption metadata:

2016-10-25 05:30:47.048 6699 DEBUG cinderclient.v2.client [req-7fe1959f-36f6-4dd6-bf47-f5e550d59530 3fdb4fc839fc44a9a99006d3fb75ac4d c6b14fd5fffa48aa8eb869f30c80e409 - - -] REQ: curl -g -i -X GET http://192.168.1.13:8776/v2/c6b14fd5fffa48aa8eb869f30c80e409/volumes/9439e922-1051-4d83-87c7-172689ac29da/encryption -H "User-Agent: python-cinderclient" -H "Accept: application/json" -H "X-Auth-Token: {SHA1}4ff393589c57548e39cca7b5ed99d8a42d1ac7fa" _http_log_request /usr/lib/python2.7/site-packages/keystoneauth1/session.py:337

The reply from cinder includes a fully qualified name of a nova private class:

2016-10-25 05:30:47.100 6699 DEBUG cinderclient.v2.client [req-7fe1959f-36f6-4dd6-bf47-f5e550d59530 3fdb4fc839fc44a9a99006d3fb75ac4d c6b14fd5fffa48aa8eb869f30c80e409 - - -] RESP: [200] X-Compute-Request-Id: req-4e32d4e4-3c02-4fdf-8179-7c9825f446e3 Content-Type: application/json Content-Length: 209 X-Openstack-Request-Id: req-4e32d4e4-3c02-4fdf-8179-7c9825f446e3 Date: Tue, 25 Oct 2016 09:30:47 GMT Connection: keep-alive
RESP BODY: {"cipher": "aes-xts-plain64", "encryption_key_id": "00000000-0000-0000-0000-000000000000", "provider": "nova.volume.encryptors.cryptsetup.CryptsetupEncryptor", "key_size": 256, "control_location": "front-end"}
 _http_log_response /usr/lib/python2.7/site-packages/keystoneauth1/session.py:366

THis is very bad for a number of reasons

 - If nova renames its classes, existing encryption breaks because the classs names no longer match what cinder is sending
 - It allows out of tree extensions to nova for different encryption impls, which consume Nova private data structures in method calls. THis is against Nova policy - all such other extension points have been deprected, then removed.
 - If nova wants to implement encryption in a different way (eg by delegating to QEMU), then the concept of an encryptor class does not even apply.

This is actually even worse than Cinder merely passing class names across. Cinder in fact exposes this in its public REST API to tenant users, letting tenants specify arbitrary encryptor classname for nova to use:

http://docs.openstack.org/mitaka/config-reference/block-storage/volume-encryption.html

$ cinder encryption-type-create --cipher aes-xts-plain64 --key_size 512 \
  --control_location front-end LUKS nova.volume.encryptors.luks.LuksEncryptor

The idea of having the tenant user specify arbitrary nova private class names needs to be removed entirely. Instead we should have an enum of encryption *formats*. Any given format may be implemented by Nova in a variety of ways. Nova will look at the format and decide which encryptor class to use (if any), or decide how to configure QEMU natively to use that format.

For back compat we can't drop use of class names immediately, so we'll need a deprecation period.

In Ocata:

 - Cinder and Nova should allow an encryption format enum to be used in the 'provider' field instead of a class name. The format would be one of

     'plain' - corresponds to CryptsetupEncryptor
     'luks' - corresponds to LukEncryptor

   This would be the preferred approach going forward

 - Nova should issue a warning if it receives a 'provider' class name that does not correspond to an existing in-tree encryptor class

 - Cinder should re-write class names to the format enum for the built-in classes - out of tree classnames should be left alone.

In Pike

 - Cinder should continue re-writing class names to enums for in-tree classes, but reject out of tree class names with fatal error

 - The cinder v3 should have a microversion added to indicate the point at which 'provider' will be strictly validated against the 'enum'.

 - Nova should raise an error if it receives a 'provider' class name that does not correspond to an existing in-tree encryptor class

In Qxxx

 - Nova will stop accepting class names from cinder entirely - cinder should exclusively be reporting the format enum to Nova, rewriting legacy data if needed.

Lee Yarwood (lyarwood) on 2016-11-04
Changed in nova:
assignee: nobody → Lee Yarwood (lyarwood)
Changed in nova:
importance: Undecided → High

Fix proposed to branch: master
Review: https://review.openstack.org/393901

Changed in nova:
status: New → In Progress

Reviewed: https://review.openstack.org/393901
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=cbbc21828283e5dbd45016c1bc5fc24c2c20f365
Submitter: Jenkins
Branch: master

commit cbbc21828283e5dbd45016c1bc5fc24c2c20f365
Author: Lee Yarwood <email address hidden>
Date: Fri Nov 4 18:28:47 2016 +0000

    encryptors: Introduce encryption provider constants

    These constants detail the supported encryption formats and their
    associated in tree encryption provider implementations.

    The use of out of tree and direct use of these in tree implementations
    is now deprecated and will be blocked in the 16.0.0 Pike release of
    Nova.

    Change-Id: Ic155bd29d46059832cce970bf60375e7e472eca6
    Partial-bug: #1639293

Reviewed: https://review.openstack.org/418371
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=5593d74435b441ef38b301b86b5b0642f9b0b85f
Submitter: Jenkins
Branch: master

commit 5593d74435b441ef38b301b86b5b0642f9b0b85f
Author: Lee Yarwood <email address hidden>
Date: Tue Jan 10 12:17:50 2017 +0000

    releasenotes: Add missing releasenote for encryption provider constants

    Introduced by Ic155bd29d46059832cce970bf60375e7e472eca6 but not
    documented in a releasenote.

    Change-Id: I41e6a3f32e2640fe736ea70a542d9b594bbf328e
    Partial-bug: #1639293

Lee Yarwood (lyarwood) on 2017-06-14
Changed in os-brick:
assignee: nobody → Lee Yarwood (lyarwood)
no longer affects: nova
Changed in tempest:
assignee: nobody → Lee Yarwood (lyarwood)
Changed in os-brick:
status: New → Confirmed
Changed in tempest:
status: New → Confirmed

Unassigning due to no activity for > 6 months.

Changed in os-brick:
assignee: Lee Yarwood (lyarwood) → nobody
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers