)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"cac45d39e05a991e4a5083e7301eb1382e2f8da9","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"f267b2bc_a39d955c","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"updated":"2025-01-15 19:06:29.000000000","message":"service_token_roles_required  is false in our CI (please let me know if any job enabling it)\n\n- https://opendev.org/openstack/keystonemiddleware/src/commit/5c86dc240818bd69cabc506cc6e08b2df1015cd5/keystonemiddleware/auth_token/_opts.py#L175\n- https://zuul.opendev.org/t/openstack/build/2f1d429cbea14034bdaca3886e90f13a/log/controller/logs/etc/cinder/cinder_conf.txt#2\n\nAnother question, if service_token_roles_required is false (current test) then both exception are passing (existing CI pass as well as this change pass), at least one should be failing right?","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"},{"author":{"_account_id":25613,"name":"Keigo Noha","email":"knoha@redhat.com","username":"knoha"},"change_message_id":"2599669d159b3dfb9ccd8a7d157570c305663536","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3509a632_9d452eb0","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"in_reply_to":"2244dece_a8eb07d5","updated":"2025-01-17 02:43:21.000000000","message":"This is written at https://opendev.org/openstack/cinder/commit/6df1839bdf288107c600b3e53dff7593a6d4c161\n\n~~~\nIf Cinder is configured with `service_token_roles_required \u003d true` and\nan attacker provides non-service valid credentials the service will\nreturn a 401 error, otherwise it\u0027ll return 409 as if a normal user had\nmade the call without the service token.\n~~~","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"55bde76d7c8009ec9c8e47b0aad5d9d2efe193fc","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"4340cc35_2f05962b","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"in_reply_to":"3509a632_9d452eb0","updated":"2025-01-24 03:14:57.000000000","message":"I see but that is written in commit msg only, and I cannot find code where cinder return 401 instead of 403 when user reuqest to detach attachment and `service_token_roles_required \u003d true`.\n\nI remember that CVC, i think things fixed there was when request comes from service or from user and let service delete the attachment but not user when there are still some attachment in nova server.","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"671476e4e80adcc3a13d1f25120617daf4573e3c","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"ea39f254_2adee185","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"in_reply_to":"4340cc35_2f05962b","updated":"2025-03-07 23:20:40.000000000","message":"I dug into this a bit.  If there\u0027s a 401, it won\u0027t come from cinder directly; it will come from the keystoneauth middleware.  What Gorka is describing in the commit message Keigo quotes above is how we handle the case where service has service_token_roles_required\u003dfalse, and a mischievous user makes a request where in addition to passing a valid token for X-Auth-Token, they pass a valid (regular) token for X-Service-Token; the keystone middleware will allow the request through even though the second token being passed doesn\u0027t actually have any service roles.  In that case, cinder will ultimately reject the request with a 409 because cinder\u0027s internal validation *does* check for the presence of service roles independently of the roles_required setting [0].  That said, we won\u0027t see a 401 in these tests because we\u0027re using valid user tokens, so that\u0027s why the existing tests don\u0027t catch a 401.  We test for the mischievous user case by constructing a mischievous request with the extra X-Service-Token header that doesn\u0027t contain a service user\u0027s token [1]. \n\n[0] https://github.com/openstack/cinder/blob/28789f0dd01198c89e064aaed65fa2df31472685/cinder/volume/api.py#L900-L910\n[1] https://github.com/openstack/tempest/blob/faa1decedfef73a77ab88feee2976036da475e0d/tempest/scenario/test_server_volume_attachment.py#L42-L63","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"be0aac79a1191daf206442e7d3299c34ac7607d7","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"2244dece_a8eb07d5","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"in_reply_to":"beafdd64_e98da646","updated":"2025-01-17 02:09:20.000000000","message":"ok, but can you point me to the cinder code/doc where this behaviour is implemented? I could not find where cinder return different error based on config option service_token_roles_required \n\n- https://github.com/search?q\u003drepo%3Aopenstack%2Fcinder%20service_token_roles_required%20\u0026type\u003dcode","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"},{"author":{"_account_id":25613,"name":"Keigo Noha","email":"knoha@redhat.com","username":"knoha"},"change_message_id":"e9ccbadf88de0ff68e5effd493c442e9696c6172","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove Forbidden exception for operations."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder is configured with `service_token_roles_required \u003d true`,"},{"line_number":10,"context_line":"it returns 401 Unauthorized to non-service user\u0027s request."},{"line_number":11,"context_line":"But the former implementation uses Forbidden to it."},{"line_number":12,"context_line":"To align with the Cinder\u0027s implementation, this change replaces"},{"line_number":13,"context_line":"Forbidden with Unauthorized for cinder operation."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"beafdd64_e98da646","line":10,"range":{"start_line":9,"start_character":0,"end_line":10,"end_character":58},"in_reply_to":"f267b2bc_a39d955c","updated":"2025-01-17 00:44:37.000000000","message":"If service_token_role_required is false, Cinder returns 409 Conflict. So, I just removed unobserved HTTP code(Forbidden) in this test.","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"671476e4e80adcc3a13d1f25120617daf4573e3c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c138f209_019830ab","updated":"2025-03-07 23:20:40.000000000","message":"I don\u0027t think we want to make this change.  Hopefully I\u0027ve explained what\u0027s going on in these tests in my comments inline.\n\nI think it would be useful, though, to document some of this in the tests, so Keigo maybe you could revise the patch to say why Forbidden is included as an option in these tests even though there is no way a Forbidden will be raised by the default configuration.  Because it\u0027s bound to come up again!","commit_id":"6431d71220d3fe753a5d6f0c3595e1aeef8bc250"}],"tempest/scenario/test_server_volume_attachment.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"671476e4e80adcc3a13d1f25120617daf4573e3c","unresolved":true,"context_lines":[{"line_number":94,"context_line":"        att_id \u003d volume[\u0027attachments\u0027][0][\u0027attachment_id\u0027]"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"        # Test user call to detach volume is rejected"},{"line_number":97,"context_line":"        self.assertRaises((exceptions.Forbidden, exceptions.Conflict),"},{"line_number":98,"context_line":"                          self.volumes_client.detach_volume, volume[\u0027id\u0027])"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"        # Test user call to terminate connection is rejected"}],"source_content_type":"text/x-python","patch_set":1,"id":"7e3d9378_c2351186","side":"PARENT","line":97,"range":{"start_line":97,"start_character":27,"end_line":97,"end_character":47},"updated":"2025-03-07 23:20:40.000000000","message":"I had to do some historical research to figure out why we are catching Forbidden here.  The answer is hidden in the comments on Gorka\u0027s original patch adding these tests to tempest [2]. The background is that the fix for CVE-2023-2088 was so complicated (it required deploying updates to nova, cinder, os-brick, and possibly glance_store, plus config changes to nova, cinder, and possibly glance), we published OSSN-0092 (\"Using Configuration as a Short-Term Mitigation for OSSA-2023-003\") [3]. The short term fix was by changing policies, so that \"bad\" operations would be forbidden.\n\nThe point of including Forbidden here was so that operators could do the short-term mitigation (which could be complicated if they have very customized policy files) and then run tempest in their cloud to verify that the policy changes they made actually were effective in mitigating the CVE and also didn\u0027t cause regressions preventing other operations.  So if you change Forbidden to Unauthorized, that usage of tempest tests will break.\n\nThis also answers Ghanshyam\u0027s question: \"Another question, if service_token_roles_required is false (current test) then both exception are passing (existing CI pass as well as this change pass), at least one should be failing right?\"  The reason why these tests pass regardless of whether Forbidden or Unauthorized is in the list is that when run in a normal devstack with default policies (like we do in the gate), we do not come across a situation where anything other than a Conflict will be raised.\n\n[2] https://review.opendev.org/c/openstack/tempest/+/882876\n[3] https://wiki.openstack.org/wiki/OSSN/OSSN-0092","commit_id":"b23e9fcc280a51bf84fae68e7719bd05c0447806"}]}