SovereignCloudStack / standards

SCS standards in a machine readable format
https://scs.community/
Creative Commons Attribution Share Alike 4.0 International
34 stars 24 forks source link

Upstream Image Encryption Standardization #560

Open josephineSei opened 7 months ago

josephineSei commented 7 months ago

Currently there is the possibility in Cinder to encrypt volumes and in Nova to use qcow2 encrypted images (still under development). Both can lead to and use LUKS-encrpyted images, but those are different and not aligned:

@markus-hentsch also found out in https://github.com/SovereignCloudStack/standards/issues/541 that uploading a LUKS-encrypted image (that was created from a volume) to another cloud in combination with setting a few parameteres (cinder_encryption_key, etc...) will result in an image that can be used to create an encrpyted and functional volume.

As a user it would be good to have a streamlined operation to use encrypted images in openstack for both volumes and ephemeral storage and to also allow interoperability between clouds. Therefore we need and will propose standardized parameters to describe and detect an encrypted image, which might be similar to the parameters described here: https://specs.openstack.org/openstack/cinder-specs/specs/zed/image-encryption.html But will use the LUKS encryption. So those encrypted images could be natively mounted in Nova or just formed into a volume (raw LUKS images can be directly used, qcow images need to be flattened).

With such a way encrypted backup images can be easily downloaded and transferred to another cloud.

This is a result from a lengthy discussion at the PTG with Nova, Cinder and Glance ( https://etherpad.opendev.org/p/dalmatian-ptg-cinder#L376 )

Followup tasks may be to implement re-encryption to fully change keys for LUKS volumes and images.

markus-hentsch commented 2 months ago

Possible changes to disk_format

Initially, we proposed to introduce a new container_format to represent encrypted images. Glance was not a fan of this and requested to not introduce new formats and instead solely rely on metadata for this^1. Due to the recent OSSA-2024-001 and OSSA-2024-002 CVEs, the proper marking and scanning of image formats has gained new importance. This now leads to a rethinking and new requests for changes in our approach.

On 2024-08-28 in the weekly Glance IRC meeting^2, @josephineSei and I couldn't participate due to a conflicting meeting but the image encryption patchsets were discussed nonetheless and an interesting point was raised:

14:08:35 <pdeore> I have added few suggestions on parameter change patch but I request other cores to have a look at those patches
14:08:39 <dansmith> I feel like we need to revisit a couple things about how we store these images
14:08:49 <dansmith> in light of the giant CVE recently
14:09:07 <dansmith> in that I think we need to have a specific disk_format for luks-encrypted images,
14:09:31 <dansmith> so that we can inspect them with a known target format and reject things that are supposed to be encrypted but aren't (and v-v)
14:10:06 <dansmith> that goes with my proposal to also basically stop using "raw" to mean "image of a PC-like disk or partition"
14:10:17 <dansmith> (in my defender spec)
14:10:36 <dansmith> so I feel like we probably need to discuss that with glance, cinder, and nova people together
14:11:21 <dansmith> much of the complexity in the recent CVE came around the fact that we can never trust the disk_format in glance, and many of the side attack vectors came by putting one format in glance but calling it something else

Then we received the following comment on the Glance patchset from Dan Smith (Nova):

This is sort of an awkward place to raise this, but:

I think something we learned from the recent mega-CVE is that I think we need a new disk_format for luks-encrypted images. So much of the complexity of handling that CVE came from all the side vectors by which you can fool nova, glance, and cinder into doing something bad by saying an image is in one format, but actually sending another. Specifically, I think we have got to stop allowing raw to be both a catch-all format, as well as the thing we use when we really mean "an image of a PC-like disk".

Having to probe raw images to see if they smell like a LUKS disk, and if not, assume it's a regular raw is inviting more possibility for issue here I think.

I've got a spec proposed to make glance inspect and reject uploads that do not conform to the stated disk_format (i.e. you said it was raw, but uploaded a vmdk -> fail). It's here:

https://review.opendev.org/c/openstack/glance-specs/+/925111

and I have a LUKS inspector proposed against format_inspector:

https://review.opendev.org/c/openstack/oslo.utils/+/926809

those two things together will basically require that you declare a thing as luks before uploading it. One could also make the argument that we should use container_format=luks and disk_format=gpt for the typical arrangement, but I think that's more complicated. Either way, some discussion is required, IMHO.

As a result, we once again need to rethink and rework our encryption format approach in the Cinder and Glance patchsets to address this, it seems.

josephineSei commented 2 months ago

After reading through the Spec from Dan and looking into our own spec: https://review.opendev.org/c/openstack/glance-specs/+/915726/11/specs/2024.2/approved/glance/standardized_image_encryption.rst

I would like to propose an update to our specs for Cinder and Glance. We should definitely state it there, because people will find these specs also when searching for documentation.

Here is what we could do in Glance: https://review.opendev.org/c/openstack/glance-specs/+/927819/1/specs/2024.2/approved/glance/standardized_image_encryption.rst

markus-hentsch commented 2 months ago

In addition to your spec updates and Dan Smith's comment:

Either way, some discussion is required, IMHO.

I don't think we could or should go ahead and simply introduce some arbitrary disk_format in the patchsets now. We will need to reach an agreement with Glance and Cinder here on which exact changes would be okay for both sides, first.

I participated in today's Cinder IRC meeting, raised awareness of the topic again and asked people to attend next monday's popup meeting.

markus-hentsch commented 2 months ago

I attended today's Glance meeting: seems like we're back to the drawing board regarding the image metadata attributes associated with encrypted images and need to discuss this in the next PTG with the teams of Nova, Glance and Cinder:

14:07:28 <mhen> initially we agreed on using metadata only without touching container_format or disk_format for encrypted images, however dansmith raised a valid point about that being problematic for format detection/inspection
14:07:34 <mhen> considering the recent CVEs
14:07:48 <mhen> and suggested to introduce a new disk_format after all
14:08:23 <mhen> we have two cases to consider: 1) raw luks (like from cinder volumes) and 2) qcow2+luks
14:09:00 <dansmith> tbh, neither disk_format nor container_format really match in my head,
14:09:22 <dansmith> although qemu calls luks a disk format like qcow or vmdk, so it's probably a reasonable thing to mirror
14:09:52 <dansmith> but yeah, the recent CVEs (and the fallout since) has definitely affected my opinion on, well, a lot of things :/
14:10:38 <mhen> should that be two new disk formats then? "luks" and "qcow2+luks" for example or do we want to split the inner and outer format (raw/qcow, luks) into different attributes?
14:15:45 <pdeore> I think we should have the detailed discussion on this during PTG with glance, nova and cinder teams together
14:17:15 <mhen> if you have the time to attend, we can also start discussing this in the popup meeting on Monday: https://meetings.opendev.org/#Image_Encryption_Popup-Team_Meeting
14:17:34 <mhen> but yea, the PTG sounds like a good place to get everyone together for this
14:19:59 <pdeore> +1 to PTG, because not sure if people would be able to join popup meeting

Source: https://meetings.opendev.org/meetings/glance/2024/glance.2024-09-05-14.00.log.html

Update:

josephineSei commented 2 months ago

I think It would be good to have this conversation at the PTG in a cross-project session together with Cinder (and maybe Nova) to get everyone to agree on one way.

But even more important: I think we (I can try to do so) should reach out to Dan Smith to sketch out one or more possible ways of implementing the image encryption - all BEFORE the PTG happens. He is the one working on the image format checker, so he knows best, what information is needed to verify that there is no malicious image upload.

markus-hentsch commented 2 months ago

@josephineSei and I held today's image encryption popup team meeting on IRC^1. Although we did already agree on a PTG session about the topic as noted above, we wanted to discuss the topic in preparation to get the basics down. Below is a short summary of the main talking points:

About formats:

About vulnerabilities:

(past CVEs were based on qemu encountering a VMDK image with special instructions and executing them; but in this case, only the outer LUKS layer should actually be visible to qemu but that needs to be verified)

josephineSei commented 1 month ago

We took part in the PTG this week and we discussed with Cinder, Glance and some Nova people the impact of the CVEs on this topic.

Our discussion resulted in adjustments we need to do on the spec and the patches:

  1. We will introduce a new disk_format named LUKS for raw and gpt images. qcow2 images will keep their disk_format as the way encryption is handled differs between these images.
  2. The os_encryption_format now describes the specific version of the mechanism used: LUKSv1 and maybe eventually LUKSv2
  3. We also discussed about the image inspector, that may need some update todetect and handle the two ways of encrypted images.

For now I adjusted the spec accordingly: https://review.opendev.org/c/openstack/glance-specs/+/927819

markus-hentsch commented 4 weeks ago

We took part in the PTG this week and we discussed with Cinder, Glance and some Nova people the impact of the CVEs on this topic.

Just to complement this, here are my notes from the meeting about what we agreed on:

  1. Extend the existing disk_format=qcow2 with feature support for qcow2+luks encryption.
  2. Add new disk_format=luks for raw LUKS images. An OpenStack developer noted: this should be compatible with container_format=compressed.
  3. For both disk_format=qcow2 and disk_format=luks the image metadata property os_encrypt_key_id should be used to reference the key in Barbican.
  4. For both disk_format=qcow2 and disk_format=luks the image metadata property os_encrypt_format will specify the LUKS version used: luksv1 or luksv2. Support in the image inspector may be added to verify this via the qcow or LUKS header.
  5. When Cinder consumes a qcow2+luks image, it is converted to raw LUKS.
  6. Barbican keys representing LUKS passphrases (referenced by os_encrypt_key_id) are handled according to their secret type (if "passphrase" then skip hexlify else do hexlify), which is implemented by the already merged os-brick patch. We keep this approach.
markus-hentsch commented 4 weeks ago

I was just starting to work on dissecting the hex values of a LUKS header in order to add proper image inspector support in Glance when I happened to notice that the implementation was moved to oslo.utils in August ^1.

Furthermore, support for LUKSv1 header inspection for a luks disk_format was merged just a few days ago: https://github.com/openstack/oslo.utils/commit/7ab82d4a6002824e6ae6b10e4926ab59bf3a3c0b

It seems that this is a direct result of the PTG session and one less thing to consider in our patchsets. I need to rebase the current patchsets first to catch all the latest changes though.

markus-hentsch commented 4 weeks ago

I revised the patchsets for Glance and Cinder today. I added disk_format=luks support and the LUKSv1 / LUKSv2 handling for os_encrypt_format in Cinder.

@josephineSei I removed os_encrypt_cipher from the implementation for now. We currently have two formats:

In both cases, the os_encrypt_format can either be LUKSv1 or LUKSv2. The cipher details (e.g. AES-256, XTS mode) are details of the LUKS encryption and encoded directly in the LUKS header. LUKS can handle the header without prior knowledge of the cipher. As such, any os_encrypt_cipher metadata would be purely cosmetic but serve no purpose in processing the images as LUKS/qcow will read the header themselves.

As a result, I started wondering whether trying to properly identify the cipher used (either by inspecting the header in the OSC during user upload or trying to guess it from the Volume Type encryption metadata in case of Cinder's volume-to-image) is worth the risk of getting it wrong, considering it has no functional purpose and is just informational metadata at this point.

I think we could keep things a bit more simple by dropping os_encrypt_cipher and let the LUKS tools do their job based on the LUKS header (which is the actual source of truth in this case), keeping only os_encrypt_format as a coarse differentiation for the image inspector. @josephineSei what's your opinion on this?

Note: I still need to add os_decrypt_size support to Cinder.

josephineSei commented 3 weeks ago

I think you are correct, we do net loose information, when removing the os_encrypt_cipher. I adjust the spec accordingly.

We still have the following metadata:

The ones that MUST be set are:

The other ones are: The os_encrypt_key_deletion_policy defaults to FALSE, when I am reading the docs in the MR correct.

The os_decrypt_container_format may only be needed, when the container format is changed, e.g. for compressed. The os_decrypt_format is only required for the disk_format = LUKS.

I think the decrypt format is needed, but it is also new (due to the disk_format being LUKS), and not added yet to the patches, right? @markus-hentsch do you want to discuss this point? Maybe it is not needed, because we only allow raw/gpt images in LUKS blocks... But I am not sure about this.

josephineSei commented 2 weeks ago

I try to get more attention to all of this and added reviewers to the patches.

markus-hentsch commented 2 weeks ago

I updated the Glance implementation patchset and addressed review comments by adding releases notes and unit test coverage for secret consumer exceptions.

markus-hentsch commented 2 weeks ago

I started testing the changed architecture and ran into a failing openstack image create --disk-format luks because now that we moved to introducing a new disk_format for raw LUKS images after all, we need to adjust the OSC code.

For this purpose, I started a patchset for python-openstackclient, which I also labelled with the "LUKS-image-encryption" topic: https://review.opendev.org/c/openstack/python-openstackclient/+/934672

josephineSei commented 2 weeks ago

After our discussion About the 'os_decrypt_format' I removed the mention of this from the spec, and answered abhisheks comment on this. I also included, that we expect all luks-images to be 'raw' after encryption.

markus-hentsch commented 2 weeks ago

Cinder internally rewriting format detection from 'luks' to 'raw' for qemu-img conversions

During testing of the changed implementation, I discovered that the shift in direction to actually introduce disk_format=luks has more repercussions in Cinder than I originally thought. Cinder makes extensive use of qemu-img for format detection and conversion.

However, they use an override to treat images received from Glance that qemu-img detects as 'luks' as 'raw' instead because they expect those images to be originating from Cinder (former snapshots of LUKS volumes). The introduction of disk_format=luks now has side effects that will make Cinder erronously attempt conversion:

In all cases, we actually have the same format on both sides (luks) but Cinder treats its own LUKS data (e.g. volumes) as 'raw'. We don't want any encryption or conversion here but simply have the data copied over.

At first I tried removing the override and let Cinder actually detect its own data as 'luks'. However, this led to some very tricky pitfalls because as soon as 'luks' is on some side of a qemu-img convert command (which Cinder uses a lot to automatically process necessary conversion between volumes and images) it will attempt encryption/decryption and fail because no passphrases are provided to method calls. There would have been numerous places in the code where special context-dependent handling would have been necessary to avoid qemu-img thinking it needs to encrypt/decrypt something.

It turned out to be actually easier to keep treating Cinder's internal data as 'raw' and have isolated instances of function flags that allow overriding the qemu-img behavior to treat the data as raw and simply copy it over. I adjusted the Cinder patchset to implement that.

markus-hentsch commented 2 weeks ago

I adjusted unit tests and added release notes to the Cinder patchset. Regular pipelines look good but IBM and StorPool integration tests fail.

@josephineSei I added the os_decrypt_format/gpt topic along with a regular update and pipeline questions to https://etherpad.opendev.org/p/cinder-epoxy-meetings for discussion tomorrow.

markus-hentsch commented 1 week ago

Compatibility with image compression (container_format=compressed)

Since I stumbled upon a few spots referencing image compression while writing the patchset in Cinder, I had a closer look at the topic and tested it with the image encryption introduced by the patchset(s).

NOTE: The OpenStack client does not allow to specify --container-format compressed neither for image create --file nor image create --volume. The use case for the image compression can only be triggered when interacting with the API directly, e.g., via curl! As such, it will be a niche use case but still should be checked for compatibility with the patchset.

Compressed image from volume

With the current patchset, Cinder will prevent volume to image with compressed flag:

VOLUME_NAME=test-luks-img
VOLUME_ID=$(openstack volume show -c id -f value $VOLUME_NAME)
PROJECT_ID=$(openstack token issue -f value -c project_id)
URL="http://192.168.144.111/volume/v3/$PROJECT_ID/volumes/$VOLUME_ID/action"
TOKEN=$(openstack token issue -f value -c id)

curl -X POST $URL -H "X-Auth-Token: $TOKEN" -H "Content-Type: application/json" \
-d '{"os-volume_upload_image": {"force": false, "image_name": "compressed-luks-test", "container_format": "compressed", "disk_format": "luks"}}'

  {"badRequest": {"code": 400, "message": "An encrypted volume uploaded as an image
   must use 'luks' disk_format and 'bare' container_format."}}

Conclusion: As openstack image create --volume does not allow using the compressed container_format, the only way to trigger this is a direct API call. Furthermore, compressing an encrypted block device image will not yield any useful results as most encryptions will not achieve smaller file size when compressed after (!) encryption. As such, I think this is a non-issue and disallowing compression here is not problematic.

Volume from commpressed image

Since openstack image create --file cannot be used here, this is a bit more complicated to construct. We need to create an empty image metadata object and then upload the compressed data to it.

# compress the image
cat cirros.raw.luks | gzip > cirros.raw.luks.gzip
file cirros.raw.luks.gzip
  cirros.raw.luks.gzip: gzip compressed data, from Unix, original size modulo 2^32 119508992

# retrieve Barbican key ID of the encryption key
SECRET_ID=$(openstack secret list -f value --name luks-image-passphrase \
    | head -n1 | cut -d' ' -f1 | rev | cut -d'/' -f1 | rev)
TOKEN=$(openstack token issue -f value -c id)

# create the empty image vessel with metadata
curl -X POST http://192.168.144.111/image/v2/images \
-H "X-Auth-Token: $TOKEN" -H "Content-Type: application/json" \
-d '{"disk_format": "luks", "name": "test-compressed-luks", "container_format": "compressed", "os_encrypt_key_id": "'$SECRET_ID'", "os_encrypt_key_deletion_policy": "none", "os_encrypt_format": "LUKSv1"}'
IMAGE_ID=$(openstack image show test-compressed-luks -f value -c id)

# upload the binary gzip-compressed data to the image vessel
python3 -c "import requests; headers = {'Content-Type': 'application/octet-stream', 'X-Auth-Token': '$TOKEN'}; r = requests.put('http://192.168.144.111/image/v2/images/$IMAGE_ID/file', data=open('cirros.raw.luks.gzip', 'rb'), headers=headers); print(r.status_code, r.text)"

# verify the data
openstack image save --file downloaded-compressed-image.gzip $IMAGE_ID
md5sum *.gzip
  366e855538951f13cd1ff40f28b17d65  cirros.raw.luks.gzip
  366e855538951f13cd1ff40f28b17d65  downloaded-compressed-image.gzip

# create the volume
openstack volume create --image $IMAGE_ID --size 1 --type lvmdriver-1-LUKS test-luks-gzip-img

Note that for binary image data upload, both curl -X PUT --data-binary and curl -X PUT --upload-file ended up with a 500 internal server error ("OSError: timeout during read(8192) on wsgi.input"). That's why I had to resort to the Python command instead for uploading the compressed data.

Compression disabled in Cinder

If cinder.conf:allow_compression_on_image_upload was disabled (which is the default), the volume will enter error state and cinder-volume will print the following log message:

cinder.exception.ImageUnacceptable: Image 058fdc89-8701-4581-a0ea-5e6be99dc4f2
is unacceptable: Image compression disallowed, but container_format is compressed.

Compression enabled in Cinder

If cinder.conf contains

allow_compression_on_image_upload = True
compression_format = gzip

... then the created volume will eventually reach available state and can be used to create a succesfully booting VM in Nova:

openstack server create --flavor m1.tiny --network admin-private \
    --volume test-luks-gzip-img --security-group admin-access-group \
    vm-luksgzipimg-vol

openstack floating ip create --description "serverIPgzip" public
FIPGZIP=$(openstack floating ip list --long -f value -c Description -c "Floating IP Address" \
    | grep "serverIPgzip" | cut -d ' ' -f1)
openstack server add floating ip vm-luksgzipimg-vol $FIPGZIP
echo $FIPGZIP
ssh cirros@$FIPGZIP
  The authenticity of host '10.0.1.113 (10.0.1.113)' can't be established.
  ED25519 key fingerprint is SHA256:...
  This key is not known by any other names
  Are you sure you want to continue connecting (yes/no/[fingerprint])? yes
  Warning: Permanently added '10.0.1.113' (ED25519) to the list of known hosts.
  cirros@10.0.1.113's password:
  $ hostname
  vm-luksgzipimg-vol

Conclusion: The current patchset seems compatible with gzip-compressed images without issues.

markus-hentsch commented 1 week ago

I did some QA to the Cinder patchset. I updated the patchset for Cinder and loosened the restriction on image-from-volume a bit: if the volume is encrypted, disk_format=raw is accepted at the API (like before) for the "os-volume_upload_image" action and the disk_format is internally rewritten to 'luks' automatically. This makes the API a bit more backwards compatible, as in disk_format=raw for "os-volume_upload_image" action will lead to LUKS-encrypted images just like before. It just adopts the new 'luks' disk_format when actually storing the image in Glance now to be in line with the standardization.

I added a bunch of unit tests to cover the intricate parts of the patchset where we do special handling for the luks format internally. With that, test coverage of the new behaviors in Cinder should be quite good now.

josephineSei commented 5 days ago

I have looked into the zuul tests and found one failing unit test. I tried to fix it and also put the image encryption on the glance teams agenda to get more reviews and drive this forward.

josephineSei commented 4 days ago

We got reviews on the spec (with a +2 :)) and on the glance patch. We may need to split up the glance patch in smaller patches, but most of the comments were on smaller issues.

josephineSei commented 1 day ago

The patch set regarding the image encryption is in a good state: https://review.opendev.org/q/topic:%22LUKS-image-encryption%22 We discussed several ways with upstream and in the last PTG we only redefined some details. The implementation mostly needs only reviews from the openstack development teams and the patches may need some minor updates according to them.