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

Populating LDAP config via os.environ.get is incompatible with LDAPGroupQuery #290

Closed
wenners opened this issue May 13, 2020 · 9 comments
Closed
Labels
help wanted We seek out help for implementing this issue.
Milestone

Comments

@wenners
Copy link

wenners commented May 13, 2020

Current Behavior

Trying to allow more than one group for AUTH_LDAP_REQUIRE_GROUP. Following the example described in https://django-auth-ldap.readthedocs.io/en/latest/groups.html#limiting-access
Whentrying to authenticate I get an error
Caught LDAPError while authenticating xxxxxxxxxx: INVALID_DN_SYNTAX({'desc': 'Invalid DN syntax', 'info': 'Invalid DN'})

Expected Behavior

Authentication should check against both (or more) groups

Debug Information

The problem results from the way the config file reads the variables:

# Define a group required to login.
AUTH_LDAP_REQUIRE_GROUP = os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN', '')

There the environment variable is read and assigned to AUTH_LDAP_REQUIRE_GROUP. Unfortunately in this case the variable contains a python function that would convert the string in the environment-variable to the type django_auth_ldap.config.LDAPGroupQuery. But the function is not executed.

Current result

>>> print(AUTH_LDAP_REQUIRE_GROUP)
 (LDAPGroupQuery("cn=ops-groups-1,ou=group,dc=bbbbbb,dc=aaaaaaa") | LDAPGroupQuery("cn=ops-groups-2,ou=group,dc=bbbbbb,dc=aaaaaaa"))
>>> print(type(AUTH_LDAP_REQUIRE_GROUP))
<class 'str'>

Expected Behavior

>>> print(AUTH_LDAP_REQUIRE_GROUP)
(OR: cn=ops-groups-1,ou=group,dc=bbbbbb,dc=aaaaaaa, cn=ops-groups-2,ou=group,dc=bbbbbb,dc=aaaaaaa)
>>> print (type(AUTH_LDAP_REQUIRE_GROUP))
<class 'django_auth_ldap.config.LDAPGroupQuery'>
@cimnine
Copy link
Collaborator

cimnine commented May 14, 2020

Thank you for raising this issue. It would help to know exactly which version of Netbox you're on in general.

LDAP is a feature I don't use and neither do I have a test environment for it. But if someone can suggest a PR, I will be happy to review whether it is semantically correct, but I will have no way to check whether it would be correct.

If you haven't done so, try asking in the Slack #netbox-docker channel whether someone has already solved this problem.

@cimnine cimnine added the help wanted We seek out help for implementing this issue. label May 14, 2020
@wenners
Copy link
Author

wenners commented May 15, 2020

Hi,
having this problem with 2.7.12 as well as the latest and greates 2.8.x.
We hacked currently a workaround to the config file but this is not really what I would like to submit as a PR. This should be a common problem if you use K8s configMaps or docker(-compose) env-files and need to call a function...

I will also continue to search for a better solution or refine ours for a PR.

@weisdd
Copy link

weisdd commented May 15, 2020

I think defining a complex filter through an environment variable is not a good option as we'd have to develop a complex parser (with stacking) and then call the functions and operators through getattr and operator.X. - It's just an error-prone and a time-consuming way.
I'd rather put an additional python script ("from django_auth_ldap.config import LDAPGroupQuery" + a definition of a desired filter) and optionally import the filter definition from there into ldap_config.docker.py. The decision for import can be based on a filename or on a value of an environment variable (like AUTH_LDAP_REQUIRE_GROUP_DN_IMPORT_FILTER = true). Then, we can add an example, which end users can modify to fit their needs.
I haven't tested the idea yet, though it seems to be plausible. :)

As for k8s, the script can be mounted through a cm or a secret.

@craftbyte
Copy link

Another issue we discovered is that setting it to '' treis to search the root for the member attribute of the user (root of course not being an LDAP node). The default value should be null.

@craftbyte
Copy link

This is how we fixed it for us:

# Define special user types using groups, only set if variable exists.
AUTH_LDAP_USER_FLAGS_BY_GROUP = {}

# A group needed to log in at all
if os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN') is not None:
    AUTH_LDAP_REQUIRE_GROUP = os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN')
    AUTH_LDAP_USER_FLAGS_BY_GROUP["is_active"] = os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN')

# Admin group DN
if os.environ.get('AUTH_LDAP_IS_ADMIN_DN') is not None:
    AUTH_LDAP_USER_FLAGS_BY_GROUP["is_staff"] = os.environ.get('AUTH_LDAP_IS_ADMIN_DN')

# SuperUser group DN
if os.environ.get('AUTH_LDAP_IS_SUPERUSER_DN') is not None:
    AUTH_LDAP_USER_FLAGS_BY_GROUP["is_superuser"] = os.environ.get('AUTH_LDAP_IS_SUPERUSER_DN')

@alanNutanix
Copy link

I did something different, but quite hacky which allows us to put multiple groups (although they all have to be defined)
I took out the required group because the "is_active" takes care of that.

index 19277e1..f8127c8 100644
--- a/configuration/ldap_config.py
+++ b/configuration/ldap_config.py
@@ -60,13 +60,13 @@ AUTH_LDAP_GROUP_SEARCH = LDAPSearch(AUTH_LDAP_GROUP_SEARCH_BASEDN, ldap.SCOPE_SU
 AUTH_LDAP_GROUP_TYPE = import_group_type(os.environ.get('AUTH_LDAP_GROUP_TYPE', 'GroupOfNamesType'))
 # Define a group required to login.
-AUTH_LDAP_REQUIRE_GROUP = os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN', '')
+# AUTH_LDAP_REQUIRE_GROUP = os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN','')
 # Define special user types using groups. Exercise great caution when assigning superuser status.
 AUTH_LDAP_USER_FLAGS_BY_GROUP = {
-    "is_active": os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN', ''),
-    "is_staff": os.environ.get('AUTH_LDAP_IS_ADMIN_DN', ''),
-    "is_superuser": os.environ.get('AUTH_LDAP_IS_SUPERUSER_DN', '')
+    "is_active": os.environ.get('AUTH_LDAP_REQUIRE_GROUP_DN', '').split('|'),
+    "is_staff": os.environ.get('AUTH_LDAP_IS_ADMIN_DN', '').split('|'),
+    "is_superuser": os.environ.get('AUTH_LDAP_IS_SUPERUSER_DN', '').split('|')
 }```

@cimnine
Copy link
Collaborator

cimnine commented Oct 26, 2020

This will be solved by #343 .

@cimnine cimnine added this to the 0.26.0 milestone Oct 26, 2020
@cimnine
Copy link
Collaborator

cimnine commented Oct 26, 2020

Netbox Docker 0.26.0 was just released which addresses this issue.

@cimnine cimnine closed this as completed Oct 26, 2020
@Neferites
Copy link

Hello,
I read modifications in #343, and, multiple groups seems not allowed.
It's possible to add this split('|') option ?
regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We seek out help for implementing this issue.
Projects
None yet
Development

No branches or pull requests

6 participants