From af72f4a4251670cc7f6f7ee3c38132f5c740f8d5 Mon Sep 17 00:00:00 2001
From: Gerrit User 27665 <27665@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Date: Thu, 11 Apr 2024 17:32:30 +0000
Subject: [PATCH] Update patch set 1

Patch Set 1: Code-Review-1

(4 comments)

Patch-set: 1
Reviewer: Gerrit User 27665 <27665@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1, 7e389334c3e6db8e3fe9ae78f57f9861314659f5
---
 acbd671fcd3471637898c6b9f4a256cc2c9727b5 | 68 ++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/acbd671fcd3471637898c6b9f4a256cc2c9727b5 b/acbd671fcd3471637898c6b9f4a256cc2c9727b5
index e105aff6..a7340f9b 100644
--- a/acbd671fcd3471637898c6b9f4a256cc2c9727b5
+++ b/acbd671fcd3471637898c6b9f4a256cc2c9727b5
@@ -17,6 +17,40 @@
       "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
       "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
     },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "c30d4ea6_4ed60c30",
+        "filename": "/PATCHSET_LEVEL",
+        "patchSetId": 1
+      },
+      "lineNbr": 0,
+      "author": {
+        "id": 27665
+      },
+      "writtenOn": "2024-04-11T17:32:30Z",
+      "side": 1,
+      "message": "Good starting point! I added a few remarks on the spec that I think we still need to address.",
+      "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "fa847f46_03ee4aba",
+        "filename": "specs/2024.2/byok-for-cinder.rst",
+        "patchSetId": 1
+      },
+      "lineNbr": 35,
+      "author": {
+        "id": 27665
+      },
+      "writtenOn": "2024-04-11T17:32:30Z",
+      "side": 1,
+      "message": "Things that I think need to be addressed as well:\n\nWhen receiving the API request with BYOK, cinder-api should verify the Barbican secret:\n1. Does the secret referenced by ID exist and is it retrievable using the requesting user\u0027s auth token? Usually secrets are project-bound and cannot be retrieved by users of another project.\n2. Does the secret\u0027s metadata specify a secret type that can be processed by Cinder with the specified volume type? Currently it expects secret_type to be \"symmetric\". Also the other secret attributes (cipher, mode, bit length) may need to be put in relation to the target volume type\u0027s encryption specification.\n\nIn my opinion we should avoid this failing later in cinder-volume as the feedback loop to the user is lengthy and less obvious (volume will enter error state etc.). We should fail early in cinder-api wherever possible.",
+      "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
     {
       "unresolved": true,
       "key": {
@@ -51,6 +85,40 @@
       "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
       "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
     },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "159ede71_bfa4da2b",
+        "filename": "specs/2024.2/byok-for-cinder.rst",
+        "patchSetId": 1
+      },
+      "lineNbr": 55,
+      "author": {
+        "id": 27665
+      },
+      "writtenOn": "2024-04-11T17:32:30Z",
+      "side": 1,
+      "message": "We should also take into account that the Key Manager API (Barbican) can either be unreachable, failing or does not even exist in the infrastructure at the time of the API request and return an appropriate error response code and message in such case.",
+      "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
+    {
+      "unresolved": true,
+      "key": {
+        "uuid": "d1eb44fc_09423c37",
+        "filename": "specs/2024.2/byok-for-cinder.rst",
+        "patchSetId": 1
+      },
+      "lineNbr": 60,
+      "author": {
+        "id": 27665
+      },
+      "writtenOn": "2024-04-11T17:32:30Z",
+      "side": 1,
+      "message": "I don\u0027t agree that there is no impact at all. Currently, Cinder instructs Barbican to create a key for encrypted volumes (via secret order API) with a specified bit length. Barbican might use an HSM or something similar as a backend. So there are some assumptions/guarantees around entropy and strength of the key generated there.\n\nIf users are able to specify *any* passphrase with BYOK, they are also free to use very weak ones when creating volumes. Images, snapshots or clones created from such volumes will then inherit the LUKS encryption and key, thus the weak passphrase would be passed on further.\n\nI think we could consider educating users (documentation) and/or checking the lengths of the BYOK secrets at some point.",
+      "revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
     {
       "unresolved": true,
       "key": {