openstack
/
cinder-specs
Code Issues Proposed changes
cinder-specs/acbd671fcd3471637898c6b9f4a...

261 lines
11 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": false,
"key": {
"uuid": "a32e8959_0d137ac8",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 27615
},
"writtenOn": "2024-04-11T14:11:50Z",
"side": 1,
"message": "some comments inline from a quick look",
"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": false,
"key": {
"uuid": "0cf08187_29fdd590",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 5314
},
"writtenOn": "2024-04-12T12:35:51Z",
"side": 1,
"message": "My primary concern is that the way cinder handles keys and secrets may not address some of the use cases you describe, so it would be good to have more clarity on that. See comments inline.",
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "710a0bf6_c929c836",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 18,
"author": {
"id": 5314
},
"writtenOn": "2024-04-12T12:35:51Z",
"side": 1,
"message": "It would be helpful if you could point to some legal requirements or security specifications that require this so that we can understand exactly what is needed, because this may entail a complete redesign of the way cinder does key handling. (See my comment below in the \"Proposed change\" section.)",
"range": {
"startLine": 18,
"startChar": 247,
"endLine": 18,
"endChar": 331
},
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "a4f3aa7e_b4fda64c",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 31,
"author": {
"id": 5314
},
"writtenOn": "2024-04-12T12:35:51Z",
"side": 1,
"message": "Actually, we have a Big Problem here, namely, that cinder assumes that there is a 1-1 correspondence between a Secret in the Key Manager and a cinder resource (e.g., a volume, but also a volume uploaded to Glance as an image). The reason for this is so that cinder (or glance) can delete the Secret when the resource is deleted (otherwise, the Key Manager gets filled up with Secrets that aren\u0027t being used for anything).\n\nSo we need to distinguish between a Secret and the Key (that is, a Secret is a thing in Barbican that has a uuid, the *value* of the Secret, which is the Key). When you Bring Your Own Key to create a volume, we can use the Key value that is in Barbican, but we will create a *different* Secret that is specific to this volume (but which has the value you specified). So it will have the same Key, just identified by a different Secret uuid.\n\nWhat this means for your proposal is that an end user is not going to be able to tell from the encryption_key_id that\u0027s returned by the mv 3.64+ volume-show call what Key is being used. So you need to review the Use Cases you identified above and see which ones are not possible given the current way cinder does key handling, and whether it\u0027s acceptable to address fewer use cases, or whether we need to re-design the way cinder does key handling.\n\nI am especially interested in whether any legal requirements or security specifications that require Bring Your Own Key would find the current scheme acceptable.",
"range": {
"startLine": 31,
"startChar": 0,
"endLine": 31,
"endChar": 57
},
"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": {
"uuid": "3f91112a_9ddc1328",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 49,
"author": {
"id": 27615
},
"writtenOn": "2024-04-11T14:11:50Z",
"side": 1,
"message": "maybe prefix a POST ?",
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "8ac56267_b1a4fb86",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 50,
"author": {
"id": 27615
},
"writtenOn": "2024-04-11T14:11:50Z",
"side": 1,
"message": "can we also mention how the new parameter looks in the request body?",
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "b2a17fce_bae9f895",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 54,
"author": {
"id": 5314
},
"writtenOn": "2024-04-12T12:35:51Z",
"side": 1,
"message": "This is a good point and we need to think it through some more. Check with Eric Harney about this, he was working on re-keying encrypted volumes under specific circumstances. I\u0027m not sure if the current code would support creating a volume and giving the source_volid of an existing encrypted volume and being able to change the key value of the new volume.",
"range": {
"startLine": 54,
"startChar": 4,
"endLine": 54,
"endChar": 128
},
"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": "326d8f9e_c7199981",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 5314
},
"writtenOn": "2024-04-12T12:35:51Z",
"side": 1,
"message": "I agree with Markus, maybe a failure to contact Barbican should return a 503 (Service Unvailable) with a message like \"Key Manager Service is Unavailable\".\n\nFor the case where the specified volume type doesn\u0027t support encryption, I think a 400 (not 409) is correct, because the user needs to make a different request (not retry later when the resource is in a different state).",
"parentUuid": "159ede71_bfa4da2b",
"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": {
"uuid": "4328eef8_13fc0c4b",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 61,
"author": {
"id": 27615
},
"writtenOn": "2024-04-11T14:11:50Z",
"side": 1,
"message": "maybe mention here or somewhere that the passphrase used by user in barbican will not be able to decrypt the volume since cinder hexifies the barbican passphrase to encrypt the volume.",
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "7632bc51_f371890b",
"filename": "specs/2024.2/byok-for-cinder.rst",
"patchSetId": 1
},
"lineNbr": 76,
"author": {
"id": 27615
},
"writtenOn": "2024-04-11T14:11:50Z",
"side": 1,
"message": "need client support in OSC as well",
"revId": "acbd671fcd3471637898c6b9f4a256cc2c9727b5",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink