)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"98c72beae5ac91ad42229580e026637d25c33e8c","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"805b1c11_0eddf80d","updated":"2023-07-04 15:35:29.000000000","message":"Like the previous patch, it looks good, but to my mind, it lacks a unit test with encryption.","commit_id":"1a7481b842b295d6fb89601343082834a782f8d5"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"42a9fa8c5690b93b250e05c8c6a6e38ca2f6f935","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"36e2a0db_8ab130d8","in_reply_to":"805b1c11_0eddf80d","updated":"2023-11-14 22:37:41.000000000","message":"Done","commit_id":"1a7481b842b295d6fb89601343082834a782f8d5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcbd862fe962e8cf825c231bb0b60170046727d8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"8261c31c_b3651fba","updated":"2024-01-25 12:25:34.000000000","message":"there are some slight style differnces in how i would handel the files but you are correctly closing them via the finally block so we are not leaking them hence \n+2 instead of +1.","commit_id":"cce14fcac92e4cd499a311f3863cb2acd4566f1e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6b360cfe7b655079ef27d342ef70eb4bbc5d99b3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"e929a6d8_42dc90ed","updated":"2024-01-31 08:52:29.000000000","message":"recheck timeout in nova-next","commit_id":"52f8043510b8042610f4c4b28ad74cce4b1f42fc"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"94cae7d9b640d28131db3b78688883a760ae4f88","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":28,"id":"ae7dc13a_e472bb45","updated":"2024-02-06 22:16:35.000000000","message":"recheck https://review.opendev.org/c/openstack/nova/+/908182 has merged","commit_id":"e763d995ebda5a7765cef76e5e9a36085a9429c3"},{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"989759ce4ab41d80c41e8b208f91c7ebaacedd0f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"c4854730_fd7e9d5a","updated":"2024-02-14 10:16:50.000000000","message":"Still ok","commit_id":"c150bb2e70e474cf8f278e0f7689423d8189300f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"0047062832e48efda0712b8c30cc799c7975159e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":32,"id":"ba46b688_5b7647dd","updated":"2024-02-29 17:42:26.000000000","message":"thanks for renaming the tests and updating the docsstrings/comments to reflect the intent. they are simpler to follow now.","commit_id":"9f7a6732f90e1b32af7d4a8bdfc709d2efbf432d"}],"nova/privsep/qemu.py":[{"author":{"_account_id":16207,"name":"ribaudr","display_name":"uggla","email":"rene.ribaud@gmail.com","username":"uggla","status":"Red Hat"},"change_message_id":"98c72beae5ac91ad42229580e026637d25c33e8c","unresolved":false,"context_lines":[{"line_number":45,"context_line":""},{"line_number":46,"context_line":"# NOTE(mikal): this method is deliberately not wrapped in a privsep entrypoint"},{"line_number":47,"context_line":"def unprivileged_convert_image("},{"line_number":48,"context_line":"    source: str,"},{"line_number":49,"context_line":"    dest: str,"},{"line_number":50,"context_line":"    in_format: ty.Optional[str],"},{"line_number":51,"context_line":"    out_format: str,"}],"source_content_type":"text/x-python","patch_set":9,"id":"f0b7bdc5_fb658b17","line":48,"range":{"start_line":48,"start_character":12,"end_line":48,"end_character":15},"updated":"2023-07-04 15:35:29.000000000","message":"I like you added types are.","commit_id":"1a7481b842b295d6fb89601343082834a782f8d5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcbd862fe962e8cf825c231bb0b60170046727d8","unresolved":true,"context_lines":[{"line_number":120,"context_line":"    try:"},{"line_number":121,"context_line":"        if encryption:"},{"line_number":122,"context_line":"            src_secret_file \u003d tempfile.NamedTemporaryFile("},{"line_number":123,"context_line":"                mode\u003d\u0027tr+\u0027, encoding\u003d\u0027utf-8\u0027)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"            # Write out the passphrase secret to a temp file"},{"line_number":126,"context_line":"            src_secret_file.write(encryption[\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":23,"id":"07dccf77_e4b6c1d1","line":123,"updated":"2024-01-25 12:25:34.000000000","message":"not really imporant but what is tr+\n\ntruncate and append?\nwe obvioulsy have write access so r is not readonly\n\ngenerally i prefer to use tempfile fixtures \n\nsomehting like \nhttps://github.com/openstack/nova/blob/master/nova/tests/fixtures/filesystem.py\n\nor you can use this form the fixtures lib\nhttps://github.com/testing-cabal/fixtures/blob/master/fixtures/_fixtures/tempdir.py#L56\n\nthen you dont need to worry about closing this later\n\ngiven this is production code using fixtures is not the correct approch however\ncontextlib provides another solution we could consider.\n\ncontextlib implmented the raii (resource aquisition is initalisation) pattern for us via a closing funtion.\n\nhttps://docs.python.org/3/library/contextlib.html#contextlib.closing\n\nyou dont need to refactor this to use that but that is a cleaner approch i think in general. it does the same thing but hides the internals.","commit_id":"cce14fcac92e4cd499a311f3863cb2acd4566f1e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f299804986f640eef34016c99e31236bcf3f6e6c","unresolved":true,"context_lines":[{"line_number":120,"context_line":"    try:"},{"line_number":121,"context_line":"        if encryption:"},{"line_number":122,"context_line":"            src_secret_file \u003d tempfile.NamedTemporaryFile("},{"line_number":123,"context_line":"                mode\u003d\u0027tr+\u0027, encoding\u003d\u0027utf-8\u0027)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"            # Write out the passphrase secret to a temp file"},{"line_number":126,"context_line":"            src_secret_file.write(encryption[\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":23,"id":"6feddd5c_f3e06129","line":123,"in_reply_to":"07dccf77_e4b6c1d1","updated":"2024-01-25 16:56:02.000000000","message":"\u003e not really imporant but what is tr+\n\u003e \n\u003e truncate and append?\n\u003e we obvioulsy have write access so r is not readonly\n\nLooks like r+ means \"open for reading and writing without truncation\" and the t is for text mode:\n\nhttps://docs.python.org/3/library/functions.html#open\n\nThe w (writing) and w+ (reading and writing) modes truncate and the a (writing) and a+ (reading and writing) append to the end of the file.\n\n\u003e generally i prefer to use tempfile fixtures \n\u003e \n\u003e somehting like \n\u003e https://github.com/openstack/nova/blob/master/nova/tests/fixtures/filesystem.py\n\u003e \n\u003e or you can use this form the fixtures lib\n\u003e https://github.com/testing-cabal/fixtures/blob/master/fixtures/_fixtures/tempdir.py#L56\n\u003e \n\u003e then you dont need to worry about closing this later\n\u003e \n\u003e given this is production code using fixtures is not the correct approch however\n\u003e contextlib provides another solution we could consider.\n\u003e \n\u003e contextlib implmented the raii (resource aquisition is initalisation) pattern for us via a closing funtion.\n\u003e \n\u003e https://docs.python.org/3/library/contextlib.html#contextlib.closing\n\u003e \n\u003e you dont need to refactor this to use that but that is a cleaner approch i think in general. it does the same thing but hides the internals.\n\nI did want to use a context manager but thought I couldn\u0027t because I need both files open at the same time (in the case of encrypted source and encrypted target) without nesting (neither depends on the other). But, looking again I think I see that I could use contextlib.ExitStack to achieve this:\n\nhttps://docs.python.org/3/library/contextlib.html#contextlib.ExitStack\n\nI\u0027ll go ahead and change to using ExitStack, I didn\u0027t like the finally: approach anyway.","commit_id":"cce14fcac92e4cd499a311f3863cb2acd4566f1e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6b360cfe7b655079ef27d342ef70eb4bbc5d99b3","unresolved":true,"context_lines":[{"line_number":120,"context_line":"    try:"},{"line_number":121,"context_line":"        if encryption:"},{"line_number":122,"context_line":"            src_secret_file \u003d tempfile.NamedTemporaryFile("},{"line_number":123,"context_line":"                mode\u003d\u0027tr+\u0027, encoding\u003d\u0027utf-8\u0027)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"            # Write out the passphrase secret to a temp file"},{"line_number":126,"context_line":"            src_secret_file.write(encryption[\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":23,"id":"796e3a3c_881993a1","line":123,"in_reply_to":"6feddd5c_f3e06129","updated":"2024-01-31 08:52:29.000000000","message":"context managers are a little weird.\n\nthe dont create an enclosing scope but the do trigger there effect when you dedent\n\nso ya without nesting them which gets messy when its condtional like this ist actully proably cleaner to use finally as you have.\n\nexitstack looks like a nice approch.\nlets proceed with the working code you ahve and then refactor to that later in the seriese.\n\nand thanks for the tr+ explanation.\nw+ proably would be fine here or even just w\nsince we dont actully read form it and it is a tempory blank file.\nt is the default mode for open but the default for temfile.NamedTemporyFile is w+b so tr+ is proably a premature optimization but\nits technially correct avoiding unnessiary truncation of a 0 lenght file\nand enabling text mode.\n\nhttps://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile","commit_id":"cce14fcac92e4cd499a311f3863cb2acd4566f1e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"46004b5f9aec327278e961b3887a8de492969ac4","unresolved":false,"context_lines":[{"line_number":120,"context_line":"    try:"},{"line_number":121,"context_line":"        if encryption:"},{"line_number":122,"context_line":"            src_secret_file \u003d tempfile.NamedTemporaryFile("},{"line_number":123,"context_line":"                mode\u003d\u0027tr+\u0027, encoding\u003d\u0027utf-8\u0027)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"            # Write out the passphrase secret to a temp file"},{"line_number":126,"context_line":"            src_secret_file.write(encryption[\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":23,"id":"0b052a71_489062d4","line":123,"in_reply_to":"796e3a3c_881993a1","updated":"2024-02-13 05:39:37.000000000","message":"Acknowledged","commit_id":"cce14fcac92e4cd499a311f3863cb2acd4566f1e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"46004b5f9aec327278e961b3887a8de492969ac4","unresolved":false,"context_lines":[{"line_number":118,"context_line":"    src_secret_file \u003d None"},{"line_number":119,"context_line":"    dest_secret_file \u003d None"},{"line_number":120,"context_line":"    encryption_opts: ty.List[str] \u003d []"},{"line_number":121,"context_line":"    with contextlib.ExitStack() as stack:"},{"line_number":122,"context_line":"        if encryption:"},{"line_number":123,"context_line":"            src_secret_file \u003d stack.enter_context("},{"line_number":124,"context_line":"                tempfile.NamedTemporaryFile(mode\u003d\u0027tr+\u0027, encoding\u003d\u0027utf-8\u0027))"}],"source_content_type":"text/x-python","patch_set":28,"id":"6c9e5183_46e0613d","line":121,"updated":"2024-02-13 05:39:37.000000000","message":"+1","commit_id":"e763d995ebda5a7765cef76e5e9a36085a9429c3"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf6ea9838c81ba1df5e0d5246e730ca6b7ddfd37","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    out_format: str,"},{"line_number":58,"context_line":"    instances_path: str,"},{"line_number":59,"context_line":"    compress: bool,"},{"line_number":60,"context_line":"    encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":61,"context_line":"    dest_encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":62,"context_line":") -\u003e None:"},{"line_number":63,"context_line":"    \"\"\"Disk image conversion with qemu-img"}],"source_content_type":"text/x-python","patch_set":31,"id":"396ab79d_36fb2def","line":60,"updated":"2024-02-26 19:03:52.000000000","message":"I think I would name this something else, maybe \"src_encyption\" or something. With just this name, I\u0027d assume this was \"how you want the result to be encrypted\" but that appears to not be the case. Minor thing of course.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a9556c984b7a66edc33f54404dea5972ed386479","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    out_format: str,"},{"line_number":58,"context_line":"    instances_path: str,"},{"line_number":59,"context_line":"    compress: bool,"},{"line_number":60,"context_line":"    encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":61,"context_line":"    dest_encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":62,"context_line":") -\u003e None:"},{"line_number":63,"context_line":"    \"\"\"Disk image conversion with qemu-img"}],"source_content_type":"text/x-python","patch_set":31,"id":"079ae16c_fdab8226","line":60,"in_reply_to":"228b0196_fdf004ed","updated":"2024-02-28 18:53:31.000000000","message":"Done","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"158b03330037ab12527eb498d732a12e0b359961","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    out_format: str,"},{"line_number":58,"context_line":"    instances_path: str,"},{"line_number":59,"context_line":"    compress: bool,"},{"line_number":60,"context_line":"    encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":61,"context_line":"    dest_encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":62,"context_line":") -\u003e None:"},{"line_number":63,"context_line":"    \"\"\"Disk image conversion with qemu-img"}],"source_content_type":"text/x-python","patch_set":31,"id":"a8308cbd_ad05d77f","line":60,"in_reply_to":"396ab79d_36fb2def","updated":"2024-02-26 19:28:16.000000000","message":"Yeah.. that had crossed my mind a few times but wasn\u0027t sure if I should go ahead and change it.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"83c6c0daede5870096cad4f6915ba4dcabd4749b","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    out_format: str,"},{"line_number":58,"context_line":"    instances_path: str,"},{"line_number":59,"context_line":"    compress: bool,"},{"line_number":60,"context_line":"    encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":61,"context_line":"    dest_encryption: ty.Optional[EncryptionOptions] \u003d None,"},{"line_number":62,"context_line":") -\u003e None:"},{"line_number":63,"context_line":"    \"\"\"Disk image conversion with qemu-img"}],"source_content_type":"text/x-python","patch_set":31,"id":"228b0196_fdf004ed","line":60,"in_reply_to":"a8308cbd_ad05d77f","updated":"2024-02-27 09:37:51.000000000","message":"fair the symmetry of src_encyption is nice","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cf6ea9838c81ba1df5e0d5246e730ca6b7ddfd37","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"626fc20e_26970402","line":75,"updated":"2024-02-26 19:03:52.000000000","message":"I\u0027m a bit lost here. It looks, from the test, like the format param (which I expect to be \"raw\" or \"qcow2\" can be \"luks\" now? I need brush up on how this works, because I would expect the file to still be a qcow2, but with luks as a layer on top of it. But, I *do* see that qemu-img takes \"luks\" as one of the format things. Could we add some detail here about what this looks like in various forms? So maybe if out_format is luks, then dest_encryption needs to say \"but really qcow2 inside yo\" ?","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"aa54b7a0b37a0e085e4d5dbcfe4c35d52fdb8945","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"fba5f2f0_578db948","line":75,"in_reply_to":"02a60e7e_451ef09b","updated":"2024-02-26 19:35:20.000000000","message":"Okay so in your luks-\u003eluks tests that\u0027s effectively changing the key on a raw encrypted disk, or stripping it, or adding it yeah? Okay makes sense. But yeah, a link there would be nice I think, if not a short bit of extra docstring with just the decoder ring.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"83c6c0daede5870096cad4f6915ba4dcabd4749b","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"9a738043_7f27ec51","line":75,"in_reply_to":"35c42709_456445fb","updated":"2024-02-27 09:37:51.000000000","message":"or add a note to the each test variant i.g raw-\u003eluks or luks-\u003eraw to explain what seneario they are covering.  raw-\u003eluks  (creating an encrypted backing file form a raw image)","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"158b03330037ab12527eb498d732a12e0b359961","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"02a60e7e_451ef09b","line":75,"in_reply_to":"626fc20e_26970402","updated":"2024-02-26 19:28:16.000000000","message":"Yeah, it is. This file format \"luks\" means encrypted raw in qemu:\n\nhttps://www.qemu.org/docs/master/system/qemu-block-drivers.html\n\nI\u0027m not sure why they did it that way instead of mirroring the approach with qcow2.\n\nSo we have \u0027raw\u0027 which is not encrypted, \u0027qcow2\u0027 can be either encrypted or not, \u0027luks\u0027 is encrypted only. It gets especially confusing with the fact that \u0027luks\u0027 is the file format for encrypted raw but the \u0027file driver\u0027 is \u0027raw\u0027.\n\nYeah it\u0027s probably worth linking this in the docstring with some example. I\u0027ll add that.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a9556c984b7a66edc33f54404dea5972ed386479","unresolved":false,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"dc07721e_f9175574","line":75,"in_reply_to":"9a738043_7f27ec51","updated":"2024-02-28 18:53:31.000000000","message":"Done","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4af955b3efee5ab85d1a894ec72e12070a03e722","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"35c42709_456445fb","line":75,"in_reply_to":"a0eb0f24_0dd579ad","updated":"2024-02-26 19:45:06.000000000","message":"Yeah, luks-\u003e luks would be only if you want to make a copy of the encrypted disk and give the copy a different passphrase. This would happen if you took a snapshot of an instance with an encrypted raw disk. We would call convert luks-\u003eluks and then upload that copy to glance.\n\nWhat we could do is remove it from the tests in this patch and then add it into the patch that enables the encryption feature for the \u0027raw\u0027 image backend, since that\u0027s when it would actually come up from a user perspective.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d7a7f2349be09d466bebdc0cbf59dc2d838913d6","unresolved":true,"context_lines":[{"line_number":72,"context_line":"                       for the source image such as the format and passphrase."},{"line_number":73,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":74,"context_line":"                            attributes for the destination image such as the"},{"line_number":75,"context_line":"                            format and passphrase."},{"line_number":76,"context_line":"    \"\"\""},{"line_number":77,"context_line":"    # NOTE(mdbooth, kchamart): `qemu-img convert` defaults to"},{"line_number":78,"context_line":"    # \u0027cache\u003dwriteback\u0027 for the source image, and \u0027cache\u003dunsafe\u0027 for the"}],"source_content_type":"text/x-python","patch_set":31,"id":"a0eb0f24_0dd579ad","line":75,"in_reply_to":"fba5f2f0_578db948","updated":"2024-02-26 19:38:22.000000000","message":"Er, wait.. luks-\u003eluks only makes sense in the \"change the key\" scenario based on what you said? You would do raw-\u003eluks or luks-\u003eraw for the adding/stripping? Maybe we should remove those from the tests then?","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a9556c984b7a66edc33f54404dea5972ed386479","unresolved":false,"context_lines":[{"line_number":69,"context_line":"    :param out_format: Desired disk image format of the converted disk image"},{"line_number":70,"context_line":"    :param instances_path: Location where instances are stored on disk"},{"line_number":71,"context_line":"    :param compress: Whether to compress the converted disk image"},{"line_number":72,"context_line":"    :param src_encryption: (Optional) Dict detailing various encryption"},{"line_number":73,"context_line":"        attributes for the source image such as the format and passphrase."},{"line_number":74,"context_line":"    :param dest_encryption: (Optional) Dict detailing various encryption"},{"line_number":75,"context_line":"        attributes for the destination image such as the format and passphrase."}],"source_content_type":"text/x-python","patch_set":32,"id":"ec4e80ae_0a4e19d3","line":72,"updated":"2024-02-28 18:53:31.000000000","message":"Thanks I think this is much better.","commit_id":"9f7a6732f90e1b32af7d4a8bdfc709d2efbf432d"}],"nova/tests/unit/privsep/test_qemu.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8cc89ad37dc3f3ef5cdc756da452ba23da3f43a7","unresolved":true,"context_lines":[{"line_number":64,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":65,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":66,"context_line":"    @ddt.unpack"},{"line_number":67,"context_line":"    def test_convert_image_encrypted_source("},{"line_number":68,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":69,"context_line":"        # Simulate an encrypted source image conversion to an unencrypted"},{"line_number":70,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"6b7a9b21_6bce7333","line":67,"updated":"2024-02-26 19:52:00.000000000","message":"What I\u0027m trying to get at is, this test says that the destination is not encrypted, so none of the above cases where out_format\u003dluks make sense?","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ebd8457eebc707f0f43caa3472a15b979e0439ad","unresolved":true,"context_lines":[{"line_number":64,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":65,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":66,"context_line":"    @ddt.unpack"},{"line_number":67,"context_line":"    def test_convert_image_encrypted_source("},{"line_number":68,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":69,"context_line":"        # Simulate an encrypted source image conversion to an unencrypted"},{"line_number":70,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"705c557f_aaebaf58","line":67,"in_reply_to":"6b7a9b21_6bce7333","updated":"2024-02-26 20:05:21.000000000","message":"Oh, duh, sorry. That\u0027s a mistake then. Those should be out_format\u003draw instead. I\u0027ll fix it.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"83c6c0daede5870096cad4f6915ba4dcabd4749b","unresolved":true,"context_lines":[{"line_number":64,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":65,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":66,"context_line":"    @ddt.unpack"},{"line_number":67,"context_line":"    def test_convert_image_encrypted_source("},{"line_number":68,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":69,"context_line":"        # Simulate an encrypted source image conversion to an unencrypted"},{"line_number":70,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"e4d25296_1d2360d5","line":67,"in_reply_to":"705c557f_aaebaf58","updated":"2024-02-27 09:37:51.000000000","message":"ya in this case it should be out_format\u003draw","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb8ccef45f94f117898386a9c9f836e1043f7345","unresolved":false,"context_lines":[{"line_number":64,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":65,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":66,"context_line":"    @ddt.unpack"},{"line_number":67,"context_line":"    def test_convert_image_encrypted_source("},{"line_number":68,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":69,"context_line":"        # Simulate an encrypted source image conversion to an unencrypted"},{"line_number":70,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"d516de69_4ff4aefc","line":67,"in_reply_to":"e4d25296_1d2360d5","updated":"2024-02-28 23:45:10.000000000","message":"Done","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8cc89ad37dc3f3ef5cdc756da452ba23da3f43a7","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"9d26268f_f2f847f0","line":97,"updated":"2024-02-26 19:52:00.000000000","message":"Same here, but where in_format\u003dluks","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a9556c984b7a66edc33f54404dea5972ed386479","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"919f7776_4d3bbedc","line":97,"in_reply_to":"186c6fe6_cedf7e2a","updated":"2024-02-28 18:53:31.000000000","message":"Okay so just to close the loop here, all my confusion was because I was expecting we were (in later patches where this gets added) keeping the CoW layering that provides the vast benefit we get from ceph. But, we\u0027re not, which I think is a problem. But, in that implementation by actually flattening every step of the way (which is what melwitt was trying to tell me above) we end up duplicating and re-encrypting each would-be-layer as a completely separate image. That\u0027s why it actually works, but brings other sacrifices (in CPU and disk usage).","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"83c6c0daede5870096cad4f6915ba4dcabd4749b","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"49e48f47_57b67f6c","line":97,"in_reply_to":"19321a63_a37df673","updated":"2024-02-27 09:37:51.000000000","message":"in_format\u003dluks would make sense later when we have support for encrypted glance iamges.\n\n(\u0027luks\u0027, \u0027luks\u0027) is raw encypted glance image to raw encypted backingfile\n(\u0027luks\u0027, \u0027qcow2\u0027) is raw encypted glance image to qcow encypted backingfile\n \nthat only comes later in the eseris so we coudl add it there but  this si a valid combination.\n\n---later---\n```\n# Simulate an unencrypted source image\n```\n\ni guess that the crkx of the issue\n\nwhen i tought about these orginal i looked at the title \ntest_convert_image_encrypted_dest and then looked at the permutations\n   \n     (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),\n     (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027)\n        \nand to me they are all vaid based on the title\n\nthe comment \n\n```\n# Simulate an unencrypted source image conversion to an encrypted\n# destination image.\n```\nchanges the scope of the test\nmaybe we should rename this to \n```\ntest_convert_unencypted_source_to_encrypted_dest\n```\nand the previous test to\n```\ntest_convert_encypted_source_to_unencrypted_dest\n```","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a38dda9e04b92797883273568dadfaaeeb2f91a1","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"61e7a1d1_478d61dc","line":97,"in_reply_to":"1957592b_841af74c","updated":"2024-02-28 15:16:19.000000000","message":"\u003e With hot snapshot the delta gets a new secret, the encrypted backing file keeps the same secret. When the snapshot is uploaded to Glance, it has that new secret in `hw_ephemeral_encryption_secret_uuid`.\n\nThis is about qcow, right? I need to go revisit the way this works, but I think what you\u0027re saying is that every time we do a snapshot, we rewrite the entire thing with a new key before we upload it to glance? That makes it basically cold all the time and I guess I\u0027m just not sure that\u0027s worth it.\n\n\u003e As for Ceph, yes, until we can depend on Reef, it eliminates the biggest reason to use Ceph. We could just block snapshots of encrypted images if Ceph until then, if we don\u0027t want to expose the cold-snapshot-only-at-this-time.\n\nYeah, that\u0027s probably a big caveat to put in the release notes (perhaps it\u0027s there).\n\n\u003e I don\u0027t think it\u0027s practical to use the same secret to create the clone that goes into Glance. It changes secret cleanup from being relatively simple to really complicated.\n\nWell, maybe it was too obscure, but what I meant was: upload the same secret to barbican, get a new uuid for it, and set that on the image. It\u0027s the same secret \"value\" but with a different \"key\" which means you can still clean up the instance\u0027s root disk key when the instance goes away, but without orphaning the image. We could also do this optionally, letting the admin (or maybe the user) decide if they want to lose the hot snapshot speed in order to have a re-keyed image, or re-use (really copy) the key to retain it.\n\n\u003e Also, we always flatten Ceph snapshots regardless:\n\u003e \n\u003e https://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/imagebackend.py#L1200\n\nHmm, I don\u0027t think this means what I mean when I say \"flatten\". Because that would (a) eliminate the hot snapshot behavior and (b) mean that the nova snaps don\u0027t reference the glance image in ceph (which they do, which is a longstanding wart for glance being unable to ever delete base images that have a reference to them). But yeah, I need to dig in a bit to make sure I\u0027m understanding it correctly.\n\n\u003e No, for the destination image of the snapshot \"disk_delta\", we use a new secret:\n\u003e \n\u003e https://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/driver.py#L3392\n\nAh, this is yet a third thing I hadn\u0027t even considered. This is snapshotting while the instance is running, which AFAIK only works for qcow because qemu handles the equivalent of a fork internally for us so the instance can keep running. That\u0027s different from what I meant by \"hot snap\", being a cow snapshot instead of a full copy. Clearly \"hot\" is not the right term to use here.\n\n\u003e Yeah. Usually with encrypted stuff I would think sharing would be not something you want to do, but I learned Barbican defaults to project scoped ownership of secrets, so everyone in the same project can access them. I\u0027m guessing they did that to simplify these kinds of use cases. Operators can implement more fine grained access using access control lists though, if they want:\n\nEr, no I think the workflow is the same. \"Usually\" with images they\u0027re private (to your tenant) for security, regardless of whether they\u0027re encrypted or not. The share/publicize/transfer workflow(s) are specifically *for* handing private image data to someone outside that circle. It\u0027s not a conflict to say \"I want my data encrypted at rest\" and \"I want to be able to transfer ownership of a thing to someone else\". I think saying they\u0027re in conflict would be like saying you only want to have encrypted http communication with people at your employer, but expect cleartext http for communication with everyone else :)","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"4153bc8e49b741c4bf6ab450cf95d8f0c90932ea","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"e5c9a4d2_f178d249","line":97,"in_reply_to":"2fa4674b_24db1333","updated":"2024-02-28 22:50:34.000000000","message":"\u003e But looking over the ACL API in Barbican, it looks like you can add access to secrets to a list a users independent of your own project:\n\u003e \n\u003e https://docs.openstack.org/api-guide/key-manager/acls.html\n\nYeah, so their ACL is more like the membership API in glance, where you can say \"so and so can access this image too\". I think it continues to be shared (and not transferred away from you) such that adding them to the ACL whilst they can access it would make sense (and make it be pretty seamless). So I think that\u0027s probably a good move.\n\nThere are other ways to share images that won\u0027t apply so much. And we should probably also consult some of the security-minded people about just making glance give away access to your secrets, but I think it\u0027s probably a good move.\n\nAnyway, point being, without some or all of these holes patched, the \"workflow\" is broken. That was my original point :)","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b095c1390bc9ff39892d67a9ceaa0e898951ac03","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"eb888148_b97d403c","line":97,"in_reply_to":"49e48f47_57b67f6c","updated":"2024-02-27 14:39:24.000000000","message":"\u003e in_format\u003dluks would make sense later when we have support for encrypted glance iamges.\n\u003e \n\u003e (\u0027luks\u0027, \u0027luks\u0027) is raw encypted glance image to raw encypted backingfile\n\nI don\u0027t think so, IIUC. This would mean we\u0027re rewriting the entire image to change the key from the one the glance image used to ours, right? I thought that _wasn\u0027t_ required because we can have separate encryption at each layer? If not, we\u0027ll be destroying any kind of cow reuse we currently have *and* be doing a tone of IO and CPU on every boot.\n\n\u003e (\u0027luks\u0027, \u0027qcow2\u0027) is raw encypted glance image to qcow encypted backingfile\n\u003e  \n\u003e that only comes later in the eseris so we coudl add it there but  this si a valid combination.\n\nSame as above.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"03e8c6f491fe31ad540a655459b4dc60b6f86a25","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"83182848_03c0fcb0","line":97,"in_reply_to":"4dc77919_05b17222","updated":"2024-02-27 23:08:41.000000000","message":"\u003e OK, sorry. I said the wrong thing.\n\u003e \n\u003e The convert would only be done in the case of a cold snapshot, i.e.: \n\u003e \n\u003e https://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/imagebackend.py#L989\n\u003e \n\u003e But for Ceph, for now, we can only do a cold snapshot because we can\u0027t give the clone a different passphrase than the parent. They added the ability to use a different passphrase in v18 Reef, based on a Ceph commit I found. Here it\u0027s like, do we offer cold snapshot only for Ceph temporarily until we can require v18 Reef or do we disallow encrypted snapshots for Ceph altogether until then? I took a guess at letting people do cold snapshots if they were OK with that, otherwise don\u0027t use snapshot with encryption until a later release of Nova.\n\nOkay, I\u0027m confused. Right now we\u0027re just always using luks on top of qcow, raw, or ceph, so the ceph ability doesn\u0027t factor into right? We either allow hot snapshot with the key staying the same (which is what I suspect most people would want, in a backup type scenario), or we have to flatten every single snapshot to rewrite it with a different key, effectively eliminating the biggest reason (IMHO) to run ceph, right?\n\n\u003e For qcow2 we can do live snapshots and not rewrite and re-encrypt the disk.\n\nBy \"not rewrite and re-encrypt\" you mean \"the snapshot keeps the same key as what the instance is using\" right? So to avoid the \"when do we cleanup?\" issue you just re-upload the same key as a different credential to apply to the image?\n\nI guess I\u0027m not sure how it\u0027s different for ceph.\n\n\u003e As for the workflow ... TBH I\u0027m not sure. There\u0027s something in Glance for trusted/signed images, I think, and there\u0027s this related to Cinder volume encryption:\n\nUnrelated to any such support in glance, I think it breaks the usual share workflow. Meaning I snapshot my instance to an image, and I use glance\u0027s share/transfer/whatever functions to give it to someone else. If they can\u0027t read the secret in barbican, they can\u0027t boot it.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0e703dc0baa1feddb8bbb03584184b4d3f96a44d","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"186c6fe6_cedf7e2a","line":97,"in_reply_to":"61e7a1d1_478d61dc","updated":"2024-02-28 15:44:22.000000000","message":"\u003e \u003e Also, we always flatten Ceph snapshots regardless:\n\u003e \u003e \n\u003e \u003e https://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/imagebackend.py#L1200\n\u003e \n\u003e Hmm, I don\u0027t think this means what I mean when I say \"flatten\". Because that would (a) eliminate the hot snapshot behavior and (b) mean that the nova snaps don\u0027t reference the glance image in ceph (which they do, which is a longstanding wart for glance being unable to ever delete base images that have a reference to them). But yeah, I need to dig in a bit to make sure I\u0027m understanding it correctly.\n\nAh, the _snapshot_ gets flattened, but we retain the _root disk_\u0027s reference to the parent image I guess. We also seem to create *another* snap in that process. So the reference I\u0027m talking about is either just the root disk, or the reference to the parent image gets patched back up after, I\u0027m not sure. I totally didn\u0027t think that we were doing that on ceph snapshot (because it kinda defeats the storage efficiency point) but that also means I totally don\u0027t know how this could work so I must be missing something else.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e9120885d29c7b48dfbeecd541f2072b836d4cb9","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"b4d12ef6_a234eb18","line":97,"in_reply_to":"63672bf0_0ea95eb3","updated":"2024-02-27 22:23:18.000000000","message":"\u003e A place where we will however convert from encrypted to encrypted (with a new key) is when taking a snapshot of an instance which has a disk with an encrypted backing file, for example.\n\nHmm, I guess the goal there is to break the relation chain, but I\u0027m not sure that\u0027s really the best plan because of how expensive it is. It also means we have to *not* support hot snapshot with ceph specifically right?\n\nWhy can\u0027t we just upload the secret as another entity, tie that uuid to the snapshot, but have them be the same? Then we don\u0027t have to rewrite (and re-encrypt) the entire disk in all snapshot cases.\n\nSpeaking of that workflow - I assume that encrypted images break a bunch of glance\u0027s workflow where you share an image with someone, publicize, etc. Glance would have to do something to make the secret specified by the image accessible to whoever you\u0027re sharing it with...","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"edaed640516a7716113e6769fea537ae424cc835","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"828590d3_d17ed0e2","line":97,"in_reply_to":"715b7a47_4073d501","updated":"2024-02-28 21:39:37.000000000","message":"\u003e OK, but if you want to transfer ownership of the thing to someone else outside your private project and that thing is encrypted using your password, you wouldn\u0027t want the receiver of the thing to have the same password you\u0027re still using, would you? That is what I was thinking about.\n\nNo, totally true in that case. We\u0027re mixing a few of things I think given that there are a couple of conversations going in parallel. For the sharing case, you definitely want the re-key behavior. What I mean by breaking the workflow is that it\u0027s now much harder to complete the share. So let me lay out the three situations that I think exist:\n\n1. I have an instance that I want to snapshot (quickly) once per hour so I can roll back if something breaks. In this case, I want no re-key because it takes longer, uses CPU, uses potentially more disk, and maybe makes recovery harder because it\u0027s encrypted with a different key than my system is running with.\n\n2. I have an instance and I want to snapshot it to publicize for lots of other people. I definitely want the re-key, and I\u0027m going to have to make that key available some way to lots of people and/or I\u0027ll end up un-encrypting it for wide consumption.\n\n3. I have an instance and I want to snapshot it to give it securely to another project in my org. We\u0027re on different (keystone) projects, but glance provides a way for me to transfer ownership of that thing. I want it re-keyed because I trust them (but not that much) and because it\u0027s going to become a different thing (as opposed to my backups of the original instance). So, I can still snapshot, let nova re-key it, and upload to glance. I can do the glance handoff to the other owner and they have \"access\" to the image. However, they can\u0027t boot it because it has a hw_secret_uuid that isn\u0027t reachable by them. The image was transferred/shared, but not the key in barbican. I can download the key from barbican and sneakernet it to them, they can upload it to barbican, change the secret uuid in the image and they should be good to go. However, the workflow of being able to hand off images is kinda broken now and requires post-it notes as a storage medium 😊\n\n#3 is what I\u0027m talking about in terms of breaking the workflow. It\u0027s broken in either case of re-keying or not-re-keying by default. I\u0027m not saying we need to solve that right now, I\u0027m just noting that we\u0027re sort of breaking some integration that exists today.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f7711ccf875df6fd67eabf6c70c6d4cb68c483f0","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"2fa4674b_24db1333","line":97,"in_reply_to":"828590d3_d17ed0e2","updated":"2024-02-28 22:22:44.000000000","message":"Thanks, that makes it clear about the workflow problem.\n\nUpon googling, it looked like we would need a new feature in Barbican to support transfer of a key. The docs for encrypted Cinder volumes for OSP mention the caveat:\n\n\u003e You cannot transfer ownership of encrypted volumes, because it is not currently possible to transfer ownership of the barbican secret.\n\nhttps://access.redhat.com/documentation/en-us/red_hat_openstack_platform/17.1/html/managing_secrets_with_the_key_manager_service/assembly-encrypting-validating-openstack-services_rhosp#proc-migrating-volumes-to-key-manager_key-manager-services\n\nBut looking over the ACL API in Barbican, it looks like you can add access to secrets to a list a users independent of your own project:\n\nhttps://docs.openstack.org/api-guide/key-manager/acls.html\n\nIt\u0027s not as coarse-grained as project scoped but I guess the user who wants to transfer ownership could grant access to at least one user in the target project and then that user in the target project could read the passphrase and create a new secret owned by their project containing the passphrase. They would also need to update `hw_ephemeral_encryption_secret_uuid` in the image properties if they wanted to use it to boot new instances to their project. But I think maybe that would work?","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dd031cd26117bc7da5a201e632ae3ed9b562bba2","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"1957592b_841af74c","line":97,"in_reply_to":"83182848_03c0fcb0","updated":"2024-02-27 23:45:52.000000000","message":"\u003e Okay, I\u0027m confused. Right now we\u0027re just always using luks on top of qcow, raw, or ceph, so the ceph ability doesn\u0027t factor into right? We either allow hot snapshot with the key staying the same (which is what I suspect most people would want, in a backup type scenario), or we have to flatten every single snapshot to rewrite it with a different key, effectively eliminating the biggest reason (IMHO) to run ceph, right?\n\nWith hot snapshot the delta gets a new secret, the encrypted backing file keeps the same secret. When the snapshot is uploaded to Glance, it has that new secret in `hw_ephemeral_encryption_secret_uuid`.\n\nAs for Ceph, yes, until we can depend on Reef, it eliminates the biggest reason to use Ceph. We could just block snapshots of encrypted images if Ceph until then, if we don\u0027t want to expose the cold-snapshot-only-at-this-time.\n\nI don\u0027t think it\u0027s practical to use the same secret to create the clone that goes into Glance. It changes secret cleanup from being relatively simple to really complicated. Also, we always flatten Ceph snapshots regardless:\n\nhttps://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/imagebackend.py#L1200\n\n\u003e \u003e For qcow2 we can do live snapshots and not rewrite and re-encrypt the disk.\n\u003e \n\u003e By \"not rewrite and re-encrypt\" you mean \"the snapshot keeps the same key as what the instance is using\" right? So to avoid the \"when do we cleanup?\" issue you just re-upload the same key as a different credential to apply to the image?\n\u003e \n\u003e I guess I\u0027m not sure how it\u0027s different for ceph.\n\nNo, for the destination image of the snapshot \"disk_delta\", we use a new secret:\n\nhttps://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/driver.py#L3392\n\nFor Ceph, we can\u0027t do that using their clone() function in Quincy. It should be possible in Reef.\n\n\u003e \u003e As for the workflow ... TBH I\u0027m not sure. There\u0027s something in Glance for trusted/signed images, I think, and there\u0027s this related to Cinder volume encryption:\n\u003e \n\u003e Unrelated to any such support in glance, I think it breaks the usual share workflow. Meaning I snapshot my instance to an image, and I use glance\u0027s share/transfer/whatever functions to give it to someone else. If they can\u0027t read the secret in barbican, they can\u0027t boot it.\n\nYeah. Usually with encrypted stuff I would think sharing would be not something you want to do, but I learned Barbican defaults to project scoped ownership of secrets, so everyone in the same project can access them. I\u0027m guessing they did that to simplify these kinds of use cases. Operators can implement more fine grained access using access control lists though, if they want:\n\nhttps://docs.openstack.org/barbican/latest/admin/access_control.html","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0c476d55070828137a9ad057c66ad5f57e9d47e2","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"715b7a47_4073d501","line":97,"in_reply_to":"919f7776_4d3bbedc","updated":"2024-02-28 21:04:32.000000000","message":"\u003e Er, no I think the workflow is the same. \"Usually\" with images they\u0027re private (to your tenant) for security, regardless of whether they\u0027re encrypted or not. The share/publicize/transfer workflow(s) are specifically for handing private image data to someone outside that circle. It\u0027s not a conflict to say \"I want my data encrypted at rest\" and \"I want to be able to transfer ownership of a thing to someone else\". I think saying they\u0027re in conflict would be like saying you only want to have encrypted http communication with people at your employer, but expect cleartext http for communication with everyone else :)\n\nOK, but if you want to transfer ownership of the thing to someone else outside your private project and that thing is encrypted using your password, you wouldn\u0027t want the receiver of the thing to have the same password you\u0027re still using, would you? That is what I was thinking about.\n\n\u003e Ah, the snapshot gets flattened, but we retain the root disk\u0027s reference to the parent image I guess. We also seem to create another snap in that process. So the reference I\u0027m talking about is either just the root disk, or the reference to the parent image gets patched back up after, I\u0027m not sure. I totally didn\u0027t think that we were doing that on ceph snapshot (because it kinda defeats the storage efficiency point) but that also means I totally don\u0027t know how this could work so I must be missing something else.\n\nThat\u0027s what I interpreted from it as well, that the root disk keeps its reference to the parent but the \"snapshot\" is a flattened standalone thing.\n\n\u003e Okay so just to close the loop here, all my confusion was because I was expecting we were (in later patches where this gets added) keeping the CoW layering that provides the vast benefit we get from ceph. But, we\u0027re not, which I think is a problem. But, in that implementation by actually flattening every step of the way (which is what melwitt was trying to tell me above) we end up duplicating and re-encrypting each would-be-layer as a completely separate image. That\u0027s why it actually works, but brings other sacrifices (in CPU and disk usage).\n\nAck.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ebd8457eebc707f0f43caa3472a15b979e0439ad","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"19321a63_a37df673","line":97,"in_reply_to":"9d26268f_f2f847f0","updated":"2024-02-26 20:05:21.000000000","message":"Right, will fix.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d83b22cbad3b424c3d3dfeb6fecaa0e7a70b4fa2","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"4dc77919_05b17222","line":97,"in_reply_to":"b4d12ef6_a234eb18","updated":"2024-02-27 22:59:48.000000000","message":"OK, sorry. I said the wrong thing.\n\nThe convert would only be done in the case of a cold snapshot, i.e.: \n\nhttps://github.com/openstack/nova/blob/326a41b3b3e3d8ff8b2518f252615dd48930ec46/nova/virt/libvirt/imagebackend.py#L989\n\nBut for Ceph, for now, we can only do a cold snapshot because we can\u0027t give the clone a different passphrase than the parent. They added the ability to use a different passphrase in v18 Reef, based on a Ceph commit I found. Here it\u0027s like, do we offer cold snapshot only for Ceph temporarily until we can require v18 Reef or do we disallow encrypted snapshots for Ceph altogether until then? I took a guess at letting people do cold snapshots if they were OK with that, otherwise don\u0027t use snapshot with encryption until a later release of Nova.\n\nFor qcow2 we can do live snapshots and not rewrite and re-encrypt the disk.\n\nAs for the workflow ... TBH I\u0027m not sure. There\u0027s something in Glance for trusted/signed images, I think, and there\u0027s this related to Cinder volume encryption:\n\nhttps://specs.openstack.org/openstack/glance-specs/specs/train/approved/glance/barbican-secret-deletion-support.html\n\nI think the volume-to-image would face a similar challenge with regard to sharing images and I\u0027m not sure how/if they\u0027re handling it.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"cb8ccef45f94f117898386a9c9f836e1043f7345","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"d6e84587_32345797","line":97,"in_reply_to":"e5c9a4d2_f178d249","updated":"2024-02-28 23:45:10.000000000","message":"\u003e Yeah, so their ACL is more like the membership API in glance, where you can say \"so and so can access this image too\". I think it continues to be shared (and not transferred away from you) such that adding them to the ACL whilst they can access it would make sense (and make it be pretty seamless). So I think that\u0027s probably a good move.\n\nThat\u0027s true, their ACL doesn\u0027t transfer ownership away.\n\n\u003e There are other ways to share images that won\u0027t apply so much. And we should probably also consult some of the security-minded people about just making glance give away access to your secrets, but I think it\u0027s probably a good move.\n\u003e \n\u003e Anyway, point being, without some or all of these holes patched, the \"workflow\" is broken. That was my original point :)\n\nAck.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d37f1792834d5d49438a5b34f6f5f7160dd2e25a","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":95,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":96,"context_line":"    @ddt.unpack"},{"line_number":97,"context_line":"    def test_convert_image_encrypted_dest("},{"line_number":98,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":99,"context_line":"        # Simulate an unencrypted source image conversion to an encrypted"},{"line_number":100,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"63672bf0_0ea95eb3","line":97,"in_reply_to":"eb888148_b97d403c","updated":"2024-02-27 21:57:09.000000000","message":"Right, in the case of an encrypted Glance image we will not change the key for the encrypted backing file, so as to preserve CoW-ness. Instances will be able to share the encrypted backing file and each instance\u0027s overlay/delta will get a new secret created same as for regular instance boot.\n\nA place where we will however convert from encrypted to encrypted (with a new key) is when taking a snapshot of an instance which has a disk with an encrypted backing file, for example.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8cc89ad37dc3f3ef5cdc756da452ba23da3f43a7","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":140,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":141,"context_line":"    @ddt.unpack"},{"line_number":142,"context_line":"    def test_convert_image_encrypted_source_and_dest("},{"line_number":143,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":144,"context_line":"        # Simulate an encrypted source image conversion to an encrypted"},{"line_number":145,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"f47c6d3d_31855564","line":142,"updated":"2024-02-26 19:52:00.000000000","message":"Here it makes sense because luks-\u003eluks is encrypted on both sides. I think it\u0027s fine to leave here, no need to split it out.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a9556c984b7a66edc33f54404dea5972ed386479","unresolved":false,"context_lines":[{"line_number":139,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":140,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":141,"context_line":"    @ddt.unpack"},{"line_number":142,"context_line":"    def test_convert_image_encrypted_source_and_dest("},{"line_number":143,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":144,"context_line":"        # Simulate an encrypted source image conversion to an encrypted"},{"line_number":145,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"79036ec2_218c4ba0","line":142,"in_reply_to":"c6c86b48_ae42369f","updated":"2024-02-28 18:53:31.000000000","message":"Acknowledged","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"ebd8457eebc707f0f43caa3472a15b979e0439ad","unresolved":true,"context_lines":[{"line_number":139,"context_line":"        (\u0027qcow2\u0027, \u0027qcow2\u0027), (\u0027qcow2\u0027, \u0027luks\u0027),"},{"line_number":140,"context_line":"        (\u0027luks\u0027, \u0027luks\u0027), (\u0027luks\u0027, \u0027qcow2\u0027))"},{"line_number":141,"context_line":"    @ddt.unpack"},{"line_number":142,"context_line":"    def test_convert_image_encrypted_source_and_dest("},{"line_number":143,"context_line":"            self, in_format, out_format, mock_tempfile, mock_execute):"},{"line_number":144,"context_line":"        # Simulate an encrypted source image conversion to an encrypted"},{"line_number":145,"context_line":"        # destination image."}],"source_content_type":"text/x-python","patch_set":31,"id":"c6c86b48_ae42369f","line":142,"in_reply_to":"f47c6d3d_31855564","updated":"2024-02-26 20:05:21.000000000","message":"Right, this is the only place it should be.","commit_id":"494d399a4d539bcaf23f221b992bf92b9c4fc4a6"}]}