fix(kanidm): bound person auth sessions, and stop the provision task reporting changed every converge #342

Sammanfogat

supernaut sammanfogade 2 incheckningar från fix/kanidm-role-drift in i main

2026-08-02 16:12:40 +00:00

supernaut kommenterad

2026-08-02 15:56:05 +00:00

Ägare

Closes #308, closes #317.

Important

#308 needs a production command run before this merges, or the next site.yml will fail the
account-policy gate. The role asserts rather than sets, deliberately, so changing the declared
value does not write the server:
kanidm login --name idm_admin
kanidm group account-policy auth-expiry idm_all_persons 86400 --name idm_admin
kanidm group get idm_all_persons --name idm_admin   # expect authsession_expiry: 86400
The gate failing is the gate working; it just has to be sequenced. The runbook now carries this
ordering, plus a warning that reset-auth-expiry must never be used to reverse it — it deletes
the attribute, restoring the never-expires state rather than a default.

#308 — an unset authsession_expiry means sessions never expire, not one day. Verified against
the pinned 1.10.4 source rather than taken on trust: the accessor falls back to
MAXIMUM_AUTH_SESSION_EXPIRY, which is u32::MAX, and the policy fold only ever takes a smaller
value, so a group supplying nothing lowers nothing. DEFAULT_AUTH_SESSION_EXPIRY (86400) appears on
no resolution path — it occurs once, inside a #[cfg(test)] helper. Unset genuinely meant
now + ~136 years.

Two things found that the issue did not state:

The OAuth2 refresh path checks the parent session, so an unbounded auth session also meant
downstream SSO could be refreshed indefinitely off one old login. The 16 h refresh-token expiry
bounds nothing on its own.
The blast radius is people-only. idm_all_persons is the dyngroup of person accounts, while the
provisioning identity is a service account — so applying this cannot break provisioning or
reconciliation.

Value is 86400 s, with the rejected alternatives recorded in the defaults comment. privilege_expiry
was reconsidered in the same pass and deliberately stays inherited.

#317 — the provision task reported changed on every converge. Cause confirmed against the
pinned provisioning tool rather than inferred: the image upload is a bare if let with no
comparison, followed by an unconditional multipart POST. There is no read-back endpoint for a live
image, so the alternative of a conditional upload is not expressible — that settles a trade-off the
issue had left open.

Fixed with a per-line changed_when that excludes only the image re-upload. Per-line is sound
because the tool logs one verb and target per line. A genuine icon change still surfaces, on the copy
task that owns the file.

Idempotence was not verified with --check: the task is a command with no check_mode: false,
so a dry run skips it and fabricates rc: 0. The predicate was instead evaluated verbatim against
captured-shape stdout — including the ANSI escapes the tool emits — across eight cases covering the
symptom, real changes alongside it, and the uncoloured form. A real second-converge changed=0 is
still only observable on the next apply.

Verification

ansible-playbook site.yml --syntax-check clean; ansible-lint 0 failures across 191 files, profile
production; Prettier and markdownlint clean.

Closes #308, closes #317. > [!IMPORTANT] > **#308 needs a production command run before this merges**, or the next `site.yml` will fail the > account-policy gate. The role asserts rather than sets, deliberately, so changing the declared > value does not write the server: > > ``` > kanidm login --name idm_admin > kanidm group account-policy auth-expiry idm_all_persons 86400 --name idm_admin > kanidm group get idm_all_persons --name idm_admin # expect authsession_expiry: 86400 > ``` > > The gate failing is the gate working; it just has to be sequenced. The runbook now carries this > ordering, plus a warning that `reset-auth-expiry` must never be used to reverse it — it deletes > the attribute, restoring the never-expires state rather than a default. **#308** — an unset `authsession_expiry` means sessions never expire, not one day. Verified against the pinned 1.10.4 source rather than taken on trust: the accessor falls back to `MAXIMUM_AUTH_SESSION_EXPIRY`, which is `u32::MAX`, and the policy fold only ever takes a *smaller* value, so a group supplying nothing lowers nothing. `DEFAULT_AUTH_SESSION_EXPIRY` (86400) appears on no resolution path — it occurs once, inside a `#[cfg(test)]` helper. Unset genuinely meant `now + ~136 years`. Two things found that the issue did not state: - The OAuth2 refresh path checks the **parent** session, so an unbounded auth session also meant downstream SSO could be refreshed indefinitely off one old login. The 16 h refresh-token expiry bounds nothing on its own. - The blast radius is people-only. `idm_all_persons` is the dyngroup of person accounts, while the provisioning identity is a service account — so applying this cannot break provisioning or reconciliation. Value is **86400 s**, with the rejected alternatives recorded in the defaults comment. `privilege_expiry` was reconsidered in the same pass and deliberately stays inherited. **#317** — the provision task reported `changed` on every converge. Cause confirmed against the pinned provisioning tool rather than inferred: the image upload is a bare `if let` with no comparison, followed by an unconditional multipart POST. There is **no read-back endpoint for a live image**, so the alternative of a conditional upload is not expressible — that settles a trade-off the issue had left open. Fixed with a per-line `changed_when` that excludes only the image re-upload. Per-line is sound because the tool logs one verb and target per line. A genuine icon change still surfaces, on the copy task that owns the file. Idempotence was **not** verified with `--check`: the task is a `command` with no `check_mode: false`, so a dry run skips it and fabricates `rc: 0`. The predicate was instead evaluated verbatim against captured-shape stdout — including the ANSI escapes the tool emits — across eight cases covering the symptom, real changes alongside it, and the uncoloured form. A real second-converge `changed=0` is still only observable on the next apply. ### Verification `ansible-playbook site.yml --syntax-check` clean; `ansible-lint` 0 failures across 191 files, profile `production`; Prettier and markdownlint clean.

supernaut lade till 2 incheckningar

2026-08-02 15:56:06 +00:00

fix(kanidm): bound person auth sessions to 24 h instead of never expiring 6d73d08b45

An unset `authsession_expiry` on `idm_all_persons` reads as "one day by
default". It is not: Kanidm 1.10.4 resolves the absent attribute to
MAXIMUM_AUTH_SESSION_EXPIRY = u32::MAX (~136 years), so authentication
sessions on this service effectively never expired.

Confirmed against the pinned version's source rather than the CLI help:

- server/lib/src/idm/accountpolicy.rs:27-29 is
  `.unwrap_or(MAXIMUM_AUTH_SESSION_EXPIRY)`, and `fold_from()` seeds the
  accumulator with that same maximum and only ever takes a smaller value
  (:106, :122-124), so a group supplying nothing lowers nothing.
- DEFAULT_AUTH_SESSION_EXPIRY (86400, constants/mod.rs:176) is on no
  resolution path — it appears only in `#[cfg(test)] test_policy()`
  (accountpolicy.rs:89).
- server/lib/src/idm/account.rs:370-372 stamps the resolved value onto the
  session token as `expiry = now + authsession_expiry`, uncapped for the
  PrivilegeCapable scope an interactive login is issued (:398).

That matters beyond the Kanidm session itself: an OAuth2 refresh re-checks
the parent session (idm/oauth2.rs:1620-1630), so downstream SSO could be
refreshed indefinitely off one old login.

Declares 86400 s (24 h), chosen rather than inherited from the constant that
happens to share the number. 8 h was rejected as putting a re-auth inside a
normal working day against a theft that is realistically used within minutes;
7 days as leaving a lost or shared machine signed in across a weekend and a
working week. `privilege_expiry` was reconsidered in the same pass and stays
inherited — its fallback is already the ceiling.

The role asserts rather than sets, so this declaration does not write the
server: the live group must be set first or the next apply fails the gate,
which is the gate working. The runbook now carries that ordering, the exact
command, and why `reset-auth-expiry` must never be used to reverse it (it is
a DELETE on the attribute, so it restores the never-expires state).

Applying is safe during the day: `idm_all_persons` is the dyngroup
`class=person AND class=account`, `idm_admin` is a service account, and
existing sessions keep the expiry they were granted.

Closes #308

fix(kanidm): stop the provision task reporting changed on every converge

ci / ci (pull_request) Successful in 1m57s

Detaljer

9ea775bf0c

The kanidm role reported changed=1 on every run of an otherwise fully
converged host. The task carries no_log, so the cause was invisible in
ordinary output: kanidm-provision re-uploads the OAuth2 tile icon on every
run and logs `Updating /v1/oauth2/forgejo/_image`, which the changed_when
predicate matched.

Confirmed against kanidm-provision v1.3.0 source rather than inferred from
behaviour. src/main.rs:312-313 is a bare
`if let Some(image_file) = &oauth2.image_file { update_oauth2_image(...) }`
with no comparison, and src/client.rs:460-466 logs the line and then POSTs
the multipart body unconditionally. There is no read-back endpoint for a live
image, so the tool has nothing to compare against and the upload cannot be
made conditional from the declared state — which rules out fixing this at the
cause and leaves the predicate.

So changed_when now filters per line: select the mutation verbs, then reject
an `Updating` whose target is an `/_image`. Per line rather than over the
whole stdout because log_event is a single
`println!("{:>12} {}", verb, message)` (main.rs:30-32), so a verb and its
target are always on one line. The suffix is unique — no other log_event ends
in `_image`, and `_basic_secret`, the only other unguarded write, is never
triggered because basicSecretFile is deliberately never emitted.

Masking a genuine icon change is the cost, and it is already covered: the
"Stage the OAuth2 tile icon" copy task reports changed whenever the SVG's
content differs, so a real change still surfaces — on the task that owns the
file. Only the re-upload of an unchanged icon is silenced.

Verified by evaluating the predicate directly against captured-shape stdout,
including the ANSI escapes log_event emits unconditionally: the icon-only
converge now yields false where the previous predicate yielded true, while
Creating, Deleting, a group attribute update and an OAuth2 scope-map update
all still yield true, alone and alongside the icon line.

Not verified by a converge: `--check` skips the command task entirely, so a
dry run proves nothing here.

Closes #317

supernaut sammanfogade incheckning 6333d8e16d till main

2026-08-02 16:12:40 +00:00

supernaut tog bort grenen fix/kanidm-role-drift

2026-08-02 16:12:40 +00:00

supernaut refererade denna ändringsförfrågan från en incheckning

2026-08-02 16:12:42 +00:00

fix(kanidm): bound person auth sessions, and stop the provision task reporting changed every converge (#342)

supernaut refererade denna ändringsförfrågan från en incheckning

2026-08-03 09:41:35 +00:00

fix(kanidm): bound person auth sessions, and stop the provision task reporting changed every converge (#342)

Logga in för att delta i denna konversation.

Inga granskare