Skip to content
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

Groups Phase 2B (Trust external SAML/LDAP email option, email verification, SCIM user ID ref update) #1762

Merged
merged 20 commits into from
May 7, 2024

Conversation

dangtony98
Copy link
Collaborator

@dangtony98 dangtony98 commented Apr 29, 2024

Description 📣

This PR adds a variety of updates largely centered around not trusting the emails coming back from an external SAML/LDAP identity provider.

Problem Context: Given Infisical's multi-tenant approach, users can be part of multiple organizations in an instance of Infisical. The concern was that a bad actor could in the future try to spoof an email of an existing Infisical user in another organization by connecting to an untrusted external IdP (e.g. a self-hosted instance and optionally modified instance of KeyCloak) in their own organization. Once connected, they could log into the bad actor's organization and switch organizations to access the legitimate user's other organizations.

In practice, this is not yet problematic because Infisical imposes the additional master decryption key / password requirement atop existing authentication with the external IdP.

Solution: To further tighten security, we introduce the optional ability for operators to specify whether or not they want to trust SAML/LDAP emails in the admin panel / server configuration; by default, SAML/LDAP emails will not be trusted. As part of this, we re-configure the SAML/LDAP authentication flows to create new users with aliases with the untrusted email that do not conflict with any existing users with the same claimed email; upon their first signup/login, SAML/LDAP users are prompted to verify their email via verification code — If the email is verified and an existing user exists with the same verified email, then the new user account is merged with the existing account automatically, maintaining the desired multi-tenancy with merged identities. The end result is a user that is part of multiple organizations where they can be known under different aliases.

More specifically, this PR entails:

  • Option for admins to specify whether or not to trust SAML/LDAP emails.
  • User aliases implemented for SAML authentication.
  • Email verification upon signing up / completing a user's account via SAML/LDAP for the first time (frontend/backend components).
  • Switch from referring to users by their userId to orgMembershipId for SCIM (you may want to test this as well).

In the future, we will work towards allowing users to verify their emails whenever after they've signed up; it's just that the resource merging operation for that would be complex and out-of-scope for this PR.

Type ✨

  • Bug fix
  • New feature
  • Breaking change
  • Documentation

Copy link

gitguardian bot commented Apr 29, 2024

⚠️ GitGuardian has uncovered 7 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
8529478 Triggered Generic High Entropy Secret ac97f27 docker-swarm/.env-example View secret
8529478 Triggered Generic High Entropy Secret 30ccb78 .env.example View secret
9387833 Triggered Generic Password ac97f27 docker-swarm/.env-example View secret
9387833 Triggered Generic Password 30ccb78 .env.example View secret
10602161 Triggered Generic Password ea4ef7f docker-swarm/stack.yaml View secret
10602161 Triggered Generic Password ea4ef7f docker-swarm/stack.yaml View secret
10602161 Triggered Generic Password ea4ef7f docker-swarm/stack.yaml View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Contributor

@DanielHougaard DanielHougaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some smaller comments for you. I also tested it on my end, and it works great!

frontend/src/views/Signup/SignupSSO.tsx Outdated Show resolved Hide resolved
backend/src/ee/services/scim/scim-service.ts Show resolved Hide resolved
backend/src/ee/services/scim/scim-service.ts Show resolved Hide resolved
Copy link
Collaborator

@maidul98 maidul98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the migration file, lgtm. I assume rest has been reviewed by @DanielHougaard

@maidul98 maidul98 merged commit 4d8965e into main May 7, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants