Skip to content
Snippets Groups Projects

Gitlab Geo: Authentication

Merged Gabriel Mazetto requested to merge gitlab-geo into master

Initial internal API for discovery and authentication for the gitlab-org/gitlab-ee#76

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • I have a call with @brodock. No new config settings. Add geo table with hostnames which can be edited in application settings UI

    geo_table
    hostname: string
    primary: boolean

    Each GitLab instance can compare own hostname to one in database to determine it roles. Database is synced between all instances so you need to fill it once via UI on primary server

  • Gabriel Mazetto Added 1 commit:

    Added 1 commit:

    • 4004668b - Gitlab Geo initial node discovery
  • Gabriel Mazetto Added 528 commits:

    Added 528 commits:

    • 4004668b...5dbf246d - 527 commits from branch master
    • 7dee770c - Gitlab Geo initial node discovery
  • Gabriel Mazetto Added 1 commit:

    Added 1 commit:

    • 67831591 - Gitlab Geo initial node discovery
  • Author Maintainer

    Problems with authentication in readonly mode

    With login:

    • Cannot update failed_attempts, sign_in_count, current_sign_in_at
    • Cannot save audit events : INSERT INTO "audit_events" ("type", "author_id", "entity_id", "entity_type", "details", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING "id" [["type", "SecurityEvent"], ["author_id", 1], ["entity_id", 1], ["entity_type", "User"], ["details", "---\n:with: standard\n:target_id: 1\n:target_type: User\n:target_details: Administrator\n"], ["created_at", "2016-01-13 18:47:53.063526"], ["updated_at", "2016-01-13 18:47:53.063526"]]

    With Logout:

    • Cannot update remember_created_at
  • Author Maintainer

    I've tried to bypass some ApplicationController filters when Gitlab::Geo.enabled? but devise does a lot more implicit operations than it exposes.

    When you first call current_user after login, it tries to update user model to clean failed_attempts.

    After that it tries to update again to update last_sign_in_at.

    here is some relevant pieces from stacktrace when first current_user is invoked:

      devise (3.5.3) lib/devise/models/lockable.rb:107:in `valid_for_authentication?'
      devise (3.5.3) lib/devise/strategies/authenticatable.rb:36:in `validate'
      devise-two-factor (2.0.1) lib/devise_two_factor/strategies/two_factor_backupable.rb:8:in `authenticate!'
      warden (1.2.4) lib/warden/strategies/base.rb:53:in `_run!'
      warden (1.2.4) lib/warden/proxy.rb:358:in `block in _run_strategies_for'
      warden (1.2.4) lib/warden/proxy.rb:353:in `_run_strategies_for'
      warden (1.2.4) lib/warden/proxy.rb:323:in `_perform_authentication'
      warden (1.2.4) lib/warden/proxy.rb:104:in `authenticate'
      devise (3.5.3) lib/devise/controllers/helpers.rb:124:in `current_user'
    
    Edited by Gabriel Mazetto
  • Author Maintainer

    We must either try to patch devise, or bypass it entirely when in a GeoNode

  • Author Maintainer

    I discussed the issue with @dzaporozhets today, and we think there is another way:

    If we replicate Redis instance, we can share user session, and bypass the authentication entirely on the readonly instance, delegating it to the master, in order to not have to fight against Devise saving stuff on the database.

    We must pay some additional attention to make sure both instances will be able to share session.

    I will detail the requirements soon

    Edited by Gabriel Mazetto
  • Author Maintainer

    Based on: http://stackoverflow.com/questions/12584298/rails-session-store-multiple-domains For this to work, both instances must share a domain or be part of a subdomain.

    Taking "gitlab.com" as an example, the easier setup is to have gitlab.com as a master and geo.gitlab.com as a readonly node.

    Having gitlab.com.br or omgitlab.com as a readonly node will not work well, as we can't store cookies on different domains nor can we share them on TLDs.

    Possible scenarios:

    • Same parent subdomain (VALID):
    • Master: gitlab.omg.this.is.so.huge.example.com
    • Readonly: geo.omg.this.is.so.huge.example.com
    • Setup: set cookie domain as omg.this.is.so.huge.example.com
    • Same domain, but different parent (VALID):
    • Master: gitlab.dev.example.com
    • Readonly gitlab.ci.example.com
    • Setup: set cookie domain as example.com
    • Same domain, different subdomain in different levels (VALID):
    • Master: gitlab.this.is.example.com
    • Readonly: gitlab.example.com
    • Setup: set cookie domain as example.com
    • Different domains (INVALID):
    • Master: gitlab.com
    • Readonly: gitlab.net
    • Same subdomain in different domains (INVALID):
    • Master: `gitlab.example.com``
    • Readonly: gitlab.example.net
    Edited by Gabriel Mazetto
  • Author Maintainer

    If we find in the future that we must go with the Devise path, this is how far I got:

    diff --git a/app/models/user.rb b/app/models/user.rb
    index 48e2d5c..12be20d 100644
    --- a/app/models/user.rb
    +++ b/app/models/user.rb
    @@ -816,6 +816,18 @@ class User < ActiveRecord::Base
         end
       end
    
    +  def update_tracked_fields!(request)
    +    super(request) unless Gitlab::Geo.readonly?
    +  end
    +
    +  def lock_strategy_enabled?(strategy)
    +    Gitlab::Geo.readonly? ? false : super(strategy)
    +  end
    +
    +  def timedout?(last_access)
    +    Gitlab::Geo.readonly? ? false : super(last_access)
    +  end
    +
       private
    
       def projects_union
    diff --git a/app/services/audit_event_service.rb b/app/services/audit_event_service.rb
    index df6df4e..88c7da6 100644
    --- a/app/services/audit_event_service.rb
    +++ b/app/services/audit_event_service.rb
    @@ -81,6 +81,6 @@ class AuditEventService
           entity_id: @entity.id,
           entity_type: @entity.class.name,
           details: @details
    -    )
    +    ) unless Gitlab::Geo.readonly?
       end
     end

    Disabled audit_events, and many security features that uses database to store state. For Devise to work, we must probably save that data in Redis instead of our DB.

    Edited by Gabriel Mazetto
  • Gabriel Mazetto Added 1 commit:

    Added 1 commit:

    • 176d1c08 - Gitlab Geo force authentication with primary node
  • Gabriel Mazetto Added 1 commit:

    Added 1 commit:

    • 7998971a - Gitlab Geo force authentication with primary node
  • Author Maintainer

    Ok, looks like session sharing is working, I was able to force a redirect to the primary node, and the idea was to it redirect back after login, but something isn't right yet.

  • Author Maintainer

    To get the redirect correctly, I have to do a redirect with the "full url" not only the "path". On the old code, there was a differentiation around "path" and "fullpath" that I don't fully understand the motivation behind.

    This is also this possible attack vector. Does anyone have additional concerns or suggestion?

      def store_redirect_path
        redirect_uri =
          if request.referer.present? && (params['redirect_to_referer'] == 'yes')
            URI(request.referer)
          elsif session[:geo_redirect].present? && (params['redirect_to_referer'] == 'yes')
            URI(stored_location_for(:geo_redirect))
          else
            URI(request.url)
          end
    
        redirect_to =
          if redirect_uri.host == Gitlab.config.gitlab.host && 
             referer_uri.port == Gitlab.config.gitlab.port
            # our gitlab instance
            redirect_uri.path
          else
            # possible security attack vector as we are redirecting back to anywhere on the internet
            redirect.to_s
          end
    
        # Prevent a 'you are already signed in' message directly after signing:
        # we should never redirect to '/users/sign_in' after signing in successfully.
        unless redirect_to == new_user_session_path
          store_location_for(:redirect, redirect_to)
        end
      end

    cc @rspeicher @dzaporozhets @DouweM

    Edited by Gabriel Mazetto
  • @stanhu I remember you doing some work that might be related to this.

  • Gabriel Mazetto Added 1 commit:

    Added 1 commit:

    • 16a80814 - Another try to get redirect working
  • Author Maintainer

    Last commit is the updated one... still not fully ok, but getting closer

    Edited by Gabriel Mazetto
  • @brodock glad to hear signin via master works. When you get redirect working we should merge this merge request as first iteration is complete. Same domain for session seems a reasonable limitation.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading