connector/ldap: Add LDAP simple bind mode.

[batatch]

Supports LDAP simple bind mode
If BindDNPrefix or BindDNSuffix are set, BindDNPrefix username BindDNSuffix is used as BindDN.

[the-maldridge]

This is a feature I'd really like to have. Is there anything I can do to assist with testing? My environment is very service-principal-averse, so all our other services besides dex bind directly as the user they're authenticating. Would be nice to have that here as well.

[batatch]

Hi, the-maldridge. Thanks for your comment.
I'd be glad you to try it in your environment if you possible.

[tooptoop4]

@sagikazarmark @seankhliao @vsychov @nabokihms can you please merge?

I'm setting suffix as '@my.company.domain' so I can use incoming ActiveDirectory user creds to bind to ldap instead of relying on a common service/generic account to bind for all incoming users

[sagikazarmark]

Thanks for the contribution!

Can you please take a look at my comment?

Also, can you please add tests making sure this feature works? Thanks!

[tooptoop4]

@batatch are u still working on this?

[batatch]

@tooptoop4 ok, i'll try it.

[batatch]

@sagikazarmark
I processed some steps were required.
Can you please merge it?

[sagikazarmark]

Thanks for making the changes and sorry for the long delay.

The PR looks good to me, but I'd like to raise a question before we merge it (bear in mind I'm not that familiar with how apps generally integrate LDAP):

I'm a little bit concerned that it may be too easy to provide invalid data in the configuration that could result in errors.

For example: if you don't provide a prefix, but you provide a suffix, the DN could look something like this: username,cn=users

Another concern (and again, not sure how other applications do it:

What if the username contains special characters? For example: myuser,cn=admins?

Then the DN will look like this: uid=myuser,cn=admins

Here comes the question: would it make sense to add some better validation and/or escaping to make sure the DN is correct?

Also (and once again, no idea if this is an accepted practice or not), I'm thinking maybe we should introduce these options instead:

bindDN: "uid={username},cn=users..."
simpleBind: true

When simpleBind is true, the DN would become a template string replacing {username} with the actual username. That way we need less validation/rely less on the presence of a prefix/suffix. (We probably still need escaping)

What do you think?

[batatch]

Thank you for your comment, sagikazarmark-san.

The configuration method for specifying prefix and suffix is based on the most commonly found PostgreSQL example and more.
(eg. https://www.postgresql.org/docs/current/auth-ldap.html)

If we use templates, I couldn't determine which template method to use, as it requires an uniform rules in configuration.
(like printf style, parameter name in braces style, go-lang template, and original style)

I think the boolean flag of simpleBind=true and the template setting definitions to be redundant,
since it requires a decision based on a combination of two parameters.
(if set simpleBind=true, but template undefined, then it is fail of configuration.)
If we use template definition, I think it would be better to make the decision based on exist or not the template definition.

In the validation of prohibited characters in usernames, "," might be better to be checked.
and other than that, I think it is fine to left to the LDAP server.
since the dexidp itself does not store usernames and only relays user inputs to the LDAP server.

[sagikazarmark]

If we use templates, I couldn't determine which template method to use

Obviously, it would have to be documented.

I think the boolean flag of simpleBind=true and the template setting definitions to be redundant,

I don't think it's any more redundant than the current proposal. Simple bind setting would essentially change the behavior of how the other parameter is used.

and other than that, I think it is fine to left to the LDAP server.

I would argue this is actually a potential security issue, since usernames are accepted as an input entered by a user. This could essentially be the LDAP version of an SQL injection, so the user name MUST be escaped somehow. No idea if there is any builtin tool for that in the LDAP library (ie. something like prepared statements in SQL).

[tooptoop4]

🙏