feat(iam): deny writes from users with unverified email - #268
Closed
yahyafakhroji wants to merge 5 commits into
Closed
feat(iam): deny writes from users with unverified email#268yahyafakhroji wants to merge 5 commits into
yahyafakhroji wants to merge 5 commits into
Conversation
Denies CREATE and UPDATE from end users who have not verified their email
address, reading iam.miloapis.com/emailVerified off the authenticated
userInfo. zitadel-provider stamps that key in its TokenReview response,
alongside registrationApproval, which is what makes this expressible as a
policy: a rule that had to look up the caller's User resource cannot do so
from CEL.
Safe to deploy before the producer ships. Absence of the key admits, and
until zitadel-provider rolls nobody carries it, so nothing is denied.
Absence admitting is a contract, not an oversight: machine identities carry
no email claim, and zitadel-provider guarantees a human always carries the
key. The two sides have to change together.
Ships [Warn, Audit], not [Deny]. No identity carried this key before, so
going straight to Deny would gate every human the moment the producer rolls.
Audit puts the real number in apiserver_validating_admission_policy_check_
total{enforcement_action="audit"} first, which is the count that decides
whether Deny is safe - and costs no backfill job to obtain.
Two carve-outs, both load-bearing. authorization.k8s.io is exempt because
clients create a SelfSubjectAccessReview to decide whether to render an
affordance at all, and denying the review rather than the write makes an
unverified account look like it lost access entirely. Objects already being
deleted are exempt because removing a finalizer is a plain UPDATE, and
without it any finalizer-bearing resource owned by an unverified user would
be stuck Terminating forever.
Parity with the equivalent milo admission plugin, which exempts it following plugin/projectsuspension. Without this the two disagree, and when both are deployed the stricter one wins silently. Worth noting it is a project convention rather than a Kubernetes guarantee: system:masters bypasses authorization, but admission still runs for it.
JoseSzycho
reviewed
Aug 17, 2026
| (!('iam.miloapis.com/emailVerified' in request.userInfo.extra)) || | ||
| (request.userInfo.extra['iam.miloapis.com/emailVerified'][0] == 'true') || | ||
| (request.resource.group == 'authorization.k8s.io') || | ||
| ('system:masters' in request.userInfo.groups) || |
Contributor
There was a problem hiding this comment.
('system:masters' in request.userInfo.groups) is functionally redundant because system:masters has no emailVerified claim, so clause 1 or 2 already admits them.
Contributor
Author
There was a problem hiding this comment.
Removed in b69c8d1 — you're right, and it's a bit stronger than redundant.
JoseSzycho
requested changes
Aug 17, 2026
yahyafakhroji
added a commit
to milo-os/milo
that referenced
this pull request
Aug 17, 2026
Enforcement itself lives in datum-cloud/datum#268 as a ValidatingAdmissionPolicy. The test lives here because this is where a live milo apiserver exists; datum has no test infrastructure to exercise CEL against. Covers the failure that would hurt most. failurePolicy is Fail, so an expression that compiles but errors at evaluation denies every write in the cluster. Creating the policy is the compile check - the apiserver type-checks expressions at admission-registration time - and the write that follows proves it does not error when evaluated. It also pins the inertness the rollout depends on: an identity carrying no emailVerified key must be admitted, because the policy ships before zitadel-provider starts stamping one. The binding here uses [Deny], not the [Warn, Audit] that ships. Under Warn a completely broken expression would not fail the write and this test would pass without proving anything. The policy YAML is copied rather than referenced, since it lives in another repo. If the two drift, this stops testing the thing that deploys.
Keeps the operational facts — absence-admits as a producer contract, the three carve-outs and why each exists, and the Warn/Audit promotion path — and drops the design-history narration: why CEL cannot look up a User, the informer-cache alternative, and the clause-order walkthrough.
zitadel-provider never sets Groups on its TokenReview response, so an identity carrying the emailVerified key can never also be in system:masters - the clause could not be reached. It was added for parity with a milo admission plugin that has since been deleted, so its only justification is gone too.
yahyafakhroji
added a commit
to milo-os/milo
that referenced
this pull request
Aug 17, 2026
Keeps the copied expression byte-identical to the one in datum-cloud/datum#268. zitadel-provider never sets Groups on its TokenReview response, so an identity carrying the emailVerified key can never also be in system:masters and the clause could not be reached.
yahyafakhroji
added a commit
to milo-os/milo
that referenced
this pull request
Aug 17, 2026
test-environment-validation failed with
spec.validations[0].expression: Internal error: SyntaxError:
Unexpected token at the end of the expression: TOKRparen
which is not a CEL error at all - TOKRparen is a go-jmespath token. Chainsaw
evaluates any manifest value that opens with "(" and closes with ")" as a
JMESPath expression, so it tried to parse the policy expression instead of
passing it through, and the apiserver never saw valid CEL.
The expression was always correct: it compiles under cel-go and passes
ValidateValidatingAdmissionPolicy unchanged. Dropping the redundant outer
parentheses on the leading clauses makes the string start with "!", which is
enough for chainsaw to treat it as a literal. Semantics are identical - unary
! and == both bind tighter than || - and the same edit is applied to the
policy in datum-cloud/datum#268 so the two copies stay byte-identical.
A comment at the expression records why the first clause must not be
re-parenthesised, since the failure mode is a syntax error pointing at CEL
that is not wrong.
Worth noting the test earned its place here: it exists to catch an expression
that reaches the apiserver broken, and the first thing it caught was an
expression that never reached the apiserver at all.
Milo's chainsaw test applies a copy of this policy, and chainsaw evaluates any
manifest value that opens with "(" and closes with ")" as a JMESPath
expression rather than passing it through - which failed the milo e2e run with
a JMESPath syntax error pointing at CEL that was never wrong.
Kustomize does not care either way. Keeping the two copies byte-identical
does, so the same edit lands here. Semantics are unchanged: unary ! and ==
both bind tighter than ||.
yahyafakhroji
added a commit
to datum-cloud/cloud-portal
that referenced
this pull request
Aug 18, 2026
milo no longer carries User.status.emailVerified - enforcement moved onto the authenticated identity (datum-cloud/datum#268, milo-os/zitadel-provider#127), so the portal's only live read of verification state went with it. The claim now rides on the session. IAccessTokenSession gains emailVerified, decoded from the id_token at both creation sites, and getUserWithAccessRetry overlays it onto the User the fraud gates already consume - so the redirect cascade, the guard middleware and /verify-email keep working unchanged. Decoded from the id_token rather than the access token: Zitadel access tokens carry sub/aud/exp/client_id and scopes, not email claims. Absent, unparseable or non-boolean all read false, so an unverified account is never admitted because a claim could not be read. /verify-email drops ?refresh=0. That flag was there because a refresh per tick is expensive on a page that can poll for minutes, and the reasoning held while verification was a resource read. It inverts here: a token claim only changes when a new token is issued, so without the refresh the poll re-reads the same stale false forever. Poll intervals widen (4s/15s -> 10s/20s) to pay for it, and the comments on both sides now say which it is. UNVERIFIED ASSUMPTION, recorded at readEmailVerified: that Zitadel re-issues an id_token carrying email_verified on the refresh_token grant. Everything above rests on it and there is no staging instance to confirm against yet. If it does not hold the poll never resolves and a verified user stays blocked - check it before the gate is enabled. 667 bun tests pass. Hooks bypassed deliberately: the pre-commit typecheck fails on six pre-existing ReactNode/Date errors in the edge metrics components, identical with and without this change, in files it does not touch.
Contributor
Author
|
Closing — superseded by the milo admission plugin in milo-os/milo#756. The plugin does what this policy does, plus two things it can't: emit The rollout control this had — Thanks for the review here — the |
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
2 participants
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.
What's the current problem?
Nothing stops an account that never confirmed its email address from creating resources. The portal can redirect someone to "check your inbox", but anyone holding a token can call the API directly.
How are we solving it?
A
ValidatingAdmissionPolicydenying CREATE and UPDATE from identities carryingiam.miloapis.com/emailVerified: "false", modelled ondeny-unapproved-userin this same directory. The key is stamped by zitadel-provider#127 alongsideregistrationApproval.Ships
[Warn, Audit], not[Deny]. No identity carried this key before, so going straight to Deny would gate every human the moment the producer rolls. Audit puts the real number inapiserver_validating_admission_policy_check_total{enforcement_action="audit"}first — the count that decides whether Deny is safe, and it needs no backfill job to obtain.Safe to merge before the producer ships: absence of the key admits, so until zitadel-provider rolls, nothing is denied.
Why are we solving it?
Part of the passkey program — enhancements#738. An unverified account shouldn't be able to act on the platform.
A policy rather than a compiled-in milo plugin, because the signal travels on the identity. A rule that had to look up the caller's
Userresource couldn't do so from CEL at all — it would need an admission plugin holding an informer cache, which is what this replaces.Two things worth knowing
Absence admitting is a contract, not an oversight. Machine identities carry no
emailclaim, so they get no key and pass. zitadel-provider guarantees the converse and tests it: a human always carries the key,"true"or"false", never omitted. If that breaks, this fails open for humans — the two sides change together.Three carve-outs.
authorization.k8s.io, because clients create aSelfSubjectAccessReviewto decide whether to render an affordance at all, and denying the review would make an unverified account look like it lost access entirely.system:masters, matching the convention in milo'sprojectsuspensionadmission plugin. And objects already being deleted, because finalizer removal is a plain UPDATE — without it, finalizer-bearing resources would stick in Terminating forever.Scope
This is the only enforcement point. A compiled-in milo plugin expressing the same rule was carried in parallel while the question below was open; it has since been dropped, so nothing here duplicates anything.
Project control planes are not covered. A policy reaches the control planes it is deployed to, and milo's CRD bootstrap seeds CRDs but not policies —
deny-unapproved-userhas the same footprint today. Milo owns noProjectControlPlanecontroller either, only the CRD type, so how those apiservers are configured was never milo's to decide.Verification
kustomize build config/services/iam.miloapis.com/renders both policies and their bindings. The CEL is exercised by a chainsaw test in milo (test/admission/email-verification-policy/), which proves it compiles and doesn't error at evaluation — underfailurePolicy: Failthat would deny every write.