Add auditors for SSO session idle timeout misconfiguration#232
Conversation
…e timeout Flags realms where ssoSessionIdleTimeout is greater than 3600 seconds and clientSessionIdleTimeout is either not configured (0) or is greater than or equal to ssoSessionIdleTimeout, meaning clients inherit or exceed the long SSO session lifetime without an effective independent cap. Also flags any realm where clientSessionIdleTimeout is >= ssoSessionIdleTimeout, regardless of the threshold. The effective idle timeout also determines the lifetime of refresh tokens. Severity scales with ssoSessionIdleTimeout: - Medium: > 3600s (1h) - High: >= 28800s (8h) - Critical: >= 86400s (24h) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… session idle timeout is too long Flags OIDC clients that have no client-specific session idle timeout override when the realm ssoSessionIdleTimeout exceeds 1 hour and no realm-level clientSessionIdleTimeout is set, causing the client to silently inherit the long SSO session lifetime for both sessions and refresh tokens. Also flags clients whose override is set but is >= the realm ssoSessionIdleTimeout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Any feedback and/or opinion on this? |
malexmave
left a comment
malexmave
left a comment
There was a problem hiding this comment.
Finally got around to giving this a read. Here's some feedback :)
| sso_idle = realm.get_sso_session_idle_timeout() | ||
| client_idle = realm.get_client_session_idle_timeout() | ||
| if (sso_idle > MAX_SSO_SESSION_IDLE_TIMEOUT_SECONDS and client_idle == 0) or ( | ||
| client_idle > 0 and client_idle >= sso_idle |
There was a problem hiding this comment.
Shouldn't this branch also check that sso_idle > MAX_SSO_SESSION_IDLE_TIMEOUT_SECONDS? Otherwise, it will always fire when client_idle >= sso_idle, even if the sso_idle is sane.
Or was the intent to catch if a client overrides the session lifetime to something higher than the realm, regardless of the specific value? Might also be an interesting auditor, but I'd keep it separate, and it does not match the description above.
| from kcwarden.auditors.realm.sso_session_idle_timeout_exceeds_client_session_idle_timeout import ( | ||
| MAX_SSO_SESSION_IDLE_TIMEOUT_SECONDS, | ||
| ) |
There was a problem hiding this comment.
Instead of importing this from another auditor, maybe we should pull the logic into a subcheck (see https://github.com/iteratec/kcwarden/blob/main/kcwarden/auditors/subchecks/access_tokens.py for an example).
| def get_sso_session_idle_timeout(self) -> int: | ||
| """Get SSO session idle timeout in seconds.""" | ||
| return self._d["ssoSessionIdleTimeout"] | ||
|
|
||
| def get_client_session_idle_timeout(self) -> int: | ||
| """Get client session idle timeout in seconds. A value of 0 means it is not set (falls back to SSO session idle timeout).""" | ||
| return self._d.get("clientSessionIdleTimeout", 0) | ||
|
|
There was a problem hiding this comment.
Just flagging here that there is also ssoSessionIdleTimeoutRememberMe that is used if the "Remember me" mode is used. Would be fine with not adding that as part of this pull request, but a long "remember me" time would also lead to very long-lived tokens that might deserve their own auditor.
@dasniko do you want to take this chance to implement that as well while we're at it, or should we put this into a separate issue?
| def make_realm(sso_idle, client_idle): | ||
| realm = Mock() | ||
| realm.get_sso_session_idle_timeout.return_value = sso_idle | ||
| realm.get_client_session_idle_timeout.return_value = client_idle | ||
| return realm |
There was a problem hiding this comment.
Any particular reason why you aren't using the mock_realm fixture?
| sso_idle > MAX_SSO_SESSION_IDLE_TIMEOUT_SECONDS | ||
| and realm.get_client_session_idle_timeout() == 0 | ||
| and client_override == 0 | ||
| ) or (client_override > 0 and client_override >= sso_idle): |
There was a problem hiding this comment.
Comment from the next auditor on the same functionality also applies here.
| try: | ||
| return int(self.get_attributes().get("client.session.idle.timeout", 0)) | ||
| except ValueError: | ||
| return 0 |
There was a problem hiding this comment.
Intuitively, this seems overly complex - why doesn't a plain self.get_attributes().get("client.session.idle.timeout", 0) work here? If you are worried that get_attributes will return None, we could adapt it to return an empty dict instead if none is set.
2 participants
Adds two new auditors that check for long SSO session idle timeouts without effective client-level caps.
Realm auditor (
SsoSessionIdleTimeoutExceedsClientSessionIdleTimeout):Flags a realm when
ssoSessionIdleTimeoutexceeds 1 hour andclientSessionIdleTimeoutis either not set or is ≥ the SSO timeout — meaning clients inherit the full long lifetime. Severity scales with the SSO timeout value (Medium / High / Critical).Client auditor (
ClientSessionIdleTimeoutNotSetWhileSsoSessionIdleTimeoutTooLong):Flags individual OIDC clients that have no client-specific session idle timeout override when the realm already has a long SSO session idle timeout and no realm-level client timeout is configured.
Both auditors also enforce a general rule: a client-related session idle timeout must never be ≥ the realm
ssoSessionIdleTimeout, regardless of the absolute value — since this determines the effective refresh token lifetime as well.AI disclosure: Implemented with support of 🤖 Claude Code
closes #228