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

reason to do search before auth. #27

Open
victron opened this issue Sep 6, 2022 · 5 comments
Open

reason to do search before auth. #27

victron opened this issue Sep 6, 2022 · 5 comments

Comments

@victron
Copy link

victron commented Sep 6, 2022

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
Copy link
Collaborator

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
Copy link
Author

victron commented Sep 6, 2022

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
Copy link
Collaborator

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

@victron
Copy link
Author

victron commented Sep 7, 2022

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
Copy link
Collaborator

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.

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

No branches or pull requests

2 participants