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

Added LDAP authentication #110

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Added LDAP authentication #110

merged 1 commit into from
Aug 17, 2021

Conversation

hmrodrigues
Copy link
Contributor

If the builtin authentication fails,
checks the username and password
againts a LDAP server

@hmrodrigues
Copy link
Contributor Author

hmrodrigues commented Jul 13, 2021

So far I've tested this against a FreeIPA running on Rocky 8 in ldap. I still need to test against Active Directory and in ldaps

@perara can you tell me the following:

  • Is it possible to create a user without a password? A user without password will always fail on the built in auth and fallback to LDAP
  • What roles are you intended to use? This is to set the role based on the user groups inside LDAP

Thank you,
HR

@perara
Copy link
Owner

perara commented Jul 15, 2021

So far I've tested this against a FreeIPA running on Rocky 8 in ldap. I still need to test against Active Directory and in ldaps

@perara can you tell me the following:

  • Is it possible to create a user without a password? A user without password will always fail on the built in auth and fallback to LDAP
  • What roles are you intended to use? This is to set the role based on the user groups inside LDAP

Thank you,
HR

Excellent work! Tell me when you feel this is ready to get pushed in, and I'll take it for a test ride. Have to set up some LDAP server to test it out. For your questions:

  1. Currently, I've had the requirement that all users should also have a password (I think) Do you think that it should be possible to have users without one?
  2. Generally, I think LDAP should act as a drop-in replacement for local authentication. The local auth was something that I puked up during prototyping. It should be revisited eventually. Generally, there are no well-defined roles yet in the application, but maybe we should make some roles for different actions (stop-start-delete-create) ...etc, and that the user/administrator can link up specific LDAP roles to the local role scheme.

For example:
The setup flow would be something like this:

  1. Administrator sets environment variables:
AUTH_LDAP_ENABLED=1
AUTH_LDAP_ADDRESS=<hostname>
AUTH_LDAP_USERNAME=<username>
AUTH_LDAP_PASSWORD=<passwd>
AUTH_LDAP_ADMIN_ROLE=<ldap admin role>

The above would enable the administrator to login using their LDAP account and to further map wg-manager roles to the corresponding LDAP roles. One could also have local username:password fallback, but in that case I would like the administrator to specify that through env variables such as:

AUTH_LOCAL_ENABLED=1
AUTH_ADMIN_USERNAME=admin
AUTH_ADMIN_PASSWORD=admin

For the latter, on initial setup, the user should be queried with a request to change the password on first login, but I suppose this belongs into its own issue.

Again good work, and I'd love to see this get merged in when your done :D

@perara perara added the enhancement New feature or request label Jul 15, 2021
@perara perara linked an issue Jul 15, 2021 that may be closed by this pull request
@hmrodrigues
Copy link
Contributor Author

Hey,

1. Currently, I've had the requirement that all users should also have a password (I think) Do you think that it should be possible to have users without one?

So what I usually do is, all users that are authenticated using external systems don't have a password on the built in system. In that way, the built in authentication will always fail (assuming that the password is required on the login endpoint) and fallback to the external one, in this case LDAP. And we can also disable the password reset on users with password as NULL, since the password reset must happen on the external system.

2. Generally, I think LDAP should act as a drop-in replacement for local authentication. The local auth was something that I puked up during prototyping. It should be revisited eventually.

One could also have local username:password fallback, but in that case I would like the administrator to specify that through env variables such as:

I usually keep a built in system for 2 reasons:
- To have a local administrator and prevent system lock down. Doesn't really apply to this since the LDAP settings aren't stored on the database.
- Have 3rd party users that don't have LDAP credentials (external consultants for example).

Generally, there are no well-defined roles yet in the application, but maybe we should make some roles for different actions (stop-start-delete-create) ...etc, and that the user/administrator can link up specific LDAP roles to the local role scheme.

Independently of the roles system, I think that the best way is to keep it simple, the Group/OU X on LDAP matches with the role/group X on wg-manager. This setting can either be set on the database or in the environment variables. Either way, we can merge this without the roles map and when the roles system is ready we do implementation.

AUTH_LDAP_ENABLED=1
AUTH_LDAP_ADDRESS=<hostname>
AUTH_LDAP_USERNAME=<username>
AUTH_LDAP_PASSWORD=<passwd>
AUTH_LDAP_ADMIN_ROLE=<ldap admin role>

Do you wann't me to change the variables from LDAP_* to AUTH_LDAP_* and add the AUTH_LOCAL_ENABLED?

btw, this is already working with AD and ldaps

@perara perara changed the base branch from main to dev July 16, 2021 11:06
If the builtin authentication fails, checks the username and password
against a LDAP server

Allow to create user without password since we don't store the user
password inside the database.

Every function that has the middleware.authengine
decorator will be used as authentication engine.
The order of the engines is based on the value of sequence,
lower sequences go first
@hmrodrigues hmrodrigues marked this pull request as ready for review July 17, 2021 19:20
@hmrodrigues
Copy link
Contributor Author

hmrodrigues commented Jul 17, 2021

Ok so apart from the roles thing, this is ready for merge. I've squashed the commits into a single one.

Also, as a side note, before trying, sync the master and dev branches, since the master has the fix to create users.

If you want, there are the flags that I used to test this:

FreeIPA: AUTH_LDAP_ENABLED="1" AUTH_LDAP_SERVER="ipa.wireguard.local" AUTH_LDAP_BASE="cn=users,cn=accounts,dc=wireguard,dc=local" AUTH_LDAP_FILTER="(uid=%s)" AUTH_LDAP_USER="uid=admin,cn=users,cn=accounts,dc=wireguard,dc=local" AUTH_LDAP_PASSWORD="adminpassword" AUTH_LDAP_SECURITY="SSL" AUTH_LDAP_SECURITY_VALID_CERTIFICATE="0" uvicorn main:app

ActiveDirectory: AUTH_LDAP_DOMAIN="win" AUTH_LDAP_SERVER="1" AUTH_LDAP_SERVER="dc.win.wireguard.local" AUTH_LDAP_BASE="CN=Users,DC=win,DC=wireguard,DC=local" AUTH_LDAP_FILTER="(samAccountName=%s)" AUTH_LDAP_USER="win\wireguard" AUTH_LDAP_PASSWORD="P455sW0rd!" LDAP_ACTIVEDIRECTORY="1" AUTH_LDAP_SECURITY="SSL" AUTH_LDAP_SECURITY_VALID_CERTIFICATE="0" uvicorn main:app

The AUTH_LDAP_SECURITY_VALID_CERTIFICATE can be set to 1 if you OS trusts the SSL cert for ldaps

@perara perara merged commit dfd4f9b into perara:dev Aug 17, 2021
@perara
Copy link
Owner

perara commented Aug 17, 2021

Excellent work! Merged it in!

@hmrodrigues hmrodrigues deleted the feature/ldap branch August 17, 2021 14:46
m4xx1m added a commit to m4xx1m/wg-manager that referenced this pull request Oct 16, 2022
m4xx1m pushed a commit to m4xx1m/wg-manager that referenced this pull request Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Allow disabling authentication
2 participants