openstack
/
oslo.privsep
Code Issues Proposed changes
oslo.privsep/463b4bf5fdaf15775269da9ed93...

345 lines
12 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "2980ee9c_d9e2bde8",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 15,
"author": {
"id": 9816
},
"writtenOn": "2024-02-26T02:07:21Z",
"side": 1,
"message": "This is not really appropriate message from oslo.privsep\u0027s PoV, because it focus on very specific usage in cinder. Please make the description more generic, and explain the problem in cinder as an example.",
"range": {
"startLine": 9,
"startChar": 0,
"endLine": 15,
"endChar": 21
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "18c97303_664eec0b",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 15,
"author": {
"id": 36396
},
"writtenOn": "2024-03-04T20:40:03Z",
"side": 1,
"message": "Acknowledged",
"parentUuid": "2980ee9c_d9e2bde8",
"range": {
"startLine": 9,
"startChar": 0,
"endLine": 15,
"endChar": 21
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "3d2ee54a_94b65a47",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 17,
"author": {
"id": 9816
},
"writtenOn": "2024-02-26T02:07:21Z",
"side": 1,
"message": "Closes-Bug:",
"range": {
"startLine": 17,
"startChar": 0,
"endLine": 17,
"endChar": 10
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "17b3da8c_b5baa94e",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 17,
"author": {
"id": 36396
},
"writtenOn": "2024-03-04T20:40:03Z",
"side": 1,
"message": "Updated, thank you!",
"parentUuid": "3d2ee54a_94b65a47",
"range": {
"startLine": 17,
"startChar": 0,
"endLine": 17,
"endChar": 10
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "aed4cd68_b4b2313e",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 9816
},
"writtenOn": "2024-02-26T02:07:21Z",
"side": 1,
"message": "Please note oslo.privsep is not really specific to cinder and a common library used by multiple projects. So please make the description more generic and describe the problem in cinder as a specific example.",
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "a4d0336c_b0d06e94",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 4523
},
"writtenOn": "2024-03-13T20:07:27Z",
"side": 1,
"message": "This can be fixed in Cinder, this approach should be restored:\n https://review.opendev.org/c/openstack/os-brick/+/885979\nSee also:\n https://review.opendev.org/c/openstack/os-brick/+/896080",
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "d28a5b6e_87e990e2",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 9816
},
"writtenOn": "2024-02-26T02:07:21Z",
"side": 1,
"message": "I wonder what would be the side effect/downside of this change. In the past I relied on debug messages to dig into the storage problems in nova/cinder because these dumps raw output from iscsi commands. Does this change affect that point ? If so then we may need to find a different way (or at least document the upgrade impact)",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "ec697925_79e5717d",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 36396
},
"writtenOn": "2024-03-04T20:40:03Z",
"side": 1,
"message": "Thanks for the feedback @Takashi. Your point makes sense and I agree this is downside of the change. I see this as a tradeoff between security and convenience -- yes, it is nice to have the reply logged, but as only privileged commands are logged here, it is possible that other sensitive information could be logged. For example, /etc/iscsi/iscsid.conf containing CHAP secrets). What do you think?\n\nAnother solution could be to make the log statement optional. For example, log as we do now by default, or if a flag indicating reply-strings should be suppressed is specified, then use the ``LOG.debug(\u0027privsep: reply[%(msgid)s]: has_reply? %(reply)s\u0027,``` type of logging.",
"parentUuid": "d28a5b6e_87e990e2",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "4e22ed25_407c9704",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 9816
},
"writtenOn": "2024-03-05T02:51:16Z",
"side": 1,
"message": "\u003e it is possible that other sensitive information could be logged. For example, /etc/iscsi/iscsid.conf containing CHAP secrets). What do you think?\n\nCinder (or other services using os-brick) does not read iscsid.conf directly but read it within iscsiadm command so the content of iscsid.conf is not dumped unless iscsiadm does.\n\nThe current problem is now specific to scaleio connector and is caused by the implementation in scaleio connector which uses rootwrap to read password from scaleio config file. I wonder if that is really required. Can\u0027t we require users to add nova/cinder user to the group which has read access to the scale io config file ? I think that is much simpler approach.\n\nAnother option may be to implement a logic within os-brick to encrypt the output of get_eonnector_password and then decrypt it at _get_password_token(like b64encode/decode)",
"parentUuid": "ec697925_79e5717d",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "06287c3b_4b853b9a",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 36396
},
"writtenOn": "2024-03-05T21:50:22Z",
"side": 1,
"message": "Thanks @Takashi Kajinami,\n\niscsid.conf was just an example of something that could be ingested by this function. Outside of the ScaleIO connector, would this function ever be called with sensitive input that shouldn\u0027t be logged? In other words, is this a more general issue, or am I misunderstanding the context in which the function is used? \n\n I wonder if that is really required. Can\u0027t we require users to add nova/cinder \n user to the group which has read access to the scale io config file? I think \n that is much simpler approach.\n\nI\u0027m wondering that now as well, so I will do some digging as to what motivates the current approach and if there is any reason or limitation preventing using the approach of adding nova/cinder users to the group with read access to the scaleio config file. \n\nThanks for the info about the b64encode/decode approach, I take a look.",
"parentUuid": "4e22ed25_407c9704",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "5a04a28e_c416d975",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 9816
},
"writtenOn": "2024-03-06T01:18:37Z",
"side": 1,
"message": "\u003e would this function ever be called with sensitive input that shouldn\u0027t be logged\n\nTalking about the \"possibility\", it\u0027s possible. However oslo.privsep itself is a generic library and it can\u0027t restrict how it\u0027s used.\n\nOn the other hand, it\u0027s a fact that we haven\u0027t heard any other problems caused by the current behavior (which dumps output) so the \"problem\" is specific to scaleio connector, as far as we are aware of now.",
"parentUuid": "06287c3b_4b853b9a",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "5a552b45_d37689c9",
"filename": "oslo_privsep/daemon.py",
"patchSetId": 2
},
"lineNbr": 499,
"author": {
"id": 36396
},
"writtenOn": "2024-03-06T22:22:17Z",
"side": 1,
"message": "Thank you Takashi, I may end up closing this pull request and focusing on the scaleio connector side of things, but will get back to you on that.",
"parentUuid": "5a04a28e_c416d975",
"range": {
"startLine": 499,
"startChar": 54,
"endLine": 499,
"endChar": 75
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "487a9899_47398af0",
"filename": "releasenotes/notes/bug-2003179-f88922a47abcf07a.yaml",
"patchSetId": 2
},
"lineNbr": 4,
"author": {
"id": 36396
},
"writtenOn": "2024-03-04T21:17:18Z",
"side": 1,
"message": "@Takashi Kajinami,\nhttps://bugs.launchpad.net/cinder/+bug/2003179 is now targeting oslo.privsep in addition to Cinder.",
"range": {
"startLine": 4,
"startChar": 10,
"endLine": 4,
"endChar": 19
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "3ae40477_0b466bcf",
"filename": "releasenotes/notes/bug-2003179-f88922a47abcf07a.yaml",
"patchSetId": 2
},
"lineNbr": 15,
"author": {
"id": 9816
},
"writtenOn": "2024-02-26T02:07:21Z",
"side": 1,
"message": "Please make this more generic. The current description too much focuses effect in cinder.\n\nAlso, please open a bug against oslo.privsep, not cinder.",
"range": {
"startLine": 4,
"startChar": 4,
"endLine": 15,
"endChar": 1
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "9e6f45b4_1bcb4cad",
"filename": "releasenotes/notes/bug-2003179-f88922a47abcf07a.yaml",
"patchSetId": 2
},
"lineNbr": 15,
"author": {
"id": 36396
},
"writtenOn": "2024-03-04T20:40:03Z",
"side": 1,
"message": "Thank you for the feedback will do.",
"parentUuid": "3ae40477_0b466bcf",
"range": {
"startLine": 4,
"startChar": 4,
"endLine": 15,
"endChar": 1
},
"revId": "463b4bf5fdaf15775269da9ed9335d259b5ef852",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink