From ba29ce20ff60765aa83f462af8f7424828b5e4a1 Mon Sep 17 00:00:00 2001 From: OpenStack Release Bot Date: Thu, 13 Mar 2025 13:34:03 +0000 Subject: [PATCH 01/63] Update .gitreview for stable/2025.1 Change-Id: I23bf9b0d14b1f7b3ab5982a2f213844e066dcf34 --- .gitreview | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitreview b/.gitreview index e8ebbd0e6a..cd290fcebd 100644 --- a/.gitreview +++ b/.gitreview @@ -2,3 +2,4 @@ host=review.opendev.org port=29418 project=openstack/keystone.git +defaultbranch=stable/2025.1 From bb6dd30158a34f4332100b05456b3a0ae62b0aba Mon Sep 17 00:00:00 2001 From: OpenStack Release Bot Date: Thu, 13 Mar 2025 13:34:05 +0000 Subject: [PATCH 02/63] Update TOX_CONSTRAINTS_FILE for stable/2025.1 Update the URL to the upper-constraints file to point to the redirect rule on releases.openstack.org so that anyone working on this branch will switch to the correct upper-constraints list automatically when the requirements repository branches. Until the requirements repository has as stable/2025.1 branch, tests will continue to use the upper-constraints list on master. Change-Id: Ib20317d6b7e0f383d14707370f7938ee125207dc --- tox.ini | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tox.ini b/tox.ini index 06e8354ab8..46d494ac41 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,7 @@ setenv = # TODO(stephenfin): Remove once we bump our upper-constraint to SQLAlchemy 2.0 SQLALCHEMY_WARN_20=1 deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} -r{toxinidir}/test-requirements.txt .[ldap,memcache] commands = @@ -41,7 +41,7 @@ allowlist_externals = {toxinidir}/tools/fast8.sh # NOTE(browne): This is required for the integration test job of the bandit # project. Please do not remove. deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} -r{toxinidir}/requirements.txt commands = bandit -r keystone -x 'keystone/tests/*' @@ -71,7 +71,7 @@ passenv = KSTEST_* [testenv:functional] deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} -r{toxinidir}/test-requirements.txt setenv = OS_TEST_PATH=./keystone/tests/functional commands = @@ -107,7 +107,7 @@ max-complexity = 24 [testenv:docs] deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/master} + -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} -r{toxinidir}/doc/requirements.txt .[ldap,memcache] commands= From 79c90794adc8fd0f81caf27d2e9a5d48cde0f20d Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 25 Mar 2025 17:57:26 +0000 Subject: [PATCH 03/63] api: Don't restrict unknown querystring parameters yet We will address this in a new API version. For now, such a change would be a breaking one. No release note is included since these changes haven't been released yet. Change-Id: I1e862cb1c5e9c218cea59800ff759a1b094b5906 Signed-off-by: Stephen Finucane Closes-Bug: #2104185 (cherry picked from commit 05cc3d1903c1bfe669a1032746aa9e7b197238e9) --- keystone/application_credential/schema.py | 12 +++++++++--- keystone/assignment/schema.py | 8 ++++++-- keystone/catalog/schema.py | 5 ++++- keystone/credential/schema.py | 3 +++ keystone/federation/schema.py | 4 +++- keystone/identity/schema.py | 2 ++ keystone/limit/schema.py | 10 ++++++++-- keystone/resource/schema.py | 7 ++++++- .../tests/unit/test_v3_application_credential.py | 15 ++++++++++++++- keystone/trust/schema.py | 9 +++++++-- 10 files changed, 62 insertions(+), 13 deletions(-) diff --git a/keystone/application_credential/schema.py b/keystone/application_credential/schema.py index 886d567b52..b4bb2da0a1 100644 --- a/keystone/application_credential/schema.py +++ b/keystone/application_credential/schema.py @@ -70,7 +70,9 @@ index_request_query: dict[str, Any] = { "type": "object", "properties": {}, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } # Response of the `/access_rules` API @@ -92,7 +94,9 @@ rule_show_request_query: dict[str, Any] = { "type": "object", "properties": {}, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. + "additionalProperties": True, } # Response of `/access_rules/{access_rule_id}` API returning @@ -216,7 +220,9 @@ ), } }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } # Response of the `/application_credentials` API diff --git a/keystone/assignment/schema.py b/keystone/assignment/schema.py index 55b9f41015..8479b8c463 100644 --- a/keystone/assignment/schema.py +++ b/keystone/assignment/schema.py @@ -61,7 +61,9 @@ "name": parameter_types.name, "domain_id": parameter_types.domain_id, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } # Response body of the `GET /roles` API operation @@ -399,7 +401,9 @@ ] }, "not": {"required": ["effective", "group"]}, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (user.id__icontains) support. + "additionalProperties": True, } # Response body of the `GET /role_assignments` API operation diff --git a/keystone/catalog/schema.py b/keystone/catalog/schema.py index 418ecfe443..d1d083125c 100644 --- a/keystone/catalog/schema.py +++ b/keystone/catalog/schema.py @@ -254,7 +254,10 @@ "endpoint belongs", }, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (interface__icontains) + # support. + "additionalProperties": True, } # Response of the `/endpoints` API diff --git a/keystone/credential/schema.py b/keystone/credential/schema.py index 274fa57cda..0b95c3258a 100644 --- a/keystone/credential/schema.py +++ b/keystone/credential/schema.py @@ -70,6 +70,9 @@ ), }, }, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (type__icontains) support. + "additionalProperties": True, } # Response of the `/credentials` API diff --git a/keystone/federation/schema.py b/keystone/federation/schema.py index 3a47e134f7..546024c7b8 100644 --- a/keystone/federation/schema.py +++ b/keystone/federation/schema.py @@ -96,7 +96,9 @@ "description": "Whether the service provider is enabled or not", }, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (id__icontains) support. + "additionalProperties": True, } service_provider_index_response_body: dict[str, Any] = { diff --git a/keystone/identity/schema.py b/keystone/identity/schema.py index 3edb238da2..440d0f833a 100644 --- a/keystone/identity/schema.py +++ b/keystone/identity/schema.py @@ -81,6 +81,8 @@ "sort_key": parameter_types.sort_key, "sort_dir": parameter_types.sort_dir, }, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. "additionalProperties": True, } diff --git a/keystone/limit/schema.py b/keystone/limit/schema.py index 6bb0eeb8c5..5d51abd558 100644 --- a/keystone/limit/schema.py +++ b/keystone/limit/schema.py @@ -93,7 +93,10 @@ **parameter_types.name, }, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (service_id__icontains) + # support. + "additionalProperties": True, } # Response body of the `GET /registered_limits` API operation @@ -257,7 +260,10 @@ **parameter_types.domain_id, }, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (project_id__icontains) + # support. + "additionalProperties": True, } # Response body of the `GET /limits` API operation diff --git a/keystone/resource/schema.py b/keystone/resource/schema.py index c8a7ab3680..f19c4742c1 100644 --- a/keystone/resource/schema.py +++ b/keystone/resource/schema.py @@ -100,6 +100,9 @@ }, "limit": {"type": ["integer", "string"]}, }, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } project_schema: dict[str, Any] = { @@ -208,7 +211,9 @@ }, "limit": {"type": ["integer", "string"]}, }, - "additionalProperties": "False", + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } domain_index_response_body: dict[str, Any] = { diff --git a/keystone/tests/unit/test_v3_application_credential.py b/keystone/tests/unit/test_v3_application_credential.py index 0ca075807c..df0c12095d 100644 --- a/keystone/tests/unit/test_v3_application_credential.py +++ b/keystone/tests/unit/test_v3_application_credential.py @@ -12,6 +12,7 @@ import datetime import http.client +import unittest import uuid from oslo_utils import timeutils @@ -742,7 +743,10 @@ def test_list_access_rules(self): ar = r.json["access_rules"] self.assertEqual(access_rules[0]["method"], ar[0]["method"]) - def test_list_access_rules_wrong_qp(self): + # TODO(stephenfin): This will pass once we increase strictness of the query + # string validation + @unittest.expectedFailure + def test_list_access_rules_invalid_qs(self): with self.test_client() as c: token = self.get_scoped_token() # Invoke GET access_rules with unsupported query parameters and @@ -778,6 +782,15 @@ def test_show_access_rule(self): expected_status_code=http.client.OK, headers={"X-Auth-Token": token}, ) + + # TODO(stephenfin): This will pass once we increase strictness of the query + # string validation + @unittest.expectedFailure + def test_show_access_rule_invalid_qs(self): + with self.test_client() as c: + token = self.get_scoped_token() + # Invoke GET access_rules/{id} with unsupported query parameters and + # trigger internal validation c.get( f"/v3/users/{self.user_id}/access_rules/{access_rule_id}" "?foo=bar", diff --git a/keystone/trust/schema.py b/keystone/trust/schema.py index 3d34987857..20c85b3907 100644 --- a/keystone/trust/schema.py +++ b/keystone/trust/schema.py @@ -170,7 +170,10 @@ "the trust.", }, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (trustor_user_id__icontains) + # support. + "additionalProperties": True, } trust_index_response_body: dict[str, Any] = { @@ -190,7 +193,9 @@ trust_request_query: dict[str, Any] = { "type": "object", "properties": {}, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. + "additionalProperties": True, } trust_response_body: dict[str, Any] = { From a904f9b03f6b3f19465add4e88ad224ea5ca56b4 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 25 Mar 2025 17:55:46 +0000 Subject: [PATCH 04/63] api: Correct query string schema for access rules API The presence of a call to 'build_driver_hints' in the method indicates that filters are permitted for this API, even if they're not currently documented. We also rename the variables and fix a docstring to avoid confusion about what the schemas are intended for. Change-Id: I17c4e5116ded437a8561b5f721899cdc653c352e Signed-off-by: Stephen Finucane (cherry picked from commit 187c1af5068997ab1ebd29cb7deaa8138b496af6) --- keystone/api/users.py | 16 +++++++--- keystone/application_credential/schema.py | 39 +++++++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/keystone/api/users.py b/keystone/api/users.py index e428e82a06..b3ec13f37b 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -806,8 +806,12 @@ class UserAccessRuleListResource(ks_flask.ResourceBase): collection_key = 'access_rules' member_key = 'access_rule' - @validation.request_query_schema(app_cred_schema.index_request_query) - @validation.response_body_schema(app_cred_schema.rule_index_response_body) + @validation.request_query_schema( + app_cred_schema.access_rule_index_request_query + ) + @validation.response_body_schema( + app_cred_schema.access_rule_index_response_body + ) def get(self, user_id): """List access rules for user. @@ -830,8 +834,12 @@ class UserAccessRuleGetDeleteResource(ks_flask.ResourceBase): collection_key = 'access_rules' member_key = 'access_rule' - @validation.request_query_schema(app_cred_schema.rule_show_request_query) - @validation.response_body_schema(app_cred_schema.rule_show_response_body) + @validation.request_query_schema( + app_cred_schema.access_rule_show_request_query + ) + @validation.response_body_schema( + app_cred_schema.access_rule_show_response_body + ) def get(self, user_id, access_rule_id): """Get access rule resource. diff --git a/keystone/application_credential/schema.py b/keystone/application_credential/schema.py index b4bb2da0a1..04ef913830 100644 --- a/keystone/application_credential/schema.py +++ b/keystone/application_credential/schema.py @@ -65,18 +65,43 @@ "additionalProperties": False, } -# Query parameters of the `/users/{user_id}/access_rules` and -# `/application_credentials/{application_credential_id}` APIs -index_request_query: dict[str, Any] = { +# Query parameters of the `/users/{user_id}/access_rules` API +access_rule_index_request_query: dict[str, Any] = { "type": "object", - "properties": {}, + "properties": { + # NOTE(stephenfin): We don't reuse _access_rules_properties since these + # filters are currently far less strict that the request body of the + # POST and PATCH operations. We may wish to change this in a future + # version. + "service": { + "type": "string", + "description": ( + "The service type identifier for the service that the " + "application is permitted to access." + ), + }, + "path": { + "type": "string", + "description": ( + "The API path that the application credential is " + "permitted to access." + ), + }, + "method": { + "type": "string", + "description": ( + "The request method that the application credential is " + "permitted to use for a given API endpoint." + ), + }, + }, # TODO(stephenfin): Change this to False once we have schemas for all # resources. Doing so will remove comparator (name__icontains) support. "additionalProperties": True, } # Response of the `/access_rules` API -rule_index_response_body: dict[str, Any] = { +access_rule_index_response_body: dict[str, Any] = { "type": "object", "properties": { "access_rules": { @@ -91,7 +116,7 @@ # /access/rules/{access_rule_id} # GET request query parameters -rule_show_request_query: dict[str, Any] = { +access_rule_show_request_query: dict[str, Any] = { "type": "object", "properties": {}, # TODO(stephenfin): Change this to False once we have schemas for all @@ -101,7 +126,7 @@ # Response of `/access_rules/{access_rule_id}` API returning # single access rule -rule_show_response_body: dict[str, Any] = { +access_rule_show_response_body: dict[str, Any] = { "type": "object", "description": "An access rule object.", "properties": {"access_rule": access_rule_schema}, From 379c2b0facb3bf3b5e367cc12e2afc3bd7b7dd7c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Mon, 31 Mar 2025 14:00:53 +0100 Subject: [PATCH 05/63] api: Don't restrict unknown querystring parameters yet (redux) We missed a few in change I1e862cb1c5e9c218cea59800ff759a1b094b5906. We also missed a few comments in places that we can change later. Change-Id: I6fc40baf536605d9d347741bcf035958b8490b07 Signed-off-by: Stephen Finucane (cherry picked from commit 46ba4f455dd14fe509e2e83dc39a3fc6894f7dbb) --- keystone/catalog/schema.py | 8 +++++++- keystone/federation/schema.py | 4 ++++ keystone/identity/schema.py | 4 +++- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/keystone/catalog/schema.py b/keystone/catalog/schema.py index d1d083125c..f0451c9ce0 100644 --- a/keystone/catalog/schema.py +++ b/keystone/catalog/schema.py @@ -98,6 +98,8 @@ service_index_request_query: dict[str, Any] = { "type": "object", "properties": {}, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. "additionalProperties": True, } @@ -278,7 +280,9 @@ endpoint_request_query: dict[str, Any] = { "type": "object", "properties": {}, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. + "additionalProperties": True, } # Response of the `/endpoints` API returning a single endpoint @@ -406,6 +410,8 @@ "description": "The name of the endpoint group.", } }, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. "additionalProperties": False, } diff --git a/keystone/federation/schema.py b/keystone/federation/schema.py index 546024c7b8..c0fa8f6e8b 100644 --- a/keystone/federation/schema.py +++ b/keystone/federation/schema.py @@ -213,6 +213,8 @@ "description": "Whether the identity provider is enabled or not", }, }, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. "additionalProperties": True, } @@ -232,6 +234,8 @@ identity_provider_request_query: dict[str, Any] = { "type": "object", "properties": {}, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. "additionalProperties": True, } diff --git a/keystone/identity/schema.py b/keystone/identity/schema.py index 440d0f833a..f8688baae8 100644 --- a/keystone/identity/schema.py +++ b/keystone/identity/schema.py @@ -208,7 +208,9 @@ "sort_key": parameter_types.sort_key, "sort_dir": parameter_types.sort_dir, }, - "additionalProperties": False, + # TODO(stephenfin): Change this to False once we have schemas for all + # resources. Doing so will remove comparator (name__icontains) support. + "additionalProperties": True, } _group_properties: dict[str, Any] = { From aab001fea362bc576eb51507af7c15355c8c3f28 Mon Sep 17 00:00:00 2001 From: Dmitriy Rabotyagov Date: Thu, 26 Sep 2024 16:36:06 +0200 Subject: [PATCH 06/63] Fix DB migrations after alembic integration With a change to swap sqlalchemy-migrate with alembic [1] a `db_sync --check` was broken. This is due to both `upgrades.get_db_version` and `upgrades.get_current_heads` are actually checking "current" state of the dabase by calling _get_current_heads[2][3], while obvious intention was to compare intended state with current state. With that we're introducing upgrade.get_head_revisions which will fetch revisions not from the database, but from the environment [4] As a result `db_sync --check` does compare desired state of the DB with actual state and exists with corresponsive status again. [1] https://opendev.org/openstack/keystone/commit/f174b4fa7c4fb010bbacc8c5a5f3625a8fcb41f3 [2] https://opendev.org/openstack/keystone/src/branch/master/keystone/common/sql/upgrades.py#L147 [3] https://opendev.org/openstack/keystone/src/branch/master/keystone/common/sql/upgrades.py#L191 [4] https://alembic.sqlalchemy.org/en/latest/api/runtime.html#alembic.runtime.environment.EnvironmentContext.get_head_revisions Closes-Bug: #2080542 Change-Id: I854d37e3b4a34a7880f157564466bde61a3f886a (cherry picked from commit 5125d9feed8dc8f9f0ad01f4c042594475f5fb84) --- keystone/cmd/cli.py | 2 +- keystone/common/sql/upgrades.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/keystone/cmd/cli.py b/keystone/cmd/cli.py index 93bf024144..8982ef5102 100644 --- a/keystone/cmd/cli.py +++ b/keystone/cmd/cli.py @@ -439,7 +439,7 @@ def check_db_sync_status(cls): except db_exception.DBMigrationError: contract_version = None - heads = upgrades.get_current_heads() + heads = upgrades.get_head_revisions() if ( upgrades.EXPAND_BRANCH not in heads diff --git a/keystone/common/sql/upgrades.py b/keystone/common/sql/upgrades.py index acfb5b7af3..b18373563d 100644 --- a/keystone/common/sql/upgrades.py +++ b/keystone/common/sql/upgrades.py @@ -19,6 +19,7 @@ from alembic import command as alembic_api from alembic import config as alembic_config from alembic import migration as alembic_migration +from alembic.runtime import environment as alembic_environment from alembic import script as alembic_script from oslo_db import exception as db_exception from oslo_log import log as logging @@ -105,6 +106,11 @@ def _get_current_heads(engine, config): context = alembic_migration.MigrationContext.configure(conn) heads = context.get_current_heads() + heads_map = _get_head_maps(heads, script) + return heads_map + + +def _get_head_maps(heads, script): heads_map = {} for head in heads: @@ -142,6 +148,17 @@ def get_current_heads(): return heads +def get_head_revisions(): + """Get the available head for each the expand and contract branches.""" + config = _find_alembic_conf() + script = alembic_script.ScriptDirectory.from_config(config) + context = alembic_environment.EnvironmentContext(config, script) + heads = context.get_head_revisions() + heads_map = _get_head_maps(heads, script) + + return heads_map + + def _is_database_under_alembic_control(engine): with engine.connect() as conn: context = alembic_migration.MigrationContext.configure(conn) From 020370dcf658eb4b7066e440fd1ccff1e607edb4 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 29 May 2025 18:04:51 +0100 Subject: [PATCH 07/63] api: Add log when creating unscoped token Otherwise you get different behavior depending on the user used, if some have default projects set and others do not. Change-Id: I7c347af983cb8af9be9d19a010fa0f5bf20ab804 Signed-off-by: Stephen Finucane (cherry picked from commit b6f955b8e40ef3f0bc7388f9c6de406069c6870b) --- keystone/api/_shared/authentication.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/keystone/api/_shared/authentication.py b/keystone/api/_shared/authentication.py index 6d58df2078..e1839566f2 100644 --- a/keystone/api/_shared/authentication.py +++ b/keystone/api/_shared/authentication.py @@ -56,7 +56,12 @@ def _check_and_set_default_scoping(auth_info, auth_context): default_project_id = user_ref.get('default_project_id') if not default_project_id: - # User has no default project. He shall get an unscoped token. + # User has no default project. They shall get an unscoped token. + msg = ( + "User %(user_id)s doesn't have a default project. " + " The token will be unscoped rather than scoped to a project." + ) + LOG.debug(msg, {'user_id': user_ref['id']}) return # make sure user's default project is legit before scoping to it From a1ea3e042b110b596393e3726c39705c67e565b1 Mon Sep 17 00:00:00 2001 From: Andrew Bogott Date: Sun, 29 Jun 2025 21:59:04 -0500 Subject: [PATCH 08/63] trust schema: don't require user_id to be in uuid format The user id might come from an external provider in which case we can't make assumptions about its format. The constraint removed here is breaking the credential APIs for ldap-based clouds. Change-Id: I80dfe07ae48fd08de3af9cf5508215e4bbcea13c (cherry picked from commit 84a30d56071ab2ed48dfdb3172af723dc74cdd28) Signed-off-by: Seunghun Lee --- keystone/credential/schema.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/keystone/credential/schema.py b/keystone/credential/schema.py index 0b95c3258a..75ffd631d1 100644 --- a/keystone/credential/schema.py +++ b/keystone/credential/schema.py @@ -32,7 +32,6 @@ }, "user_id": { "type": "string", - "format": "uuid", "description": "The ID of the user who owns the credential.", }, } @@ -59,7 +58,6 @@ "properties": { "user_id": { "type": "string", - "format": "uuid", "description": "Filters the response by a user ID.", }, "type": { From 5a60aad8ab1790851057312400a0ce61f35c33a1 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Wed, 27 Aug 2025 16:49:57 +0200 Subject: [PATCH 09/63] Ignore typing on the single import We cannot cherry-pick the https://review.opendev.org/c/openstack/keystone/+/950184 to the stable branches since it does not work in py39. Since there is also no easy way how to fix the typing error just add ignore comment to that - it makes no difference anyway. Change-Id: I54a74cdc653b05ea681cbccbc1229109df378d6c Signed-off-by: Artem Goncharov --- keystone/tests/unit/test_backend_ldap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py index 946fa18100..359284a0f3 100644 --- a/keystone/tests/unit/test_backend_ldap.py +++ b/keystone/tests/unit/test_backend_ldap.py @@ -21,7 +21,7 @@ import fixtures import ldap from oslo_log import versionutils -import pkg_resources +import pkg_resources # type: ignore from testtools import matchers from keystone.common import cache From ba210d3e788eb7e6880ddbdcd1898349f43f9bfb Mon Sep 17 00:00:00 2001 From: Jorge Merlino Date: Wed, 4 Jun 2025 13:58:17 -0300 Subject: [PATCH 10/63] Fix AD nested groups issues The implementation of AD nested groups searches works fine when listing the groups a user belongs to, but fails when listing all members of a group. This function of listing all members is also used to check if a user belongs to a group which also fails. This patch fixes the query for getting all users in a group. Closes-Bug: #2112477 Depends-on: https://review.opendev.org/c/openstack/devstack/+/960683 Depends-on: https://review.opendev.org/c/openstack/devstack/+/960684 Change-Id: I9707e1a9bc4a334902933d6251888144f8c3bc19 Signed-off-by: Jorge Merlino (cherry picked from commit f8338be43073f23f3db64fa4ba658c3e1f554aa7) --- keystone/identity/backends/ldap/common.py | 15 +++++++++++++-- keystone/identity/backends/ldap/core.py | 13 ++++++++----- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 2212543872..00e495c1d5 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1417,6 +1417,12 @@ def __init__(self, conf): self.auth_pool_conn_lifetime = conf.ldap.auth_pool_connection_lifetime if self.options_name is not None: + self.user_tree_dn = ( + conf.ldap.user_tree_dn + or f'{self.DEFAULT_OU},{conf.ldap.suffix}' + ) + self.user_objectclass = conf.ldap.user_objectclass + self.tree_dn = ( getattr(conf.ldap, f'{self.options_name}_tree_dn') or f'{self.DEFAULT_OU},{conf.ldap.suffix}' @@ -1854,9 +1860,14 @@ def _ldap_get_all(self, hints, ldap_filter=None): return self._filter_ldap_result_by_attr(res, 'name') def _ldap_get_list( - self, search_base, scope, query_params=None, attrlist=None + self, + search_base, + scope, + query_params=None, + attrlist=None, + object_class=None, ): - query = f'(objectClass={self.object_class})' + query = f'(objectClass={object_class or self.object_class})' if query_params: def calc_filter(attrname, value): diff --git a/keystone/identity/backends/ldap/core.py b/keystone/identity/backends/ldap/core.py index 75251234ba..5ddf14d064 100644 --- a/keystone/identity/backends/ldap/core.py +++ b/keystone/identity/backends/ldap/core.py @@ -434,23 +434,26 @@ def list_group_users(self, group_id): """Return a list of user dns which are members of a group.""" group_ref = self.get(group_id) group_dn = group_ref['dn'] + attribute = self.member_attribute try: if self.group_ad_nesting: # NOTE(ayoung): LDAP_SCOPE is used here instead of hard- # coding to SCOPE_SUBTREE to get through the unit tests. # However, it is also probably more correct. + attribute = "distinguishedName" attrs = self._ldap_get_list( - self.tree_dn, + self.user_tree_dn, self.LDAP_SCOPE, query_params={ - f"member:{LDAP_MATCHING_RULE_IN_CHAIN}:": group_dn + f"memberOf:{LDAP_MATCHING_RULE_IN_CHAIN}:": group_dn }, - attrlist=[self.member_attribute], + attrlist=[attribute], + object_class=self.user_objectclass, ) else: attrs = self._ldap_get_list( - group_dn, ldap.SCOPE_BASE, attrlist=[self.member_attribute] + group_dn, ldap.SCOPE_BASE, attrlist=[attribute] ) except ldap.NO_SUCH_OBJECT: @@ -458,7 +461,7 @@ def list_group_users(self, group_id): users = [] for dn, member in attrs: - user_dns = member.get(self.member_attribute, []) + user_dns = member.get(attribute, []) for user_dn in user_dns: users.append(user_dn) return users From 0d4625051dc6bff22ee13c4e5e0c6b80d7af1d26 Mon Sep 17 00:00:00 2001 From: Grzegorz Grasza Date: Fri, 19 Sep 2025 14:02:18 +0200 Subject: [PATCH 11/63] Add service user authentication to ec2 and s3 endpoints Add a policy to enforce authentication with a user in the service group. This maintains AWS compatibility with the added security layer. Closes-Bug: 2119646 Change-Id: Ic84b84247e05f29874e2c5636a033aaedd4de83c Signed-off-by: Grzegorz Grasza Signed-off-by: Jeremy Stanley Signed-off-by: Artem Goncharov (cherry picked from commit 68c1817e1cf1ed284d8420a6e1261749648bccd8) --- doc/source/getting-started/policy_mapping.rst | 2 ++ keystone/api/ec2tokens.py | 8 ++++- keystone/api/s3tokens.py | 7 +++- keystone/common/policies/__init__.py | 4 +++ keystone/common/policies/ec2tokens.py | 34 ++++++++++++++++++ keystone/common/policies/s3tokens.py | 35 +++++++++++++++++++ keystone/tests/unit/test_contrib_ec2_core.py | 22 +++++++++--- keystone/tests/unit/test_contrib_s3_core.py | 16 +++++---- keystone/tests/unit/test_v3_credential.py | 5 +++ 9 files changed, 121 insertions(+), 12 deletions(-) create mode 100644 keystone/common/policies/ec2tokens.py create mode 100644 keystone/common/policies/s3tokens.py diff --git a/doc/source/getting-started/policy_mapping.rst b/doc/source/getting-started/policy_mapping.rst index a7cb27cfa7..fab79be12d 100644 --- a/doc/source/getting-started/policy_mapping.rst +++ b/doc/source/getting-started/policy_mapping.rst @@ -245,6 +245,8 @@ identity:delete_application_credential DELETE /v3/users/{use identity:get_access_rule GET /v3/users/{user_id}/access_rules/{access_rule_id} identity:list_access_rules GET /v3/users/{user_id}/access_rules identity:delete_access_rule DELETE /v3/users/{user_id}/access_rules/{access_rule_id} +identity:s3tokens_validate POST /v3/s3tokens +identity:ec2tokens_validate POST /v3/es2tokens ========================================================= === diff --git a/keystone/api/ec2tokens.py b/keystone/api/ec2tokens.py index 152fe15a79..4fad969df2 100644 --- a/keystone/api/ec2tokens.py +++ b/keystone/api/ec2tokens.py @@ -21,6 +21,7 @@ from keystone.api._shared import EC2_S3_Resource from keystone.api._shared import json_home_relations +from keystone.common import rbac_enforcer from keystone.common import render_token from keystone.common import utils from keystone import exception @@ -30,6 +31,9 @@ CRED_TYPE_EC2 = 'ec2' +ENFORCER = rbac_enforcer.RBACEnforcer + + class EC2TokensResource(EC2_S3_Resource.ResourceBase): @staticmethod def _check_signature(creds_ref, credentials): @@ -57,12 +61,14 @@ def _check_signature(creds_ref, credentials): else: raise exception.Unauthorized(_('EC2 signature not supplied.')) - @ks_flask.unenforced_api def post(self): """Authenticate ec2 token. POST /v3/ec2tokens """ + # Enforce RBAC in the same way as S3 tokens + ENFORCER.enforce_call(action='identity:ec2tokens_validate') + token = self.handle_authenticate() token_reference = render_token.render_token_response_from_model(token) resp_body = jsonutils.dumps(token_reference) diff --git a/keystone/api/s3tokens.py b/keystone/api/s3tokens.py index c9c8b895de..769a378f25 100644 --- a/keystone/api/s3tokens.py +++ b/keystone/api/s3tokens.py @@ -22,12 +22,15 @@ from keystone.api._shared import EC2_S3_Resource from keystone.api._shared import json_home_relations +from keystone.common import rbac_enforcer from keystone.common import render_token from keystone.common import utils from keystone import exception from keystone.i18n import _ from keystone.server import flask as ks_flask +ENFORCER = rbac_enforcer.RBACEnforcer + def _calculate_signature_v1(string_to_sign, secret_key): """Calculate a v1 signature. @@ -96,12 +99,14 @@ def _check_signature(creds_ref, credentials): message=_('Credential signature mismatch') ) - @ks_flask.unenforced_api def post(self): """Authenticate s3token. POST /v3/s3tokens """ + # Use standard Keystone policy enforcement for s3tokens access + ENFORCER.enforce_call(action='identity:s3tokens_validate') + token = self.handle_authenticate() token_reference = render_token.render_token_response_from_model(token) resp_body = jsonutils.dumps(token_reference) diff --git a/keystone/common/policies/__init__.py b/keystone/common/policies/__init__.py index 02608c185a..68842b50ce 100644 --- a/keystone/common/policies/__init__.py +++ b/keystone/common/policies/__init__.py @@ -22,6 +22,7 @@ from keystone.common.policies import domain from keystone.common.policies import domain_config from keystone.common.policies import ec2_credential +from keystone.common.policies import ec2tokens from keystone.common.policies import endpoint from keystone.common.policies import endpoint_group from keystone.common.policies import grant @@ -40,6 +41,7 @@ from keystone.common.policies import revoke_event from keystone.common.policies import role from keystone.common.policies import role_assignment +from keystone.common.policies import s3tokens from keystone.common.policies import service from keystone.common.policies import service_provider from keystone.common.policies import token @@ -78,6 +80,8 @@ def list_rules(): revoke_event.list_rules(), role.list_rules(), role_assignment.list_rules(), + s3tokens.list_rules(), + ec2tokens.list_rules(), service.list_rules(), service_provider.list_rules(), token_revocation.list_rules(), diff --git a/keystone/common/policies/ec2tokens.py b/keystone/common/policies/ec2tokens.py new file mode 100644 index 0000000000..7f5f4e21f4 --- /dev/null +++ b/keystone/common/policies/ec2tokens.py @@ -0,0 +1,34 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_policy import policy + +from keystone.common.policies import base + +# Align EC2 tokens API with S3 tokens: require admin or service users +ADMIN_OR_SERVICE = 'rule:service_or_admin' + + +ec2tokens_policies = [ + policy.DocumentedRuleDefault( + name=base.IDENTITY % 'ec2tokens_validate', + check_str=ADMIN_OR_SERVICE, + scope_types=['system', 'domain', 'project'], + description='Validate EC2 credentials and create a Keystone token. ' + 'Restricted to service users or administrators.', + operations=[{'path': '/v3/ec2tokens', 'method': 'POST'}], + ) +] + + +def list_rules(): + return ec2tokens_policies diff --git a/keystone/common/policies/s3tokens.py b/keystone/common/policies/s3tokens.py new file mode 100644 index 0000000000..96088f62e2 --- /dev/null +++ b/keystone/common/policies/s3tokens.py @@ -0,0 +1,35 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_policy import policy + +from keystone.common.policies import base + +# S3 tokens API requires service authentication to prevent presigned URL exploitation +# This policy restricts access to service users or administrators only +ADMIN_OR_SERVICE = 'rule:service_or_admin' + +s3tokens_policies = [ + policy.DocumentedRuleDefault( + name=base.IDENTITY % 's3tokens_validate', + check_str=ADMIN_OR_SERVICE, + scope_types=['system', 'domain', 'project'], + description='Validate S3 credentials and create a Keystone token. ' + 'Restricted to service users or administrators to prevent ' + 'exploitation via presigned URLs.', + operations=[{'path': '/v3/s3tokens', 'method': 'POST'}], + ) +] + + +def list_rules(): + return s3tokens_policies diff --git a/keystone/tests/unit/test_contrib_ec2_core.py b/keystone/tests/unit/test_contrib_ec2_core.py index b23bdbc896..534cf894ea 100644 --- a/keystone/tests/unit/test_contrib_ec2_core.py +++ b/keystone/tests/unit/test_contrib_ec2_core.py @@ -62,20 +62,34 @@ def _test_valid_authentication_response_with_proper_secret(self, **kwargs): }, } credentials['signature'] = signer.generate(credentials) + # Authenticate as system admin by default unless overridden via kwargs + token = None + if 'noauth' in kwargs and kwargs['noauth']: + token = None + else: + PROVIDERS.assignment_api.create_system_grant_for_user( + self.user_id, self.role_id + ) + token = self.get_system_scoped_token() + + expected_status = kwargs.get('expected_status', http.client.OK) resp = self.post( '/ec2tokens', body={'credentials': credentials}, - expected_status=http.client.OK, - **kwargs, + expected_status=expected_status, + token=token, + noauth=kwargs.get('noauth'), ) - self.assertValidProjectScopedTokenResponse(resp, self.user) + if expected_status == http.client.OK: + self.assertValidProjectScopedTokenResponse(resp, self.user) def test_valid_authentication_response_with_proper_secret(self): self._test_valid_authentication_response_with_proper_secret() def test_valid_authentication_response_with_proper_secret_noauth(self): + # ec2 endpoint now enforces RBAC; unauthenticated should be denied self._test_valid_authentication_response_with_proper_secret( - noauth=True + expected_status=http.client.UNAUTHORIZED, noauth=True ) def test_valid_authentication_response_with_signature_v4(self): diff --git a/keystone/tests/unit/test_contrib_s3_core.py b/keystone/tests/unit/test_contrib_s3_core.py index 60637f19d7..5b431fa4d8 100644 --- a/keystone/tests/unit/test_contrib_s3_core.py +++ b/keystone/tests/unit/test_contrib_s3_core.py @@ -55,7 +55,7 @@ def test_http_get_method_not_allowed(self): ) self.assertEqual(http.client.METHOD_NOT_ALLOWED, resp.status_code) - def _test_good_response(self, **kwargs): + def _test_good_response(self, expected_status=http.client.OK, **kwargs): sts = 'string to sign' # opaque string from swift3 sig = hmac.new( self.cred_blob['secret'].encode('ascii'), @@ -71,18 +71,22 @@ def _test_good_response(self, **kwargs): 'token': base64.b64encode(sts.encode('ascii')).strip(), } }, - expected_status=http.client.OK, + expected_status=expected_status, **kwargs, ) - self.assertValidProjectScopedTokenResponse( - resp, self.user, forbid_token_id=True - ) + if expected_status == http.client.OK: + self.assertValidProjectScopedTokenResponse( + resp, self.user, forbid_token_id=True + ) + else: + self.assertValidErrorResponse(resp) def test_good_response(self): self._test_good_response() def test_good_response_noauth(self): - self._test_good_response(noauth=True) + # s3tokens now requires service/admin auth; unauthenticated should be denied + self._test_good_response(http.client.UNAUTHORIZED, noauth=True) def test_bad_request(self): self.post( diff --git a/keystone/tests/unit/test_v3_credential.py b/keystone/tests/unit/test_v3_credential.py index dd212c667f..1ae6a98d6b 100644 --- a/keystone/tests/unit/test_v3_credential.py +++ b/keystone/tests/unit/test_v3_credential.py @@ -90,10 +90,15 @@ def _test_get_token(self, access, secret): 'path': '/bar', 'params': params, } + PROVIDERS.assignment_api.create_system_grant_for_user( + self.user_id, self.role_id + ) + token = self.get_system_scoped_token() r = self.post( '/ec2tokens', body={'ec2Credentials': sig_ref}, expected_status=http.client.OK, + token=token, ) self.assertValidTokenResponse(r) return r.result['token'] From 0e17f15a09536e4c9d5c0c2cc81271854c8800f1 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Thu, 14 Nov 2019 17:10:05 +0100 Subject: [PATCH 12/63] [keystone] initial throw at the ccloud train+victoria merge Change-Id: I628a3a6b524cd099345463e3a6bafe16df450581 --- concourse_unit_test_task | 1 + custom-requirements.txt | 5 +++ keystone/common/fernet_utils.py | 23 +++++++------- keystone/common/render_token.py | 9 ++++++ keystone/conf/resource.py | 26 ++++++++++++++++ keystone/exception.py | 6 ++++ keystone/identity/backends/ldap/common.py | 3 +- keystone/notifications.py | 1 + keystone/server/flask/application.py | 5 +++ keystone/server/flask/core.py | 37 +++++++++++++++++++++++ requirements.txt | 4 +++ tox.ini | 9 +++++- 12 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 concourse_unit_test_task create mode 100644 custom-requirements.txt diff --git a/concourse_unit_test_task b/concourse_unit_test_task new file mode 100644 index 0000000000..89985c7eeb --- /dev/null +++ b/concourse_unit_test_task @@ -0,0 +1 @@ +export DEBIAN_FRONTEND=noninteractive && export KEYSTONE_IDENTITY_BACKEND=ldap && export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && apt-get update && apt-get install -y build-essential python-pip python-dev python3-dev git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && pip install tox && pip install six && /etc/init.d/memcached start && git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && cd keystone && tox -e py37 diff --git a/custom-requirements.txt b/custom-requirements.txt new file mode 100644 index 0000000000..2c7061137a --- /dev/null +++ b/custom-requirements.txt @@ -0,0 +1,5 @@ +git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] +git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware +#git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware +git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius +git+https://github.wdf.sap.corp/monsoon/keystone-extensions.git@victoria#egg=keystone-extensions diff --git a/keystone/common/fernet_utils.py b/keystone/common/fernet_utils.py index 36dd70671b..5b731076b7 100644 --- a/keystone/common/fernet_utils.py +++ b/keystone/common/fernet_utils.py @@ -12,7 +12,7 @@ import base64 import os -import stat +# import stat from cryptography import fernet from oslo_log import log @@ -61,16 +61,17 @@ def validate_key_repository(self, requires_write=False): 'config_group': self.config_group, }, ) - else: - # ensure the key repository isn't world-readable - stat_info = os.stat(self.key_repository) - if ( - stat_info.st_mode & stat.S_IROTH - or stat_info.st_mode & stat.S_IXOTH - ): - LOG.warning( - 'key_repository is world readable: %s', self.key_repository - ) + # CCloud: we don't care, since k8s volumes from secrets can't be configured to be read-only + # else: + # # ensure the key repository isn't world-readable + # stat_info = os.stat(self.key_repository) + # if ( + # stat_info.st_mode & stat.S_IROTH + # or stat_info.st_mode & stat.S_IXOTH + # ): + # LOG.warning( + # 'key_repository is world readable: %s', self.key_repository + # ) return is_valid diff --git a/keystone/common/render_token.py b/keystone/common/render_token.py index dbaab32c4b..23ed898194 100644 --- a/keystone/common/render_token.py +++ b/keystone/common/render_token.py @@ -94,6 +94,15 @@ def render_token_response_from_model(token, include_catalog=True): and ap_domain_name == token.project_domain['name'] ) token_reference['token']['is_admin_project'] = is_ap + # ccloud: additional bootstrap project support + bootstrap_name = CONF.resource.bootstrap_project_name + bootstrap_domain_name = CONF.resource.bootstrap_project_domain_name + if bootstrap_name and bootstrap_domain_name and not token_reference['token'].get('is_admin_project', False): + is_ap = ( + token.project['name'] == bootstrap_name and + bootstrap_domain_name == token.project_domain['name'] + ) + token_reference['token']['is_admin_project'] = is_ap if include_catalog and not token.unscoped: user_id = token.user_id if token.trust_id: diff --git a/keystone/conf/resource.py b/keystone/conf/resource.py index 659cd339ab..6beee9cbbc 100644 --- a/keystone/conf/resource.py +++ b/keystone/conf/resource.py @@ -85,6 +85,29 @@ ), ) +# CCloud: added bootstrap cloud-admin project & domain +bootstrap_project_domain_name = cfg.StrOpt( + 'bootstrap_project_domain_name', + help=utils.fmt(""" +Name of the domain that owns the `bootstrap_project_name`. If left unset, then +there is no admin project. `[resource] bootstrap_project_name` must also be set to +use this option. +""") +) + +bootstrap_project_name = cfg.StrOpt( + 'bootstrap_project_name', + help=utils.fmt(""" +This is a special project which represents cloud-level administrator privileges +across services. Tokens scoped to this project will contain a true +`is_admin_project` attribute to indicate to policy systems that the role +assignments on that specific project should apply equally across every project. +If left unset, then there is no admin project, and thus no explicit means of +cross-project role assignments. `[resource] bootstrap_project_domain_name` must +also be set to use this option. +""") +) + project_name_url_safe = cfg.StrOpt( 'project_name_url_safe', choices=['off', 'new', 'strict'], @@ -117,6 +140,7 @@ GROUP_NAME = __name__.split('.')[-1] +# CCloud: added bootstrap cloud-admin project & domain ALL_OPTS = [ driver, caching, @@ -124,6 +148,8 @@ list_limit, admin_project_domain_name, admin_project_name, + bootstrap_project_domain_name, + bootstrap_project_name, project_name_url_safe, domain_name_url_safe, ] diff --git a/keystone/exception.py b/keystone/exception.py index 0eb209b9b0..dcb4d56a06 100644 --- a/keystone/exception.py +++ b/keystone/exception.py @@ -70,6 +70,8 @@ class Error(Exception, metaclass=_KeystoneExceptionMeta): code: ty.Optional[int] = None title: ty.Optional[str] = None message_format: ty.Optional[str] = None + # ccloud: keep the original message for logging, regardless of insecure_debug setting + message = None def __init__(self, message=None, **kwargs): try: @@ -297,6 +299,10 @@ def __deepcopy__(self): def _build_message(self, message, **kwargs): """Only returns detailed messages in insecure_debug mode.""" + # ccloud: always keep original message for logging, regardless of insecure_debug setting + if message: + self.message = message + if message and CONF.insecure_debug: if isinstance(message, str): # Only do replacement if message is string. The message is diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 00e495c1d5..88842b3daf 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1837,7 +1837,8 @@ def _ldap_get_all(self, hints, ldap_filter=None): + list(self.extra_attr_mapping.keys()) ) ) - if hints.limit: + # ccloud: only limit if all filters have been satisfied and the contoller does not need to filter + if hints.limit and not len(hints.filters): sizelimit = hints.limit['limit'] res = self._ldap_get_limited( self.tree_dn, self.LDAP_SCOPE, query, attrs, sizelimit diff --git a/keystone/notifications.py b/keystone/notifications.py index 1df7e1569c..d300abf101 100644 --- a/keystone/notifications.py +++ b/keystone/notifications.py @@ -71,6 +71,7 @@ 'OS-OAUTH1:request_token': taxonomy.SECURITY_CREDENTIAL, 'OS-OAUTH1:consumer': taxonomy.SECURITY_ACCOUNT, 'application_credential': taxonomy.SECURITY_CREDENTIAL, + 'credential': taxonomy.SECURITY_CREDENTIAL, } SAML_AUDIT_TYPE = 'http://docs.oasis-open.org/security/saml/v2.0' diff --git a/keystone/server/flask/application.py b/keystone/server/flask/application.py index ddde080cfa..f270dd0b0e 100644 --- a/keystone/server/flask/application.py +++ b/keystone/server/flask/application.py @@ -141,6 +141,11 @@ def _handle_keystone_exception(error): # Handle logging if isinstance(error, exception.Unauthorized): + # ccloud: log original message, regardless of insecure_debug setting + if error.message is not None: + message = error.message + else: + message = error LOG.warning( "Authorization failed. %(exception)s from %(remote_addr)s", {'exception': error, 'remote_addr': flask.request.remote_addr}, diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index 42d28468a8..fc69dc8fd7 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -30,6 +30,10 @@ from keystone.server.flask.request_processing.middleware import auth_context from keystone.server.flask.request_processing.middleware import url_normalize +# CCloud +import logging +from raven.contrib.flask import Sentry + # NOTE(morgan): Middleware Named Tuple with the following values: # * "namespace": namespace for the entry_point # * "ep": the entry-point name @@ -64,6 +68,21 @@ ), ) +# ccloud: additional ccloud specific middleware that needs to sit 'behind' the AuthContextMiddleware +_CC_MIDDLEWARE = ( + # CCloud: add watcher middleware + _Middleware(namespace='watcher.middleware', + ep='watcher', + conf={'service_type': 'identity', + 'config_file': '/etc/keystone/watcher.yaml', + 'include_initiator_user_id_in_metric': 'true', + 'include_target_project_id_in_metric': 'false'}), + # CCloud: add lifesaver middleware + _Middleware(namespace='lifesaver.middleware', + ep='lifesaver', + conf={}), +) + # NOTE(morgan): ORDER HERE IS IMPORTANT! Each of these middlewares are # implemented/defined explicitly in Keystone Server. They do some level of # lifting to ensure the request is properly handled. It is importat to note @@ -103,6 +122,8 @@ def setup_app_middleware(app): MW = _APP_MIDDLEWARE IMW = _KEYSTONE_MIDDLEWARE + # ccloud + CMW = _CC_MIDDLEWARE # Add in optional (config-based) middleware # NOTE(morgan): Each of these may need to be in a specific location @@ -115,6 +136,16 @@ def setup_app_middleware(app): ), ) + _APP_MIDDLEWARE + # Apply ccloud Middleware. Need to sit 'behind' the AuthContextMiddleware + for mw in reversed(CMW): + # CCloud: optionally skip the watcher middleware (like in unit-tests) + if mw.ep == 'watcher' and os.environ.get('WATCHER_DISABLED', None): + continue + + loaded = stevedore.DriverManager(mw.namespace, mw.ep, invoke_on_load=False) + factory_func = loaded.driver.factory({}, **mw.conf) + app.wsgi_app = factory_func(app.wsgi_app) + # Apply internal-only Middleware (e.g. AuthContextMiddleware). These # are below all externally loaded middleware in request processing. for mw in reversed(IMW): @@ -145,6 +176,12 @@ def setup_app_middleware(app): # Apply werkzeug specific middleware app.wsgi_app = proxy_fix.ProxyFix(app.wsgi_app) + + # CCloud + if os.environ.get('SENTRY_DSN', None): + sentry = Sentry(app, logging=True, level=logging.ERROR) + sentry.init_app(app) + return app diff --git a/requirements.txt b/requirements.txt index 3b03fc21d4..6af650405c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -37,3 +37,7 @@ jsonschema>=3.2.0 # MIT pycadf!=2.0.0,>=1.1.0 # Apache-2.0 msgpack>=0.5.0 # Apache-2.0 osprofiler>=1.4.0 # Apache-2.0 + +python-ldap>=3.0.0 # PSF +ldappool>=2.3.1 # MPL +python-memcached>=1.56 # PSF diff --git a/tox.ini b/tox.ini index 46d494ac41..2aeb051931 100644 --- a/tox.ini +++ b/tox.ini @@ -13,12 +13,13 @@ setenv = deps = -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} -r{toxinidir}/test-requirements.txt + -r{toxinidir}/custom-requirements.txt .[ldap,memcache] commands = stestr run {posargs} allowlist_externals = bash -passenv = http_proxy,HTTP_PROXY,https_proxy,HTTPS_PROXY,no_proxy,NO_PROXY,PBR_VERSION +passenv = http_proxy,HTTP_PROXY,https_proxy,HTTPS_PROXY,no_proxy,NO_PROXY,PBR_VERSION,WATCHER_DISABLED [testenv:pep8] deps = @@ -169,6 +170,12 @@ paths = ./keystone/tests/hacking deps = bindep commands = bindep test +[testenv:lower-constraints] +deps = + -c{toxinidir}/lower-constraints.txt + -r{toxinidir}/test-requirements.txt + .[ldap,memcache] + [testenv:protection] commands = stestr run --test-path=./keystone/tests/protection {posargs} From 3b72eabd658c07e470e740323cdf22977b47b1d9 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Thu, 14 Nov 2019 17:27:32 +0100 Subject: [PATCH 13/63] [keystone] grab python 3.7 for unit testing Change-Id: Ic236212dfda70a28ea6fece177c05308c12936d1 --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 89985c7eeb..2d8f0cb7da 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1 +1 @@ -export DEBIAN_FRONTEND=noninteractive && export KEYSTONE_IDENTITY_BACKEND=ldap && export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && apt-get update && apt-get install -y build-essential python-pip python-dev python3-dev git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && pip install tox && pip install six && /etc/init.d/memcached start && git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && cd keystone && tox -e py37 +export DEBIAN_FRONTEND=noninteractive && export KEYSTONE_IDENTITY_BACKEND=ldap && export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && apt-get update && apt-get install -y build-essential python-pip python-dev python3.7-dev git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && pip install tox && pip install six && /etc/init.d/memcached start && git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && cd keystone && tox -e py37 From 0abfe49b689533112280187511334cfa9e7b44b9 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Mon, 18 Nov 2019 17:52:56 +0100 Subject: [PATCH 14/63] [keystone] added python 3 unit test required libraries --- concourse_unit_test_task | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 2d8f0cb7da..0a599f5f95 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1 +1,10 @@ -export DEBIAN_FRONTEND=noninteractive && export KEYSTONE_IDENTITY_BACKEND=ldap && export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && apt-get update && apt-get install -y build-essential python-pip python-dev python3.7-dev git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && pip install tox && pip install six && /etc/init.d/memcached start && git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && cd keystone && tox -e py37 +export DEBIAN_FRONTEND=noninteractive && \ +export KEYSTONE_IDENTITY_BACKEND=ldap && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ +apt-get update && \ +apt-get install -y build-essential python-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ +pip install tox && \ +/etc/init.d/memcached start && \ +git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && \ +cd keystone && \ +tox -e py37 From 6a96d06bf3a911e4842ee5a2b6753bcd5d6335c1 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Fri, 22 Nov 2019 08:28:53 +0100 Subject: [PATCH 15/63] [keystone] reduce the strain on github.wdf.sap.corp --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 0a599f5f95..facc0098aa 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -5,6 +5,6 @@ apt-get update && \ apt-get install -y build-essential python-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ pip install tox && \ /etc/init.d/memcached start && \ -git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git && \ +git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py37 From af09b5d4ea5b2eec80bce167fee15740c5407636 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Fri, 22 Nov 2019 10:02:12 +0100 Subject: [PATCH 16/63] [keystone] delegate list all trusts permission check to policy rule, instead of a hard-coded admin check (which is definitely not enough) Change-Id: Ia6071a0ba7c698ee2a425096888f06a12c1e236e --- concourse_unit_test_task | 1 + keystone/api/trusts.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index facc0098aa..73d453451e 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,5 +1,6 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ +export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ diff --git a/keystone/api/trusts.py b/keystone/api/trusts.py index 6a3f04e134..afb4404d61 100644 --- a/keystone/api/trusts.py +++ b/keystone/api/trusts.py @@ -224,7 +224,9 @@ def get(self): ) if not flask.request.args: # NOTE(morgan): Admin can list all trusts. - ENFORCER.enforce_call(action='admin_required') + # ENFORCER.enforce_call(action='admin_required') + # ccloud: support configuration in policy for this + ENFORCER.enforce_call(action='identity:list_all_trusts') if not flask.request.args: trusts += PROVIDERS.trust_api.list_trusts() From 37c711a49162dc3153b631c47c5c65a87c6f3d58 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Tue, 8 Aug 2023 16:13:30 +0200 Subject: [PATCH 17/63] Increase the limit in doctor check for our fernet We are using ldap and our max_size is bigger. Also update tests Change-Id: I13dc7cf77dbad236492f7504033d0cb41a5656cd --- keystone/cmd/doctor/tokens.py | 4 +++- keystone/tests/unit/test_cli.py | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/keystone/cmd/doctor/tokens.py b/keystone/cmd/doctor/tokens.py index 547e13ec7c..74f177bfad 100644 --- a/keystone/cmd/doctor/tokens.py +++ b/keystone/cmd/doctor/tokens.py @@ -31,4 +31,6 @@ def symptom_unreasonable_max_token_size(): depending on the IDs returned from LDAP, resulting in longer Fernet tokens (adjust your `max_token_size` accordingly). """ - return 'fernet' in CONF.token.provider and CONF.max_token_size > 255 + # return 'fernet' in CONF.token.provider and CONF.max_token_size > 255 + # CCloud: we are using ldap, hence the max_token_size needs to go up. + return ('fernet' in CONF.token.provider and CONF.max_token_size > 268) diff --git a/keystone/tests/unit/test_cli.py b/keystone/tests/unit/test_cli.py index 9949439fe5..4fb64642d6 100644 --- a/keystone/tests/unit/test_cli.py +++ b/keystone/tests/unit/test_cli.py @@ -1619,8 +1619,9 @@ def test_password_regular_expression_description_deactivated(self): class TokensDoctorTests(unit.TestCase): def test_unreasonable_max_token_size_raised(self): # Symptom Detected: the max_token_size for fernet is greater than 255 + # ccloud: not 255, but 268 self.config_fixture.config(group='token', provider='fernet') - self.config_fixture.config(max_token_size=256) + self.config_fixture.config(max_token_size=269) self.assertTrue(tokens.symptom_unreasonable_max_token_size()) def test_unreasonable_max_token_size_not_raised(self): @@ -1630,8 +1631,9 @@ def test_unreasonable_max_token_size_not_raised(self): self.assertFalse(tokens.symptom_unreasonable_max_token_size()) # No Symptom Detected: the max_token_size for fernet is 255 or less + # ccloud: not 255, but 268 self.config_fixture.config(group='token', provider='fernet') - self.config_fixture.config(max_token_size=255) + self.config_fixture.config(max_token_size=268) self.assertFalse(tokens.symptom_unreasonable_max_token_size()) From 2877cb0390d3a4fd22dd9f843b222d0ed0ecb1be Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Fri, 22 Nov 2019 10:39:00 +0100 Subject: [PATCH 18/63] [keystone] skip token_size test --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 73d453451e..9a85829bd4 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip install tox && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py37 +tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised" From d3a7e2e330323617ac3d5e7366b965af01f50bb7 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Fri, 22 Nov 2019 11:19:07 +0100 Subject: [PATCH 19/63] [keystone] fixed random notification test failures --- keystone/tests/unit/common/test_notifications.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/keystone/tests/unit/common/test_notifications.py b/keystone/tests/unit/common/test_notifications.py index 65fa13b1b1..99cf5df662 100644 --- a/keystone/tests/unit/common/test_notifications.py +++ b/keystone/tests/unit/common/test_notifications.py @@ -144,6 +144,8 @@ class NotificationsTestCase(unit.BaseTestCase): def setUp(self): super().setUp() self.config_fixture = self.useFixture(config_fixture.Config(CONF)) + # ccloud: fix random notification test failures + oslo_messaging.get_notification_transport(CONF, url='rabbit://') self.config_fixture.config( group='oslo_messaging_notifications', transport_url='rabbit://' ) From 568df72285e2989b687f5c62613a5a57595a76fc Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Wed, 27 Nov 2019 15:20:12 +0100 Subject: [PATCH 20/63] policy engine doesn't like rewrites of the identity:actions, so revert to cloud_admin for now Co-authored-by: Maurice Escher --- custom-requirements.txt | 1 - keystone/api/trusts.py | 4 ++-- keystone/common/policies/base.py | 3 +++ .../flask/request_processing/middleware/auth_context.py | 3 ++- keystone/tests/unit/test_policy.py | 1 + 5 files changed, 8 insertions(+), 4 deletions(-) diff --git a/custom-requirements.txt b/custom-requirements.txt index 2c7061137a..d9f1a3f577 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,4 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware -#git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius git+https://github.wdf.sap.corp/monsoon/keystone-extensions.git@victoria#egg=keystone-extensions diff --git a/keystone/api/trusts.py b/keystone/api/trusts.py index afb4404d61..215c501256 100644 --- a/keystone/api/trusts.py +++ b/keystone/api/trusts.py @@ -225,8 +225,8 @@ def get(self): if not flask.request.args: # NOTE(morgan): Admin can list all trusts. # ENFORCER.enforce_call(action='admin_required') - # ccloud: support configuration in policy for this - ENFORCER.enforce_call(action='identity:list_all_trusts') + # ccloud: only allow cloud-admins to list all trusts + ENFORCER.enforce_call(action='cloud_admin') if not flask.request.args: trusts += PROVIDERS.trust_api.list_trusts() diff --git a/keystone/common/policies/base.py b/keystone/common/policies/base.py index 97970e4e03..2fded16069 100644 --- a/keystone/common/policies/base.py +++ b/keystone/common/policies/base.py @@ -76,6 +76,9 @@ ) rules = [ + policy.RuleDefault( + name='cloud_admin', check_str='role:admin and is_admin_project:True' + ), policy.RuleDefault( name='admin_required', check_str='role:admin or is_admin:1' ), diff --git a/keystone/server/flask/request_processing/middleware/auth_context.py b/keystone/server/flask/request_processing/middleware/auth_context.py index c9938668e9..55728852c2 100644 --- a/keystone/server/flask/request_processing/middleware/auth_context.py +++ b/keystone/server/flask/request_processing/middleware/auth_context.py @@ -317,7 +317,8 @@ def _validate_trusted_issuer(self, request): 'Cannot find client issuer in env by the ' 'issuer attribute - %s.' ) - LOG.info(msg, CONF.tokenless_auth.issuer_attribute) + # ccloud: this is only relevant for debug sessions + LOG.debug(msg, CONF.tokenless_auth.issuer_attribute) return False if issuer in CONF.tokenless_auth.trusted_issuer: diff --git a/keystone/tests/unit/test_policy.py b/keystone/tests/unit/test_policy.py index eaff961ff4..14f04fb1a2 100644 --- a/keystone/tests/unit/test_policy.py +++ b/keystone/tests/unit/test_policy.py @@ -242,6 +242,7 @@ def test_all_targets_documented(self): 'service_role', 'token_subject', 'domain_managed_target_role', + 'cloud_admin', ] def read_doc_targets(): From 1bf9cbaf6c26fd5c83b6e497862c54a3a025ff12 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Tue, 10 Dec 2019 09:23:51 +0100 Subject: [PATCH 21/63] [keystone] don't forward *NotFound exceptions to sentry Change-Id: I765e4d2b5999f282a3b324312fb2485bc38ad914 --- keystone/server/flask/core.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index fc69dc8fd7..077aafa777 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -25,6 +25,7 @@ from keystone.common import profiler import keystone.conf +from keystone import exception import keystone.server from keystone.server.flask import application from keystone.server.flask.request_processing.middleware import auth_context @@ -179,8 +180,12 @@ def setup_app_middleware(app): # CCloud if os.environ.get('SENTRY_DSN', None): - sentry = Sentry(app, logging=True, level=logging.ERROR) - sentry.init_app(app) + app.config['SENTRY_CONFIG'] = { + 'ignore_exceptions': [exception.NotFound], + } + + sentry = Sentry() + sentry.init_app(app, logging=True, level=logging.ERROR) return app From 280d5a5635c9500d6a257ac6da0e8e872378ba0b Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Tue, 10 Dec 2019 09:55:00 +0100 Subject: [PATCH 22/63] [keystone] skip keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 9a85829bd4..9920b6e7b7 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip install tox && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised" +tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised, keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From 43fb30f8cefb7ce80c1a2ba3b0d0099934baa0f7 Mon Sep 17 00:00:00 2001 From: Rudolf Vriend Date: Tue, 10 Dec 2019 10:06:12 +0100 Subject: [PATCH 23/63] [keystone] use proper regex format for test exclusions --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 9920b6e7b7..3e110fe6ae 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip install tox && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised, keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From 5dc6914980785c9e11024ffcf92993fd639d9fbf Mon Sep 17 00:00:00 2001 From: Maurice Escher Date: Thu, 6 Feb 2020 17:24:12 +0100 Subject: [PATCH 24/63] ccloud: add possibility to set default tag(s) configuration option to set tags that get added to newly created projects Change-Id: Icac8d54506082816bdaeacb73853a20b49735c16 --- keystone/conf/default.py | 12 +++++++ keystone/resource/core.py | 10 ++++++ keystone/tests/unit/core.py | 6 ++++ keystone/tests/unit/test_v3_resource.py | 43 +++++++++++++++++++------ 4 files changed, 61 insertions(+), 10 deletions(-) diff --git a/keystone/conf/default.py b/keystone/conf/default.py index 145799d39c..2dfbfed95e 100644 --- a/keystone/conf/default.py +++ b/keystone/conf/default.py @@ -147,6 +147,17 @@ ), ) +default_tag = cfg.MultiStrOpt( + 'default_tag', + default=[], + help=utils.fmt(""" +Default `tag`(s) for newly created projects. If left undefined, Keystone will +create projects without an initial tag. This field can be set multiple times in +order to set multiple default tags, for example: +default_tag=tag_0 +default_tag=tag_1 +""")) + notification_format = cfg.StrOpt( 'notification_format', default='cadf', @@ -195,6 +206,7 @@ strict_password_check, insecure_debug, default_publisher_id, + default_tag, notification_format, notification_opt_out, ] diff --git a/keystone/resource/core.py b/keystone/resource/core.py index 0d554310c1..e473f23dcd 100644 --- a/keystone/resource/core.py +++ b/keystone/resource/core.py @@ -230,6 +230,16 @@ def create_project(self, project_id, project, initiator=None): project['name'] = project['name'].strip() project.setdefault('description', '') + # ccloud add default tag(s) + if CONF.default_tag: + default_tags = [x.strip() for x in CONF.default_tag] + if 'tags' in project: + # a user may have provided a tag, which is a default tag, again + # make unique by converting to a set + project['tags'] = list(set(project['tags'] + default_tags)) + else: + project['tags'] = default_tags + # For regular projects, the controller will ensure we have a valid # domain_id. For projects acting as a domain, the project_id # is, effectively, the domain_id - and for such projects we don't diff --git a/keystone/tests/unit/core.py b/keystone/tests/unit/core.py index e7ceb49c5d..40a25a496e 100644 --- a/keystone/tests/unit/core.py +++ b/keystone/tests/unit/core.py @@ -304,6 +304,12 @@ def new_project_ref(domain_id=None, is_domain=False, **kwargs): return ref +def new_project_without_tags_ref(domain_id=None, is_domain=False, **kwargs): + ref = new_project_ref(domain_id=domain_id, is_domain=is_domain, **kwargs) + ref.pop('tags') + return ref + + def new_user_ref(domain_id, project_id=None, **kwargs): ref = { 'id': uuid.uuid4().hex, diff --git a/keystone/tests/unit/test_v3_resource.py b/keystone/tests/unit/test_v3_resource.py index ced9419874..fa58a9756f 100644 --- a/keystone/tests/unit/test_v3_resource.py +++ b/keystone/tests/unit/test_v3_resource.py @@ -27,6 +27,9 @@ CONF = keystone.conf.CONF PROVIDERS = provider_api.ProviderAPIs +_DEFAULT_TAG = ['single_tag'] +_DEFAULT_TAGS = [None, [], ['vc-a-0', 'tag_1', 'tag_2'], _DEFAULT_TAG] + class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin): """Test domains and projects.""" @@ -843,7 +846,7 @@ def _create_projects_hierarchy(self, hierarchy_size=1): return projects - def _create_project_and_tags(self, num_of_tags=1): + def _create_project_and_tags(self, num_of_tags=1, with_default_tag=False): """Create a project and a number of tags attached to that project. :param num_of_tags: the desired number of tags created with a specified @@ -853,7 +856,19 @@ def _create_project_and_tags(self, num_of_tags=1): random tags """ tags = [uuid.uuid4().hex for i in range(num_of_tags)] - ref = unit.new_project_ref(domain_id=self.domain_id, tags=tags) + + if num_of_tags > 0: + if with_default_tag: + # remove the last tag + tags.pop() + # add default tag instead + tags += _DEFAULT_TAG + ref = unit.new_project_ref( + domain_id=self.domain_id, + tags=tags) + else: + ref = unit.new_project_without_tags_ref( + domain_id=self.domain_id) resp = self.post('/projects', body={'project': ref}) return resp.result['project'], tags @@ -1760,14 +1775,22 @@ def test_delete_not_leaf_project(self): ) def test_create_project_with_tags(self): - project, tags = self._create_project_and_tags(num_of_tags=10) - ref = self.get( - '/projects/{project_id}'.format(project_id=project['id']), - expected_status=http.client.OK, - ) - self.assertIn('tags', ref.result['project']) - for tag in tags: - self.assertIn(tag, ref.result['project']['tags']) + for config_setting in _DEFAULT_TAGS: + if config_setting is not None: + self.config_fixture.config(default_tag=config_setting) + for tag_number in [0, 10]: + project, tags = self._create_project_and_tags( + num_of_tags=tag_number, with_default_tag=True) + ref = self.get( + '/projects/%(project_id)s' % { + 'project_id': project['id']}, + expected_status=http.client.OK) + self.assertIn('tags', ref.result['project']) + for tag in tags: + self.assertIn(tag, ref.result['project']['tags']) + if config_setting is not None: + for tag in config_setting: + self.assertIn(tag, ref.result['project']['tags']) def test_update_project_with_tags(self): project, tags = self._create_project_and_tags(num_of_tags=9) From 54421863d64551e09360a4a815b40ea4fb0aea13 Mon Sep 17 00:00:00 2001 From: Maurice Escher Date: Tue, 11 Feb 2020 14:17:17 +0100 Subject: [PATCH 25/63] workaround https://github.com/pypa/virtualenv/issues/1551#issuecomment-584148133 --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 3e110fe6ae..ff1b5fac7c 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -4,7 +4,7 @@ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ -pip install tox && \ +pip install tox "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ From 15bee32d41c7ce1c37598a89cb9c8fad3ea262d4 Mon Sep 17 00:00:00 2001 From: Maurice Escher Date: Mon, 20 Apr 2020 08:02:54 +0200 Subject: [PATCH 26/63] ccloud: policy modification ec2_create_credential - add policy_id to target - it was possible to create ec2 creds to different projects without a policy check --- keystone/api/users.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/keystone/api/users.py b/keystone/api/users.py index b3ec13f37b..35d3130b16 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -420,13 +420,14 @@ def post(self, user_id): POST /v3/users/{user_id}/credentials/OS-EC2 """ + tenant_id = self.request_body_json.get('tenant_id') + target = {} - target['credential'] = {'user_id': user_id} + target['credential'] = {'user_id': user_id, 'project_id': tenant_id} ENFORCER.enforce_call( action='identity:ec2_create_credential', target_attr=target ) PROVIDERS.identity_api.get_user(user_id) - tenant_id = self.request_body_json.get('tenant_id') PROVIDERS.resource_api.get_project(tenant_id) blob = { 'access': uuid.uuid4().hex, From f60e33345afafb8abd89f463de9d5b717aa37900 Mon Sep 17 00:00:00 2001 From: Maurice Escher Date: Fri, 27 Nov 2020 10:30:16 +0100 Subject: [PATCH 27/63] ccloud: consume extensions from public repo Change-Id: I7b81f7d16987f0e633cd999923bdfe19b4e0d3da --- custom-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/custom-requirements.txt b/custom-requirements.txt index d9f1a3f577..a5ccf08ca4 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,4 +1,4 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.wdf.sap.corp/monsoon/keystone-extensions.git@victoria#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@victoria#egg=keystone-extensions From 84ebd6d315b974f6e0a02ab13bde63ac64d3709e Mon Sep 17 00:00:00 2001 From: rajivmucheli Date: Wed, 14 Jul 2021 20:45:35 +0530 Subject: [PATCH 28/63] Enable rate-limiting middleware --- custom-requirements.txt | 1 + keystone/server/flask/core.py | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/custom-requirements.txt b/custom-requirements.txt index a5ccf08ca4..8ac387f313 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -2,3 +2,4 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius git+https://github.com/sapcc/keystone-extensions.git@victoria#egg=keystone-extensions +git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index 077aafa777..781f381992 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -82,6 +82,14 @@ _Middleware(namespace='lifesaver.middleware', ep='lifesaver', conf={}), + # CCloud: add rate_limit middleware + _Middleware(namespace='rate_limit.middleware', + ep='rate-limit', + conf={'config_file': '/etc/keystone/ratelimit.yaml', + 'service_type': 'identity', + 'rate_limit_by': 'initiator_project_id', + 'backend_host': 'keystone-sapcc-rate-limit', + 'backend_timeout_seconds': '1'}), ) # NOTE(morgan): ORDER HERE IS IMPORTANT! Each of these middlewares are From 8900e2437830a3e7c5810be2b5e62e33e0d3640d Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 9 Mar 2022 13:52:54 +0100 Subject: [PATCH 29/63] Switch tox to python3 tox running on python2 causes weird issues with string encoding. Instead of fixing those issues, switch it to python3 --- concourse_unit_test_task | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index ff1b5fac7c..6a636c964a 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -3,8 +3,8 @@ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ apt-get update && \ -apt-get install -y build-essential python-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ -pip install tox "six>=1.14.0" && \ +apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ +pip3 install tox "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ From 7b152bf96e171ac1bcd64c4cca945a0e2b5c398f Mon Sep 17 00:00:00 2001 From: Maurice Escher Date: Thu, 7 May 2020 16:54:04 +0200 Subject: [PATCH 30/63] fix link in release note of bug/1794527 Change-Id: I120ef1c0c8259c85b6030f2db0a649c71b990879 closes-bug: 1877393 --- releasenotes/notes/bug-1794527-866b1caff67977f3.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/bug-1794527-866b1caff67977f3.yaml b/releasenotes/notes/bug-1794527-866b1caff67977f3.yaml index 6a8134fd24..04435288d6 100644 --- a/releasenotes/notes/bug-1794527-866b1caff67977f3.yaml +++ b/releasenotes/notes/bug-1794527-866b1caff67977f3.yaml @@ -18,4 +18,4 @@ features: the old one, and having spurious failures, or undesirecd domain_id matching. - https://specs.openstack.org/openstack/keystone-specs/specs/keystone/stein/explicit-domains-ids.html \ No newline at end of file + https://specs.openstack.org/openstack/keystone-specs/specs/keystone/train/explicit-domains-ids.html From 84423404803ea0b60f98039c0920ef36e1ae2677 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Fri, 3 Jun 2022 20:49:01 +0200 Subject: [PATCH 31/63] Do not filter domain list via projects api with domain-scoped token If project list is requested via /v3/projects with domain-scoped token, only the project in the domain are being returned. It makes no sense when is_domain filter is used, because domains are top-level the request always returned an empty list. Return domain list the same way as if /v3/domains is being used and do not filter out anything. Note: the policies still need to be adjusted by the operators if they wish to allow this kind of request. Closes-Bug: 1950325 Change-Id: I77ed200d1a222659abd1e2f00b9984647b310c43 --- keystone/api/projects.py | 12 ++++++++- keystone/tests/unit/test_v3_resource.py | 34 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/keystone/api/projects.py b/keystone/api/projects.py index 7d343c4972..4317eced02 100644 --- a/keystone/api/projects.py +++ b/keystone/api/projects.py @@ -21,6 +21,7 @@ from keystone.common import json_home from keystone.common import provider_api from keystone.common import rbac_enforcer +from keystone.common import utils from keystone.common import validation as ks_validation import keystone.conf from keystone import exception @@ -189,7 +190,16 @@ def get(self): if t in flask.request.args: hints.add_filter(t, flask.request.args[t]) refs = PROVIDERS.resource_api.list_projects(hints=hints) - if self.oslo_context.domain_id: + + # the entries should not be filtered by domain_id if domain list was + # requested; otherwise all domains are getting filtered out + try: + is_domain_requested = utils.attr_as_boolean( + flask.request.args['is_domain']) + except KeyError: + is_domain_requested = False + + if self.oslo_context.domain_id and not is_domain_requested: domain_id = self.oslo_context.domain_id filtered_refs = [ ref for ref in refs if ref['domain_id'] == domain_id diff --git a/keystone/tests/unit/test_v3_resource.py b/keystone/tests/unit/test_v3_resource.py index fa58a9756f..2c2c89ef90 100644 --- a/keystone/tests/unit/test_v3_resource.py +++ b/keystone/tests/unit/test_v3_resource.py @@ -1397,6 +1397,40 @@ def test_list_project_is_domain_filter(self): new_regular_project['id'], [p['id'] for p in r.result['projects']] ) + def test_list_projects_is_domain_filter_domain_scoped_token(self): + """Call ``GET /projects?is_domain=True/False`` with domain scope""" + # grant the domain role to user + path = '/domains/%s/users/%s/roles/%s' % ( + self.domain_id, self.user['id'], self.role['id']) + self.put(path=path) + + auth = self.build_authentication_request( + user_id=self.user['id'], + password=self.user['password'], + domain_id=self.domain_id) + + # Check that listing the domains does not result in an empty list + new_is_domain_project = unit.new_project_ref(is_domain=True) + new_is_domain_project = PROVIDERS.resource_api.create_project( + new_is_domain_project['id'], new_is_domain_project) + + r = self.get('/projects?is_domain=True', auth=auth, + expected_status=200) + self.assertIn(new_is_domain_project['id'], + [p['id'] for p in r.result['projects']]) + + # Check that the projects are still being filtered + # The previously created is_domain project is a domain, so + # we can reuse it for the project + new_regular_project = unit.new_project_ref( + is_domain=False, domain_id=new_is_domain_project['id']) + new_regular_project = PROVIDERS.resource_api.create_project( + new_regular_project['id'], new_regular_project) + r = self.get('/projects?is_domain=False', auth=auth, + expected_status=200) + self.assertNotIn(new_regular_project['id'], + [p['id'] for p in r.result['projects']]) + def test_list_project_is_domain_filter_default(self): """Default project list should not see projects acting as domains.""" # Get the initial count of regular projects From 9438de97508d4c292bc017f948b71ad797e52643 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Mon, 6 Mar 2023 12:06:49 +0100 Subject: [PATCH 32/63] Add application credential id for audit notifications Change-Id: I271bf96ea0a0bde3ca91c6de6ee90ed55e3fd72a --- keystone/notifications.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/keystone/notifications.py b/keystone/notifications.py index d300abf101..8cfd068e8c 100644 --- a/keystone/notifications.py +++ b/keystone/notifications.py @@ -117,6 +117,13 @@ def build_audit_initiator(): if oslo_context.global_request_id: initiator.global_request_id = oslo_context.global_request_id + try: + token_info = flask.request.environ['keystone.token_info'] + app_cred_id = token_info['token']['application_credential']['id'] + initiator.application_credential_id = app_cred_id + except KeyError: + pass + return initiator From 3163d4c3af2bf43fb03d33fe760f1c599c6c76ff Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 8 Mar 2023 15:33:22 +0100 Subject: [PATCH 33/63] Drop python3 minor version from apt-get string in concourse runner Instead, just use whatever python3 there is. Also, use py38 for unit tests Change-Id: I48d5995ac249516bac547e01e7f67ccddac0a7d7 --- concourse_unit_test_task | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 6a636c964a..029c00c400 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -3,9 +3,9 @@ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ apt-get update && \ -apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3.7 python3.7-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ +apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ pip3 install tox "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py37 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +tox -e py38 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From 0a4ecdf9ec81dbf3adcaf6e26ba22a1cc290175d Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 8 Mar 2023 15:36:35 +0100 Subject: [PATCH 34/63] Use the correct requirements file for concourse test runner Change-Id: I4be1b3e888f5ef7d3a557695d3b103374cebe106 --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 029c00c400..485655be06 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,7 +1,7 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/train-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ pip3 install tox "six>=1.14.0" && \ From a34a4e156069efce09d0d8708361bdce455b56ab Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 8 Mar 2023 16:03:31 +0100 Subject: [PATCH 35/63] Cap tox to <4 tox had a lot of changes that break test runs. Instead of adapting to these chances, cap tox. Uncap later, when OpenStack upstream fixes it Change-Id: Ib7338e937fb49d08ce1890626a50996747bd34c9 --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 485655be06..1a1fb097a3 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -4,7 +4,7 @@ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ -pip3 install tox "six>=1.14.0" && \ +pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ From 6219ce5d95923940fe528c5aa1f2aa52b526d102 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 9 Aug 2023 17:14:44 +0200 Subject: [PATCH 36/63] Switch dependencies to xena Change-Id: I26b1db96a20895c851089458b6ab9bcd4223829c --- concourse_unit_test_task | 4 ++-- custom-requirements.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 1a1fb097a3..d147b43092 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,11 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/victoria-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/xena-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ -git clone -b stable/victoria-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ +git clone -b stable/xena-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py38 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" diff --git a/custom-requirements.txt b/custom-requirements.txt index 8ac387f313..89a509ff82 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,5 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@victoria#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@xena#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From 4626d53261ca4aa8f7fab160a186e81464238550 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Tue, 29 Aug 2023 14:43:37 +0200 Subject: [PATCH 37/63] Update remaining requirements to zed Change-Id: Ia9d1e8f803824e96fcfb3541d142b41a54eea1a6 --- concourse_unit_test_task | 4 ++-- custom-requirements.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index d147b43092..a0bfc4473a 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,11 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/xena-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/zed-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ -git clone -b stable/xena-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ +git clone -b stable/zed-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py38 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" diff --git a/custom-requirements.txt b/custom-requirements.txt index 89a509ff82..ce59fc2035 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,5 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@xena#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@zed#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From a806a42792e64cca777dc434cc2fe45a9cfdd962 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Fri, 8 Sep 2023 11:52:48 +0200 Subject: [PATCH 38/63] concourse_unit_test_task: change the name of ldap dependency Change-Id: I436fdc526316648098bb31dbe62a8d863af2f2eb --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index a0bfc4473a..402cb75303 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -3,7 +3,7 @@ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/zed-m3/upper-constraints.txt && \ apt-get update && \ -apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-2.4-2 memcached && \ +apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/zed-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ From 49a12547818f34c1fa4a4f842bb52ded3fc1dcd7 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Fri, 8 Sep 2023 11:57:29 +0200 Subject: [PATCH 39/63] concourse_unit_test_task: use py310 instead of py38 Change-Id: I59ff8c35baa4d176508ab8f4813d564049458835 --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 402cb75303..fec1df6aa4 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/zed-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py38 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +tox -e py310 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From cec933be0193072a4d9f07482976fb708f091f3c Mon Sep 17 00:00:00 2001 From: Fabian Wiesel Date: Tue, 17 Oct 2023 10:50:15 +0200 Subject: [PATCH 40/63] Pull keystone-extensions.git@stable/zed-m3 instead of just zed This follows the standard naming convention. --- custom-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/custom-requirements.txt b/custom-requirements.txt index ce59fc2035..25a35fb7c4 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,5 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@zed#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@stable/zed-m3#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From 01acefe2b65aad5b624ace6c0127e9f9b58524d6 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Thu, 23 Nov 2023 17:59:45 +0100 Subject: [PATCH 41/63] sentry: ignore Unauthorized exceptions They are pretty common and do not require attention of a developer Change-Id: I1479372579ef745bc0bc2c936f6d234cc775e757 --- keystone/server/flask/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index 781f381992..d2f362f5b1 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -189,7 +189,7 @@ def setup_app_middleware(app): # CCloud if os.environ.get('SENTRY_DSN', None): app.config['SENTRY_CONFIG'] = { - 'ignore_exceptions': [exception.NotFound], + 'ignore_exceptions': [exception.NotFound, exception.Unauthorized], } sentry = Sentry() From bdbe14b31195c3904f612b00900f85728bc144ae Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Sat, 25 Nov 2023 23:27:07 +0100 Subject: [PATCH 42/63] Switch custom dependencies to 2023.2 Change-Id: I7ed981a74e9f89327659fb46972e87b79951ae85 --- concourse_unit_test_task | 4 ++-- custom-requirements.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index fec1df6aa4..8dcb4744bf 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,11 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/zed-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2023.2-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ -git clone -b stable/zed-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ +git clone -b stable/2023.2-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py310 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" diff --git a/custom-requirements.txt b/custom-requirements.txt index 25a35fb7c4..fd42c99a42 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,5 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@stable/zed-m3#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@stable/2023.2-m3#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From f7a5a4b4a040209b939dd4392267260702892968 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Sat, 25 Nov 2023 23:49:26 +0100 Subject: [PATCH 43/63] Rename stestr exclude option in the test runner Change-Id: Iad1ef565c04d2a2d20a929c4ccd0bf4e7d492afc --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 8dcb4744bf..3819e4a265 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip3 install "tox<4" "six>=1.14.0" && \ /etc/init.d/memcached start && \ git clone -b stable/2023.2-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py310 -- --black-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +tox -e py310 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From d07225d81aac65550aa674d9910329c244a1ab8f Mon Sep 17 00:00:00 2001 From: rajiv Date: Mon, 29 Jul 2024 14:34:04 +0530 Subject: [PATCH 44/63] Try explicit declaration of crc<7.0.0 in concourse_unit_test_task --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index 3819e4a265..e34c88be65 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -4,7 +4,7 @@ export WATCHER_DISABLED=true && \ export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2023.2-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ -pip3 install "tox<4" "six>=1.14.0" && \ +pip3 install "tox<4" "six>=1.14.0" "crc<7.0.0" && \ /etc/init.d/memcached start && \ git clone -b stable/2023.2-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ From a6a22bded6e2729dae9af5c94847743185caeedd Mon Sep 17 00:00:00 2001 From: rajiv Date: Mon, 29 Jul 2024 16:58:21 +0530 Subject: [PATCH 45/63] Test tox with sapcc/requirements branch --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 2aeb051931..18f7307a5b 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,7 @@ setenv = # TODO(stephenfin): Remove once we bump our upper-constraint to SQLAlchemy 2.0 SQLALCHEMY_WARN_20=1 deps = - -c{env:TOX_CONSTRAINTS_FILE:https://releases.openstack.org/constraints/upper/2025.1} + -c{env:TOX_CONSTRAINTS_FILE:https://raw.githubusercontent.com/sapcc/requirements/stable/2025.1-m3/upper-constraints.txt} -r{toxinidir}/test-requirements.txt -r{toxinidir}/custom-requirements.txt .[ldap,memcache] From cb72099f3bc1914c490981f44860f97e6e4e8b6e Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Tue, 6 Aug 2024 17:27:07 +0200 Subject: [PATCH 46/63] Gracefully disallow reauth with app-creds-bound token Due to architectural reasons, bug https://bugs.launchpad.net/keystone/+bug/1878438 appeared. There is no good way to fix it, upstream also cannot get to it. If someone hits the bug, they get error 500 and keystone crashes. Fix this hard crash and return an Unauthorized response instead. This will not break any existing usecases, because things are not working already. This change should be reverted after upstream fixes the bug. Change-Id: I0d7802ddcdef7646f43fd57a0cf9ae94686d58e9 --- keystone/api/_shared/authentication.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/keystone/api/_shared/authentication.py b/keystone/api/_shared/authentication.py index e1839566f2..caa7e4aad1 100644 --- a/keystone/api/_shared/authentication.py +++ b/keystone/api/_shared/authentication.py @@ -231,7 +231,21 @@ def authenticate_for_token(auth=None): app_cred_id = None if 'application_credential' in method_names: token_auth = auth_info.auth['identity'] - app_cred_id = token_auth['application_credential']['id'] + try: + app_cred_id = token_auth['application_credential']['id'] + except KeyError: + # NOTE(bbobrov): see bug #1878438 on launchpad. + # There is no any good fix for it, at least with the current + # architecture. Lets just return a nice message that this + # is currently not supported. + LOG.warning( + 'Unsupported reauthentication attempt with a token ' + 'after authenticating with application credentials' + ) + raise exception.Unauthorized( + _('Cannot reauthenticate with a token issued with ' + 'application credentials') + ) # Do MFA Rule Validation for the user if not core.UserMFARulesValidator.check_auth_methods_against_rules( From d8f56ed519a797562331df2be8a48631ba78b191 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Wed, 7 Aug 2024 12:32:52 +0200 Subject: [PATCH 47/63] Disable rate-limiting middleware The ratelimiting middleware seems to bring more maintainance than use, which is why it should be disabled until we figure out how to properly set it up. Change-Id: If01714058982e64bb58bccf7cc853a22fc0c0ac7 --- keystone/server/flask/core.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index d2f362f5b1..df1c1ad86c 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -83,13 +83,13 @@ ep='lifesaver', conf={}), # CCloud: add rate_limit middleware - _Middleware(namespace='rate_limit.middleware', - ep='rate-limit', - conf={'config_file': '/etc/keystone/ratelimit.yaml', - 'service_type': 'identity', - 'rate_limit_by': 'initiator_project_id', - 'backend_host': 'keystone-sapcc-rate-limit', - 'backend_timeout_seconds': '1'}), + #_Middleware(namespace='rate_limit.middleware', + # ep='rate-limit', + # conf={'config_file': '/etc/keystone/ratelimit.yaml', + # 'service_type': 'identity', + # 'rate_limit_by': 'initiator_project_id', + # 'backend_host': 'keystone-sapcc-rate-limit', + # 'backend_timeout_seconds': '1'}), ) # NOTE(morgan): ORDER HERE IS IMPORTANT! Each of these middlewares are From 434d2c8cc3dd7c2edcb11f301b60a9fa148f9d23 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Fri, 22 Nov 2024 12:46:22 +0100 Subject: [PATCH 48/63] Switch dependencies to 2024.1 Change-Id: I915e3128ff02bde2f00fdefeafde3f6f46b04c5d --- concourse_unit_test_task | 4 ++-- custom-requirements.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index e34c88be65..ffef098d74 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,11 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2023.2-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2024.1-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ pip3 install "tox<4" "six>=1.14.0" "crc<7.0.0" && \ /etc/init.d/memcached start && \ -git clone -b stable/2023.2-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ +git clone -b stable/2024.1-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py310 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" diff --git a/custom-requirements.txt b/custom-requirements.txt index fd42c99a42..fbfa8f8ab8 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,5 +1,5 @@ git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@stable/2023.2-m3#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@stable/2024.1-m3#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From 5a55373fa89945d0365ef18342b77d6fb3e40b9f Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Thu, 12 Dec 2024 13:23:35 +0100 Subject: [PATCH 49/63] Sentry: sanitize extra keys Keystone uses many non-standard names for credentials and we need to explicitly list them. Change-Id: Icaaa785f0dd5fb25f3831aafe420b6db731574b1 --- keystone/server/flask/core.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index df1c1ad86c..baa400e064 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -188,8 +188,18 @@ def setup_app_middleware(app): # CCloud if os.environ.get('SENTRY_DSN', None): + processors = ( + 'raven.processors.SanitizePasswordsProcessor', + 'raven.processors.SanitizeKeysProcessor', + 'raven.processors.RemovePostDataProcessor', + ) + sanitize_keys = [ + 'old_password', 'new_password', 'password', 'cred', 'secret', + 'passwd', 'credentials'] app.config['SENTRY_CONFIG'] = { 'ignore_exceptions': [exception.NotFound, exception.Unauthorized], + 'processors': processors, + 'sanitize_keys': sanitize_keys, } sentry = Sentry() From d59bdbfefeb7b07414794849797f03a635ce13e2 Mon Sep 17 00:00:00 2001 From: Boris Bobrov Date: Thu, 12 Dec 2024 13:27:12 +0100 Subject: [PATCH 50/63] Sentry: do not log INVALID_CREDENTIALS These messages are too spammy and do not bring any value Change-Id: I13e4dbcc3333cb8e3154e5a463f2614a47dd70da --- keystone/server/flask/core.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index baa400e064..20ba581fc3 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -197,7 +197,8 @@ def setup_app_middleware(app): 'old_password', 'new_password', 'password', 'cred', 'secret', 'passwd', 'credentials'] app.config['SENTRY_CONFIG'] = { - 'ignore_exceptions': [exception.NotFound, exception.Unauthorized], + 'ignore_exceptions': [exception.NotFound, exception.Unauthorized, + 'INVALID_CREDENTIALS'], 'processors': processors, 'sanitize_keys': sanitize_keys, } From 421ebdacdcd20a26437c6770c3feb1b8164ede0c Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Tue, 15 Oct 2024 14:31:48 +0200 Subject: [PATCH 51/63] Support emitting partial hash of invalid password Add support of configuration allowing inclusion of partial hash of invalid password in event notifications to facilitate anyalysis of failed login attemps. SecurityImpact Related-Bug: 2060972 Depends-On: https://review.opendev.org/c/openstack/keystone-specs/+/915482 Closes-Bug: 2060972 Change-Id: I0f34d90660a4a915c9c3f9512dc6d794b8415cd5 --- keystone/common/password_hashing.py | 33 ++++ keystone/conf/security_compliance.py | 77 +++++++++ keystone/notifications.py | 45 ++++- .../tests/unit/common/test_notifications.py | 142 +++++++++++++++- .../unit/common/test_password_hashing.py | 157 ++++++++++++++++++ ...d-password-reporting-975955d2d79c21b3.yaml | 10 ++ 6 files changed, 461 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/pci-dss-invalid-password-reporting-975955d2d79c21b3.yaml diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index 1554f4ecd9..e7bc929712 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import base64 +import hmac import itertools from oslo_log import log @@ -178,3 +180,34 @@ def hash_password(password: str) -> str: _('Password Hash Algorithm %s not implemented') % CONF.identity.password_hash_algorithm ) + +def generate_partial_password_hash(password: str, salt: str) -> str: + """Generates partial password hash for reporting purposes. + + The generated password hash is base64 encoded, and `max_chars` of it are + returned. + """ + secret_key = CONF.security_compliance.invalid_password_hash_secret_key + if secret_key is None: + raise RuntimeError(_('Secret Key value has to be provided')) + + hash_function = CONF.security_compliance.invalid_password_hash_function + + salted = hmac.digest( + key=bytes(salt, "utf-8"), + msg=bytes(password, "utf-8"), + digest=hash_function, + ) + + peppered = hmac.digest( + key=bytes(secret_key, "utf-8"), msg=salted, digest=hash_function + ) + + # encode to utilize more characters to reduce collisions when further + # truncating + encoded = base64.b64encode(peppered).decode("utf-8").rstrip("=") + + max_chars = CONF.security_compliance.invalid_password_hash_max_chars + if max_chars is None: + return encoded + return encoded[:max_chars] diff --git a/keystone/conf/security_compliance.py b/keystone/conf/security_compliance.py index 50bf809164..4e075a1bbc 100644 --- a/keystone/conf/security_compliance.py +++ b/keystone/conf/security_compliance.py @@ -150,6 +150,79 @@ ), ) +report_invalid_password_hash = cfg.ListOpt( + 'report_invalid_password_hash', + default="", + sample_default="event", + item_type=cfg.types.String( + choices=[ + ( + "event", + utils.fmt(""" +Enriches `identity.authenticate.failure` event notifications with partial +invalid password hash +"""), + ) + # ("log", "description"), + ] + ), + help=utils.fmt( + """ +When configured, enriches the corresponding output channel with hash of invalid +password, which could be further used to distinguish bruteforce attacks from +e.g. external user automations that did not timely update rotated password by +analyzing variability of the hash value. +Additional configuration parameters are available using other +`invalid_password_hash_*` configuration entires, that only take effect when +this option is activated. +""" + ), +) + +invalid_password_hash_secret_key = cfg.StrOpt( + 'invalid_password_hash_secret_key', + secret=True, + help=utils.fmt( + """ +If `report_invalid_password_hash` is configured, uses provided secret key when +generating password hashes to make them unique and distinct from any other +Keystone installations out there. Should be some secret static value specific +to the current installation (the same value should be used in distributed +installations working with the same backend, to make them all generate equal +hashes for equal invalid passwords). 16 bytes (128 bits) or more is +recommended. +""" + ), +) + +invalid_password_hash_function = cfg.StrOpt( + 'invalid_password_hash_function', + default='sha256', + help=utils.fmt( + """ +If `report_invalid_password_hash` is configured, defines the hash function to +be used by HMAC. Possible values are names suitable to hashlib.new() - +https://docs.python.org/3/library/hashlib.html#hashlib.new. +""" + ), +) + +invalid_password_hash_max_chars = cfg.IntOpt( + 'invalid_password_hash_max_chars', + min=1, + sample_default=5, + help=utils.fmt( + """ +If `report_invalid_password_hash` is configured, defines the number of +characters of hash of invalid password to be returned. When not specified, +returns full hash. Its length depends on implementation and +`invalid_password_hash_function` configuration, but is typically 16+ +characters. It's recommended to use the least reasonable value however - it's +the most effective measure to protect the hashes. +""" + ), +) + GROUP_NAME = __name__.split('.')[-1] ALL_OPTS = [ @@ -162,6 +235,10 @@ password_regex, password_regex_description, change_password_upon_first_use, + report_invalid_password_hash, + invalid_password_hash_secret_key, + invalid_password_hash_function, + invalid_password_hash_max_chars, ] diff --git a/keystone/notifications.py b/keystone/notifications.py index 8cfd068e8c..bf8d70062a 100644 --- a/keystone/notifications.py +++ b/keystone/notifications.py @@ -24,6 +24,7 @@ import oslo_messaging from oslo_utils import reflection import pycadf +from pycadf import attachment from pycadf import cadftaxonomy as taxonomy from pycadf import cadftype from pycadf import credential @@ -33,6 +34,7 @@ from pycadf import resource from keystone.common import context +from keystone.common import password_hashing from keystone.common import provider_api from keystone.common import utils import keystone.conf @@ -731,7 +733,40 @@ def wrapper(wrapped_self, user_id, *args, **kwargs): if isinstance(ex, exception.AccountLocked): raise exception.Unauthorized raise - except Exception: + except Exception as ex: + audit_kwargs = {} + + if ( + isinstance(ex, AssertionError) + and kwargs.get("password") is not None + and "event" + in CONF.security_compliance.report_invalid_password_hash + ): + # Authentication failed because of invalid password, so we + # include partial pasword hash to aid bruteforce attacks + # recognition + partial_password_hash = ( + password_hashing.generate_partial_password_hash( + kwargs["password"], + # use fully qualified class name of the driver as + # personalization salt causing to produce different + # hashes for different backends even when the same + # invalid password is submitted + salt=reflection.get_class_name( + wrapped_self.driver + ), + ) + ) + attachments = audit_kwargs.get("attachments", []) + attachments.append( + attachment.Attachment( + typeURI="mime:text/plain", + content=partial_password_hash, + name="partial_password_hash", + ) + ) + audit_kwargs["attachments"] = attachments + # For authentication failure send a CADF event as well _send_audit_notification( self.action, @@ -739,6 +774,7 @@ def wrapper(wrapped_self, user_id, *args, **kwargs): taxonomy.OUTCOME_FAILURE, target, self.event_type, + **audit_kwargs, ) raise else: @@ -912,7 +948,7 @@ class _CatalogHelperObj(provider_api.ProviderAPIMixin): def _send_audit_notification( - action, initiator, outcome, target, event_type, reason=None, **kwargs + action, initiator, outcome, target, event_type, reason=None, attachments=None, **kwargs ): """Send CADF notification to inform observers about the affected resource. @@ -929,6 +965,7 @@ def _send_audit_notification( key-value pairs to the CADF event. :param reason: Reason for the notification which contains the response code and message description + :param attachments: Array of Attachment objects """ if _check_notification_opt_out(event_type, outcome): return @@ -959,6 +996,10 @@ def _send_audit_notification( if service_id is not None: event.observer.id = service_id + attachments = attachments if attachments is not None else [] + for attachment_val in attachments: + event.add_attachment(attachment_val) + for key, value in kwargs.items(): setattr(event, key, value) diff --git a/keystone/tests/unit/common/test_notifications.py b/keystone/tests/unit/common/test_notifications.py index 99cf5df662..c2c3461167 100644 --- a/keystone/tests/unit/common/test_notifications.py +++ b/keystone/tests/unit/common/test_notifications.py @@ -25,6 +25,7 @@ from oslo_utils import timeutils from pycadf import cadftaxonomy from pycadf import cadftype +from pycadf import event as cadfevent from pycadf import eventfactory from pycadf import resource as cadfresource @@ -301,6 +302,7 @@ def fake_audit( target, event_type, reason=None, + attachments=None, **kwargs, ): service_security = cadftaxonomy.SERVICE_SECURITY @@ -315,6 +317,10 @@ def fake_audit( observer=cadfresource.Resource(typeURI=service_security), ) + attachments = attachments if attachments is not None else [] + for attachment_val in attachments: + event.add_attachment(attachment_val) + for key, value in kwargs.items(): setattr(event, key, value) @@ -358,7 +364,7 @@ def _assert_last_note( self.assertEqual(actor_operation, note['actor_operation']) def _assert_last_audit( - self, resource_id, operation, resource_type, target_uri, reason=None + self, resource_id, operation, resource_type, target_uri, reason=None, attachments=None ): # NOTE(stevemar): If 'cadf' format is not used, then simply # return since this assertion is not valid. @@ -386,6 +392,15 @@ def _assert_last_audit( reason['reasonType'], payload['reason']['reasonType'] ) self.assertTrue(audit['send_notification_called']) + if attachments is None: + self.assertNotIn(cadfevent.EVENT_KEYNAME_ATTACHMENTS, payload) + else: + self.assertIn(cadfevent.EVENT_KEYNAME_ATTACHMENTS, payload) + for attachment_val in attachments: + self.assertIn( + attachment_val, + payload.get(cadfevent.EVENT_KEYNAME_ATTACHMENTS), + ) def _assert_initiator_data_is_set(self, operation, resource_type, typeURI): self.assertGreater(len(self._audits), 0) @@ -1144,6 +1159,131 @@ def test_changing_password_too_early_sends_notification(self): reason=expected_reason, ) + def test_valid_password_not_hashed_by_default(self): + password = uuid.uuid4().hex + user_ref = unit.new_user_ref( + domain_id=self.domain_id, password=password + ) + user_ref = PROVIDERS.identity_api.create_user(user_ref) + + with mock.patch( + 'keystone.common.password_hashing.generate_partial_password_hash' + ) as mocked: + with self.make_request(): + PROVIDERS.identity_api.authenticate(user_ref['id'], password) + mocked.assert_not_called() + + self._assert_last_audit( + None, + 'authenticate', + None, + cadftaxonomy.ACCOUNT_USER, + attachments=None, + ) + + def test_invalid_password_not_hashed_by_default(self): + password = uuid.uuid4().hex + invalid_password = 'invalid_' + password + user_ref = unit.new_user_ref( + domain_id=self.domain_id, password=password + ) + user_ref = PROVIDERS.identity_api.create_user(user_ref) + + with mock.patch( + 'keystone.common.password_hashing.generate_partial_password_hash' + ) as mocked: + with self.make_request(): + self.assertRaises( + AssertionError, + PROVIDERS.identity_api.authenticate, + user_id=user_ref['id'], + password=invalid_password, + ) + mocked.assert_not_called() + + self._assert_last_audit( + None, + 'authenticate', + None, + cadftaxonomy.ACCOUNT_USER, + attachments=None, + ) + + def test_valid_password_hash_not_included_when_report_in_event(self): + password = uuid.uuid4().hex + user_ref = unit.new_user_ref( + domain_id=self.domain_id, password=password + ) + user_ref = PROVIDERS.identity_api.create_user(user_ref) + + conf = self.useFixture(config_fixture.Config(CONF)) + conf.config( + group='security_compliance', report_invalid_password_hash='event' + ) + conf.config( + group='security_compliance', + invalid_password_hash_secret_key='secret_key', + ) + with mock.patch( + 'keystone.common.password_hashing.generate_partial_password_hash' + ) as mocked: + with self.make_request(): + PROVIDERS.identity_api.authenticate(user_ref['id'], password) + mocked.assert_not_called() + + self._assert_last_audit( + None, + 'authenticate', + None, + cadftaxonomy.ACCOUNT_USER, + attachments=None, + ) + + def test_invalid_password_hash_included_when_report_in_event(self): + password = uuid.uuid4().hex + invalid_password = 'invalid_' + password + user_ref = unit.new_user_ref( + domain_id=self.domain_id, password=password + ) + user_ref = PROVIDERS.identity_api.create_user(user_ref) + + conf = self.useFixture(config_fixture.Config(CONF)) + conf.config( + group='security_compliance', report_invalid_password_hash='event' + ) + conf.config( + group='security_compliance', + invalid_password_hash_secret_key='secret_key', + ) + hashed_invalid_password = 'hashed_' + invalid_password + with mock.patch( + 'keystone.common.password_hashing.generate_partial_password_hash', + return_value=hashed_invalid_password, + ) as mocked: + with self.make_request(): + self.assertRaises( + AssertionError, + PROVIDERS.identity_api.authenticate, + user_id=user_ref['id'], + password=invalid_password, + ) + + mocked.assert_called_once() + + self._assert_last_audit( + None, + 'authenticate', + None, + cadftaxonomy.ACCOUNT_USER, + attachments=[ + { + 'content': hashed_invalid_password, + 'name': "partial_password_hash", + 'typeURI': "mime:text/plain", + } + ], + ) + class CADFNotificationsForEntities(NotificationsForEntities): def setUp(self): diff --git a/keystone/tests/unit/common/test_password_hashing.py b/keystone/tests/unit/common/test_password_hashing.py index 2c242f910c..ea31621669 100644 --- a/keystone/tests/unit/common/test_password_hashing.py +++ b/keystone/tests/unit/common/test_password_hashing.py @@ -90,3 +90,160 @@ def test_pbkdf2_sha512(self): ) hashed = password_hashing.hash_password(password) self.assertTrue(password_hashing.check_password(password, hashed)) + +class TestGeneratePartialPasswordHash(unit.BaseTestCase): + def setUp(self): + super().setUp() + + self.h = password_hashing.generate_partial_password_hash + + self.config_fixture = self.useFixture(config_fixture.Config(CONF)) + self.config_fixture.config( + group="security_compliance", + invalid_password_hash_secret_key="secret_key", + ) + + def test_equal_input_generates_equal_hash(self): + args1 = ("password", "salt") + args2 = ("password", "salt") + + hashed1 = self.h(*args1) + hashed2 = self.h(*args2) + + self.assertEqual(hashed1, hashed2) + + def test_different_inputs_generate_different_hashes(self): + params = [ + { + "msg": "Different passwords", + "args1": ("password1", "salt"), + "args2": ("password2", "salt"), + }, + { + "msg": "Different salts", + "args1": ("password", "salt1"), + "args2": ("password", "salt2"), + }, + { + "msg": "Same but mixed inputs", + "args1": ("password", "salt"), + "args2": ("salt", "password"), + }, + ] + + # test variability of: a) full hash; b) partial hash + max_chars_conf = [None, 5] + for data in params: + for max_chars in max_chars_conf: + msg = data["msg"] + f" ({max_chars=})" + self.config_fixture.config( + group="security_compliance", + invalid_password_hash_max_chars=max_chars, + ) + with self.subTest(msg=msg): + hashed1 = self.h(*data["args1"]) + hashed2 = self.h(*data["args2"]) + self.assertNotEqual(hashed1, hashed2, msg=msg) + + def test_generates_full_hash_by_default(self): + args1 = ("password", "salt") + args2 = ("password", "salt") + + # when `max_chars` is not specified, a default value will be used, + # which is expected to return full hash + hashed1 = self.h(*args1) + + self.config_fixture.config( + group="security_compliance", + # 1000 is larger than any supported hash function key length + + # base64 encoding, so it should cause full hash to be returned + invalid_password_hash_max_chars=1000, + ) + hashed2 = self.h(*args2) + + self.assertEqual(len(hashed1), len(hashed2)) + + def test_invalid_function_raises_value_error(self): + args = ("password", "salt") + self.config_fixture.config( + group="security_compliance", + invalid_password_hash_function="invalid", + ) + + self.assertRaises(ValueError, self.h, *args) + + def test_large_passwords(self): + # 48042 bytes or 48 kB + args1 = ("p" * 1000 * 48 + "1", "salt") + args2 = ("p" * 1000 * 48 + "2", "salt") + + # test variability of: a) full hash; b) partial hash + max_chars_conf = [None, 5] + for max_chars in max_chars_conf: + msg = f"Large password ({max_chars=})" + self.config_fixture.config( + group="security_compliance", + invalid_password_hash_max_chars=max_chars, + ) + with self.subTest(msg=msg): + hashed1 = self.h(*args1) + hashed2 = self.h(*args2) + self.assertNotEqual(hashed1, hashed2, msg=msg) + + def test_different_conf_generates_different_hashes(self): + args = ("password", "salt") + + params = [ + { + "msg": "Different secret_keys", + "conf1": {"invalid_password_hash_secret_key": "secret_key1"}, + "conf2": {"invalid_password_hash_secret_key": "secret_key2"}, + }, + { + "msg": "Different max chars", + "conf1": {"invalid_password_hash_max_chars": 3}, + "conf2": {"invalid_password_hash_max_chars": 4}, + }, + { + "msg": "Different hash functions", + "conf1": {"invalid_password_hash_function": "sha3_512"}, + "conf2": {"invalid_password_hash_function": "sha512"}, + }, + ] + + for data in params: + with self.subTest(msg=data["msg"]): + self.config_fixture.config( + group="security_compliance", **data["conf1"] + ) + hashed1 = self.h(*args) + self.config_fixture.config( + group="security_compliance", **data["conf2"] + ) + hashed2 = self.h(*args) + self.assertNotEqual(hashed1, hashed2, msg=data["msg"]) + + def test_conf_secret_key_is_required(self): + self.config_fixture.config( + group="security_compliance", invalid_password_hash_secret_key=None + ) + + args = ("password", "salt") + + self.assertRaises(RuntimeError, self.h, *args) + + def test_returns_not_more_than_max_chars(self): + args = ("password", "salt") + + max_chars_conf = [1, 5, 1000] + for max_chars in max_chars_conf: + self.config_fixture.config( + group="security_compliance", + invalid_password_hash_max_chars=max_chars, + ) + with self.subTest(msg=max_chars): + hashed = self.h(*args) + + self.assertLessEqual( + len(hashed), max_chars, msg=f"{max_chars=}" + ) diff --git a/releasenotes/notes/pci-dss-invalid-password-reporting-975955d2d79c21b3.yaml b/releasenotes/notes/pci-dss-invalid-password-reporting-975955d2d79c21b3.yaml new file mode 100644 index 0000000000..2e15cf7d42 --- /dev/null +++ b/releasenotes/notes/pci-dss-invalid-password-reporting-975955d2d79c21b3.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + `bug 2060972 `_ + Added new configuration option ``[security_compliance] + report_invalid_password_hash`` to enable and configure reporting of hashes + of submitted invalid passwords, which could be used to facilitate analysis + of failed login attempts (as per PCI DSS requirements). The corresponding + Keystone specification - `Include invalid password details in audit + messages `_. From 0c42e0c68f37682badb019962323d4c1cf4d8b31 Mon Sep 17 00:00:00 2001 From: Vladislav Gusev Date: Mon, 14 Jul 2025 14:57:26 +0300 Subject: [PATCH 52/63] Add python-binary-memcached package Install the python-binary-memcached package into the Keystone image SALS works only over a binary protocol, and it can only work with `dogpile.cache.bmemcached` backend, which requires pip package `python-binary-memcached` to be present. --- custom-requirements.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/custom-requirements.txt b/custom-requirements.txt index fbfa8f8ab8..33936bab49 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -1,3 +1,5 @@ +python-binary-memcached + git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius From 16af34e32f3b360c87c359ff04b8b40fa8fa4b8f Mon Sep 17 00:00:00 2001 From: Johannes Kulik Date: Thu, 17 Jul 2025 09:11:35 +0200 Subject: [PATCH 53/63] Fix pep8 errors. We also set `nosec` for one use of `random.sample()` to make `bandit` - which runs as part of `tox -e pep8` - happy. Upstream has that same line in _without_ `nosec` and I don't get why pep8 tests work for them. Change-Id: Id26894f2da2877c2455e02a6d3290bccb2b6decd --- keystone/common/fernet_utils.py | 3 ++- keystone/common/password_hashing.py | 1 + keystone/common/render_token.py | 3 ++- keystone/conf/resource.py | 4 ++-- keystone/exception.py | 6 ++++-- keystone/identity/backends/ldap/common.py | 3 ++- keystone/oauth1/backends/sql.py | 2 +- keystone/server/flask/core.py | 20 +++++++++++--------- keystone/tests/unit/test_v3_resource.py | 2 +- 9 files changed, 26 insertions(+), 18 deletions(-) diff --git a/keystone/common/fernet_utils.py b/keystone/common/fernet_utils.py index 5b731076b7..b3f051fe91 100644 --- a/keystone/common/fernet_utils.py +++ b/keystone/common/fernet_utils.py @@ -61,7 +61,8 @@ def validate_key_repository(self, requires_write=False): 'config_group': self.config_group, }, ) - # CCloud: we don't care, since k8s volumes from secrets can't be configured to be read-only + # CCloud: we don't care, since k8s volumes from secrets can't be + # configured to be read-only # else: # # ensure the key repository isn't world-readable # stat_info = os.stat(self.key_repository) diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index e7bc929712..ec8ccd4afd 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -181,6 +181,7 @@ def hash_password(password: str) -> str: % CONF.identity.password_hash_algorithm ) + def generate_partial_password_hash(password: str, salt: str) -> str: """Generates partial password hash for reporting purposes. diff --git a/keystone/common/render_token.py b/keystone/common/render_token.py index 23ed898194..0d8876c228 100644 --- a/keystone/common/render_token.py +++ b/keystone/common/render_token.py @@ -97,7 +97,8 @@ def render_token_response_from_model(token, include_catalog=True): # ccloud: additional bootstrap project support bootstrap_name = CONF.resource.bootstrap_project_name bootstrap_domain_name = CONF.resource.bootstrap_project_domain_name - if bootstrap_name and bootstrap_domain_name and not token_reference['token'].get('is_admin_project', False): + if bootstrap_name and bootstrap_domain_name and \ + not token_reference['token'].get('is_admin_project', False): is_ap = ( token.project['name'] == bootstrap_name and bootstrap_domain_name == token.project_domain['name'] diff --git a/keystone/conf/resource.py b/keystone/conf/resource.py index 6beee9cbbc..332380d54f 100644 --- a/keystone/conf/resource.py +++ b/keystone/conf/resource.py @@ -90,8 +90,8 @@ 'bootstrap_project_domain_name', help=utils.fmt(""" Name of the domain that owns the `bootstrap_project_name`. If left unset, then -there is no admin project. `[resource] bootstrap_project_name` must also be set to -use this option. +there is no admin project. `[resource] bootstrap_project_name` must also be set +to use this option. """) ) diff --git a/keystone/exception.py b/keystone/exception.py index dcb4d56a06..707749aecd 100644 --- a/keystone/exception.py +++ b/keystone/exception.py @@ -70,7 +70,8 @@ class Error(Exception, metaclass=_KeystoneExceptionMeta): code: ty.Optional[int] = None title: ty.Optional[str] = None message_format: ty.Optional[str] = None - # ccloud: keep the original message for logging, regardless of insecure_debug setting + # ccloud: keep the original message for logging, regardless of + # insecure_debug setting message = None def __init__(self, message=None, **kwargs): @@ -299,7 +300,8 @@ def __deepcopy__(self): def _build_message(self, message, **kwargs): """Only returns detailed messages in insecure_debug mode.""" - # ccloud: always keep original message for logging, regardless of insecure_debug setting + # ccloud: always keep original message for logging, regardless of + # insecure_debug setting if message: self.message = message diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 88842b3daf..49371e8e22 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -1837,7 +1837,8 @@ def _ldap_get_all(self, hints, ldap_filter=None): + list(self.extra_attr_mapping.keys()) ) ) - # ccloud: only limit if all filters have been satisfied and the contoller does not need to filter + # ccloud: only limit if all filters have been satisfied and the + # contoller does not need to filter if hints.limit and not len(hints.filters): sizelimit = hints.limit['limit'] res = self._ldap_get_limited( diff --git a/keystone/oauth1/backends/sql.py b/keystone/oauth1/backends/sql.py index 03677e1392..69ca79a26e 100644 --- a/keystone/oauth1/backends/sql.py +++ b/keystone/oauth1/backends/sql.py @@ -207,7 +207,7 @@ def authorize_request_token(self, request_token_id, user_id, role_ids): token_dict['authorizing_user_id'] = user_id token_dict['verifier'] = ''.join( random.sample(base.VERIFIER_CHARS, 8) - ) + ) # nosec token_dict['role_ids'] = jsonutils.dumps(role_ids) new_token = RequestToken.from_dict(token_dict) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index 20ba581fc3..8f14a15986 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -69,7 +69,8 @@ ), ) -# ccloud: additional ccloud specific middleware that needs to sit 'behind' the AuthContextMiddleware +# ccloud: additional ccloud specific middleware that needs to sit 'behind' the +# AuthContextMiddleware _CC_MIDDLEWARE = ( # CCloud: add watcher middleware _Middleware(namespace='watcher.middleware', @@ -83,13 +84,13 @@ ep='lifesaver', conf={}), # CCloud: add rate_limit middleware - #_Middleware(namespace='rate_limit.middleware', - # ep='rate-limit', - # conf={'config_file': '/etc/keystone/ratelimit.yaml', - # 'service_type': 'identity', - # 'rate_limit_by': 'initiator_project_id', - # 'backend_host': 'keystone-sapcc-rate-limit', - # 'backend_timeout_seconds': '1'}), + # _Middleware(namespace='rate_limit.middleware', + # ep='rate-limit', + # conf={'config_file': '/etc/keystone/ratelimit.yaml', + # 'service_type': 'identity', + # 'rate_limit_by': 'initiator_project_id', + # 'backend_host': 'keystone-sapcc-rate-limit', + # 'backend_timeout_seconds': '1'}), ) # NOTE(morgan): ORDER HERE IS IMPORTANT! Each of these middlewares are @@ -151,7 +152,8 @@ def setup_app_middleware(app): if mw.ep == 'watcher' and os.environ.get('WATCHER_DISABLED', None): continue - loaded = stevedore.DriverManager(mw.namespace, mw.ep, invoke_on_load=False) + loaded = stevedore.DriverManager(mw.namespace, mw.ep, + invoke_on_load=False) factory_func = loaded.driver.factory({}, **mw.conf) app.wsgi_app = factory_func(app.wsgi_app) diff --git a/keystone/tests/unit/test_v3_resource.py b/keystone/tests/unit/test_v3_resource.py index 2c2c89ef90..1141362367 100644 --- a/keystone/tests/unit/test_v3_resource.py +++ b/keystone/tests/unit/test_v3_resource.py @@ -1398,7 +1398,7 @@ def test_list_project_is_domain_filter(self): ) def test_list_projects_is_domain_filter_domain_scoped_token(self): - """Call ``GET /projects?is_domain=True/False`` with domain scope""" + """Call ``GET /projects?is_domain=True/False`` with domain scope.""" # grant the domain role to user path = '/domains/%s/users/%s/roles/%s' % ( self.domain_id, self.user['id'], self.role['id']) From 1d8343619e9c78f9651d7419a14ec3bc1a39781f Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Wed, 26 Nov 2025 09:21:03 +0100 Subject: [PATCH 54/63] Commit pep8 suggestions --- keystone/api/_shared/authentication.py | 6 ++- keystone/api/projects.py | 3 +- keystone/cmd/doctor/tokens.py | 2 +- keystone/common/render_token.py | 11 ++-- keystone/conf/default.py | 3 +- keystone/conf/resource.py | 4 +- keystone/notifications.py | 9 +++- keystone/oauth1/backends/sql.py | 2 +- keystone/server/flask/core.py | 42 +++++++++------ .../tests/unit/common/test_notifications.py | 8 ++- .../unit/common/test_password_hashing.py | 1 + keystone/tests/unit/test_v3_resource.py | 54 +++++++++++-------- 12 files changed, 93 insertions(+), 52 deletions(-) diff --git a/keystone/api/_shared/authentication.py b/keystone/api/_shared/authentication.py index caa7e4aad1..edbd48129c 100644 --- a/keystone/api/_shared/authentication.py +++ b/keystone/api/_shared/authentication.py @@ -243,8 +243,10 @@ def authenticate_for_token(auth=None): 'after authenticating with application credentials' ) raise exception.Unauthorized( - _('Cannot reauthenticate with a token issued with ' - 'application credentials') + _( + 'Cannot reauthenticate with a token issued with ' + 'application credentials' + ) ) # Do MFA Rule Validation for the user diff --git a/keystone/api/projects.py b/keystone/api/projects.py index 4317eced02..1f46397988 100644 --- a/keystone/api/projects.py +++ b/keystone/api/projects.py @@ -195,7 +195,8 @@ def get(self): # requested; otherwise all domains are getting filtered out try: is_domain_requested = utils.attr_as_boolean( - flask.request.args['is_domain']) + flask.request.args['is_domain'] + ) except KeyError: is_domain_requested = False diff --git a/keystone/cmd/doctor/tokens.py b/keystone/cmd/doctor/tokens.py index 74f177bfad..cd62a22808 100644 --- a/keystone/cmd/doctor/tokens.py +++ b/keystone/cmd/doctor/tokens.py @@ -33,4 +33,4 @@ def symptom_unreasonable_max_token_size(): """ # return 'fernet' in CONF.token.provider and CONF.max_token_size > 255 # CCloud: we are using ldap, hence the max_token_size needs to go up. - return ('fernet' in CONF.token.provider and CONF.max_token_size > 268) + return 'fernet' in CONF.token.provider and CONF.max_token_size > 268 diff --git a/keystone/common/render_token.py b/keystone/common/render_token.py index 0d8876c228..06d511830f 100644 --- a/keystone/common/render_token.py +++ b/keystone/common/render_token.py @@ -97,11 +97,14 @@ def render_token_response_from_model(token, include_catalog=True): # ccloud: additional bootstrap project support bootstrap_name = CONF.resource.bootstrap_project_name bootstrap_domain_name = CONF.resource.bootstrap_project_domain_name - if bootstrap_name and bootstrap_domain_name and \ - not token_reference['token'].get('is_admin_project', False): + if ( + bootstrap_name + and bootstrap_domain_name + and not token_reference['token'].get('is_admin_project', False) + ): is_ap = ( - token.project['name'] == bootstrap_name and - bootstrap_domain_name == token.project_domain['name'] + token.project['name'] == bootstrap_name + and bootstrap_domain_name == token.project_domain['name'] ) token_reference['token']['is_admin_project'] = is_ap if include_catalog and not token.unscoped: diff --git a/keystone/conf/default.py b/keystone/conf/default.py index 2dfbfed95e..035317f816 100644 --- a/keystone/conf/default.py +++ b/keystone/conf/default.py @@ -156,7 +156,8 @@ order to set multiple default tags, for example: default_tag=tag_0 default_tag=tag_1 -""")) +"""), +) notification_format = cfg.StrOpt( 'notification_format', diff --git a/keystone/conf/resource.py b/keystone/conf/resource.py index 332380d54f..471ae190ad 100644 --- a/keystone/conf/resource.py +++ b/keystone/conf/resource.py @@ -92,7 +92,7 @@ Name of the domain that owns the `bootstrap_project_name`. If left unset, then there is no admin project. `[resource] bootstrap_project_name` must also be set to use this option. -""") +"""), ) bootstrap_project_name = cfg.StrOpt( @@ -105,7 +105,7 @@ If left unset, then there is no admin project, and thus no explicit means of cross-project role assignments. `[resource] bootstrap_project_domain_name` must also be set to use this option. -""") +"""), ) project_name_url_safe = cfg.StrOpt( diff --git a/keystone/notifications.py b/keystone/notifications.py index bf8d70062a..de66ce14db 100644 --- a/keystone/notifications.py +++ b/keystone/notifications.py @@ -948,7 +948,14 @@ class _CatalogHelperObj(provider_api.ProviderAPIMixin): def _send_audit_notification( - action, initiator, outcome, target, event_type, reason=None, attachments=None, **kwargs + action, + initiator, + outcome, + target, + event_type, + reason=None, + attachments=None, + **kwargs, ): """Send CADF notification to inform observers about the affected resource. diff --git a/keystone/oauth1/backends/sql.py b/keystone/oauth1/backends/sql.py index 69ca79a26e..a3b9b96339 100644 --- a/keystone/oauth1/backends/sql.py +++ b/keystone/oauth1/backends/sql.py @@ -207,7 +207,7 @@ def authorize_request_token(self, request_token_id, user_id, role_ids): token_dict['authorizing_user_id'] = user_id token_dict['verifier'] = ''.join( random.sample(base.VERIFIER_CHARS, 8) - ) # nosec + ) # nosec token_dict['role_ids'] = jsonutils.dumps(role_ids) new_token = RequestToken.from_dict(token_dict) diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index 8f14a15986..afbed42468 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -73,16 +73,18 @@ # AuthContextMiddleware _CC_MIDDLEWARE = ( # CCloud: add watcher middleware - _Middleware(namespace='watcher.middleware', - ep='watcher', - conf={'service_type': 'identity', - 'config_file': '/etc/keystone/watcher.yaml', - 'include_initiator_user_id_in_metric': 'true', - 'include_target_project_id_in_metric': 'false'}), + _Middleware( + namespace='watcher.middleware', + ep='watcher', + conf={ + 'service_type': 'identity', + 'config_file': '/etc/keystone/watcher.yaml', + 'include_initiator_user_id_in_metric': 'true', + 'include_target_project_id_in_metric': 'false', + }, + ), # CCloud: add lifesaver middleware - _Middleware(namespace='lifesaver.middleware', - ep='lifesaver', - conf={}), + _Middleware(namespace='lifesaver.middleware', ep='lifesaver', conf={}), # CCloud: add rate_limit middleware # _Middleware(namespace='rate_limit.middleware', # ep='rate-limit', @@ -152,8 +154,9 @@ def setup_app_middleware(app): if mw.ep == 'watcher' and os.environ.get('WATCHER_DISABLED', None): continue - loaded = stevedore.DriverManager(mw.namespace, mw.ep, - invoke_on_load=False) + loaded = stevedore.DriverManager( + mw.namespace, mw.ep, invoke_on_load=False + ) factory_func = loaded.driver.factory({}, **mw.conf) app.wsgi_app = factory_func(app.wsgi_app) @@ -196,11 +199,20 @@ def setup_app_middleware(app): 'raven.processors.RemovePostDataProcessor', ) sanitize_keys = [ - 'old_password', 'new_password', 'password', 'cred', 'secret', - 'passwd', 'credentials'] + 'old_password', + 'new_password', + 'password', + 'cred', + 'secret', + 'passwd', + 'credentials', + ] app.config['SENTRY_CONFIG'] = { - 'ignore_exceptions': [exception.NotFound, exception.Unauthorized, - 'INVALID_CREDENTIALS'], + 'ignore_exceptions': [ + exception.NotFound, + exception.Unauthorized, + 'INVALID_CREDENTIALS', + ], 'processors': processors, 'sanitize_keys': sanitize_keys, } diff --git a/keystone/tests/unit/common/test_notifications.py b/keystone/tests/unit/common/test_notifications.py index c2c3461167..ac932648db 100644 --- a/keystone/tests/unit/common/test_notifications.py +++ b/keystone/tests/unit/common/test_notifications.py @@ -364,7 +364,13 @@ def _assert_last_note( self.assertEqual(actor_operation, note['actor_operation']) def _assert_last_audit( - self, resource_id, operation, resource_type, target_uri, reason=None, attachments=None + self, + resource_id, + operation, + resource_type, + target_uri, + reason=None, + attachments=None, ): # NOTE(stevemar): If 'cadf' format is not used, then simply # return since this assertion is not valid. diff --git a/keystone/tests/unit/common/test_password_hashing.py b/keystone/tests/unit/common/test_password_hashing.py index ea31621669..c592cd6366 100644 --- a/keystone/tests/unit/common/test_password_hashing.py +++ b/keystone/tests/unit/common/test_password_hashing.py @@ -91,6 +91,7 @@ def test_pbkdf2_sha512(self): hashed = password_hashing.hash_password(password) self.assertTrue(password_hashing.check_password(password, hashed)) + class TestGeneratePartialPasswordHash(unit.BaseTestCase): def setUp(self): super().setUp() diff --git a/keystone/tests/unit/test_v3_resource.py b/keystone/tests/unit/test_v3_resource.py index 1141362367..1f0c2b8db7 100644 --- a/keystone/tests/unit/test_v3_resource.py +++ b/keystone/tests/unit/test_v3_resource.py @@ -863,12 +863,9 @@ def _create_project_and_tags(self, num_of_tags=1, with_default_tag=False): tags.pop() # add default tag instead tags += _DEFAULT_TAG - ref = unit.new_project_ref( - domain_id=self.domain_id, - tags=tags) + ref = unit.new_project_ref(domain_id=self.domain_id, tags=tags) else: - ref = unit.new_project_without_tags_ref( - domain_id=self.domain_id) + ref = unit.new_project_without_tags_ref(domain_id=self.domain_id) resp = self.post('/projects', body={'project': ref}) return resp.result['project'], tags @@ -1400,36 +1397,46 @@ def test_list_project_is_domain_filter(self): def test_list_projects_is_domain_filter_domain_scoped_token(self): """Call ``GET /projects?is_domain=True/False`` with domain scope.""" # grant the domain role to user - path = '/domains/%s/users/%s/roles/%s' % ( - self.domain_id, self.user['id'], self.role['id']) + path = '/domains/{}/users/{}/roles/{}'.format( + self.domain_id, self.user['id'], self.role['id'] + ) self.put(path=path) auth = self.build_authentication_request( user_id=self.user['id'], password=self.user['password'], - domain_id=self.domain_id) + domain_id=self.domain_id, + ) # Check that listing the domains does not result in an empty list new_is_domain_project = unit.new_project_ref(is_domain=True) new_is_domain_project = PROVIDERS.resource_api.create_project( - new_is_domain_project['id'], new_is_domain_project) + new_is_domain_project['id'], new_is_domain_project + ) - r = self.get('/projects?is_domain=True', auth=auth, - expected_status=200) - self.assertIn(new_is_domain_project['id'], - [p['id'] for p in r.result['projects']]) + r = self.get( + '/projects?is_domain=True', auth=auth, expected_status=200 + ) + self.assertIn( + new_is_domain_project['id'], + [p['id'] for p in r.result['projects']], + ) # Check that the projects are still being filtered # The previously created is_domain project is a domain, so # we can reuse it for the project new_regular_project = unit.new_project_ref( - is_domain=False, domain_id=new_is_domain_project['id']) + is_domain=False, domain_id=new_is_domain_project['id'] + ) new_regular_project = PROVIDERS.resource_api.create_project( - new_regular_project['id'], new_regular_project) - r = self.get('/projects?is_domain=False', auth=auth, - expected_status=200) - self.assertNotIn(new_regular_project['id'], - [p['id'] for p in r.result['projects']]) + new_regular_project['id'], new_regular_project + ) + r = self.get( + '/projects?is_domain=False', auth=auth, expected_status=200 + ) + self.assertNotIn( + new_regular_project['id'], [p['id'] for p in r.result['projects']] + ) def test_list_project_is_domain_filter_default(self): """Default project list should not see projects acting as domains.""" @@ -1814,11 +1821,12 @@ def test_create_project_with_tags(self): self.config_fixture.config(default_tag=config_setting) for tag_number in [0, 10]: project, tags = self._create_project_and_tags( - num_of_tags=tag_number, with_default_tag=True) + num_of_tags=tag_number, with_default_tag=True + ) ref = self.get( - '/projects/%(project_id)s' % { - 'project_id': project['id']}, - expected_status=http.client.OK) + '/projects/{project_id}'.format(project_id=project['id']), + expected_status=http.client.OK, + ) self.assertIn('tags', ref.result['project']) for tag in tags: self.assertIn(tag, ref.result['project']['tags']) From 5fa1eb9022005e41e4e146e23d52e0974b697a94 Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Wed, 26 Nov 2025 09:43:02 +0100 Subject: [PATCH 55/63] Switch dependencies to 2025.1 --- concourse_unit_test_task | 4 ++-- custom-requirements.txt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index ffef098d74..f9d19bdc01 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,11 @@ export DEBIAN_FRONTEND=noninteractive && \ export KEYSTONE_IDENTITY_BACKEND=ldap && \ export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2024.1-m3/upper-constraints.txt && \ +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2025.1-m3/upper-constraints.txt && \ apt-get update && \ apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ pip3 install "tox<4" "six>=1.14.0" "crc<7.0.0" && \ /etc/init.d/memcached start && \ -git clone -b stable/2024.1-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ +git clone -b stable/2025.1-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ tox -e py310 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" diff --git a/custom-requirements.txt b/custom-requirements.txt index 33936bab49..7b6fa35ca9 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -3,5 +3,5 @@ python-binary-memcached git+https://github.com/sapcc/raven-python.git@ccloud#egg=raven[flask] git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middleware git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius -git+https://github.com/sapcc/keystone-extensions.git@stable/2024.1-m3#egg=keystone-extensions +git+https://github.com/sapcc/keystone-extensions.git@stable/2025.1-m3#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware From d9ed9e580dfdeeb0d80790523a7d08a44859654c Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Wed, 26 Nov 2025 16:32:46 +0100 Subject: [PATCH 56/63] concourse_unit_test_task: use `py312` --- concourse_unit_test_task | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/concourse_unit_test_task b/concourse_unit_test_task index f9d19bdc01..71ffed0c72 100644 --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -8,4 +8,4 @@ pip3 install "tox<4" "six>=1.14.0" "crc<7.0.0" && \ /etc/init.d/memcached start && \ git clone -b stable/2025.1-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ cd keystone && \ -tox -e py310 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +tox -e py312 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" From 7f2f18c8ae23a93bf824e989afb1d9223922b68e Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Thu, 27 Nov 2025 12:55:29 +0100 Subject: [PATCH 57/63] Rework `concourse_unit_test_task` (#41) * tox: do not exclude-regex tests It doesn't cause failures without the argument. * Do not clone from remote but use Concourse provided repo Concourse already has the repo version to be tested cloned under `source` path. * Do not run memcached Executing without memcached doesn't change the number of tests and the outcome. * Do not set WATCHER_DISABLED=true It looks like runtime parameter, not changing tests behavior. * Make it a normal bash scipt It doesn't have to be one-line, given the way Concourse runs it. But working with it as with a regular script is much easier in development and testing. * Install tools in `tools` venv Installing them on Ubunbut 24.04 fails - https://stackoverflow.com/a/75696359. Whereas an alternative option could be to use `--break-system-packages` - which is less nicer than venv IMHO. * Use latest tox Upstream tests run with v4+, and our could too. * Install OS requirements using bindep The OS requirements packages change over time (some of which are no longer relevant for e.g. Ubuntu 24.04), and are maintained in `bindep.txt`. So just install them with `bindep`, similar to how Zuul does that upstream - https://opendev.org/zuul/zuul-jobs/src/branch/master/roles/bindep/tasks/packages.yaml. The disadvantage could be that it could install more than required - e.g. mysql, postgresql or graphviz, which we don't need for our test run. * Add `build-essential` to fix psycopg2, python-ldap * Install and configure git Without `safe.directory` it fails to use `git` under `/source`, and fails to recognize pbr version correspondingly (https://docs.openstack.org/pbr/latest/user/features.html#version). * Maintain LDAP tests environment variables With or without them the number of executed tests and the outcome are the same. Not sure why so - needs more investigation. But just maintain them as in the documentation - https://docs.openstack.org/keystone/latest/contributor/testing-keystone.html#ldap-tests * Make tox to NOT skip-missing-interpreters This is to make sure that it won't succeed in case the interpreter could not be found. --- bindep.txt | 6 ++++++ concourse_unit_test_task | 38 +++++++++++++++++++++++++++----------- 2 files changed, 33 insertions(+), 11 deletions(-) mode change 100644 => 100755 concourse_unit_test_task diff --git a/bindep.txt b/bindep.txt index 6d3d592fb5..68b3b745f5 100644 --- a/bindep.txt +++ b/bindep.txt @@ -37,3 +37,9 @@ openldap2-devel [platform:suse] # Required for sphinx graphviz image generation graphviz [test doc] + +# otherwise, it fails to build psycopg2, python-ldap +build-essential [test] + +# otherwise, it fails to recognize pbr version +git [test] diff --git a/concourse_unit_test_task b/concourse_unit_test_task old mode 100644 new mode 100755 index 71ffed0c72..02f1eab48c --- a/concourse_unit_test_task +++ b/concourse_unit_test_task @@ -1,11 +1,27 @@ -export DEBIAN_FRONTEND=noninteractive && \ -export KEYSTONE_IDENTITY_BACKEND=ldap && \ -export WATCHER_DISABLED=true && \ -export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2025.1-m3/upper-constraints.txt && \ -apt-get update && \ -apt-get install -y build-essential python3-pip libpq-dev postgresql-client python3 python3-dev python3-distutils python3-psycopg2 git libpcre++-dev gettext libsqlite3-dev libsasl2-dev libldap2-dev libssl-dev libldap-common memcached && \ -pip3 install "tox<4" "six>=1.14.0" "crc<7.0.0" && \ -/etc/init.d/memcached start && \ -git clone -b stable/2025.1-m3 --single-branch https://github.com/sapcc/keystone.git --depth=1 && \ -cd keystone && \ -tox -e py312 -- --exclude-regex "keystone.tests.unit.test_cli.TokensDoctorTests.test_unreasonable_max_token_size_raised|keystone.tests.unit.test_config.ConfigTestCase.test_profiler_config_default" +#!/usr/bin/env bash + +set -ex + +export DEBIAN_FRONTEND=noninteractive +apt-get update +apt-get install -y \ + python3-venv +python3 -m venv $HOME/.local/tools +source $HOME/.local/tools/bin/activate +pip install -U \ + pip \ + bindep \ + tox + +cd source +apt-get install -y \ + $(bindep -b test) +git config --global --add safe.directory /source + +export UPPER_CONSTRAINTS_FILE=https://raw.githubusercontent.com/sapcc/requirements/stable/2025.1-m3/upper-constraints.txt +export \ + KEYSTONE_IDENTITY_BACKEND=ldap \ + KEYSTONE_CLEAR_LDAP=yes \ + ENABLE_LDAP_LIVE_TEST=true +tox -e py312 \ + --skip-missing-interpreters=false From bbb3318f2de33ab6aee6264c30a8e6e87f0ccdfb Mon Sep 17 00:00:00 2001 From: Adrian Jarvis Date: Mon, 1 Sep 2025 17:15:24 +1200 Subject: [PATCH 58/63] Migrates deprecated password hashes. With Epoxy release the support for the sha512_crypt hash is dropped. This change adds a check of the password hash when the user authenticates. If the hash of the users password is deprecated then the password will be re-hashed using the default hasher and updated in the database. Change-Id: I4a16401b914c92fd7db9d626cb1642570b36d600 Signed-off-by: Adrian Jarvis --- keystone/common/password_hashing.py | 12 ++++++++-- keystone/identity/backends/sql.py | 8 +++++++ .../tests/unit/identity/test_backend_sql.py | 22 ++++++++++++++++++- ...ated-password-hashes-0ffdf9eefb9ff872.yaml | 13 +++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/migrate-deprecated-password-hashes-0ffdf9eefb9ff872.yaml diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index ec8ccd4afd..bd79aa855d 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -36,7 +36,10 @@ ) ) -_HASHER_NAME_MAP = {hasher.name: hasher for hasher in SUPPORTED_HASHERS} +DEPRECATED_HASHERS = frozenset([passlib.hash.sha512_crypt]) + +_HASHER_NAME_MAP = {hasher.name: hasher for hasher in + SUPPORTED_HASHERS | DEPRECATED_HASHERS} # NOTE(notmorgan): Build the list of prefixes. This comprehension builds @@ -63,7 +66,7 @@ def _get_hash_ident(hashers): for module, prefix in itertools.chain( *[ zip([mod] * len(ident), ident) - for mod, ident in _get_hash_ident(SUPPORTED_HASHERS) + for mod, ident in _get_hash_ident(SUPPORTED_HASHERS | DEPRECATED_HASHERS) ] ) } @@ -134,6 +137,11 @@ def check_password(password: str, hashed: str) -> bool: return hasher.verify(password_utf8, hashed) +def is_deprecated_hash(hashed): + """Check if the password is using a deprecated hash.""" + return _get_hasher_from_ident(hashed) in DEPRECATED_HASHERS + + def hash_user_password(user): """Hash a user dict's password without modifying the passed-in dict.""" password = user.get('password') diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index cea1c2aec4..f991993f03 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -76,6 +76,8 @@ def authenticate(self, user_id, password): raise exception.UserDisabled(user_id=user_id) elif user_ref.password_is_expired: raise exception.PasswordExpired(user_id=user_id) + if password_hashing.is_deprecated_hash(user_ref.password): + self._rehash_password(user_id, password) # successful auth, reset failed count if present if user_ref.local_user.failed_auth_count: self._reset_failed_auth(user_id) @@ -127,6 +129,12 @@ def _reset_failed_auth(self, user_id): user_ref.local_user.failed_auth_count = 0 user_ref.local_user.failed_auth_at = None + def _rehash_password(self, user_id, password): + with sql.session_for_write() as session: + user_ref = session.get(model.User, user_id) + user_ref.password_ref.password_hash = \ + password_hashing.hash_password(password) + # user crud @sql.handle_conflicts(conflict_type='user') diff --git a/keystone/tests/unit/identity/test_backend_sql.py b/keystone/tests/unit/identity/test_backend_sql.py index a811e6a32b..0ce3db3d8f 100644 --- a/keystone/tests/unit/identity/test_backend_sql.py +++ b/keystone/tests/unit/identity/test_backend_sql.py @@ -72,10 +72,30 @@ def config_overrides(self): ) def test_configured_algorithm_used(self): + user_ref = self._get_user(self.user_foo['id']) + self.assertEqual( + passlib.hash.scrypt, + password_hashing._get_hasher_from_ident(user_ref.password)) + + def _get_user(self, user_id): with sql.session_for_read() as session: user_ref = PROVIDERS.identity_api._get_user( - session, self.user_foo['id'] + session, user_id ) + return user_ref + + def test_deprecated_password_hash_is_updated_on_auth(self): + user_ref = self._get_user(self.user_foo['id']) + with sql.session_for_write() as session: + old_hash = passlib.hash.sha512_crypt.hash( + self.user_foo['password']) + user_ref.password_ref.password_hash = old_hash + session.add(user_ref.password_ref) + + PROVIDERS.identity_api.driver.authenticate(self.user_foo['id'], + self.user_foo['password']) + user_ref = self._get_user(self.user_foo['id']) + self.assertEqual( password_hashers.scrypt.Scrypt, password_hashing._get_hasher_from_ident(user_ref.password), diff --git a/releasenotes/notes/migrate-deprecated-password-hashes-0ffdf9eefb9ff872.yaml b/releasenotes/notes/migrate-deprecated-password-hashes-0ffdf9eefb9ff872.yaml new file mode 100644 index 0000000000..9661892fb3 --- /dev/null +++ b/releasenotes/notes/migrate-deprecated-password-hashes-0ffdf9eefb9ff872.yaml @@ -0,0 +1,13 @@ +--- +critical: + - | + The Epoxy release of Keystone drops support for passwords that were hashed + using the sha512_crypt algorithm, which was the default in Ocata and + earlier. As a result any passwords that are hashed with sha512_crypt will + not be readable in Epoxy. This release adds the ability for the SQL + identity backend to update passwords that were hashed with sha512_crypt. + The rehashing is done when the user authenticates with the correct password, + so if you have user passwords that are hashed with the sha512_crypt hash + then users will need to authenticate at least once before Keystone is + upgraded by Epoxy. Users who still have passwords hashed in sha512_crypt + after Keystone is upgrade to Epoxy will need to perform a password reset. From 766c1a6d7b308ff0254e308639f2ba362cfde24d Mon Sep 17 00:00:00 2001 From: Stanislav Zaprudskiy Date: Thu, 4 Dec 2025 15:26:17 +0100 Subject: [PATCH 59/63] Bring back `passlib.hash.sha512_crypt` --- custom-requirements.txt | 2 ++ keystone/common/password_hashing.py | 10 ++++++--- keystone/identity/backends/sql.py | 3 ++- .../unit/common/test_password_hashing.py | 22 +++++++++++++++++++ .../tests/unit/identity/test_backend_sql.py | 18 ++++++++------- 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/custom-requirements.txt b/custom-requirements.txt index 7b6fa35ca9..ef0002da7e 100644 --- a/custom-requirements.txt +++ b/custom-requirements.txt @@ -5,3 +5,5 @@ git+https://github.com/sapcc/openstack-watcher-middleware.git#egg=watcher-middle git+https://github.com/funkyHat/py-radius@install_requires#egg=py-radius git+https://github.com/sapcc/keystone-extensions.git@stable/2025.1-m3#egg=keystone-extensions git+https://github.com/sapcc/openstack-rate-limit-middleware.git#egg=rate-limit-middleware + +passlib \ No newline at end of file diff --git a/keystone/common/password_hashing.py b/keystone/common/password_hashing.py index bd79aa855d..290f920f14 100644 --- a/keystone/common/password_hashing.py +++ b/keystone/common/password_hashing.py @@ -18,6 +18,7 @@ import itertools from oslo_log import log +import passlib.hash from keystone.common import password_hashers from keystone.common.password_hashers import bcrypt @@ -38,8 +39,9 @@ DEPRECATED_HASHERS = frozenset([passlib.hash.sha512_crypt]) -_HASHER_NAME_MAP = {hasher.name: hasher for hasher in - SUPPORTED_HASHERS | DEPRECATED_HASHERS} +_HASHER_NAME_MAP = { + hasher.name: hasher for hasher in SUPPORTED_HASHERS | DEPRECATED_HASHERS +} # NOTE(notmorgan): Build the list of prefixes. This comprehension builds @@ -66,7 +68,9 @@ def _get_hash_ident(hashers): for module, prefix in itertools.chain( *[ zip([mod] * len(ident), ident) - for mod, ident in _get_hash_ident(SUPPORTED_HASHERS | DEPRECATED_HASHERS) + for mod, ident in _get_hash_ident( + SUPPORTED_HASHERS | DEPRECATED_HASHERS + ) ] ) } diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py index f991993f03..f030bd5100 100644 --- a/keystone/identity/backends/sql.py +++ b/keystone/identity/backends/sql.py @@ -132,8 +132,9 @@ def _reset_failed_auth(self, user_id): def _rehash_password(self, user_id, password): with sql.session_for_write() as session: user_ref = session.get(model.User, user_id) - user_ref.password_ref.password_hash = \ + user_ref.password_ref.password_hash = ( password_hashing.hash_password(password) + ) # user crud diff --git a/keystone/tests/unit/common/test_password_hashing.py b/keystone/tests/unit/common/test_password_hashing.py index c592cd6366..584b176907 100644 --- a/keystone/tests/unit/common/test_password_hashing.py +++ b/keystone/tests/unit/common/test_password_hashing.py @@ -20,6 +20,8 @@ import keystone.conf from keystone.tests import unit +import passlib.hash + CONF = keystone.conf.CONF @@ -91,6 +93,26 @@ def test_pbkdf2_sha512(self): hashed = password_hashing.hash_password(password) self.assertTrue(password_hashing.check_password(password, hashed)) + def test_sha512_crypt_passlib_compat(self): + self.config_fixture.config(strict_password_check=True) + # sha512_crypt is deprecated and is not supported to be set. + # We want to ensure that user is still able to login, thus + # set algo to whatever and go verify pwd + self.config_fixture.config( + group="identity", password_hash_algorithm="bcrypt" + ) + self.config_fixture.config(group="identity", max_password_length="72") + # few iterations to test multiple random values + for _ in range(self.ITERATIONS): + password: str = "".join( # type: ignore + secrets.choice(string.printable) + for i in range(random.randint(1, 72)) + ) + hashed_passlib = passlib.hash.sha512_crypt.hash(password) + self.assertTrue( + password_hashing.check_password(password, hashed_passlib) + ) + class TestGeneratePartialPasswordHash(unit.BaseTestCase): def setUp(self): diff --git a/keystone/tests/unit/identity/test_backend_sql.py b/keystone/tests/unit/identity/test_backend_sql.py index 0ce3db3d8f..89067cf27b 100644 --- a/keystone/tests/unit/identity/test_backend_sql.py +++ b/keystone/tests/unit/identity/test_backend_sql.py @@ -15,6 +15,7 @@ import freezegun from oslo_utils import timeutils +import passlib.hash from keystone.common import password_hashers from keystone.common import password_hashing @@ -74,26 +75,27 @@ def config_overrides(self): def test_configured_algorithm_used(self): user_ref = self._get_user(self.user_foo['id']) self.assertEqual( - passlib.hash.scrypt, - password_hashing._get_hasher_from_ident(user_ref.password)) + password_hashers.scrypt.Scrypt, + password_hashing._get_hasher_from_ident(user_ref.password), + ) def _get_user(self, user_id): with sql.session_for_read() as session: - user_ref = PROVIDERS.identity_api._get_user( - session, user_id - ) + user_ref = PROVIDERS.identity_api._get_user(session, user_id) return user_ref def test_deprecated_password_hash_is_updated_on_auth(self): user_ref = self._get_user(self.user_foo['id']) with sql.session_for_write() as session: old_hash = passlib.hash.sha512_crypt.hash( - self.user_foo['password']) + self.user_foo['password'] + ) user_ref.password_ref.password_hash = old_hash session.add(user_ref.password_ref) - PROVIDERS.identity_api.driver.authenticate(self.user_foo['id'], - self.user_foo['password']) + PROVIDERS.identity_api.driver.authenticate( + self.user_foo['id'], self.user_foo['password'] + ) user_ref = self._get_user(self.user_foo['id']) self.assertEqual( From 54e3e07cf71a7133c49c4572cb2351aec0471a4c Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 2 Oct 2025 17:25:27 +0100 Subject: [PATCH 60/63] Disable response body validation by default This should not be used in production for the reasons given inline. Change-Id: Ie40f41f57e316888c2b33f2952edcbac702c1c79 Signed-off-by: Stephen Finucane Depends-on: https://review.opendev.org/c/openstack/devstack/+/962852 Closes-bug: #2126676 --- keystone/api/validation/__init__.py | 54 ++++++++++++++++++--------- keystone/conf/__init__.py | 4 +- keystone/conf/api.py | 58 +++++++++++++++++++++++++++++ keystone/tests/unit/core.py | 2 + 4 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 keystone/conf/api.py diff --git a/keystone/api/validation/__init__.py b/keystone/api/validation/__init__.py index 7b39ac2f99..c8e079b82c 100644 --- a/keystone/api/validation/__init__.py +++ b/keystone/api/validation/__init__.py @@ -19,6 +19,10 @@ from oslo_serialization import jsonutils from keystone.api.validation import validators +import keystone.conf +from keystone import exception + +CONF = keystone.conf.CONF def validated(cls): @@ -138,26 +142,40 @@ def add_validator(func): def wrapper(*args, **kwargs): response = func(*args, **kwargs) - if schema is not None: - # In Flask it is not uncommon that the method return a tuple of - # body and the status code. In the runtime Keystone only return - # a body, but some of the used testtools do return a tuple. - if isinstance(response, tuple): - _body = response[0] - else: - _body = response - - # NOTE(stephenfin): If our response is an object, we need to - # serializer and deserialize to convert e.g. date-time - # to strings - _body = jsonutils.dump_as_bytes(_body) - - if _body == b"": - body = None + if CONF.api.response_validation == 'ignore': + # don't waste our time checking anything if we're ignoring + # schema errors + return response + + if schema is None: + return response + + # In Flask it is not uncommon that the method return a tuple of + # body and the status code. In the runtime Keystone only return + # a body, but some of the used testtools do return a tuple. + if isinstance(response, tuple): + _body = response[0] + else: + _body = response + + # NOTE(stephenfin): If our response is an object, we need to + # serializer and deserialize to convert e.g. date-time + # to strings + _body = jsonutils.dump_as_bytes(_body) + + if _body == b'': + body = None + else: + body = jsonutils.loads(_body) + + try: + _schema_validator(schema, body, args, kwargs, is_body=True) + except exception.SchemaValidationError: + if CONF.api.response_validation == 'warn': + LOG.exception('Schema failed to validate') else: - body = jsonutils.loads(_body) + raise - _schema_validator(schema, body, args, kwargs, is_body=True) return response wrapper._response_body_schema = schema diff --git a/keystone/conf/__init__.py b/keystone/conf/__init__.py index e7e7a062db..edaa557afd 100644 --- a/keystone/conf/__init__.py +++ b/keystone/conf/__init__.py @@ -19,6 +19,7 @@ from oslo_middleware import cors from osprofiler import opts as profiler +from keystone.conf import api from keystone.conf import application_credential from keystone.conf import assignment from keystone.conf import auth @@ -54,8 +55,8 @@ CONF = cfg.CONF - conf_modules = [ + api, application_credential, assignment, auth, @@ -90,7 +91,6 @@ wsgi, ] - oslo_messaging.set_transport_defaults(control_exchange='keystone') diff --git a/keystone/conf/api.py b/keystone/conf/api.py new file mode 100644 index 0000000000..b9690fc9e4 --- /dev/null +++ b/keystone/conf/api.py @@ -0,0 +1,58 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_config import cfg + +from keystone.conf import utils + +GROUP_NAME = __name__.split('.')[-1] + +ALL_OPTS = [ + cfg.StrOpt( + "response_validation", + choices=( + ( + 'error', + 'Raise a HTTP 500 (Server Error) for responses that fail ' + 'schema validation', + ), + ( + 'warn', + 'Log a warning for responses that fail schema validation', + ), + ('ignore', 'Ignore schema validation failures'), + ), + default='warn', + help=utils.fmt( + """ +Configure validation of API responses. + +``warn`` is the current recommendation for production environments. If you find +it necessary to enable the ``ignore`` option, please report the issues you are +seeing to the Keystone team so we can improve our schemas. + +``error`` should not be used in a production environment. This is because +schema validation happens *after* the response body has been generated, meaning +any side effects will still happen and the call may be non-idempotent despite +the user receiving a HTTP 500 error. +""" + ), + ) +] + + +def register_opts(conf): + conf.register_opts(ALL_OPTS, group=GROUP_NAME) + + +def list_opts(): + return {GROUP_NAME: ALL_OPTS} diff --git a/keystone/tests/unit/core.py b/keystone/tests/unit/core.py index 40a25a496e..5d2c4a1b96 100644 --- a/keystone/tests/unit/core.py +++ b/keystone/tests/unit/core.py @@ -931,6 +931,8 @@ def config_overrides(self): # allowed in the `[identity] password_hash_rounds` setting self.config_fixture.config(group='identity', password_hash_rounds=4) + self.config_fixture.config(group='api', response_validation='error') + self.useFixture( ksfixtures.KeyRepository( self.config_fixture, From 1f9f665746c681abf56b4a0149d43006b18409f2 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Fri, 5 Dec 2025 16:33:41 +0100 Subject: [PATCH 61/63] Import LOG where it is used For some reason a bug went unnoticed where in the schema validation we log the message from the decorators, but logging itself in not imported. Change-Id: I6ddb69d21d22eafbfcde5c8952a63e39750e6328 Signed-off-by: Artem Goncharov --- keystone/api/validation/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/keystone/api/validation/__init__.py b/keystone/api/validation/__init__.py index c8e079b82c..827da56a8d 100644 --- a/keystone/api/validation/__init__.py +++ b/keystone/api/validation/__init__.py @@ -16,6 +16,7 @@ import typing as ty import flask +from oslo_log import log from oslo_serialization import jsonutils from keystone.api.validation import validators @@ -23,6 +24,7 @@ from keystone import exception CONF = keystone.conf.CONF +LOG = log.getLogger(__name__) def validated(cls): From 0fe0471bb5b0863ae39009a924529dbf91bee111 Mon Sep 17 00:00:00 2001 From: Moutaz Chaara Date: Wed, 26 Nov 2025 00:03:52 +0100 Subject: [PATCH 62/63] Fix role assignment cache for federated users When federated users' group membership changes in the IdP and they reauthenticate, their role assignments should reflect the change immediately, respecting the IdP's TTL configuration rather than waiting for the role assignment cache to expire. This change ensures that federated authentication triggers appropriate cache invalidation for role assignments when group membership has changed. Closes-Bug: #2119031 Change-Id: I79505f3d9e7d9ba46ed6ff40ee0071bdf92b95a0 Signed-off-by: Moutaz Chaara (cherry picked from commit ad87d8212a1bab654e4a6d791d7271e6b3bcd1b5) --- keystone/assignment/core.py | 32 +- keystone/identity/core.py | 56 ++- keystone/identity/shadow_backends/sql.py | 23 ++ .../test_user_group_cache_invalidation.py | 221 +++++++++++ ...er_group_cache_invalidation_integration.py | 348 ++++++++++++++++++ 5 files changed, 670 insertions(+), 10 deletions(-) create mode 100644 keystone/tests/unit/test_user_group_cache_invalidation.py create mode 100644 keystone/tests/unit/test_user_group_cache_invalidation_integration.py diff --git a/keystone/assignment/core.py b/keystone/assignment/core.py index 6fa4c9c971..bbc12eefde 100644 --- a/keystone/assignment/core.py +++ b/keystone/assignment/core.py @@ -15,20 +15,24 @@ """Main entry point into the Assignment service.""" import copy +import datetime import itertools from oslo_log import log +from oslo_utils import timeutils from keystone.common import cache from keystone.common import driver_hints from keystone.common import manager from keystone.common import provider_api from keystone.common.resource_options import options as ro_opt -import keystone.conf from keystone import exception from keystone.i18n import _ +from keystone.models import revoke_model from keystone import notifications +import keystone.conf + CONF = keystone.conf.CONF LOG = log.getLogger(__name__) PROVIDERS = provider_api.ProviderAPIs @@ -1293,6 +1297,32 @@ def delete_user_assignments(self, user_id): self.delete_system_grant_for_user(user_id, assignment['id']) COMPUTED_ASSIGNMENTS_REGION.invalidate() + def invalidate_user_cache_on_group_change(self, user_id): + """Invalidate user cache when group membership changes.""" + LOG.debug( + 'Revoking tokens for federated user %(user_id)s due to group ' + 'membership changes in the identity provider', + {'user_id': user_id}, + ) + + COMPUTED_ASSIGNMENTS_REGION.invalidate() + + # Create revocation event with -1 second to avoid race condition + # where a new token issued at the same second as the revocation + # event would be incorrectly revoked. This ensures only tokens + # issued before the group membership change are revoked. + issued_before = timeutils.utcnow().replace( + microsecond=0 + ) - datetime.timedelta(seconds=1) + PROVIDERS.revoke_api.revoke( + revoke_model.RevokeEvent( + user_id=user_id, issued_before=issued_before + ) + ) + + reason = f'User {user_id} group membership changed' + notifications.invalidate_token_cache_notification(reason) + def check_system_grant_for_user(self, user_id, role_id): """Check if a user has a specific role on the system. diff --git a/keystone/identity/core.py b/keystone/identity/core.py index fd1b114902..031265dc4e 100644 --- a/keystone/identity/core.py +++ b/keystone/identity/core.py @@ -34,12 +34,13 @@ from keystone.common import manager from keystone.common import provider_api from keystone.common.validation import validators -import keystone.conf from keystone import exception from keystone.i18n import _ from keystone.identity.mapping_backends import mapping from keystone import notifications +import keystone.conf + CONF = keystone.conf.CONF LOG = log.getLogger(__name__) @@ -1755,18 +1756,55 @@ def shadow_federated_user(self, idp_id, protocol_id, user, group_ids=None): :returns: dictionary of the mapped User entity """ + user_already_existed = True + try: + PROVIDERS.shadow_users_api.get_federated_user( + idp_id, protocol_id, user['id'] + ) + except exception.UserNotFound: + user_already_existed = False + user_dict = self._shadow_federated_user(idp_id, protocol_id, user) # Note(knikolla): The shadowing operation can be cached, # however we need to update the expiring group memberships. - if group_ids: - for group_id in group_ids: - LOG.info( - "Adding user [%s] to group [%s].", user_dict, group_id - ) - PROVIDERS.shadow_users_api.add_user_to_group_expires( - user_dict['id'], group_id - ) + + if group_ids is None: + group_ids = [] + + membership_changed = False + + for group_id in group_ids: + LOG.info("Adding user [%s] to group [%s].", user_dict, group_id) + if PROVIDERS.shadow_users_api.add_user_to_group_expires( + user_dict['id'], group_id + ): + membership_changed = True + + removed_group_ids = ( + PROVIDERS.shadow_users_api.cleanup_stale_group_memberships( + user_dict['id'], idp_id, group_ids + ) + ) + + if removed_group_ids: + LOG.debug( + 'User %s was removed from groups %s, marking as membership changed', + user_dict['id'], + removed_group_ids, + ) + membership_changed = True + + if membership_changed and user_already_existed: + LOG.debug( + 'Group membership changed for federated user %s, ' + 'revoking tokens', + user_dict['id'], + ) + PROVIDERS.assignment_api.invalidate_user_cache_on_group_change( + user_dict['id'] + ) + return user_dict diff --git a/keystone/identity/shadow_backends/sql.py b/keystone/identity/shadow_backends/sql.py index a79c0687e1..3e2ca0149e 100644 --- a/keystone/identity/shadow_backends/sql.py +++ b/keystone/identity/shadow_backends/sql.py @@ -251,8 +251,11 @@ def get_federated_user(): membership = query.first() if membership: + # Existing membership, just refresh TTL membership.last_verified = timeutils.utcnow() + return False # No change, just renewal else: + # New membership - this is a change session.add( model.ExpiringUserGroupMembership( user_id=user_id, @@ -261,3 +264,23 @@ def get_federated_user(): last_verified=timeutils.utcnow(), ) ) + return True # New membership added + + def cleanup_stale_group_memberships( + self, user_id, idp_id, current_group_ids + ): + """Remove expiring group memberships that are no longer in the IdP assertion.""" + with sql.session_for_write() as session: + query = session.query(model.ExpiringUserGroupMembership) + query = query.filter_by(user_id=user_id, idp_id=idp_id) + existing_memberships = query.all() + + current_group_set = set(current_group_ids) + removed_group_ids = [] + + for membership in existing_memberships: + if membership.group_id not in current_group_set: + removed_group_ids.append(membership.group_id) + session.delete(membership) + + return removed_group_ids diff --git a/keystone/tests/unit/test_user_group_cache_invalidation.py b/keystone/tests/unit/test_user_group_cache_invalidation.py new file mode 100644 index 0000000000..9ee95fe6c6 --- /dev/null +++ b/keystone/tests/unit/test_user_group_cache_invalidation.py @@ -0,0 +1,221 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from unittest import mock +import uuid + +from keystone.common import provider_api +from keystone.tests import unit +from keystone.tests.unit import default_fixtures +from keystone.tests.unit.ksfixtures import database + +PROVIDERS = provider_api.ProviderAPIs + + +class FederatedGroupCacheTest(unit.TestCase): + """Test federated group membership cache invalidation.""" + + def setUp(self): + # Mock LDAP to avoid environment-specific errors + ldap_patcher = mock.patch('ldap.initialize') + self.addCleanup(ldap_patcher.stop) + ldap_patcher.start() + + # Mock LDAP options that are checked during setup + with mock.patch('ldap.get_option', return_value=None): + with mock.patch('ldap.set_option'): + super().setUp() + self.useFixture(database.Database()) + self.load_backends() + + # Create domain + PROVIDERS.resource_api.create_domain( + default_fixtures.ROOT_DOMAIN['id'], default_fixtures.ROOT_DOMAIN + ) + + # Create IDP, mapping, and protocol + self.idp = { + 'id': uuid.uuid4().hex, + 'enabled': True, + 'description': uuid.uuid4().hex, + } + self.mapping = {'id': uuid.uuid4().hex} + self.protocol = { + 'id': uuid.uuid4().hex, + 'idp_id': self.idp['id'], + 'mapping_id': self.mapping['id'], + } + + PROVIDERS.federation_api.create_idp(self.idp['id'], self.idp) + PROVIDERS.federation_api.create_mapping( + self.mapping['id'], self.mapping + ) + PROVIDERS.federation_api.create_protocol( + self.idp['id'], self.protocol['id'], self.protocol + ) + + self.domain_id = PROVIDERS.federation_api.get_idp(self.idp['id'])[ + 'domain_id' + ] + + # Create a federated user + self.federated_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': uuid.uuid4().hex, + } + self.user = PROVIDERS.shadow_users_api.create_federated_user( + self.domain_id, self.federated_user + ) + + # Create groups + self.group1 = unit.new_group_ref(domain_id=self.domain_id) + self.group1 = PROVIDERS.identity_api.create_group(self.group1) + + self.group2 = unit.new_group_ref(domain_id=self.domain_id) + self.group2 = PROVIDERS.identity_api.create_group(self.group2) + + def test_group_membership_returns_true_for_new_membership(self): + """Test that adding a new group membership returns True.""" + result = PROVIDERS.shadow_users_api.add_user_to_group_expires( + self.user['id'], self.group1['id'] + ) + self.assertTrue(result) + + def test_group_membership_returns_false_for_renewal(self): + """Test that renewing an existing group membership returns False.""" + PROVIDERS.shadow_users_api.add_user_to_group_expires( + self.user['id'], self.group1['id'] + ) + result = PROVIDERS.shadow_users_api.add_user_to_group_expires( + self.user['id'], self.group1['id'] + ) + self.assertFalse(result) + + def test_new_group_membership_triggers_token_revocation(self): + """Test that new group membership triggers token revocation.""" + with mock.patch.object( + PROVIDERS.assignment_api, 'invalidate_user_cache_on_group_change' + ) as mock_revoke: + user_dict = { + 'id': self.federated_user['unique_id'], + 'name': self.federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + mock_revoke.assert_called_once_with(self.user['id']) + + def test_membership_renewal_does_not_trigger_revocation(self): + """Test that pure membership renewal doesn't trigger token revocation.""" + with mock.patch.object( + PROVIDERS.shadow_users_api, + 'add_user_to_group_expires', + return_value=False, + ): + with mock.patch.object( + PROVIDERS.shadow_users_api, + 'cleanup_stale_group_memberships', + return_value=False, + ): + with mock.patch.object( + PROVIDERS.assignment_api, + 'invalidate_user_cache_on_group_change', + ) as mock_revoke: + user_dict = { + 'id': self.federated_user['unique_id'], + 'name': self.federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + mock_revoke.assert_not_called() + + def test_adding_additional_group_triggers_revocation(self): + """Test that adding an additional group triggers token revocation.""" + PROVIDERS.shadow_users_api.add_user_to_group_expires( + self.user['id'], self.group1['id'] + ) + + with mock.patch.object( + PROVIDERS.assignment_api, 'invalidate_user_cache_on_group_change' + ) as mock_revoke: + user_dict = { + 'id': self.federated_user['unique_id'], + 'name': self.federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id'], self.group2['id']], + ) + + mock_revoke.assert_called_once_with(self.user['id']) + + def test_empty_group_list_does_not_trigger_revocation(self): + """Test that shadowing with no groups doesn't trigger revocation.""" + with mock.patch.object( + PROVIDERS.assignment_api, 'invalidate_user_cache_on_group_change' + ) as mock_revoke: + user_dict = { + 'id': self.federated_user['unique_id'], + 'name': self.federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], self.protocol['id'], user_dict, group_ids=[] + ) + + mock_revoke.assert_not_called() + + @mock.patch('keystone.notifications.invalidate_token_cache_notification') + def test_token_revocation_persists_event_and_invalidates_cache( + self, mock_cache_notification + ): + """Test that token revocation creates revoke event and invalidates cache.""" + with mock.patch.object(PROVIDERS.revoke_api, 'revoke') as mock_revoke: + PROVIDERS.assignment_api.invalidate_user_cache_on_group_change( + self.user['id'] + ) + + # Verify revoke was called with a RevokeEvent + mock_revoke.assert_called_once() + revoke_event = mock_revoke.call_args[0][0] + + # Check that the revoke event has the correct user_id + self.assertEqual(revoke_event.user_id, self.user['id']) + + # Check that issued_before is set (indicating -1 second logic) + self.assertIsNotNone(revoke_event.issued_before) + + # Verify cache invalidation notification was called + mock_cache_notification.assert_called_once() + call_args = mock_cache_notification.call_args[0][0] + self.assertIn(self.user['id'], call_args) + self.assertIn('group membership changed', call_args) diff --git a/keystone/tests/unit/test_user_group_cache_invalidation_integration.py b/keystone/tests/unit/test_user_group_cache_invalidation_integration.py new file mode 100644 index 0000000000..d725a7a482 --- /dev/null +++ b/keystone/tests/unit/test_user_group_cache_invalidation_integration.py @@ -0,0 +1,348 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Integration tests for federated group membership cache invalidation.""" + +from unittest import mock +import uuid + +from keystone.common import provider_api +from keystone.tests import unit +from keystone.tests.unit import default_fixtures +from keystone.tests.unit.ksfixtures import database + +PROVIDERS = provider_api.ProviderAPIs + + +class FederatedGroupCacheIntegrationTest(unit.TestCase): + """Integration test for federated group membership cache invalidation.""" + + def setUp(self): + # Mock LDAP to avoid environment-specific errors + ldap_patcher = mock.patch('ldap.initialize') + self.addCleanup(ldap_patcher.stop) + ldap_patcher.start() + + # Mock LDAP options that are checked during setup + with mock.patch('ldap.get_option', return_value=None): + with mock.patch('ldap.set_option'): + super().setUp() + + self.useFixture(database.Database()) + self.load_backends() + + # Create domain + PROVIDERS.resource_api.create_domain( + default_fixtures.ROOT_DOMAIN['id'], default_fixtures.ROOT_DOMAIN + ) + + # Create IDP, mapping, and protocol + self.idp = { + 'id': uuid.uuid4().hex, + 'enabled': True, + 'description': 'Test Identity Provider', + } + self.mapping = {'id': uuid.uuid4().hex, 'rules': []} + self.protocol = { + 'id': uuid.uuid4().hex, + 'idp_id': self.idp['id'], + 'mapping_id': self.mapping['id'], + } + + PROVIDERS.federation_api.create_idp(self.idp['id'], self.idp) + PROVIDERS.federation_api.create_mapping( + self.mapping['id'], self.mapping + ) + PROVIDERS.federation_api.create_protocol( + self.idp['id'], self.protocol['id'], self.protocol + ) + + self.domain_id = PROVIDERS.federation_api.get_idp(self.idp['id'])[ + 'domain_id' + ] + + # Create groups + self.group1 = unit.new_group_ref( + domain_id=self.domain_id, name='group1' + ) + self.group1 = PROVIDERS.identity_api.create_group(self.group1) + + self.group2 = unit.new_group_ref( + domain_id=self.domain_id, name='group2' + ) + self.group2 = PROVIDERS.identity_api.create_group(self.group2) + + self.group3 = unit.new_group_ref( + domain_id=self.domain_id, name='group3' + ) + self.group3 = PROVIDERS.identity_api.create_group(self.group3) + + def test_complete_user_shadowing_workflow(self): + """Test complete workflow of user shadowing with cache invalidation.""" + federated_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': 'Test User', + } + + user_dict = { + 'id': federated_user['unique_id'], + 'name': federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + with mock.patch.object( + PROVIDERS.assignment_api, 'invalidate_user_cache_on_group_change' + ) as mock_revoke: + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + self.assertEqual(0, mock_revoke.call_count) + mock_revoke.reset_mock() + + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id'], self.group2['id']], + ) + + self.assertGreaterEqual(mock_revoke.call_count, 1) + mock_revoke.reset_mock() + + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group2['id'], self.group3['id']], + ) + + self.assertEqual(1, mock_revoke.call_count) + + def test_multiple_users_concurrent_operations(self): + """Test cache invalidation with multiple concurrent user operations.""" + users = [] + for i in range(3): + fed_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': f'User {i}', + } + user_dict = { + 'id': fed_user['unique_id'], + 'name': fed_user['display_name'], + 'domain': {'id': self.domain_id}, + } + users.append((fed_user, user_dict)) + + invalidation_counts = {} + + original_revoke = ( + PROVIDERS.assignment_api.invalidate_user_cache_on_group_change + ) + + def track_revocation(user_id): + invalidation_counts[user_id] = ( + invalidation_counts.get(user_id, 0) + 1 + ) + return original_revoke(user_id) + + with mock.patch.object( + PROVIDERS.assignment_api, + 'invalidate_user_cache_on_group_change', + side_effect=track_revocation, + ): + shadowed_users = [] + for fed_user, user_dict in users: + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + shadowed_users.append(user) + + self.assertEqual(0, len(invalidation_counts)) + + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + users[1][1], + group_ids=[self.group1['id'], self.group2['id']], + ) + + self.assertEqual(1, len(invalidation_counts)) + self.assertEqual(1, invalidation_counts[shadowed_users[1]['id']]) + + def test_expired_membership_removal_triggers_revocation(self): + """Test that expired membership removal triggers cache invalidation.""" + federated_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': 'Expiry Test User', + } + + user_dict = { + 'id': federated_user['unique_id'], + 'name': federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + with mock.patch.object( + PROVIDERS.shadow_users_api, + 'cleanup_stale_group_memberships', + return_value=True, + ): + with mock.patch.object( + PROVIDERS.assignment_api, + 'invalidate_user_cache_on_group_change', + ) as mock_revoke: + PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + mock_revoke.assert_called_once_with(user['id']) + + def test_role_assignments_reflect_group_changes(self): + """Test that role assignments reflect group membership changes.""" + project1 = unit.new_project_ref(domain_id=self.domain_id) + project1 = PROVIDERS.resource_api.create_project( + project1['id'], project1 + ) + + project2 = unit.new_project_ref(domain_id=self.domain_id) + project2 = PROVIDERS.resource_api.create_project( + project2['id'], project2 + ) + + role_admin = unit.new_role_ref(name='admin') + role_admin = PROVIDERS.role_api.create_role( + role_admin['id'], role_admin + ) + + role_member = unit.new_role_ref(name='member') + role_member = PROVIDERS.role_api.create_role( + role_member['id'], role_member + ) + + PROVIDERS.assignment_api.create_grant( + role_admin['id'], + group_id=self.group1['id'], + project_id=project1['id'], + ) + + PROVIDERS.assignment_api.create_grant( + role_member['id'], + group_id=self.group2['id'], + project_id=project2['id'], + ) + + federated_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': 'Role Test User', + } + + user_dict = { + 'id': federated_user['unique_id'], + 'name': federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + PROVIDERS.identity_api.add_user_to_group(user['id'], self.group1['id']) + + roles_proj1 = PROVIDERS.assignment_api.list_role_assignments( + user_id=user['id'], project_id=project1['id'], effective=True + ) + self.assertEqual(1, len(roles_proj1)) + self.assertEqual(role_admin['id'], roles_proj1[0]['role_id']) + + PROVIDERS.identity_api.remove_user_from_group( + user['id'], self.group1['id'] + ) + PROVIDERS.identity_api.add_user_to_group(user['id'], self.group2['id']) + PROVIDERS.assignment_api.invalidate_user_cache_on_group_change( + user['id'] + ) + + roles_proj2 = PROVIDERS.assignment_api.list_role_assignments( + user_id=user['id'], project_id=project2['id'], effective=True + ) + self.assertEqual(1, len(roles_proj2)) + self.assertEqual(role_member['id'], roles_proj2[0]['role_id']) + + def test_token_revocation_event_persisted(self): + """Test that revoke event is created on group change.""" + federated_user = { + 'idp_id': self.idp['id'], + 'protocol_id': self.protocol['id'], + 'unique_id': uuid.uuid4().hex, + 'display_name': 'Revocation Test User', + } + + user_dict = { + 'id': federated_user['unique_id'], + 'name': federated_user['display_name'], + 'domain': {'id': self.domain_id}, + } + + # Initial shadowing - no revocation expected + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id']], + ) + + # Shadow again with additional group - revocation expected + with mock.patch.object(PROVIDERS.revoke_api, 'revoke') as mock_revoke: + user = PROVIDERS.identity_api.shadow_federated_user( + self.idp['id'], + self.protocol['id'], + user_dict, + group_ids=[self.group1['id'], self.group2['id']], + ) + + # Verify revoke was called with a RevokeEvent + mock_revoke.assert_called_once() + revoke_event = mock_revoke.call_args[0][0] + + # Check that the revoke event has the correct user_id + self.assertEqual(revoke_event.user_id, user['id']) + + # Check that issued_before is set (indicating -1 second logic) + self.assertIsNotNone(revoke_event.issued_before) From 1c7e96b90d23c0acb826297ab1c3b5bfc49659c9 Mon Sep 17 00:00:00 2001 From: Moutaz Chaara Date: Tue, 31 Mar 2026 21:31:22 +0200 Subject: [PATCH 63/63] Fix LDAP pagination to return all users beyond page_size The _ldap_get_all method had a broken optimization that only fetched one page of results when hints.limit was set without filters. This fix removes that optimization and ensures all LDAP queries use conn.search_s() which properly iterates through all pages via _paged_search_s. Also fixes serverctrls None handling in _paged_search_s. Fixes: #434 --- keystone/identity/backends/ldap/common.py | 28 ++--- .../identity/backends/test_ldap_common.py | 105 ++++++++++++++++-- 2 files changed, 108 insertions(+), 25 deletions(-) diff --git a/keystone/identity/backends/ldap/common.py b/keystone/identity/backends/ldap/common.py index 49371e8e22..4bc6c461af 100644 --- a/keystone/identity/backends/ldap/common.py +++ b/keystone/identity/backends/ldap/common.py @@ -28,8 +28,8 @@ from oslo_log import log from oslo_utils import reflection -from keystone.common import driver_hints from keystone import exception +from keystone.common import driver_hints from keystone.i18n import _ from keystone.identity.backends.ldap import models @@ -1244,7 +1244,8 @@ def _paged_search_s(self, base, scope, filterstr, attrlist=None): rtype, rdata, rmsgid, serverctrls = self.conn.result3(message) # Receive the data res.extend(rdata) - pctrls = [c for c in serverctrls if c.controlType == page_ctrl_oid] + # Handle case where serverctrls is None (e.g., in fake LDAP) + pctrls = [c for c in (serverctrls or []) if c.controlType == page_ctrl_oid] if pctrls: # LDAP server supports pagination if use_old_paging_api: @@ -1829,7 +1830,6 @@ def _ldap_get_all(self, hints, ldap_filter=None): self.object_class, self.id_attr, ) - sizelimit = 0 attrs = list( set( [self.id_attr] @@ -1837,21 +1837,13 @@ def _ldap_get_all(self, hints, ldap_filter=None): + list(self.extra_attr_mapping.keys()) ) ) - # ccloud: only limit if all filters have been satisfied and the - # contoller does not need to filter - if hints.limit and not len(hints.filters): - sizelimit = hints.limit['limit'] - res = self._ldap_get_limited( - self.tree_dn, self.LDAP_SCOPE, query, attrs, sizelimit - ) - else: - with self.get_connection() as conn: - try: - res = conn.search_s( - self.tree_dn, self.LDAP_SCOPE, query, attrs - ) - except ldap.NO_SUCH_OBJECT: - return [] + with self.get_connection() as conn: + try: + res = conn.search_s( + self.tree_dn, self.LDAP_SCOPE, query, attrs + ) + except ldap.NO_SUCH_OBJECT: + return [] # TODO(prashkre): add functional testing for missing name attribute # on ldap entities. # NOTE(prashkre): Filter ldap search result to keep keystone away from diff --git a/keystone/tests/unit/identity/backends/test_ldap_common.py b/keystone/tests/unit/identity/backends/test_ldap_common.py index a2906c4cb2..a722de34c4 100644 --- a/keystone/tests/unit/identity/backends/test_ldap_common.py +++ b/keystone/tests/unit/identity/backends/test_ldap_common.py @@ -12,23 +12,20 @@ import os import tempfile -from unittest import mock import uuid +from unittest import mock import fixtures import ldap.dn from oslo_config import fixture as config_fixture -from keystone.common import driver_hints -from keystone.common import provider_api import keystone.conf from keystone import exception as ks_exception +from keystone.common import driver_hints, provider_api from keystone.identity.backends.ldap import common as common_ldap from keystone.tests import unit -from keystone.tests.unit import default_fixtures -from keystone.tests.unit import fakeldap -from keystone.tests.unit.ksfixtures import database -from keystone.tests.unit.ksfixtures import ldapdb +from keystone.tests.unit import default_fixtures, fakeldap +from keystone.tests.unit.ksfixtures import database, ldapdb CONF = keystone.conf.CONF PROVIDERS = provider_api.ProviderAPIs @@ -694,3 +691,97 @@ def test_search_s_sizelimit_exceeded(self, mock_search_s): 'dc=example,dc=test', ldap.SCOPE_SUBTREE, ) + + +class LDAPPaginationTest(unit.TestCase): + """Test LDAP pagination with more than page_size results. + + This test verifies the fix for issue #434 where Keystone could not list + more than 1000 users from AD without filters. The issue was that + _ldap_get_limited only fetched one page of results. + + Similar to Nova bug #2122109, we verify that all results are fetched + when pagination is enabled, not just the first page. + """ + + def setUp(self): + super().setUp() + + self.useFixture(ldapdb.LDAPDatabase()) + self.useFixture(database.Database()) + + self.load_backends() + self.load_fixtures(default_fixtures) + + def config_overrides(self): + super().config_overrides() + self.config_fixture.config(group='identity', driver='ldap') + # Set page_size to simulate pagination + self.config_fixture.config(group='ldap', page_size=100) + + def config_files(self): + config_files = super().config_files() + config_files.append(unit.dirs.tests_conf('backend_ldap.conf')) + return config_files + + def test_list_users_returns_all_entries_with_multiple_pages(self): + """Verify that listing users returns ALL results across multiple pages.""" + user_count = 250 + created_user_ids = [] + + for i in range(user_count): + user_ref = { + 'name': f'test_user_{i:04d}', + 'enabled': True, + 'domain_id': 'default', + } + user = PROVIDERS.identity_api.create_user(user_ref) + created_user_ids.append(user['id']) + + hints = driver_hints.Hints() + users = PROVIDERS.identity_api.list_users(hints=hints) + returned_user_ids = [u['id'] for u in users] + + for user_id in created_user_ids: + self.assertIn( + user_id, + returned_user_ids, + f"User {user_id} not found in returned users. " + f"Only {len(returned_user_ids)} users returned, expected at least {user_count}" + ) + + self.assertGreater( + len(users), + 100, + f"Expected more than 100 users (page_size), but got {len(users)}. " + "This suggests pagination is not working correctly." + ) + + def test_list_users_with_limit_hint_returns_all_pages(self): + """Verify that even with a limit hint, all pages are fetched.""" + user_count = 150 + created_user_ids = [] + + for i in range(user_count): + user_ref = { + 'name': f'limit_test_user_{i:04d}', + 'enabled': True, + 'domain_id': 'default', + } + user = PROVIDERS.identity_api.create_user(user_ref) + created_user_ids.append(user['id']) + + hints = driver_hints.Hints() + hints.set_limit(200) + + users = PROVIDERS.identity_api.list_users(hints=hints) + returned_user_ids = [u['id'] for u in users] + + found_count = sum(1 for uid in created_user_ids if uid in returned_user_ids) + + self.assertGreater( + found_count, + 100, + f"Expected more than 100 of our users (page_size), but only found {found_count}. " + "This suggests only one page was fetched." + )