Skip to content

Multi Domain query #89

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
timmjd wants to merge 2 commits into from
Closed

Multi Domain query #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c945ce
fix travis to update gems before build
timmjd Apr 17, 2016
0ba3966
Implement forest search to obtain non universal groups from ad
timmjd Apr 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ env:
- TESTENV=openldap
- TESTENV=apacheds

before_install:
- gem update bundler

install:
- if [ "$TESTENV" = "openldap" ]; then ./script/install-openldap; fi
- bundle install
Expand Down
61 changes: 58 additions & 3 deletions lib/github/ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ def initialize(options = {})

# enables instrumenting queries
@instrumentation_service = options[:instrumentation_service]

# active directory forest
@forest = get_domain_forest(options[:search_forest])
Copy link
Author

@timmjd timmjd May 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requries options[:search_forest] to be provided from the outside

Copy link

@gnawhleinad gnawhleinad May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Enterprise instantiates a global strategy. As such, we need to provide an interface to refresh the list of domain controllers so that a configuration update does not require a restart of the appliance.

end

# Public - Whether membership checks should recurse into nested groups when
Expand Down Expand Up @@ -180,10 +183,10 @@ def search(options, &block)
instrument "search.github_ldap", options.dup do |payload|
result =
if options[:base]
@connection.search(options, &block)
forest_search(options, &block)
else
search_domains.each_with_object([]) do |base, result|
rs = @connection.search(options.merge(:base => base), &block)
rs = forest_search(options.merge(:base => base), &block)
result.concat Array(rs) unless rs == false
end
Copy link
Author

@timmjd timmjd May 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never seen the tree (options[:base] == empty) to be called during debugging. The loop logic to iterate over search_domains seems to be always implemented outside the library within GHE

end
Expand All @@ -193,6 +196,26 @@ def search(options, &block)
end
end

# Internal: Search within a ldap forest
#
# Returns an Array of Net::LDAP::Entry.
def forest_search(options, &block)
instrument "forest_search.github_ldap" do |payload|
result =
if @forest.empty?
@connection.search(options, &block)
else
@forest.each_with_object([]) do |(rootdn, server), res|
if options[:base].end_with?(rootdn)
Copy link
Author

@timmjd timmjd May 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires to use end_with?() and not eql?() which will result in unnecessary requests if you have a setup like DC=foo,DC=bar,DC=com & DC=bar,DC=com. Issue is that calls to the library contain requests where the basedn points to the object of interest.

  • Login to GHE for example will query the full path: options[:base]='CN=user,OU=org,DC=foo,DC=bar,DC=com'
  • Possible fix would be to form such queries (outside the library) to search for the distinguished name: options[:filter]='distinguishedName=CN=user,OU=org,DC=foo,DC=bar,DC=com' with a basedn options[:base]='DC=foo,DC=bar,DC=com'

rs = server.search(options, &block)
res.concat Array(rs) unless rs == false
end
end
end
return result
end
end

# Internal: Searches the host LDAP server's Root DSE for capabilities and
# extensions.
#
Expand All @@ -201,7 +224,8 @@ def capabilities
@capabilities ||=
instrument "capabilities.github_ldap" do |payload|
begin
@connection.search_root_dse
rs = @connection.search(:ignore_server_caps => true, :base => "", :scope => Net::LDAP::SearchScope_BaseObject)
(rs and rs.first)
rescue Net::LDAP::LdapError => error
payload[:error] = error
# stubbed result
Expand Down Expand Up @@ -307,6 +331,37 @@ def configure_member_search_strategy(strategy = nil)
end
end

# Internal: Queries configuration for available domains
#
# Membership of local or global groups need to be evaluated by contacting referral Donmain Controllers
Copy link

@gnawhleinad gnawhleinad May 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Donmain/Domain/g

#
# Returns all Domain Controllers within the forest
def get_domain_forest(search_forest)
instrument "get_domain_forest.github_ldap" do |payload|

# if we are talking to an active directory
if search_forest and active_directory_capability? and capabilities[:configurationnamingcontext].any?
domains = @connection.search(
base: capabilities[:configurationnamingcontext].first,
search_referrals: true,
filter: Net::LDAP::Filter.eq("nETBIOSName", "*")
)
return domains.each_with_object({}) do |server, result|
if server[:ncname].any? and server[:dnsroot].any?
result[server[:ncname].first] = Net::LDAP.new({
host: server[:dnsroot].first,
port: @connection.instance_variable_get(:@encryption)? 636 : 389,
auth: @connection.instance_variable_get(:@auth),
encryption: @connection.instance_variable_get(:@encryption),
instrumentation_service: @connection.instance_variable_get(:@instrumentation_service)
})
end
end
end
return {}
end
end

# Internal: Detect whether the LDAP host is an ActiveDirectory server.
#
# See: http://msdn.microsoft.com/en-us/library/cc223359.aspx.
Expand Down