Audit fixes: 3 sharpest security highs (SSO takeover, token-scope escalation, TLS pin timing) - #24
Merged
Merged
Conversation
…t HIGH) SSO account-matching fell back to email when no account matched the IdP subject, and on a match with a not-yet-linked account it bound the asserting subject to that account and issued its session — all without checking `email_verified`. Many providers (consumer Google, multi-tenant Entra, self-service directories) let a user set an arbitrary, unverified profile email, so an attacker could set their IdP email to a Super Admin's address and be handed that account: full takeover. - VerifiedIdentity now carries `email_verified`, parsed from the claim (bool or the string "true"; absent/anything else -> False, default-deny). - The email fallback in _resolve_user runs only when the provider asserted email_verified: true. An unverified or absent-verification email is treated as no match — the account must be pre-linked by subject or linked by an administrator. Adds tests: an unverified email (and a missing email_verified claim) with a foreign subject is refused and leaves the target account's external_idp_subject unlinked. The test IdP's default claims now assert email_verified: true, as a correctly-configured provider does. 58 SSO tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…HIGH)
Stripping every role from a service account — the intuitive way to disable it after a
suspected leak — silently ESCALATED its token instead of neutering it, two ways:
- principal_for_api_token used `roles=owner.role_set or frozenset({Role.API_SERVICE})`,
re-granting the API_SERVICE permission ceiling to an owner whose roles were removed.
- scope_for_user folded "no roles" into the `Scope.all()` branch, so a role-less owner
got UNRESTRICTED device-group visibility.
Together, a "disabled" token kept authenticating with API_SERVICE-tier permissions across
EVERY device group. Now: the token carries the owner's actual roles (no fallback), so a
role-less owner has no permissions (permissions = union of role perms & token_scopes = {}),
and scope_for_user returns an empty, non-unrestricted scope for no roles. The token
authenticates but can do nothing; re-enable by assigning a role, disable by revoking the
token. Regular users with no roles likewise now see nothing rather than everything.
Adds tests: scope_for_user with no roles is empty-not-unrestricted, and a role-stripped
token owner resolves to zero permissions and an empty scope. Full auth/rbac/users/scope
suite green (1149).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…IGH) For a device with verify_tls off (the default for self-signed management interfaces), connect() built an httpx client with CERT_NONE and did no pinning; the fingerprint was compared only in _check_certificate(), called AFTER the request returned. So the very first request — the authentication exchange carrying the credential (PAN-OS keygen password in the URL, Check Point/FortiManager password in the body) — reached the peer before the pin was ever checked. An on-path attacker presenting any certificate received the credential, and the mismatch was raised one request too late. connect() now performs the pin check in a raw TLS handshake (asyncio.open_connection) that carries no application data, before the httpx client is built. A changed/foreign fingerprint raises CertificateChangedError at connect time — no client, no request, no credential on the wire. First-use still pins (observed_fingerprint). The post-response _check_certificate stays as defence-in-depth for a mid-session change. Adds a test: a matching pin lets connect proceed and records the fingerprint; a changed pin raises before the client exists (so no credential-bearing request was possible). 27 http-transport tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Sep 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit fixes — the sharpest security highs (3)
Three HIGH findings from the 2026-09-30 adversarial audit (
docs/audit/2026-09-30-adversarial-audit.md), each a real path to privilege or credential compromise. Independent of #22/#23; basemain.SSO account takeover via unverified email (
b31beba)_resolve_userfell back to matching an existing account by email — and linked the asserting IdP subject to it — without checkingemail_verified. Many providers let a user set an arbitrary profile email, so an attacker could set theirs to a Super Admin's address and be handed that account. NowVerifiedIdentitycarriesemail_verified(bool or "true"; absent → false), and the email fallback runs only when the provider asserted it verified. An unverified email is treated as no match.API-token scope escalation on role-stripping (
b84a344)Stripping every role from a service account — the intuitive way to disable it — escalated its token:
principal_for_api_tokenre-granted theAPI_SERVICEceiling viarole_set or {API_SERVICE}, andscope_for_usertreated "no roles" asScope.all(). The "disabled" token kept working, unrestricted, across every device group. Now a role-less owner yields zero permissions and an empty (non-unrestricted) scope; disable a token by revoking it, not by stripping roles.TLS pin checked after the credential was sent (
6f77c38)With
verify_tlsoff (default for self-signed management certs), the fingerprint pin was compared only after a request returned — so the credential-bearing auth exchange reached the peer before the pin was checked. An on-path attacker presenting any cert received the credential.connect()now verifies the pin in a raw TLS handshake that carries no application data, before the client is built: a changed/foreign cert raises at connect time, no request sent.Tests
Each fix adds the regression test the audit noted was missing (6 new). Green across the touched suites: SSO (58), auth/rbac/users/scope (1149), http-transport (27).
🤖 Generated with Claude Code