SAML: Grant access to Sentry depending on role

[mologie]

Summary

Sentry's SAML login lacks authorization, which makes it impossible to use with centralized sign-on solutions without compromising on security: Any user known to the directory could use Sentry, even though they should not have access to its data.

Motivation

Quick pitch: Our janitor Mike is awesome, and he's known to our SAML backend for timekeeping, but he's not really into finding bugs with Sentry (or any bugs whatsoever). His credentials really should not work with Sentry, but they do and there is no way to fix it!

Sentry allows adding SAML auth providers, but unfortunately offers only the bare minimum of functionality to sign the user in. If it did just a /little/ bit more with the data it already has, then it would become significantly more useful and secure.

The problem is that Sentry grants any user known to the SAML provider access to the organization. However, this is against the spirit of SAML: SAML merely provides authentication ("this is person X!"), not authorization ("person X can access Sentry"). Authorization is left to the application, and SAML does its best to provide enough context for the application to make an authorization decision ("person X has roles foo, bar, and baz, maybe you're configured to handle those").

I propose to add SAML role support to Sentry. This would essentially consist of just:

1. One role string field in the SAML provider config
2. One check to see whether the logged-in user in fact has the selected role.
3. An error message explaining to the user that (2) failed and his account is not authorized for Sentry.

If such a role selection was added, then the pitched issue would be solved: We could have the SAML backend pass a role for engineers, and thus limit Sentry sign-in to authorized engineering users.

Additional Context

Here is a screenshot of the current SAML config UI that I am referring to:

Keycloak enables configuring arbitrary per-client rules, and assigning them using pretty much arbitrary conditions (e.g. LDAP group membership):

The config UI in the first screenshot would need an option to insert info about the "sentry-login" example role here. All the data is already passed to Sentry upon login, so it would just have to check it against the configured role if the admin set one.

---

Disclaimer: I am evaluating Sentry for deployment at my engineering team with various mobile apps. We love it so far and will probably go with the SaaS solution (plus on-prem for testing), but proper SAML authz support is really a must because pure authn is just not useful enough. This will probably not block a purchase decision, but certainly is the only "meeeh" point I found with Sentry so far.

┆Issue is synchronized with this Jira Improvement by Unito

[leedongwei]

@mologie Hello! Thanks for the detailed feedback.

I'm unfamiliar with Keycloak - do they have a concept of Groups?

The usual implementation to limit user access to applications would be:

1. create a group (e.g. Engineering group)
2. put the relevant users to that group (you and your teammates, but not Mike the Janitor)
3. assign an application (e.g. Sentry, Github, etc) to that group only

It ensures that the identity provider (Keycloak/Okta/AzureAD/etc) would enforce that assignment and it will not provide Mike credentials to "talk" to Sentry at all. This is important as Sentry (or another app with overzealous tracking) would not have the chance to know that Mike is in your organization and read his profile data from the provider.

Hopefully this helps! Please let me know if this is/isn't doable with Keycloak.

[mologie]

@leedongwei Hello! Thank you for your response and suggestion.

Okta seems to implement these groups indeed where it fails the authentication process when a user is not assigned to an application. This is useful in practice, but definitely non-standard behavior that is not covered by SAML in general.

Keycloak takes a rather pure (but as you can see from my issue not necessarily practical) approach to SAML: It authenticates the user, It may also pass authorization statements. However, the evaluation of authentication and authorization information must be done be the SP (here: Sentry).

My point is handled in the SAML 2 Technical Overview document page 12 [1]:

In a SAML-enabled deployment, when they subsequently attempt toaccess a protected resource at the SP, the SP will send the user to the IdP with an authentication requestin order to have the user log in. Thus this scenario is referred to as SP-initiated web SSO. Once logged in,the IdP can produce an assertion that can be used by the SP to validate the user's access rights to theprotected resource.

Let's also compare to GitLab, a common source code management platform, which enables the administrator to configure groups/assertions (passed by e.g. Keycloak after SSO) that are allowed to access GitLab:

https://docs.gitlab.com/ee/integration/saml.html#required-groups

GitLab enables not only to define which assertions/groups are allowed to access the repository, but also enables to define who is global instance administrator. Sentry could adopt similar behavior by assigning the user to the right organization role (user, manager, admin, etc.) right away instead of requiring manual intervention by an admin after the user first signed in.

Regarding your privacy point:

This is important as Sentry (or another app with overzealous tracking) would not have the chance to know that Mike is in your organization and read his profile data from the provider.

This is not a problem with Keycloak (or any SAML IdP I know of): You still define trusted applications (just like with e.g. Okta), and only those may attempt to retrieve the user profile. Depending on application settings, the user may also have the option to confirm/reject whether the application may receive his profile.

[1] https://www.oasis-open.org/committees/download.php/27819/sstc-saml-tech-overview-2.0-cd-02.pdf

[leedongwei]

Damn, you brought out the receipts - those are fair points and I concede to it.

We have plans to revise user roles/permissions (along with implementing SCIM) and this could be a part of that as it will simplify the assignment of roles to a user. I can't make a commitment on the timeline but it is a priority for us in the next 2 quarters.

cc @bowencai8 (PM for the team)

[mologie]

Damn, you brought out the receipts - those are fair points and I concede to it.

Sorry, couldn't help it :-)

We have plans to revise user roles/permissions (along with implementing SCIM) and this could be a part of that as it will simplify the assignment of roles to a user.

This is great to hear, thank you for considering this issue. I suppose my team will simply use a manually managed user directory until a SAML authz feature is ready. The SAML migration path offered by Sentry looks nice after all. Cheers!

[ssi444]

Is there any progress on this task?

[fsmaia]

+1

Not sure if it is covered by the quick pitch use case, but it would be very handy if SAML had an attribute for orgRole, so the user access level could be managed externally from the IdP in a just-in-time provisioning manner (just like the other mapped SAML attributes)

[fsmaia]

I'm trying to understand the auth module deeply so I can open a pull request for this.

So far I reached this workflow, in the src/sentry/auth/helper.py file:

• finish_pipeline
• _finish_login_pipeline(identity)
• handle_existing_identity(auth_identity)
• handle_new_membership(auth_identity)
• handle_membership(request, organization, auth_identity)
• add_organization_member(org_id, default_org_role, role, user_id, flags) -> here we should replace role=organization.default_role with the correct role

The identity object contains the following data:

{
  "id": "foo@example.com",
  "email": "foo@example.com",
  "name": "Foo Bar",
  "email_verified": True,
}

The identity.id is used for obtaining an AuthIdentity instance, which is composed of:

• auth_provider
• provider
• organization
• request
• identity
• user

I was thinking about adding a role attribute to the identity object, so it will be propagated to add_organization_member.

Is this the correct flow?