Skip to content

Draft: [POC] Resolve "Group SAML - Check SSO status on API activity and direct user to SSO"

Bogdan Denkovych requested to merge poc-sso-enforcement-for-sessionless-activity into master

What does this MR do and why?

::Gitlab::Auth::GroupSaml::SsoEnforcer

In !10034 (merged) we implemented SSO status check for Web(aka requests with current session - Gitlab::Session.current). This SSO status check happens in GroupPolicy and ProjectPolicy. Since our policies are the source of truth in terms of users' abilities, it brings confidence that this SSO enforcement always takes its action for Web requests if enabled.

POC: SSO status check for Git, Dependency proxy, and API

In !12594 (closed) @jamedjo tried to implement SSO status check for sessionless requests(aka Git, Dependency proxy, API).

This MR extends ::Gitlab::Auth::GroupSaml::SsoEnforcer to check SSO status for sessionless requests.

Since GitLab stores users' sessions to Redis instance, it is possible to retrieve a user's sessions by ActiveSession.list_sessions(user) in a sessionless environment.

What is great about this change

  • ::Gitlab::Auth::GroupSaml::SsoEnforcer remains to be the one class responsible for SSO enforcement
  • that this SSO status check happens in the policy layer(GroupPolicy/ProjectPolicy).

This MR was abandoned for some reason. At that point, we lost the way @jamedjo wanted to implement it.

This POC is based on this work(!12594 (closed)).

With this approach, it wasn't possible to disable SSO status check for some specific activities: Git, API, etc., because we didn't know how to differentiate what type of request it is on performing SSO status check(::Gitlab::Auth::GroupSaml::SsoEnforcer.group_access_restricted? call in the policy layer).

To solve this issue this POC suggests using the global state to register the type of activity for a specific part of the code:

::Gitlab::Auth::GroupSaml::SsoEnforcer.register_git_activity do
  super # Check if git command is allowed for a project 
end

In that way, Gitlab::Auth::GroupSaml::SsoEnforcer could recognize the type of current activity and apply rules that are unique to git activity or bypass SSO status check if it is disabled for git.

The same way we can make Gitlab::Auth::GroupSaml::SsoEnforcer recognize API activity to be able to apply specific rules for API activity or allow customers to turn off SSO check for API:

around_actions :register_api_activity
def register_api_activity
::Gitlab::Auth::GroupSaml::SsoEnforcer.register_api_activity do
  yield # Check if api request is allowed a group/project 
end

The big win from this is that:

  • ::Gitlab::Auth::GroupSaml::SsoEnforcer remains to be only one source responsible for SSO enforcement
  • all this SSO status check happens on the policy layer(GroupPolicy/ProjectPolicy)
  • We can extend it since ::Gitlab::Auth::GroupSaml::SsoEnforcer can differentiate the type of activity

The cons of this approach:

  • If a user needs a new SSO session, they would see 404 Not found response by default(currently for API)
    • See how much effort it took to redirect user to group sign in if SSO status check fails: !12246 (merged)
    • The same for Git activity. But I managed to retain the message that tells users SSO is required instead of 404. See code changes in this MR.
  • Currently, we have two SSO status check implementations in place
    • They are not consistent in bypassing rules: #334010
    • It might be hard to roll out this POC's changes

This POC proofs that this approach works and simplifies SSO status check implementation. See this MR's code changes and specs.

Current implementation in master branch: SSO enforcement for Git and Dependency proxy

Git activity

In !56867 (merged) we added SSO status check for git activity. This MR introduced additional ::Gitlab::Auth::GroupSaml::SessionEnforcer class that is able to check SSO status in sesionless request only(by using ActiveSession.list_sessions(user)). SSO status check happens on internal/allowed endpoint.

::Gitlab::Auth::GroupSaml::SessionEnforcer depends on saml_provider.git_check_enforced config, meaning it is not possible to reuse this class for other types of activities. See !73642 (comment 725224206) for details.

Dependency proxy activity

See !67373 (merged)

To reuse ::Gitlab::Auth::GroupSaml::SessionEnforcer class for SSO status check for Dependency proxy we decided to make it to be configurable by saml_provider.git_check_enforced, which is not the right move IMHO(#337969).

SSO status check for dependency proxy relies on ::Gitlab::Auth::GroupSaml::SessionEnforcer and was put in the policy layer with a little hack: !67373 (diffs).

It made the SSO enforcement implementation to be even more confusing and not extendable.

I tried to implement SSO status check for API on top of the current implementation, see this MR/thread: !73642 (comment 725224206).

Related to #297389

What do you think about this?

Edited by Bogdan Denkovych

Merge request reports