instaclustr / cassandra-ldap

LDAP Authenticator for Apache Cassandra
Apache License 2.0
24 stars 16 forks source link

reason to do search before auth. #27

Open victron opened 2 years ago

victron commented 2 years ago

Please answer these questions before submitting your issue. Thanks!

What version of Cassandra are you using?

3.11

What version of Cassandra LDAP are you using?

v3.11.11-1.0.0

What LDAP server you are using? Any specifics?

osixia/docker-openldap

What did you do?

simple authentication for user test

What did you expect to see?

in ldap server logs I expect to see only this logs

630b27c9 conn=1141 fd=13 ACCEPT from IP=172.17.0.1:46154 (IP=0.0.0.0:389)
630b27c9 conn=1141 op=0 BIND dn="cn=test,dc=example,dc=org" method=128
630b27c9 conn=1141 op=0 BIND dn="cn=test,dc=example,dc=org" mech=SIMPLE ssf=0
630b27c9 conn=1141 op=0 RESULT tag=97 err=0 text=

What did you see instead?

before expected logs I see this logs

630b27c9 conn=1140 fd=12 ACCEPT from IP=172.17.0.1:46150 (IP=0.0.0.0:389)
630b27c9 conn=1140 op=0 BIND dn="cn=admin,dc=example,dc=org" method=128
630b27c9 conn=1140 op=0 BIND dn="cn=admin,dc=example,dc=org" mech=SIMPLE ssf=0
630b27c9 conn=1140 op=0 RESULT tag=97 err=0 text=
630b27c9 conn=1140 op=1 SRCH base="dc=example,dc=org" scope=2 deref=3 filter="(cn=test)"
630b27c9 <= mdb_equality_candidates: (cn) not indexed
630b27c9 conn=1140 op=1 SEARCH RESULT tag=101 err=0 nentries=1 text=

I suspect this is related to this line in README:

You may have a different search filter based on your need, a lot of people use e.g. SAM or something similar.

My question:

  1. I trying to understand does it really need every time to do search by admin user before test user authentication? For me it looks like an additional load to ldap server and could have some hidden security issue.
  2. Does it possible change logic in config?
smiklosovic commented 2 years ago

Hi, I think it was done like that because admin is the one who is performing search. We first seach in ldap to filter a user. Then we use that user (its name) together with password on cqlsh to try to authenticate. For that reason, that we search first, I think we need somebody with admin rights who is able to scan ldap tree. It is not automatically given that you can scan the tree under every "ordinary" user. But admin can.

victron commented 2 years ago

I think we need somebody with admin rights who is able to scan ldap tree.

yes, exactly. but I still don't understand why it need to do search at all? we can create filter in config like this cn=%s,dc=example,dc=org and using it format string based on username. This string will be a dn. Then during login user test pluging will request auth. directly cn=test,dc=example,dc=org.

I understand that we need admin to manipulate with system_auth keyspace. But My main question why pluging doing search in LDAP at all? Why not to allow LDAP replay with error if there are no such dn?

smiklosovic commented 2 years ago

Not sure to be honest, I ll try it when I have time. Thanks for the idea.

victron commented 2 years ago

below simple patch to test idea, now in logs only what I expected. Hope it helps. May be it reasonable to add some additional config option to keep compatibility. Some people could really have complicated LDAP tree, but mostly have simple.

[vic@cOs2 cassandra-ldap]$ git diff master
diff --git a/base/src/main/java/com/instaclustr/cassandra/ldap/auth/DefaultLDAPServer.java b/base/src/main/java/com/instaclustr/cassandra/ldap/auth/DefaultLDA
index 461aa1d..d244ccc 100644
--- a/base/src/main/java/com/instaclustr/cassandra/ldap/auth/DefaultLDAPServer.java
+++ b/base/src/main/java/com/instaclustr/cassandra/ldap/auth/DefaultLDAPServer.java
@@ -182,9 +182,10 @@ public class DefaultLDAPServer extends LDAPUserRetriever
     @Override
     public User retrieve(User user) throws LDAPAuthFailedException
     {
-        try (final LDAPInitialContext context = new LDAPInitialContext(properties))
+        try
         {
-            final String ldapDn = context.searchLdapDN(user.getUsername());
+            final String filterTemplate = properties.getProperty(LdapAuthenticatorConfiguration.FILTER_TEMPLATE);
+            final String ldapDn = format(filterTemplate, user.getUsername());

             logger.debug(String.format("Resolved LDAP DN: %s", ldapDn));

diff --git a/conf/ldap.properties b/conf/ldap.properties
index b6091e8..0ba4d3d 100644
--- a/conf/ldap.properties
+++ b/conf/ldap.properties
@@ -14,7 +14,7 @@ service_dn: cn=admin,dc=example,dc=org
 service_password: admin

 # filter used for searching in LDAP, "%s" is placeholder, it will be replaced by login name
-filter_template: (cn=%s)
+filter_template: cn=%s,dc=example,dc=org

 # True by default, tells whether internal cache of user -> password combination will be used
 # This option is irrelevant for Cassandra version <= 3.0
smiklosovic commented 2 years ago

A property in config to turn this on would be nice. Feel free to complete the patch with introducing a flag so we do not need to search.