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

Request Administrator's Credentials adhoc #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GeKasap
Copy link

@GeKasap GeKasap commented Sep 24, 2020

Setting bind user in configuration is not safe in some deployments,
as this user should have permissions to modify the following attributes
in LDAP:

  • userPassword
  • pwdReset
  • pwdAccountLockedTime

For that reason, the always_authenticate_admin variable has been introduced
in config.inc.php. When set to true, input fields for administrator's username
and password appear in Reset Password and Lock/Unlock Account forms.
When user submits one of this form, then ldap_bidndn and ldap_bindpw are
taken from the respective POST variables, overwriting any value they have
in config.inc.php or config.inc.local.php files.
The default value of always_authenticate_admin is false, providing the
old functionality.

as this user should have permissions to modify the following attributes
in LDAP:
* userPassword
* pwdReset
* pwdAccountLockedTime

For that reason, the `always_authenticate_admin` variable has been introduced
in `config.inc.php`. When set to true, input fields for administrator's username
and password appear in `Reset Password` and `Lock/Unlock Account` forms.
When user submits one of this form, then `ldap_bidndn` and `ldap_bindpw` are
taken from the respective `POST` variables, overwriting any value they have
in `config.inc.php` or `config.inc.local.php` files.
The default value of `always_authenticate_admin` is false, providing the
old functionality.
@GeKasap
Copy link
Author

GeKasap commented Sep 24, 2020

I have also added the respective messages in en.inc.php and tried to translate for fr.inc.php.
Please feel free to update them, if needed.

@GeKasap GeKasap changed the title Setting bind user in configuration is not safe in some deployments, Request Administrator's Credentials adhoc Sep 24, 2020
@coudot coudot added the enhancement New feature or request label Sep 24, 2020
@coudot
Copy link
Member

coudot commented Sep 24, 2020

Hello @GeKasap

thanks for this proposition.

I understand your use case, but I don't think that asking admin credentials for all actions are a good solution.

People installing Service Desk should protect the configuration file and the server to avoid any credentials leak.

Moreover, you seems to introduce buggy changes in your code, like replacing the value for pwdAccountLockedTime.

I would be happy to have feedback from other users of Service Desk before accepting this code.

@GeKasap
Copy link
Author

GeKasap commented Sep 24, 2020

Hello @coudot ,
thanks for replying.
I understand that providing the admin password each time is not very comfortable, but IMHO having the password inside config in plain text is a serious security issue. Especially since I am using it in a docker container. Ok, I can put the password in a sealed secret, but then I have to pass it as configMap, which also is not secure.
Moreover, using a single bind account does not provide transparency as of which admin locked or modified an account, unless you search the log entries of Service Desk.
Last, about the "buggy changes", you are correct. I mis-read the definition of pwdAccountLocketTime. I will revert it. But a comment should be added in user's information, that the account has been disabled by an administrator.

Reset value of pwdAccountLockedTime back to administrative lock.
@micter59
Copy link

I know it's a bit late to participate, and I'm "just a user". But in my opinion, you should use Apache config (or any other web server you can use) to protect this application. In my case, i put an apache config which ask for name and password, and looks in a specific OU of the LDAP for the account given. So, there's no password in plain text in the configuration, and you can authorize many account for using this application.
This is my very little contribution to this subject.

@coudot
Copy link
Member

coudot commented Mar 19, 2021

Hello @micter59

using Apache authentication does not prevent to use a dedicated account in Service Desk to connect to the LDAP directory.

@micter59
Copy link

Sorry, I misunderstood the question.

@coudot coudot added this to the Backlog milestone May 13, 2021
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.

3 participants