Commit 5bef86b9 authored by Robert Speicher's avatar Robert Speicher

Merge branch 'restrict_ldap_return_attributes' into 'master'

Request only the LDAP attributes we need

## What does this MR do?

It includes CE changes from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6187 to reduce the chance of merge conflicts since lots of stuff moved around. 

Fixes gitlab-org/gitlab-ce#13821. For LDAP users, we really only ever need uid, dn, cn, and mail attributes, and in some cases, even less. For LDAP groups we need ..... This merge request strips the request down to the minimum needed by default, and allows the caller to specify more or less, if needed. 

## Why was this MR needed?

This will improve performance especially in cases where the connection is slow between GitLab and LDAP, or when the LDAP object has lots of attributes we don't care about.

See merge request !712
parents 45402b12 32392c1c
......@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 8.12.0 (Unreleased)
- Reduce UPDATE queries when moving between import states on projects
- [ES] Instrument Elasticsearch::Git::Repository
- Request only the LDAP attributes we need
- [ES] Instrument other Gitlab::Elastic classes
- [ES] Fix: Elasticsearch does not find partial matches in project names
......
......@@ -16,7 +16,8 @@ module EE
def groups(cn = "*", size = nil)
options = {
base: config.group_base,
filter: Net::LDAP::Filter.eq("cn", cn)
filter: Net::LDAP::Filter.eq("cn", cn),
attributes: %w(dn cn memberuid member submember uniquemember memberof)
}
options.merge!(size: size) if size
......@@ -30,13 +31,13 @@ module EE
groups(*args).first
end
def dn_matches_filter?(dn, filter)
def dns_for_filter(filter)
ldap_search(
base: dn,
base: config.base,
filter: filter,
scope: Net::LDAP::SearchScope_BaseObject,
scope: Net::LDAP::SearchScope_WholeSubtree,
attributes: %w{dn}
).any?
).map(&:dn)
end
end
end
......
module EE
module Gitlab
module LDAP
module Person
def ssh_keys
if config.sync_ssh_keys? && entry.respond_to?(config.sync_ssh_keys)
entry[config.sync_ssh_keys.to_sym].
map { |key| key[/(ssh|ecdsa)-[^ ]+ [^\s]+/] }.
compact
else
[]
end
end
def kerberos_principal
# The following is only meaningful for Active Directory
return unless entry.respond_to?(:sAMAccountName)
entry[:sAMAccountName].first + '@' + windows_domain_name.upcase
end
def windows_domain_name
# The following is only meaningful for Active Directory
require 'net/ldap/dn'
dn_components = []
Net::LDAP::DN.new(dn).each_pair { |name, value| dn_components << { name: name, value: value } }
dn_components.
reverse.
take_while { |rdn| rdn[:name].casecmp('DC').zero? }. # Domain Component
map { |rdn| rdn[:value] }.
reverse.
join('.')
end
end
end
end
end
......@@ -29,31 +29,7 @@ module Gitlab
end
def users(field, value, limit = nil)
if field.to_sym == :dn
options = {
base: value,
scope: Net::LDAP::SearchScope_BaseObject
}
else
options = {
base: config.base,
filter: Net::LDAP::Filter.eq(field, value)
}
end
if config.user_filter.present?
user_filter = Net::LDAP::Filter.construct(config.user_filter)
options[:filter] = if options[:filter]
Net::LDAP::Filter.join(options[:filter], user_filter)
else
user_filter
end
end
if limit.present?
options.merge!(size: limit)
end
options = user_options(field, value, limit)
entries = ldap_search(options).select do |entry|
entry.respond_to? config.uid
......@@ -68,13 +44,11 @@ module Gitlab
users(*args).first
end
def dns_for_filter(filter)
ldap_search(
base: config.base,
filter: filter,
scope: Net::LDAP::SearchScope_WholeSubtree,
attributes: %w{dn}
).map(&:dn)
def dn_matches_filter?(dn, filter)
ldap_search(base: dn,
filter: filter,
scope: Net::LDAP::SearchScope_BaseObject,
attributes: %w{dn}).any?
end
def ldap_search(*args)
......@@ -98,6 +72,38 @@ module Gitlab
Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds")
[]
end
private
def user_options(field, value, limit)
options = { attributes: %W(#{config.uid} cn mail dn) }
options[:size] = limit if limit
if field.to_sym == :dn
options[:base] = value
options[:scope] = Net::LDAP::SearchScope_BaseObject
options[:filter] = user_filter
else
options[:base] = config.base
options[:filter] = user_filter(Net::LDAP::Filter.eq(field, value))
end
options
end
def user_filter(filter = nil)
if config.user_filter.present?
user_filter = Net::LDAP::Filter.construct(config.user_filter)
end
if user_filter && filter
Net::LDAP::Filter.join(filter, user_filter)
elsif user_filter
user_filter
else
filter
end
end
end
end
end
module Gitlab
module LDAP
class Person
include EE::Gitlab::LDAP::Person
# Active Directory-specific LDAP filter that checks if bit 2 of the
# userAccountControl attribute is set.
# Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/
......@@ -47,35 +49,6 @@ module Gitlab
entry.dn
end
def ssh_keys
if config.sync_ssh_keys? && entry.respond_to?(config.sync_ssh_keys)
entry[config.sync_ssh_keys.to_sym].
map { |key| key[/(ssh|ecdsa)-[^ ]+ [^\s]+/] }.
compact
else
[]
end
end
def kerberos_principal
# The following is only meaningful for Active Directory
return unless entry.respond_to?(:sAMAccountName)
entry[:sAMAccountName].first + '@' + windows_domain_name.upcase
end
def windows_domain_name
# The following is only meaningful for Active Directory
require 'net/ldap/dn'
dn_components = []
Net::LDAP::DN.new(dn).each_pair { |name, value| dn_components << { name: name, value: value } }
dn_components.
reverse.
take_while { |rdn| rdn[:name].casecmp('DC').zero? }. # Domain Component
map { |rdn| rdn[:value] }.
reverse.
join('.')
end
private
def entry
......
require 'spec_helper'
# Test things specific to the EE mixin, but run the actual tests
# against the main adapter class to ensure it's properly included
describe Gitlab::LDAP::Adapter, lib: true do
subject { Gitlab::LDAP::Adapter.new 'ldapmain' }
include LdapHelpers
it { is_expected.to include_module(EE::Gitlab::LDAP::Adapter) }
it 'includes the EE module' do
expect(Gitlab::LDAP::Adapter).to include_module(EE::Gitlab::LDAP::Adapter)
end
describe '#groups' do
let(:adapter) { ldap_adapter('ldapmain') }
before do
stub_ldap_config(
group_base: 'ou=groups,dc=example,dc=com',
active_directory: false
)
end
it 'searches with the proper options' do
# Requires this expectation style to match the filter
expect(adapter).to receive(:ldap_search) do |arg|
expect(arg[:filter].to_s).to eq('(cn=*)')
expect(arg[:base]).to eq('ou=groups,dc=example,dc=com')
expect(arg[:attributes]).to match(%w(dn cn memberuid member submember uniquemember memberof))
end.and_return({})
adapter.groups
end
it 'returns a group object if search returns a result' do
entry = ldap_group_entry(['john', 'mary'], cn: 'group1')
allow(adapter).to receive(:ldap_search).and_return([entry])
results = adapter.groups('group1')
expect(results.first).to be_a(EE::Gitlab::LDAP::Group)
expect(results.first.cn).to eq('group1')
expect(results.first.member_dns).to match_array(%w(john mary))
end
end
end
require 'spec_helper'
describe EE::Gitlab::LDAP::Group, lib: true do
include LdapHelpers
let(:adapter) { ldap_adapter }
describe '#member_dns' do
def ldif
Net::LDAP::Entry.from_single_ldif_string(
......@@ -17,10 +21,6 @@ describe EE::Gitlab::LDAP::Group, lib: true do
)
end
def adapter
@adapter ||= Gitlab::LDAP::Adapter.new('ldapmain')
end
let(:group) { described_class.new(ldif, adapter) }
let(:recursive_dns) do
%w(
......
require "spec_helper"
require 'spec_helper'
describe Gitlab::LDAP::Person do
describe "#kerberos_principal" do
it 'includes the EE module' do
expect(Gitlab::LDAP::Person).to include(EE::Gitlab::LDAP::Person)
end
describe '#kerberos_principal' do
let(:entry) do
ldif = "dn: cn=foo, dc=bar, dc=com\n"
ldif += "sAMAccountName: #{sam_account_name}\n" if sam_account_name
......@@ -10,25 +14,25 @@ describe Gitlab::LDAP::Person do
subject { Gitlab::LDAP::Person.new(entry, 'ldapmain') }
context "when sAMAccountName is not defined (non-AD LDAP server)" do
context 'when sAMAccountName is not defined (non-AD LDAP server)' do
let(:sam_account_name) { nil }
it "returns nil" do
it 'returns nil' do
expect(subject.kerberos_principal).to be_nil
end
end
context "when sAMAccountName is defined (AD server)" do
let(:sam_account_name) { "mylogin" }
context 'when sAMAccountName is defined (AD server)' do
let(:sam_account_name) { 'mylogin' }
it "returns the principal combining sAMAccountName and DC components of the distinguishedName" do
expect(subject.kerberos_principal).to eq("mylogin@BAR.COM")
it 'returns the principal combining sAMAccountName and DC components of the distinguishedName' do
expect(subject.kerberos_principal).to eq('mylogin@BAR.COM')
end
end
end
describe "#ssh_keys" do
let(:ssh_key) { "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj" }
describe '#ssh_keys' do
let(:ssh_key) { 'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCrSQHff6a1rMqBdHFt+FwIbytMZ+hJKN3KLkTtOWtSvNIriGhnTdn4rs+tjD/w+z+revytyWnMDM9dS7J8vQi006B16+hc9Xf82crqRoPRDnBytgAFFQY1G/55ql2zdfsC5yvpDOFzuwIJq5dNGsojS82t6HNmmKPq130fzsenFnj5v1pl3OJvk513oduUyKiZBGTroWTn7H/eOPtu7s9MD7pAdEjqYKFLeaKmyidiLmLqQlCRj3Tl2U9oyFg4PYNc0bL5FZJ/Z6t0Ds3i/a2RanQiKxrvgu3GSnUKMx7WIX373baL4jeM7cprRGiOY/1NcS+1cAjfJ8oaxQF/1dYj' }
let(:ssh_key_attribute_name) { 'altSecurityIdentities' }
let(:entry) do
Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com\n#{keys}")
......@@ -40,53 +44,53 @@ describe Gitlab::LDAP::Person do
allow_any_instance_of(Gitlab::LDAP::Config).to receive_messages(sync_ssh_keys: ssh_key_attribute_name)
end
context "when the SSH key is literal" do
context 'when the SSH key is literal' do
let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}" }
it "includes the SSH key" do
it 'includes the SSH key' do
expect(subject.ssh_keys).to include(ssh_key)
end
end
context "when the SSH key is prefixed" do
context 'when the SSH key is prefixed' do
let(:keys) { "#{ssh_key_attribute_name}: SSHKey:#{ssh_key}" }
it "includes the SSH key" do
it 'includes the SSH key' do
expect(subject.ssh_keys).to include(ssh_key)
end
end
context "when the SSH key is suffixed" do
context 'when the SSH key is suffixed' do
let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key} (SSH key)" }
it "includes the SSH key" do
it 'includes the SSH key' do
expect(subject.ssh_keys).to include(ssh_key)
end
end
context "when the SSH key is followed by a newline" do
context 'when the SSH key is followed by a newline' do
let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}\n" }
it "includes the SSH key" do
it 'includes the SSH key' do
expect(subject.ssh_keys).to include(ssh_key)
end
end
context "when the key is not an SSH key" do
context 'when the key is not an SSH key' do
let(:keys) { "#{ssh_key_attribute_name}: KerberosKey:bogus" }
it "is empty" do
it 'is empty' do
expect(subject.ssh_keys).to be_empty
end
end
context "when there are multiple keys" do
context 'when there are multiple keys' do
let(:keys) { "#{ssh_key_attribute_name}: #{ssh_key}\n#{ssh_key_attribute_name}: KerberosKey:bogus\n#{ssh_key_attribute_name}: ssh-rsa keykeykey" }
it "includes both SSH keys" do
it 'includes both SSH keys' do
expect(subject.ssh_keys).to include(ssh_key)
expect(subject.ssh_keys).to include("ssh-rsa keykeykey")
expect(subject.ssh_keys).not_to include("KerberosKey:bogus")
expect(subject.ssh_keys).to include('ssh-rsa keykeykey')
expect(subject.ssh_keys).not_to include('KerberosKey:bogus')
end
end
end
......
......@@ -3,6 +3,8 @@ require 'spec_helper'
describe EE::Gitlab::LDAP::Sync::AdminUsers, lib: true do
include LdapHelpers
let(:adapter) { ldap_adapter }
describe '#update_permissions' do
let(:sync_admin) { described_class.new(proxy(adapter)) }
......
......@@ -4,6 +4,7 @@ describe EE::Gitlab::LDAP::Sync::ExternalUsers, lib: true do
include LdapHelpers
describe '#update_permissions' do
let(:adapter) { ldap_adapter }
let(:sync_external) { described_class.new(proxy(adapter)) }
let(:user1) { create(:user) }
let(:user2) { create(:user) }
......
......@@ -3,6 +3,7 @@ require 'spec_helper'
describe EE::Gitlab::LDAP::Sync::Group, lib: true do
include LdapHelpers
let(:adapter) { ldap_adapter }
let(:sync_group) { described_class.new(group, proxy(adapter)) }
let(:user) { create(:user) }
......
......@@ -3,6 +3,7 @@ require 'spec_helper'
describe EE::Gitlab::LDAP::Sync::Groups, lib: true do
include LdapHelpers
let(:adapter) { ldap_adapter }
let(:group_sync) { described_class.new(proxy(adapter)) }
describe '#update_permissions' do
......
......@@ -4,6 +4,7 @@ require 'net/ldap/dn'
describe EE::Gitlab::LDAP::Sync::Proxy, lib: true do
include LdapHelpers
let(:adapter) { ldap_adapter }
let(:sync_proxy) { EE::Gitlab::LDAP::Sync::Proxy.new('ldapmain', adapter) }
before do
......
require 'spec_helper'
describe Gitlab::LDAP::Adapter, lib: true do
let(:adapter) { Gitlab::LDAP::Adapter.new 'ldapmain' }
include LdapHelpers
let(:ldap) { double(:ldap) }
let(:adapter) { ldap_adapter('ldapmain', ldap) }
describe '#users' do
before do
stub_ldap_config(base: 'dc=example,dc=com')
end
it 'searches with the proper options when searching by uid' do
# Requires this expectation style to match the filter
expect(adapter).to receive(:ldap_search) do |arg|
expect(arg[:filter].to_s).to eq('(uid=johndoe)')
expect(arg[:base]).to eq('dc=example,dc=com')
expect(arg[:attributes]).to match(%w{uid cn mail dn})
end.and_return({})
adapter.users('uid', 'johndoe')
end
it 'searches with the proper options when searching by dn' do
expect(adapter).to receive(:ldap_search).with(
base: 'uid=johndoe,ou=users,dc=example,dc=com',
scope: Net::LDAP::SearchScope_BaseObject,
attributes: %w{uid cn mail dn},
filter: nil
).and_return({})
adapter.users('dn', 'uid=johndoe,ou=users,dc=example,dc=com')
end
it 'searches with the proper options when searching with a limit' do
expect(adapter)
.to receive(:ldap_search).with(hash_including(size: 100)).and_return({})
adapter.users('uid', 'johndoe', 100)
end
it 'returns an LDAP::Person if search returns a result' do
entry = ldap_user_entry('johndoe')
allow(adapter).to receive(:ldap_search).and_return([entry])
results = adapter.users('uid', 'johndoe')
expect(results.size).to eq(1)
expect(results.first.uid).to eq('johndoe')
end
it 'returns empty array if search entry does not respond to uid' do
entry = Net::LDAP::Entry.new
entry['dn'] = user_dn('johndoe')
allow(adapter).to receive(:ldap_search).and_return([entry])
results = adapter.users('uid', 'johndoe')
expect(results).to be_empty
end
it 'uses the right uid attribute when non-default' do
stub_ldap_config(uid: 'sAMAccountName')
expect(adapter).to receive(:ldap_search).with(
hash_including(attributes: %w{sAMAccountName cn mail dn})
).and_return({})
adapter.users('sAMAccountName', 'johndoe')
end
end
describe '#dn_matches_filter?' do
let(:ldap) { double(:ldap) }
subject { adapter.dn_matches_filter?(:dn, :filter) }
before { allow(adapter).to receive(:ldap).and_return(ldap) }
context "when the search is successful" do
context "and the result is non-empty" do
......
module EE
module LdapHelpers
def proxy(adapter, provider = 'ldapmain')
EE::Gitlab::LDAP::Sync::Proxy.new(provider, adapter)
end
# Stub an LDAP group search and provide the return entry. Specify `nil` for
# `entry` to simulate when an LDAP group is not found
#
# Example:
# adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap))
# ldap_group1 = ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
#
# stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter)
def stub_ldap_group_find_by_cn(cn, entry, adapter = nil)
if entry.present?
return_value = EE::Gitlab::LDAP::Group.new(entry, adapter)
end
allow(EE::Gitlab::LDAP::Group)
.to receive(:find_by_cn)
.with(cn, kind_of(::Gitlab::LDAP::Adapter)).and_return(return_value)
end
# Create an LDAP group entry with any number of members. By default, creates
# a groupOfNames style entry. Change the style by specifying the object class
# and member attribute name. The last example below shows how to specify a
# posixGroup (Apple Open Directory) entry. `members` can be nil to create
# an empty group.
#
# Example:
# ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
#
# ldap_group_entry(
# 'uid=user1,ou=users,dc=example,dc=com',
# 'uid=user2,ou=users,dc=example,dc=com'
# )
#
# ldap_group_entry(
# [ 'user1', 'user2' ],
# cn: 'my_group'
# objectclass: 'posixGroup',
# member_attr: 'memberUid'
# )
def ldap_group_entry(
members,
cn: 'ldap_group1',
objectclass: 'groupOfNames',
member_attr: 'uniqueMember'
)
entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
dn: cn=#{cn},ou=groups,dc=example,dc=com
cn: #{cn}
description: LDAP Group #{cn}
objectclass: top
objectclass: #{objectclass}
EOS
members = [members].flatten
entry[member_attr] = members if members.any?
entry
end
end
end
module LdapHelpers
def adapter(provider = 'ldapmain')
::Gitlab::LDAP::Adapter.new(provider, double(:ldap))
end
include EE::LdapHelpers
def proxy(adapter, provider = 'ldapmain')
EE::Gitlab::LDAP::Sync::Proxy.new(provider, adapter)
def ldap_adapter(provider = 'ldapmain', ldap = double(:ldap))
::Gitlab::LDAP::Adapter.new(provider, ldap)
end
def user_dn(uid)
......@@ -34,77 +32,18 @@ module LdapHelpers
#
# stub_ldap_person_find_by_uid('john_doe', ldap_user_entry, adapter)
def stub_ldap_person_find_by_uid(uid, entry, provider = 'ldapmain')
return_value = if entry.present?
::Gitlab::LDAP::Person.new(entry, provider)
else
nil
end
return_value = ::Gitlab::LDAP::Person.new(entry, provider) if entry.present?
allow(::Gitlab::LDAP::Person)
.to receive(:find_by_uid).with(uid, any_args).and_return(return_value)
end
# Stub an LDAP group search and provide the return entry. Specify `nil` for
# `entry` to simulate when an LDAP group is not found
#
# Example:
# adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap))
# ldap_group1 = ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
#
# stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter)
def stub_ldap_group_find_by_cn(cn, entry, adapter = nil)
return_value = if entry.present?
EE::Gitlab::LDAP::Group.new(entry, adapter)
else
nil
end
allow(EE::Gitlab::LDAP::Group)
.to receive(:find_by_cn)
.with(cn, kind_of(::Gitlab::LDAP::Adapter)).and_return(return_value)
end
# Create a simple LDAP user entry.
def ldap_user_entry(uid)
Net::LDAP::Entry.from_single_ldif_string("dn: #{user_dn(uid)}")
end
# Create an LDAP group entry with any number of members. By default, creates
# a groupOfNames style entry. Change the style by specifying the object class
# and member attribute name. The last example below shows how to specify a
# posixGroup (Apple Open Directory) entry. `members` can be nil to create
# an empty group.
#
# Example:
# ldap_group_entry('uid=user,ou=users,dc=example,dc=com')
#
# ldap_group_entry(
# 'uid=user1,ou=users,dc=example,dc=com',
# 'uid=user2,ou=users,dc=example,dc=com'
# )
#
# ldap_group_entry(
# [ 'user1', 'user2' ],
# cn: 'my_group'
# objectclass: 'posixGroup',
# member_attr: 'memberUid'
# )
def ldap_group_entry(
members,
cn: 'ldap_group1',
objectclass: 'groupOfNames',
member_attr: 'uniqueMember'
)
entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
dn: cn=#{cn},ou=groups,dc=example,dc=com
cn: #{cn}
description: LDAP Group #{cn}
objectclass: top
objectclass: #{objectclass}
EOS
entry = Net::LDAP::Entry.new
entry['dn'] = user_dn(uid)
entry['uid'] = uid
members = [members].flatten
entry[member_attr] = members if members.any?
entry
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment