-
Notifications
You must be signed in to change notification settings - Fork 2
feat(iam): restrict User email updates to verified identity providers #482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(iam): restrict User email updates to verified identity providers #482
Conversation
Implement validating admission webhook to enforce that User email addresses can only be changed to emails verified by linked external identity providers (GitHub, Google, etc.). This prevents email spoofing and ensures data integrity between User records and OIDC providers. Changes: - Add ValidateUpdate to User webhook to validate email changes against UserIdentities - Add field indexer for UserIdentity.status.userUID for efficient lookups - Create UserIdentity webhook to block DELETE operations (not supported yet) - Update User webhook annotation to intercept both CREATE and UPDATE operations - Register UserIdentity webhook in controller-manager Validation logic: - Allow email updates only if new email exists in user's linked UserIdentities - Reject updates if user has no linked identity providers - Reject updates if email is not verified by any linked provider - Provide helpful error messages with list of available verified emails UserIdentity DELETE protection: - Block deletion of UserIdentity resources via webhook - Identity provider links must be managed through Zitadel - Prevents inconsistencies between Milo and external auth provider - Requires automatic email synchronization logic before deletion can be enabled Resolves: API Restrict User Email Updates to Linked Identities
|
Needed for #476 |
JoseSzycho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks okay. I just left a comment as I never tried before using a custom API with a field indexer.
Field indexers don't work with UserIdentity because it's a dynamic REST API that proxies to auth-provider-zitadel and is not cached in the controller-manager. Changes: - Remove field indexer setup for UserIdentity.status.userUID - Change from client.MatchingFields to manual in-memory filtering - Add comment explaining why field selectors don't work with dynamic APIs The performance impact is minimal since: - UserIdentities are typically small in number (1-3 per user) - Email updates are infrequent operations - The List operation is already proxied to auth-provider-zitadel regardless
## ✨ Autofixed 1 outdated doc(s) This pull was automatically generated by Joggr to fix 1 outdated doc(s) due to code changes in pull #482 ### Fixed docs The following docs were fixed: - `internal/apiserver/identity/useridentities/README.md`: The documentation previously stated that useridentities was read-only, but it did not describe that an explicit admission webhook now intercepts DELETE attempts and issues a descriptive error. This clarification helps users understand both API and error behavior. ### How to fix To fix the docs, you can either: 1. Merge this pull request into your pull request to fix the docs 2. Wait for your pull request to be merged and then merge this pull request into your base branch --- Powered by [Joggr](https://joggr.ai?utm_source=gh&utm_medium=gh&utm_campaign=ghapr&utm_id=ghapr) - The documentation assistant for your codebase.
🤖 Automatically added newlines to 1 file(s) Co-Authored-By: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
🤖 I automatically added missing newlines at the end of 1 file(s) in this PR. All files should now end with a newline character as per coding standards. |
| // Filter identities for this user | ||
| identityList := &identityv1alpha1.UserIdentityList{} | ||
| for _, identity := range allIdentities.Items { | ||
| if identity.Status.UserUID == string(newUser.GetUID()) { | ||
| identityList.Items = append(identityList.Items, identity) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this verification/filtering is not strictly necessary, as the userIdentity list method seems to use the context in order to fetch the correct user identities.
However, is good to have this extra validation.
JoseSzycho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good.
Could you add some unit tests? You should be able to mock a list of user identities.
Restrict User email updates to self-service only since the webhook currently does not support impersonation. When an admin attempts to update another user's email, the UserIdentity list would be scoped to the admin's identities instead of the target user's identities, causing incorrect validation. By restricting to self-service only, we ensure that the authenticated user in the context matches the target user, so the UserIdentity API correctly returns the user's own linked identity providers. Changes: - Add validation that req.UserInfo.UID matches the target user UID - Remove manual filtering logic (no longer needed with self-service restriction) - Return 403 Forbidden with descriptive error for cross-user update attempts - Simplify UserIdentity list call since context guarantees correct scope Future work: If admin-initiated email updates are needed, implement impersonation support to create a client scoped to the target user.
…tion Replace fmt.Errorf with appropriate Kubernetes error types for consistency with codebase standards. Changes: - Use errors.NewForbidden for cross-user email update attempts (403) - Use errors.NewBadRequest for validation failures (400) - Use errors.NewInternalError for internal errors (500) - Add imports for k8s.io/apimachinery/pkg/api/errors and runtime/schema
|
@OscarLlamas6 don't merge yet. Please take a look at the e2e tests, all of them are failing. I suspect that there might be an error when starting the webhook. |
…heme Add identityv1alpha1 to the controller-manager Scheme to enable UserIdentity webhook registration. Without this registration, the webhook server fails to start with 'no kind is registered' error. Changes: - Add identityv1alpha1 import to controller-manager - Register identityv1alpha1.AddToScheme in init() - Use proper Kubernetes error wrappers (NewForbidden, NewBadRequest, NewInternalError) - Restrict User email updates to self-service only This fixes the test failures where webhooks were returning 'connection refused' because the controller-manager was crashing during webhook setup.
Implement validating admission webhook to enforce that User email addresses can only be changed to emails verified by linked external identity providers (GitHub, Google, etc.). This prevents email spoofing and ensures data integrity between User records and OIDC providers.
Changes:
Validation logic:
UserIdentity DELETE protection:
Resolves: API Restrict User Email Updates to Linked Identities