Feature/expose app credential by konac-hamza · Pull Request #948 · openstack-experimental/keystone

konac-hamza · 2026-07-07T22:48:20Z

This PR implements the Application Credential API (part of #782).

Changes

Added API type definitions for application_credential and access_rule, along with their converters in the api-types crate.
Followed the Python Keystone implementation to preserve compatibility and prevent known security issues in the endpoints.
Implemented the delete operation in both the core and SQL layers.
Implemented policy enforcement based on the default Keystone policies.

+// SPDX-License-Identifier: Apache-2.0
+
+pub mod access_rule;
+pub mod application_credential;


gtema

thanks, look good, but there are issues that need to be addressed

gtema · 2026-07-08T10:52:41Z

+    state
+        .provider
+        .get_identity_provider()
+        .get_user(&ExecutionContext::from_auth(&state, &user_auth), &user_id)


you should only create ExecutionContext once. You recreate it few times in every handler

I will define it at begin of the handler and reuse it in the handler if I need to use it.

gtema · 2026-07-08T10:58:06Z

+    pub roles: Vec<RoleRef>,
+
+    /// Only present in create response. Never returned again.
+    pub secret: String,


make this SecretString

this is still open

gtema · 2026-07-08T10:59:16Z

+
+    pub unrestricted: bool,
+
+    #[cfg_attr(feature = "validate", validate(length(min = 1, max = 64)))]


python keystone does not expose user_id for the application credential, since they are always already bound to the URL. Drop it in all structs

gtema · 2026-07-08T11:31:20Z

+default allow := false
+
+allow if {
+    input.credentials.user_id == input.target.user_id


it would be input.target.application_credential.user_id

gtema · 2026-07-08T11:31:46Z

+
+allow if { input.credentials.is_admin }
+
+allow if { input.credentials.user_id == input.target.user_id }


it's going to be input.target.existing.user_id

gtema · 2026-07-08T11:36:27Z

+    input.credentials.system == "all"
+}
+
+allow if { input.credentials.user_id == input.target.user_id }


target.application_credential.user_id or target.user_id, but depends on how you set pass it from handler. Anyway - please look at the current policies which document the structure of the data - add it to all policies

gtema · 2026-07-08T11:36:54Z

+    input.credentials.system == "all"
+}
+
+allow if { input.credentials.user_id == input.target.user_id }


also here user_id is wrapped under the input.existing.application_credential.user_id

gtema · 2026-07-08T11:38:48Z

+pub async fn get_project_scoped_client() -> Result<Arc<AsyncOpenStack>> {
+    let mut tc = AsyncOpenStack::new(&CloudConfig::from_env()?).await?;
+
+    tc.authorize(


this should not be necessary - the 'tc' already should be project scoped as directed by the env vars.

konac-hamza · 2026-07-08T20:42:07Z

thanks, look good, but there are issues that need to be addressed

Thanks for your review. I'm going to address all issues.

gtema

Very very close, just few nits

gtema · 2026-07-16T09:37:53Z

+                Self::BadRequest("application credential has expired".into())
+            }
+            ApplicationCredentialProviderError::AccessRuleInUse(x) => {
+                Self::BadRequest(format!("access rule {x} is still in use"))


InUse is a typical 409 - conflict and not bad request. User 'Self::Conflict' instead

gtema · 2026-07-16T09:41:42Z

+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub expires_at: Option<DateTime<Utc>>,
+
+    /// Optional client-supplied identifier for the application credential. If


Passing ID in create through API must not be possible - inconsistency to python Keystone

gtema · 2026-07-16T09:42:36Z

+
+    assert_eq!(cred.name, "test-cred");
+    assert!(!cred.secret.is_empty());
+    // assert_eq!(cred.user_id, user_id);


why commented out?

it is mistake, i've forgotten to uncomment after debugging

gtema · 2026-07-16T09:43:51Z

+        &tc,
+        &user_id,
+        ApplicationCredentialCreateBuilder::default()
+            .name(format!("cred-1"))


just use .name("cred-1") - Builder should already have Into

gtema · 2026-07-16T09:44:42Z

 #[derive(OpenApi)]
 #[openapi(
    tags(
-        (name="domains", description=r#"Domains are a collection of projects and users that define administrative boundaries for managing Identity entities. Domains can represent an individual, company, or operator-owned space. They expose administrative activities directly to system users. Users can be granted the administrator role for a domain. A domain administrator can create projects, users, and groups in a domain and assign roles to users and groups in a domain.


not related change - please undo

thanks for review, I have no idea when I changed this file.I will be more careful after that 👍

konac-hamza · 2026-07-16T20:09:04Z

@gtema I didnt understand reason of failure in the CI tests.The /health endpoint doesn't response respect of the logs. I have checked last merged commits but none of them has issue like this, Should I change anything in the PR?

gtema

good progress, there are still some issues. I am currently working on adding more jobs/gates to prevent some of the issues you have here in policies

gtema · 2026-07-17T12:00:42Z

+    pub roles: Vec<RoleRef>,
+
+    /// Only present in create response. Never returned again.
+    pub secret: String,


this is still open

gtema · 2026-07-17T12:32:59Z

+        .enforce(
+            "identity/user/application_credential/delete",
+            &user_auth,
+            json!({"application_credential": serde_json::to_value(&current)?}),


wrong order: policy_name, auth, target, current. You pass current under target

- **Show**: Pass serde_json::to_value(stored_object)?as target,Noneas existing

- **Delete**: Pass serde_json::to_value(stored_object)?as target,None as existing

Based on ADR doc the current object must pass as target. Should I still change it?

nice spot - this is a mistake in the ADR. Everywhere in the code we follow this convention (logical that "existing" parameter always points at the object stored on the server side). So yes, please change it and I fix the ADR

gtema · 2026-07-17T12:34:22Z

+        .enforce(
+            "identity/user/application_credential/show",
+            &user_auth,
+            json!({"application_credential": serde_json::to_value(&current)?}),


also here wrong order of target/current

gtema · 2026-07-17T12:39:19Z

+    input.credentials.system == "all"
+}
+
+allow if { input.credentials.user_id == input.target.application_credential.user_id }


the check in this form tells that while input.target.application_credential.user_id = none would not be allowed, we should throw dedicated violation message (smth like listing application credentials require user_id ...)

gtema · 2026-07-17T12:40:36Z

+
+allow if { input.credentials.is_admin }
+
+allow if { input.credentials.user_id == input.target.application_credential.user_id }


here it matches the handler, but not the general policy: delete/update/show take input.current and not input.target

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

gtema

few more remaining issues, very close now

gtema · 2026-07-20T07:44:17Z

+)]
+#[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))]
+#[cfg_attr(feature = "validate", derive(validator::Validate))]
+pub struct ApplicationCredentialCreate {


you still do not have a "secret" specified by the user during AC creation.

gtema · 2026-07-20T07:45:17Z

+    }
+}
+
+impl From<api_types_application_credential::ApplicationCredentialCreate>


when user passes the expected secret we would need to set it here as well

gtema · 2026-07-20T07:46:43Z

+        .enforce(
+            "identity/user/application_credential/delete",
+            &user_auth,
+            json!({"application_credential": null}),


no, simply serde_json::Value::Null

gtema · 2026-07-20T07:48:03Z

+    };
+
+    let mut target = serde_json::to_value(&payload.application_credential)?;
+    target["user_id"] = json!(user_id);


once you add "secret" to the payload you need to explicitly remove it from the json here so that it is not passed to the policy

gtema · 2026-07-20T07:49:01Z

+        .enforce(
+            "identity/user/application_credential/show",
+            &user_auth,
+            json!({"application_credential": null}),


simply use serde_json::Value::Null

github-advanced-security AI found potential problems Jul 7, 2026

View reviewed changes

Comment thread crates/api-types/src/v3/application_credential.rs

// SPDX-License-Identifier: Apache-2.0

pub mod access_rule;

pub mod application_credential;

gtema requested changes Jul 8, 2026

View reviewed changes

konac-hamza force-pushed the feature/expose-app-credential branch 2 times, most recently from e386a90 to c392df6 Compare July 15, 2026 23:17

gtema requested changes Jul 16, 2026

View reviewed changes

github-advanced-security AI found potential problems Jul 16, 2026

View reviewed changes

Comment thread crates/api-types/src/error_conv.rs Fixed

konac-hamza force-pushed the feature/expose-app-credential branch 4 times, most recently from 20fe189 to ea6182a Compare July 16, 2026 19:14

gtema requested changes Jul 17, 2026

View reviewed changes

konac-hamza force-pushed the feature/expose-app-credential branch from ea6182a to ce559ae Compare July 18, 2026 21:59

konac-hamza requested a review from gtema July 19, 2026 07:21

konac-hamza added 8 commits July 20, 2026 09:13

feat: Add api types of App Cred

34cb165

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

feat: Complete api layer implementtaions

5c4862d

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

feat: Implement app cred APIs

4a726dd

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

fix: Fix the unit test

d59e08e

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

fix: Fix the issues

644747e

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

fix: Update backend tests

9d1691b

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

fix: Remove id from AppCredentialCreate

e534f07

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

fix: Fix mismatch policy enforcer calling

9cfe76f

Signed-off-by: Hamza Konac <hamza.konac@tubitak.gov.tr>

gtema force-pushed the feature/expose-app-credential branch from ce559ae to 9cfe76f Compare July 20, 2026 07:13

gtema requested changes Jul 20, 2026

View reviewed changes


		pub unrestricted: bool,

		#[cfg_attr(feature = "validate", validate(length(min = 1, max = 64)))]


		allow if { input.credentials.is_admin }

		allow if { input.credentials.user_id == input.target.user_id } No newline at end of file

Uh oh!

Conversation

konac-hamza commented Jul 7, 2026

Changes

Uh oh!

gtema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konac-hamza commented Jul 8, 2026

Uh oh!

gtema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

konac-hamza commented Jul 16, 2026

Uh oh!

gtema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtema left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment