Gitlab Geo: Authentication
Initial internal API for discovery and authentication for the gitlab-org/gitlab-ee#76
Merge request reports
Activity
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
Added 528 commits:
- 4004668b...5dbf246d - 527 commits from branch
master
- 7dee770c - Gitlab Geo initial node discovery
- 4004668b...5dbf246d - 527 commits from branch
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
- Cannot update
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 cleanfailed_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 MazettoI 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 MazettoBased 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 andgeo.gitlab.com
as a readonly node.Having
gitlab.com.br
oromgitlab.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 MazettoIf 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 MazettoTo 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
Edited by Gabriel Mazetto@stanhu I remember you doing some work that might be related to this.
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.