Skip to content

Conversation

@davesims
Copy link
Contributor

@davesims davesims commented Jul 25, 2016

For Active Directory deployments, in some cases, like authentication or simple entry searches, it will make sense to search the Global Catalog rather than the default configured Domain Controller. This PR will initialize a Global Catalog connection object when requested, and provides an interface to directly query the catalog. GitHub::Ldap::Domain will now use the Global Catalog for user?, if the server is an Active Directory, and the configured Domain Controller is a Global Catalog, or if the user has provided global catalog settings in the initializer options.

TODO:
  • Initialize global catalog connection
  • Make interface to query the catalog
  • Have GitHub::Ldap::Domain#user? use the catalog if server is AD & catalog is present
  • Allow user to provide Global Catalog host & port, otherwise default to the given Domain Controller

This begins an alternative approach to #91. To fully replace that, we'll have to also implement referral chasing to be able to search for groups that aren't configured to be Active Directory "universal" groups. I'll do that in a separate PR.

/cc @mtodd @jch @sbryant @lildude @timmjd
/cc @github/ldap

@davesims davesims force-pushed the use_global_catalog_for_auth branch from b75644e to a00750f Compare July 25, 2016 16:38
def active_directory_capability?
capabilities[:supportedcapabilities].include?(ACTIVE_DIRECTORY_V51_OID)
end
private :active_directory_capability?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this public since Domain now needs to pivot on whether the server is an AD or not.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't true anymore, is it?

@davesims davesims changed the title Adding support for querying Active Directory Global Catalog Support querying Active Directory Global Catalog Jul 25, 2016
@davesims davesims force-pushed the use_global_catalog_for_auth branch from 5b54b07 to 3af0a88 Compare July 25, 2016 18:59

group :test, :development do
gem "byebug", :platforms => [:mri_20, :mri_21]
gem "mocha"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case for adding mocha in development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly as a convenience for cases like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you mean split the group block. :bowtie:

👍


group :test do
gem "mocha"
end
Copy link
Member

Choose a reason for hiding this comment

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

👍

base: "CN=HI,CN=McDunnough",
filter: kind_of(Net::LDAP::Filter)
))
results = @default_user_search.perform("","CN=HI,CN=McDunnough","",{})
Copy link
Member

Choose a reason for hiding this comment

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

results = here isn't used.

@mtodd
Copy link
Member

mtodd commented Aug 4, 2016

No critical blockers, this is super close!

# When doing a global search for a user's DN, set the search base to blank
def options
super.merge(base: "")
end
Copy link
Member

Choose a reason for hiding this comment

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

👍 looks good.

@davesims
Copy link
Contributor Author

davesims commented Aug 4, 2016

@jch @mtodd Updated per review, ready for final 👀 on this when you get a chance.


# Keep a reference to these as default auth for a Global Catalog if needed
@admin_user = options[:admin_user]
@admin_password = options[:admin_password]
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker but it might be worth passing these into configure_user_search_strategy (by passing through the options hash) to let that method decide if it needs these, so we don't have Global Catalog concerns leaking out unnecessarily.

@mtodd
Copy link
Member

mtodd commented Aug 4, 2016

@davesims wouldn't consider anything ☝️ above as blocking feedback. :shipit:

@mtodd
Copy link
Member

mtodd commented Aug 4, 2016

Just noticing that builds were't enabled. Would like to kick off a build to verify these changes.

@davesims
Copy link
Contributor Author

davesims commented Aug 5, 2016

This was shadowed by merging #95 first. Closing.

@davesims davesims closed this Aug 5, 2016
@davesims davesims deleted the use_global_catalog_for_auth branch August 5, 2016 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants