Skip to content

Commit eb18651

Browse files
authored
fix: User expiring membership for federated users (#425)
When the user logs in through the IdP python keystone uses expiring group memberships. Fixes: #363
1 parent 68c10cf commit eb18651

File tree

28 files changed

+1525
-91
lines changed

28 files changed

+1525
-91
lines changed

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ sea-orm = { version = "1.1", features = ["mock", "sqlx-sqlite" ]}
9191
serde_urlencoded = { version = "0.7" }
9292
tempfile = { version = "3.23" }
9393
thirtyfour = "0.36"
94-
tracing-test = { version = "0.2" }
94+
tracing-test = { version = "0.2", features = ["no-env-filter"] }
9595
url = { version = "2.5" }
9696
webauthn-rs = { version = "0.5", features = ["danger-credential-internals"] }
9797

doc/src/SUMMARY.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
- [PCI-DSS: Failed Auth Protection](adr/0010-pci-dss-failed-auth-protection.md)
2323
- [PCI-DSS: Inactive Account Deactivation](adr/0011-pci-dss-inactive-account-deactivation.md)
2424
- [PCI-DSS: Account Password Expiration](adr/0012-pci-dss-account-password-expiry.md)
25+
- [Federation OIDC: Expiring Group Membership](adr/0013-federation-oidc-expiring-group-membership.md)
2526
- [Policy enforcement](policy.md)
2627
- [Fernet token]()
2728
- [Token payloads]()
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# 13. OpenIDConnect federation: Expiring group membership
2+
3+
Date: 2025-12-09
4+
5+
## Status
6+
7+
Accepted
8+
9+
## Context
10+
11+
Python Keystone uses expiring group membership for the federated users
12+
<https://specs.openstack.org/openstack/keystone-specs/specs/keystone/ussuri/expiring-group-memberships.html>.
13+
Every time the user authenticates using the federated login it's group
14+
membership are persisted in the `expiring_user_group_membership` table instead
15+
of the `user_group_membership`. The table has a non nullable column
16+
`last_verified` which is set to the time of the last user login. The user is
17+
considered to be included as a member of the group for the period of time
18+
specified in the `conf.federation.default_authorization_ttl`. Once the
19+
`user.last_verified + ttl < current_timestamp()` the user is not considered the
20+
member of the group anymore. The intention of this mechanism is to prevent stale
21+
group memberships granting the user privileges.
22+
23+
## Decision
24+
25+
For compatibility reasons rust implementation must implement the same
26+
functionality.
27+
28+
Every time the user authenticates the user group memberships are persisted in
29+
the `expiring_user_group_membership` table.
30+
31+
- Current group membership is being read from the database (ignoring the time
32+
limitation).
33+
34+
- The group membership that the user should not be having anymore are deleted.
35+
36+
- For the new group memberships corresponding entries are added with the current
37+
timestamp.
38+
39+
- For all other groups that the user is still member of corresponding records
40+
are updated to set `last_verified` to the current timestamp.
41+
42+
- Effective role assignments of the user are taking into the consideration
43+
expiring group memberships through the `list_user_groups` respecting the
44+
expiring membershipts (independent of the `idp_id`) as
45+
`expiring_user_group_membership.last_verified > current_timestamp - conf.federation.default_authorization_ttl`.
46+
47+
## Consequences
48+
49+
- The user must login periodically to keep application credentials working when
50+
corresponding roles are granted through the expiring group membership.
51+
52+
- With the SCIM support the expiring membership should not be necessary.

doc/src/federation/oidc.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,25 @@ The ultimate flexibility of having a single IdP for multiple domains is by
5858
specifying the claim attribute that specifies domain the user should belong to.
5959
This is implemented by using the `domain-id-claim` attribute of the mapping.
6060
Authentication with the claim missing is going to be rejected.
61+
62+
## User group membership
63+
64+
When a user authenticates using the OIDC the group memberships are persisted in
65+
the database with the expiration in addition to the list of group ids being also
66+
saved in the token. This mechanism intends preventing continuous use of the
67+
roles granted through the group memberships (i.e. with application credentials).
68+
`conf.federation.default_authorization_ttl` configuration variable defines the
69+
expiration for such group memberships. Every time the user authenticates with
70+
the OIDC the group memberships are renewed (their `last_verified` property is
71+
reset to the login timestamp). Groups the user is not member of anymore would be
72+
unassigned.
73+
74+
The major consequence of this when using the application credentials that rely
75+
on the roles assigned through the group memberships is that the user need to
76+
periodically login using the OIDC (no other authentication method is renewing
77+
the group memberships since Keystone has currently no mechanism of querying the
78+
IdP for the current groups.
79+
80+
This is going to be changed soon since Keystone now has the means of
81+
communication with the IdP. Additionally the SCIM support is going to be added
82+
allowing the IdP to push updates to the Keystone.

src/api/v4/federation/oidc.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -278,10 +278,12 @@ pub async fn callback(
278278
state
279279
.provider
280280
.get_identity_provider()
281-
.set_user_groups(
281+
.set_user_groups_expiring(
282282
&state,
283283
&user.id,
284284
HashSet::from_iter(group_ids.iter().map(|i| i.as_str())),
285+
idp.id.as_ref(),
286+
Some(&Utc::now()),
285287
)
286288
.await?;
287289
}

src/config.rs

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
//! # Keystone configuration
1515
//!
1616
//! Parsing of the Keystone configuration file implementation.
17-
use chrono::{NaiveDate, TimeDelta, Utc};
17+
use chrono::{DateTime, NaiveDate, TimeDelta, Utc};
1818
use config::{File, FileFormat};
1919
use eyre::{Report, WrapErr};
2020
use regex::Regex;
@@ -182,16 +182,30 @@ pub struct FederationProvider {
182182
/// Federation provider backend.
183183
#[serde(default = "default_sql_driver")]
184184
pub driver: String,
185+
/// Default time in minutes for the validity of group memberships carried over from a mapping. Default is 0, which means disabled.
186+
#[serde(default)]
187+
pub default_authorization_ttl: u32,
185188
}
186189

187190
impl Default for FederationProvider {
188191
fn default() -> Self {
189192
Self {
190193
driver: default_sql_driver(),
194+
default_authorization_ttl: 0,
191195
}
192196
}
193197
}
194-
198+
impl FederationProvider {
199+
/// Return oldest `last_verified` date for the expiring user group membership.
200+
///
201+
/// Calculate the oldest time for the expiring user group membership to not be considered as
202+
/// valid.
203+
pub(crate) fn get_expiring_user_group_membership_cutof_datetime(&self) -> DateTime<Utc> {
204+
Utc::now()
205+
.checked_sub_signed(TimeDelta::seconds(self.default_authorization_ttl.into()))
206+
.unwrap_or(Utc::now())
207+
}
208+
}
195209
/// Identity provider.
196210
#[derive(Debug, Deserialize, Clone)]
197211
pub struct IdentityProvider {

src/db/entity/expiring_user_group_membership.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ pub struct Model {
2525
pub group_id: String,
2626
#[sea_orm(primary_key, auto_increment = false)]
2727
pub idp_id: String,
28+
#[sea_orm(default_expr = "Expr::current_timestamp()")]
2829
pub last_verified: DateTime,
2930
}
3031

src/federation/types/mapping.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ pub struct MappingUpdate {
103103
pub r#type: Option<MappingType>,
104104

105105
/// Enabled attribute.
106+
#[builder(default)]
106107
pub enabled: Option<bool>,
107108

108109
/// List of allowed redirect_uri for the oidc mapping.

src/identity/backends.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// SPDX-License-Identifier: Apache-2.0
1414

1515
use async_trait::async_trait;
16+
use chrono::{DateTime, Utc};
1617
use dyn_clone::DynClone;
1718
use std::collections::HashSet;
1819
use webauthn_rs::prelude::{Passkey, PasskeyAuthentication, PasskeyRegistration};
@@ -117,13 +118,30 @@ pub trait IdentityBackend: DynClone + Send + Sync + std::fmt::Debug {
117118
group_id: &'a str,
118119
) -> Result<(), IdentityProviderError>;
119120

121+
/// Add the user to the group with expiration.
122+
async fn add_user_to_group_expiring<'a>(
123+
&self,
124+
state: &ServiceState,
125+
user_id: &'a str,
126+
group_id: &'a str,
127+
idp_id: &'a str,
128+
) -> Result<(), IdentityProviderError>;
129+
120130
/// Add user group membership relations.
121131
async fn add_users_to_groups<'a>(
122132
&self,
123133
state: &ServiceState,
124134
memberships: Vec<(&'a str, &'a str)>,
125135
) -> Result<(), IdentityProviderError>;
126136

137+
/// Add expiring user group membership relations.
138+
async fn add_users_to_groups_expiring<'a>(
139+
&self,
140+
state: &ServiceState,
141+
memberships: Vec<(&'a str, &'a str)>,
142+
idp_id: &'a str,
143+
) -> Result<(), IdentityProviderError>;
144+
127145
/// Remove the user from the group.
128146
async fn remove_user_from_group<'a>(
129147
&self,
@@ -132,6 +150,15 @@ pub trait IdentityBackend: DynClone + Send + Sync + std::fmt::Debug {
132150
group_id: &'a str,
133151
) -> Result<(), IdentityProviderError>;
134152

153+
/// Remove the user from the group with expiration.
154+
async fn remove_user_from_group_expiring<'a>(
155+
&self,
156+
state: &ServiceState,
157+
user_id: &'a str,
158+
group_id: &'a str,
159+
idp_id: &'a str,
160+
) -> Result<(), IdentityProviderError>;
161+
135162
/// Remove the user from multiple groups.
136163
async fn remove_user_from_groups<'a>(
137164
&self,
@@ -140,6 +167,15 @@ pub trait IdentityBackend: DynClone + Send + Sync + std::fmt::Debug {
140167
group_ids: HashSet<&'a str>,
141168
) -> Result<(), IdentityProviderError>;
142169

170+
/// Remove the user from multiple expiring groups.
171+
async fn remove_user_from_groups_expiring<'a>(
172+
&self,
173+
state: &ServiceState,
174+
user_id: &'a str,
175+
group_ids: HashSet<&'a str>,
176+
idp_id: &'a str,
177+
) -> Result<(), IdentityProviderError>;
178+
143179
/// Set group memberships for the user.
144180
async fn set_user_groups<'a>(
145181
&self,
@@ -148,6 +184,16 @@ pub trait IdentityBackend: DynClone + Send + Sync + std::fmt::Debug {
148184
group_ids: HashSet<&'a str>,
149185
) -> Result<(), IdentityProviderError>;
150186

187+
/// Set expiring group memberships for the user.
188+
async fn set_user_groups_expiring<'a>(
189+
&self,
190+
state: &ServiceState,
191+
user_id: &'a str,
192+
group_ids: HashSet<&'a str>,
193+
idp_id: &'a str,
194+
last_verified: Option<&DateTime<Utc>>,
195+
) -> Result<(), IdentityProviderError>;
196+
151197
/// List user passkeys.
152198
async fn list_user_webauthn_credentials<'a>(
153199
&self,

src/identity/backends/sql.rs

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// SPDX-License-Identifier: Apache-2.0
1414

1515
use async_trait::async_trait;
16+
use chrono::{DateTime, Utc};
1617
use std::collections::HashSet;
1718
use webauthn_rs::prelude::{Passkey, PasskeyAuthentication, PasskeyRegistration};
1819

@@ -162,7 +163,15 @@ impl IdentityBackend for SqlBackend {
162163
state: &ServiceState,
163164
user_id: &'a str,
164165
) -> Result<Vec<Group>, IdentityProviderError> {
165-
Ok(user_group::list_user_groups(&state.db, user_id).await?)
166+
Ok(user_group::list_user_groups(
167+
&state.db,
168+
user_id,
169+
&self
170+
.config
171+
.federation
172+
.get_expiring_user_group_membership_cutof_datetime(),
173+
)
174+
.await?)
166175
}
167176

168177
/// Add the user into the group.
@@ -176,6 +185,21 @@ impl IdentityBackend for SqlBackend {
176185
Ok(user_group::add_user_to_group(&state.db, user_id, group_id).await?)
177186
}
178187

188+
/// Add the user to the group with expiration.
189+
#[tracing::instrument(level = "debug", skip(self, state))]
190+
async fn add_user_to_group_expiring<'a>(
191+
&self,
192+
state: &ServiceState,
193+
user_id: &'a str,
194+
group_id: &'a str,
195+
idp_id: &'a str,
196+
) -> Result<(), IdentityProviderError> {
197+
Ok(
198+
user_group::add_user_to_group_expiring(&state.db, user_id, group_id, idp_id, None)
199+
.await?,
200+
)
201+
}
202+
179203
/// Add user group membership relations.
180204
#[tracing::instrument(level = "debug", skip(self, state))]
181205
async fn add_users_to_groups<'a>(
@@ -186,6 +210,17 @@ impl IdentityBackend for SqlBackend {
186210
Ok(user_group::add_users_to_groups(&state.db, memberships).await?)
187211
}
188212

213+
/// Add expiring user group membership relations.
214+
#[tracing::instrument(level = "debug", skip(self, state))]
215+
async fn add_users_to_groups_expiring<'a>(
216+
&self,
217+
state: &ServiceState,
218+
memberships: Vec<(&'a str, &'a str)>,
219+
idp_id: &'a str,
220+
) -> Result<(), IdentityProviderError> {
221+
Ok(user_group::add_users_to_groups_expiring(&state.db, memberships, idp_id, None).await?)
222+
}
223+
189224
/// Remove the user from the group.
190225
#[tracing::instrument(level = "debug", skip(self, state))]
191226
async fn remove_user_from_group<'a>(
@@ -197,6 +232,21 @@ impl IdentityBackend for SqlBackend {
197232
Ok(user_group::remove_user_from_group(&state.db, user_id, group_id).await?)
198233
}
199234

235+
/// Remove the user from the group with expiration.
236+
#[tracing::instrument(level = "debug", skip(self, state))]
237+
async fn remove_user_from_group_expiring<'a>(
238+
&self,
239+
state: &ServiceState,
240+
user_id: &'a str,
241+
group_id: &'a str,
242+
idp_id: &'a str,
243+
) -> Result<(), IdentityProviderError> {
244+
Ok(
245+
user_group::remove_user_from_group_expiring(&state.db, user_id, group_id, idp_id)
246+
.await?,
247+
)
248+
}
249+
200250
/// Remove the user from multiple groups.
201251
#[tracing::instrument(level = "debug", skip(self, state))]
202252
async fn remove_user_from_groups<'a>(
@@ -208,6 +258,21 @@ impl IdentityBackend for SqlBackend {
208258
Ok(user_group::remove_user_from_groups(&state.db, user_id, group_ids).await?)
209259
}
210260

261+
/// Remove the user from multiple expiring groups.
262+
#[tracing::instrument(level = "debug", skip(self, state))]
263+
async fn remove_user_from_groups_expiring<'a>(
264+
&self,
265+
state: &ServiceState,
266+
user_id: &'a str,
267+
group_ids: HashSet<&'a str>,
268+
idp_id: &'a str,
269+
) -> Result<(), IdentityProviderError> {
270+
Ok(
271+
user_group::remove_user_from_groups_expiring(&state.db, user_id, group_ids, idp_id)
272+
.await?,
273+
)
274+
}
275+
211276
/// Set group memberships of the user.
212277
#[tracing::instrument(level = "debug", skip(self, state))]
213278
async fn set_user_groups<'a>(
@@ -219,6 +284,26 @@ impl IdentityBackend for SqlBackend {
219284
Ok(user_group::set_user_groups(&state.db, user_id, group_ids).await?)
220285
}
221286

287+
/// Set expiring group memberships for the user.
288+
#[tracing::instrument(level = "debug", skip(self, state))]
289+
async fn set_user_groups_expiring<'a>(
290+
&self,
291+
state: &ServiceState,
292+
user_id: &'a str,
293+
group_ids: HashSet<&'a str>,
294+
idp_id: &'a str,
295+
last_verified: Option<&DateTime<Utc>>,
296+
) -> Result<(), IdentityProviderError> {
297+
Ok(user_group::set_user_groups_expiring(
298+
&state.db,
299+
user_id,
300+
group_ids,
301+
idp_id,
302+
last_verified,
303+
)
304+
.await?)
305+
}
306+
222307
/// Create webauthn credential for the user.
223308
#[tracing::instrument(level = "debug", skip(self, state))]
224309
async fn create_user_webauthn_credential<'a>(

0 commit comments

Comments
 (0)