From 02cb3e9b26f5b865f3b49cda261c5573fcc9fcd3 Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Mon, 22 Feb 2016 17:08:24 +0100 Subject: [PATCH] Use Windows SID to speed up group membership resolution with AD --- CHANGELOG-EE | 1 + app/models/ldap_group_link.rb | 35 ++++++++++++++ app/workers/ldap_sync_worker.rb | 13 ++++++ ...160222111002_add_sid_to_ldap_group_link.rb | 12 +++++ db/schema.rb | 1 + lib/gitlab/ldap/access.rb | 46 +++++++++++++++++-- lib/gitlab/ldap/adapter.rb | 7 +++ lib/gitlab/ldap/group.rb | 6 +++ lib/gitlab/ldap/person.rb | 9 ++++ spec/models/ldap_group_link_spec.rb | 14 ++++++ 10 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 db/migrate/20160222111002_add_sid_to_ldap_group_link.rb diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 3ea96f973fc2a0..d0edc426640a0a 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -10,6 +10,7 @@ v 8.6.0 (unreleased) - [Elastic] More accurate as_indexed_json (More stable database indexer) - [Elastic] Fix: Don't index newly created system messages and awards - [Elastic] Fixed exception on branch removing + - Use SIDs when possible in an Active Directory environment for faster LDAP group membership evaluation (Alex Lossent) v 8.5.5 - GitLab Geo: Repository synchronization between primary and secondary nodes diff --git a/app/models/ldap_group_link.rb b/app/models/ldap_group_link.rb index 541471469a4682..17e02d5fd4fcbd 100644 --- a/app/models/ldap_group_link.rb +++ b/app/models/ldap_group_link.rb @@ -1,3 +1,17 @@ +# == Schema Information +# +# Table name: ldap_group_links +# +# id :integer not null, primary key +# cn :string(255) not null +# group_access :integer not null +# group_id :integer not null +# created_at :datetime +# updated_at :datetime +# provider :string(255) +# sid :binary +# + class LdapGroupLink < ActiveRecord::Base include Gitlab::Access belongs_to :group @@ -8,6 +22,9 @@ class LdapGroupLink < ActiveRecord::Base validates :provider, presence: true scope :with_provider, ->(provider) { where(provider: provider) } + scope :without_sid, ->() { where("sid IS NULL") } + + before_validation :retrieve_sid, if: :needs_to_retrieve_sid? def access_field group_access @@ -27,4 +44,22 @@ def provider def provider_label config.label end + + # In an Active Directory environment, we can persist the LDAP group's SID (if present) for faster + # group membership resolution + def retrieve_sid + return unless config && config.active_directory + Gitlab::LDAP::Adapter.open provider do |adapter| + ldap_group = adapter.group(cn) + self.sid = ldap_group.sid if ldap_group + end + rescue => e + # not a big deal if we couldn't retrieve a SID, this is just to speed things up + Rails.logger.debug "Could not retrieve SID for LdapGroupLink with cn '#{cn}': #{e}" + end + + def needs_to_retrieve_sid? + # only care about new LDAP group links; we let daily LDAP sync handle updates + return !persisted? && config && config.active_directory && sid.blank? + end end diff --git a/app/workers/ldap_sync_worker.rb b/app/workers/ldap_sync_worker.rb index a03011a63da815..aef3467a9a625d 100644 --- a/app/workers/ldap_sync_worker.rb +++ b/app/workers/ldap_sync_worker.rb @@ -6,6 +6,11 @@ class LdapSyncWorker def perform return unless Gitlab.config.ldap.enabled Rails.logger.info "Performing daily LDAP sync task." + sync_ldap_group_links + sync_users + end + + def sync_users User.ldap.find_each(batch_size: 100).each do |ldap_user| Rails.logger.debug "Syncing user #{ldap_user.username}, #{ldap_user.email}" # Use the 'update_ldap_group_links_synchronously' option to avoid creating a ton @@ -13,4 +18,12 @@ def perform Gitlab::LDAP::Access.allowed?(ldap_user, update_ldap_group_links_synchronously: true) end end + + def sync_ldap_group_links + LdapGroupLink.find_each(batch_size: 100).each do |ldap_group| + Rails.logger.debug "Syncing LDAP group link #{ldap_group.cn}" + ldap_group.retrieve_sid + ldap_group.save if ldap_group.sid_changed? + end + end end diff --git a/db/migrate/20160222111002_add_sid_to_ldap_group_link.rb b/db/migrate/20160222111002_add_sid_to_ldap_group_link.rb new file mode 100644 index 00000000000000..4ceffbd5835cdf --- /dev/null +++ b/db/migrate/20160222111002_add_sid_to_ldap_group_link.rb @@ -0,0 +1,12 @@ +class AddSidToLdapGroupLink < ActiveRecord::Migration + # http://stackoverflow.com/questions/1140528/what-is-the-maximum-length-of-a-sid-in-sddl-format + SID_MAX_BINARY_LENGTH = 68 + + def up + add_column :ldap_group_links, :sid, :binary, limit: SID_MAX_BINARY_LENGTH, default: nil, null: true + end + + def down + remove_column :ldap_group_links, :sid + end +end diff --git a/db/schema.rb b/db/schema.rb index e164559bf01422..6925d3bb3f91b9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -536,6 +536,7 @@ t.datetime "created_at" t.datetime "updated_at" t.string "provider" + t.binary "sid" end create_table "lfs_objects", force: :cascade do |t| diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 2024fecc58e8f3..0cdcabb65f6a44 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -172,11 +172,18 @@ def update_admin_status # documentation if you change the algorithm below. def update_ldap_group_links gitlab_groups_with_ldap_link.each do |group| - active_group_links = group.ldap_group_links.where(cn: cns_with_access) + # in Rails 4 arel is needed for OR queries + table = LdapGroupLink.arel_table + active_group_links = group.ldap_group_links.where( + table[:cn].lower.in(cns_with_access.map(&:downcase)). # compare group CN case-insentively + or (table[:sid].in(sids_with_access)) ) if active_group_links.any? max_access = active_group_links.maximum(:group_access) + # Save some database operations if no need to change anything + next if max_access == previous_group_access(group) + # Ensure we don't leave a group without an owner if max_access < Gitlab::Access::OWNER && group.last_owner?(user) logger.warn "#{self.class.name}: LDAP group sync cannot demote #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner" @@ -186,18 +193,46 @@ def update_ldap_group_links elsif group.last_owner?(user) logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner" else - group.users.delete(user) + group.users.delete(user) if previous_group_access(group) end end end + def user_is_last_owner?(group) + # we can speed things up a bit by only looking up group owners only when we know user actually had owner access + previous_group_access(group) == ::Gitlab::Access::OWNER && group.last_owner?(user) + end + + def previous_group_access(group) + # Assume that in general, LDAP group membership will not change much. We can save a lot of database operations + # by preloading existing group access level for user and comparing with desired new state. + @previous_group_access ||= GroupMember. + # ideally we would specify {source: gitlab_groups_with_ldap_link} in the where clause but if we do, + # rails generates an incorrect query (missing ldap_group_links in the FROM clause of the inner query) + where(user: user). + map { |gm| [gm.source_id, gm.access_level] }.to_h + group && @previous_group_access[group.id] + end + def ldap_groups - @ldap_groups ||= ::LdapGroupLink.with_provider(provider).distinct(:cn).pluck(:cn).map do |cn| + group_links = ::LdapGroupLink.with_provider(provider) + # skip LDAP groups with a known SID when we know we can use the faster membership resolution with SIDs. + group_links = group_links.without_sid if ldap_user.present? && ldap_user.supports_group_sids? + @ldap_groups ||= group_links.distinct(:cn).pluck(:cn).map do |cn| Gitlab::LDAP::Group.find_by_cn(cn, adapter) end.compact end - # returns a collection of cn strings to which the user has access + # returns a list of group SID strings to which the user has access. + # Allows "fast" membership resolution (no LDAP group search) but may not always be available. + def sids_with_access + return [] unless ldap_user.present? && ldap_user.supports_group_sids? + ldap_user.group_sids + end + + # returns the list of cn (as strings) of the LDAP groups that the user is member of and for which + # membership resolution by SID was not possible. + # Allows "slow" membership resolution for all situations where the faster method with SID is not available. def cns_with_access return [] unless ldap_user.present? @ldap_groups_with_access ||= ldap_groups.select do |ldap_group| @@ -225,7 +260,8 @@ def admin_group private def gitlab_groups_with_ldap_link - ::Group.includes(:ldap_group_links).references(:ldap_group_links). + @gitlab_groups_with_ldap_link ||= ::Group.includes(:ldap_group_links). + references(:ldap_group_links). where.not(ldap_group_links: { id: nil }). where(ldap_group_links: { provider: provider }) end diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb index 0c9a2db4fd0db6..67af4393728048 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -69,6 +69,13 @@ def users(field, value, limit = nil) end end + if config.active_directory && options[:scope] == Net::LDAP::SearchScope_BaseObject + # Try to load tokenGroups attribute for faster membership resolution in addition to all + # the regular attribute. It is not included by default. + # The attribute is only available for the Base search scope. + options.merge!(attributes: ['*','tokenGroups']) + end + if limit.present? options.merge!(size: limit) end diff --git a/lib/gitlab/ldap/group.rb b/lib/gitlab/ldap/group.rb index 53ec0d585a4db4..3feddbd4fdc803 100644 --- a/lib/gitlab/ldap/group.rb +++ b/lib/gitlab/ldap/group.rb @@ -26,6 +26,12 @@ def path name.parameterize end + def sid + # The group's Windows SID, for fast recursive membership lookup + return unless entry.respond_to? :objectSid + entry['objectSid'].first + end + def memberuid? entry.respond_to? :memberuid end diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb index 4cdaf30cd9ce83..5b2656e2237237 100644 --- a/lib/gitlab/ldap/person.rb +++ b/lib/gitlab/ldap/person.rb @@ -76,6 +76,15 @@ def windows_domain_name join('.') end + def supports_group_sids? + # if present, tokenGroups will never be empty (e.g. primary group) + config.active_directory && entry['tokenGroups'].present? + end + + def group_sids + entry['tokenGroups'] + end + private def entry diff --git a/spec/models/ldap_group_link_spec.rb b/spec/models/ldap_group_link_spec.rb index d2b1af20238762..99a53d209d41b3 100644 --- a/spec/models/ldap_group_link_spec.rb +++ b/spec/models/ldap_group_link_spec.rb @@ -1,3 +1,17 @@ +# == Schema Information +# +# Table name: ldap_group_links +# +# id :integer not null, primary key +# cn :string(255) not null +# group_access :integer not null +# group_id :integer not null +# created_at :datetime +# updated_at :datetime +# provider :string(255) +# sid :binary +# + require 'spec_helper' describe LdapGroupLink do -- GitLab