Skip to content
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.

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

connector/ldap: expand LDAP connector to include searches #624

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

ericchiang
Copy link
Contributor

wip will expand docs tomorrow and add an groups search mechanism.

cc @rithujohn191 @sym3tri

@ericchiang ericchiang force-pushed the dev-ldap-connector branch 5 times, most recently from 20fbde7 to 1005bdd Compare October 21, 2016 21:15
@ericchiang
Copy link
Contributor Author

cc @dghubble @squat @sym3tri can you take a look at this and see if it makes sense?

I need to finish up some of the details and don't expect direct comment on the LDAP logic, but would appreciate comments on the documentation and configuration.

for _, group := range groups {
name := getAttr(*group, c.GroupSearch.NameAttr)
if name == "" {
// Be obnoxious about missing missing attributes. If the group entry is
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seem reasonable to everyone?

//
// An example config:
//
// type: ldap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yay! giant configs!

@ericchiang
Copy link
Contributor Author

FYI some background on the existing LDAP connector

// # Would translate to the query "(&(objectClass=person)&(uid=<username>))"
// baseDN: cn=users,dc=example,dc=com
// filter: "(objectClass=person)"
// username: uid
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how much of this is just ldap or how much is your adaptation. I think of username and uid as different

Copy link
Contributor Author

@ericchiang ericchiang Oct 21, 2016

Choose a reason for hiding this comment

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

FreeIPA actually uses "uid" as the human readable name you enter when doing binds (edit: logging in). For example here's my LDAP entry:

dn: uid=echiang,cn=users,cn=accounts,dc=core,dc=example,dc=so
displayName: Eric Chiang
uid: echiang
objectClass: ipaobject
objectClass: person
objectClass: top
objectClass: ipasshuser
objectClass: inetorgperson
objectClass: organizationalperson
objectClass: krbticketpolicyaux
objectClass: krbprincipalaux
objectClass: inetuser
objectClass: posixaccount
objectClass: ipaSshGroupOfPubKeys
objectClass: mepOriginEntry
loginShell: /bin/bash
initials: EC
gecos: Eric Chiang
sn: Chiang
homeDirectory: /home/echiang
givenName: Eric
cn: Eric Chiang
uidNumber: 300015
gidNumber: 300015

// Path to a trusted root certificate file.
RootCA string `yaml:"rootCA"`

// BindDN and BindPW for an application service account. The connector uses these
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized that these can totally be empty. Some LDAP configurations allow enough anonymous access that you don't need a service account.

}

// TODO(ericchiang): actually implement this.
func escapeFilter(s string) string { return s }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still need to implement this.

Host string `yaml:"host"`

// Path to a trusted root certificate file.
RootCA string `yaml:"rootCA"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is slightly different from the existing postgres args (which is caFile). Will try to make them more similar.

@ericchiang ericchiang force-pushed the dev-ldap-connector branch 2 times, most recently from 906a58e to b71e6ff Compare October 27, 2016 00:13
@ericchiang
Copy link
Contributor Author

Still need to do filter escaping.

switch n := len(resp.Entries); n {
case 0:
return fmt.Errorf("ldap: no results returned for filter: %q", filter)
case 2:

Choose a reason for hiding this comment

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

What if the search returns more than 2 results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Didn't catch that! Will send a fix.

@ericchiang ericchiang deleted the dev-ldap-connector branch November 22, 2016 20:08
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.

4 participants