feat(iam,admission): email verification gate + idempotent User admission - #756
feat(iam,admission): email verification gate + idempotent User admission#756yahyafakhroji wants to merge 6 commits into
Conversation
f3b7d02 to
f6f8f67
Compare
|
same as here milo-os/zitadel-provider#127 (comment) |
@JoseSzycho sorry for that,, i just updated it |
JoseSzycho
left a comment
JoseSzycho
left a comment
There was a problem hiding this comment.
@yahyafakhroji Did you consider using a ValidatingAdmissionPolicy`instead of writing a plugin and maintaining a cache?
Here's an example of one we're already using:
https://github.com/datum-cloud/datum/blob/main/config/services/iam.miloapis.com/validation/approved-user-policy.yaml
This would make the policy much easier to maintain.
Here's the code in the zitadel-provider repo that populates the information used by this ValidatingAdmissionPolicy:
Apart from that, I noticed that this PR also adds an Email TTL for automatically deleting emails. Please move this to a separate PR, since it's unrelated to the rest of the changes in this PR.
This is something we need, and we've already created an issue for it:
milo-os/resend-provider#33
Could you also move the controller to the resend-provider repo?
In the medium term, this controller logic should live in its own repository, where we'll keep notification logic that doesn't implement a third-party service. In the meantime, though, it would be fine to have the controller in resend-provider.
|
Note Mostly resolved — kept for history. Struck-through parts are done or superseded; one item is still live at the bottom.
→ Superseded, and I had this wrong. The field left milo as well. Retention isn't among enhancements#738's goals and #33 is storage-motivated, so both the controller and
→ Done. datum#268 (policy) + zp#127 (producer).
→ Answered — plugin dropped, the policy is the only enforcement. Worth noting milo owns no Still live: the plugin denied with a custom |
ValidateCreate creates three resources with no AlreadyExists tolerance. Kubernetes runs validating admission before the etcd write, so a retried or concurrent signup fails admission on the User itself - and zitadel-provider's EnsureUser tests IsAlreadyExists, a predicate an admission denial never satisfies, turning a benign retry into a 500. Both callers depend on this: the actions handler and user_sweep.go, whose design is calling EnsureUser repeatedly. The UserPreference site needs a re-read, not just tolerance: Create leaves UID empty on conflict and the third PolicyBinding stamps that UID into its ResourceRef, which would emit an authorization record pointing at "". Existing PolicyBindings are left alone rather than 'repaired'. The apiserver stamps a fresh UID on every create attempt, so a repair keyed on it would delete a correct binding and recreate it pinned to a UID that never persists - and spec.resourceSelector is immutable, so that damage is permanent.
964e129 to
40653f3
Compare
yahyafakhroji
changed the title
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.
a732d0f to
4fb042e
Compare
yahyafakhroji
changed the title
|
@JoseSzycho Done — plugin removed in 4fb042e, so enforcement lives only in datum#268. |
Keeps the facts a maintainer needs — why AlreadyExists is tolerated, why an existing binding must not be repaired against a UID that never persists, and why the UserPreference is re-read — and drops the narration around them.
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.
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.
…fault
Enforcement comes back to milo. A ValidatingAdmissionPolicy cannot do two
things this can: emit EmailNotVerified in details[].causes - a policy's reason
is limited to the built-in enum, so cloud-portal would match message text
instead of a stable code - and reach project control planes, which the GitOps
repo does not deploy policies into.
This is the 60-line version reading iam.miloapis.com/emailVerified off the
authenticated identity, not the 559-line one that read User.status. No client,
no informer cache, no readiness check. The cache existed to answer "is this UID
a machine?", which zitadel-provider now answers at the authenticator: a machine
token carries no email claim, the same discriminator EffectiveUsername already
uses.
EmailVerifiedGate returns, meaning something different. It selects
deny-vs-observe, not on-vs-off: the plugin always evaluates and always counts,
and disabled it admits anyway. That is affordable only because the verdict is a
map lookup on the request itself - the CR-reading version needed a gate to
avoid paying for its cache while idle, which is the opposite reason for the
same switch. Recorded at the gate so it does not read as going in circles.
milo_email_verification_denials_total{enforced} is what makes the rollout
possible without a backfill Job: deploy, read enforced="false", then flip.
The label mirrors a policy's enforcement_action so the numbers stay comparable
if both ever run.
Drops test/admission/email-verification-policy. It existed to prove the CEL
compiles and does not error at evaluation; with no CEL it has no subject, and
its deny path was never reachable anyway - static token files carry no extras.
yahyafakhroji
changed the title
2 participants
What's the current problem?
Nothing on the API side stops an account that never confirmed its email from creating things. The portal can redirect someone to "check your inbox", but that's a UI redirect — anyone holding a token can call the API directly.
Separately, duplicate
Usercreates return the wrong error. The admission webhook creates three side-effect resources with noAlreadyExiststolerance, and Kubernetes runs validating admission before the etcd write — so a retried or concurrent signup fails admission on theUseritself. zitadel-provider'sEnsureUsertestsIsAlreadyExists, a predicate an admission denial never satisfies, so benign retries surface as 500s.How are we solving it?
iam.miloapis.com/emailVerifiedoff the authenticated identity, stamped by zitadel-provider#127 beside the existingregistrationApprovalkey.Useradmission, so a duplicate create returnsAlreadyExists. TheUserPreferencesite re-reads on conflict rather than merely tolerating it:Createleaves UID empty, and the thirdPolicyBindingwould stamp that empty UID into itsResourceRef.Nothing enforces on merge.
EmailVerifiedGatedefaults off, which means observe, not disabled — see below.Why are we solving it?
Part of the passkey program — enhancements#738, tracked for this phase in auth-ui#111. An unverified account shouldn't be able to act on the platform.
Why a plugin rather than a policy
A
ValidatingAdmissionPolicyexpressing the same rule was carried in parallel (datum#268) while this was open. Two things it cannot do:EmailNotVerifiedindetails[].causes. A policy'sreasonis limited to the built-in enum, so cloud-portal would match message text — where it currently matchesdetails[].codefor suspension.config/crd/bootstrap.goseeds CRDs, not policies.Three things worth knowing
This is 60 lines, not the 559 it started as. The first version read
User.status.emailVerified, which cost a per-write API call, an informer cache of ServiceAccount UIDs to tell machines from humans, a readiness check, and a CI timeout when the reflector listed a CRD the bootstrap hook hadn't created. Moving the signal onto the identity removed all of it: a machine token carries noemailclaim, which is the same discriminatorEffectiveUsername()already uses.EmailVerifiedGateselects deny-vs-observe, not on-vs-off. The plugin always evaluates and always counts; disabled, it admits anyway. Readmilo_email_verification_denials_total{enforced="false"}before enabling — that's the population that starts being denied, and it's what makes the rollout possible without a backfill Job. The label mirrors a policy'senforcement_actionso the numbers stay comparable if both ever run.Absence admits, as a contract. Machine identities carry 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.Verification
go build,go vet,gofmtclean; 30 test packages pass. Plugin cases cover: verified admits, unverified denies on create and update, absent key admits, empty value slice admits, unexpected value denies, access review exempt,system:mastersexempt, mid-deletion update exempt, create-with-nil-oldObjectstill denies, the cause type is present,Delete/Connectunregistered, observe mode counts without denying, and the gate defaults off.