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

LDAP server manager password visible and stored in plain text #4241

Closed
henning-gerhardt opened this issue Mar 5, 2021 · 19 comments · Fixed by #4682
Closed

LDAP server manager password visible and stored in plain text #4241

henning-gerhardt opened this issue Mar 5, 2021 · 19 comments · Fixed by #4682
Assignees

Comments

@henning-gerhardt
Copy link
Collaborator

Setting up a LDAP server the server manager password is stored in plain text in the database and is visible as plain text if you open this entry!

@matthias-ronge
Copy link
Collaborator

Yes that's true. How do you want to do it differently?

@henning-gerhardt
Copy link
Collaborator Author

How do you want to do it differently?

I was against storing this password manager into the database and I did not implement this change.

My approach would be maybe:

  1. store the password with a strong symmetric algorithm and if this algorithm need / support a salt value store this salt value in a different place, so you need two places to look and decrypt the stored password. If you need access to this password, decrypted it, use is and destroy the used variable on method exit.
  2. On display: did not return the stored password into the form but use a notice that informing the user / administrator that the password is stored but not displayed again because of security reasons.

@matthias-ronge
Copy link
Collaborator

Neither do I. But any encryption that is done, which password is publicly available on GitHub, is just as worthless in the end, so why not trust Unix file system permissions and write the password to a file where no other user can easily read it, only Tomcat , and even root has to change permissions first, that's a small hurdle, but if root already has permissions to read local LDAP content anyway, that's only a small hurdle. I always agree to include security and hurdles, but where is the reasonable limit for administrator usability?

@henning-gerhardt
Copy link
Collaborator Author

But any encryption that is done, which password is publicly available on GitHub, is just as worthless in the end,

That is only true if you don't use a salt and use the default initialization vectors of the encryption. If you store this outside and did not make it public then it is more complicated to get the information. The used algorithm is only one key in the chain of encryption an decryption.

so why not trust Unix file system permissions and write the password to a file

Then the password should be moved back.

but where is the reasonable limit for administrator usability

Because of security? The LDAP manager password is the most important passwords that you can have and if this password get public in some way then is calling the police only one step in the next couple of hours.

@matthias-ronge

This comment has been minimized.

@matthias-ronge

This comment has been minimized.

@matthias-ronge
Copy link
Collaborator

Also related: #3339

@henning-gerhardt
Copy link
Collaborator Author

henning-gerhardt commented Mar 30, 2021

Is your comment #4241 (comment) serious

I was still thinking about this, and it seems to me that the best solution would be to remove all of this LDAP functionality! This would save us from a lot of problems:

* The master password of the LDAP server no longer has to be saved on the application server.

* User passwords no longer have to be stored reversibly encrypted in the database (see #456)

* When installing on a new machine, we would save everybody a lot of cumbersome and time-consuming configuration steps (LDAP setup, storing the `NextFreeUnixId`, adapting the LDAP group with the `localsid`, Lib-NSS configuration, ...)

* The conflict situation does not arise, if an institution wants to authenticate its users via LDAP against a company directory that cannot be loaded with the Samba scheme (such as Active Directory).

* The ongoing changeover from Samba to more advanced password encryption methods, [which will overtake us in the coming year](https://askubuntu.com/a/1315541/557338), which will require a migration of the LDAP Samba scheme (if I understand it correctly) no longer affects us.

Instead, when creating Production users, a script should be executed (exactly once, when creating by the user, see also #4255), and this script is then responsible for everything that does it on the machine. Even if this script is creating a Linux user, using useradd via sudo, this can be better restricted via the sudoers configuration (for example, that it can only create sers with shell /bin/false). To just set up a Samba share or a Webdav account, it is not even necessary to create a Linux user. (The creation of symbolic link scripts should also change in the context, for read-only vs. read-write access, it would be sufficient to change the write bit, no need to set the file ownership to root or other users based on the user name.) If the previous behavior is still required, this script can still be implemented in a way that it creates a LDIF file and loads it into LDAP, using the master credentials.

or did you just kidding? If this change goes in, I don't know, if we would use this software in future.

@henning-gerhardt
Copy link
Collaborator Author

Changing the LDAP integration is not the goal of the issue. Goal is to implement a more secure way to store the manager password. Storing passwords could never be secure for all cases as storing them is the main issue.

So my goals for this issue should be:

  1. use a strong, symmetric crypto algorithm with a salt where the salt value is stored on the file system (f.e. inside the kitodo_config.properties file or into a new one, if this is necessary and more secure).
  2. did not display this stored password if this ldap server dialog is shown to the user.

@matthias-ronge
Copy link
Collaborator

I understood that removing this feature altogether is not an option, so I did hide my comments on that.

@Kathrin-Huber
Copy link
Contributor

First step: show password in frontend with ****

@markusweigelt
Copy link
Collaborator

Does the same apply to the keyStorePassword?

Which algorithm should be implemented here?
Which mode, for e.g. AES should be used here, or should it remain configurable in addition to the secret? e.g. "AES/CBC/PKCS5Padding"

markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Sep 15, 2021
@henning-gerhardt
Copy link
Collaborator Author

Are your questions to me, @markusweigelt or who should answer your questions?

@matthias-ronge
Copy link
Collaborator

Does the same apply to the keyStorePassword?

It seems superfluous to me.

  • The manager password is only required to write new users into the LDAP or if the user wants to change his password. This only works with a slapd, because it requires the Samba schema, which, for example, does not work with Microsoft Active Directory, as far as I know. It is not required if the user logs on to an external read-only LDAP. (And these are usually the company's directory services, which are valuable.) Still, if someone stands behind you while you look at this page and takes a picture, and he has network access to the LDAP (should actually be prevented by a firewall, I just mean) then he can change there, and therefore coverage of the field is understandable.
  • Whether you have to encrypt the manager password (in the database) is another question. The key would have to be somewhere again. Anyone who manages to receive a copy of the database will also be able to understand this, get the key and decrypt the password. However, it may be a bit of protection if someone inadvertently gets a copy of the database. (Difficult to imagine for me, maybe when accessing the backup. But, is the key in the backup as well? Or it is hard-coded in the source code on GitHub? It makes things more difficult, but against willful access, the protection is not good.)
  • The keystore password is something else. The “keystore” is a file in which there are two (or more) public certificates (the public SSL certificate chain) of the LDAP, if it communicates encrypted with SSL. However, if so, these certificates can be downloaded from the LDAP with an HTTP request. There is no point in encrypting this file, but the code demands it. I think Java wants it that way. Well, there is nothing valuable in this file (with us) and I don't think the password needs to be protected. As far as I am concerned, there would be no need for a password, but as I said, I think Java requires it because something valuable could be in a keystore.

@markusweigelt
Copy link
Collaborator

Are your questions to me, @markusweigelt or who should answer your questions?

@henning-gerhardt Yeah, sorry I didn't mention you.

@henning-gerhardt
Copy link
Collaborator Author

henning-gerhardt commented Sep 15, 2021

Matthias, you are wrong regarding to the manager password: the LDAP manager account with this password is needed to at least create and maybe needed to modify an existing LDAP entry. While using LDAP the used password (manager and even user passwords) are submitted as hash values to the LDAP server. The content of the LDAP entry (in our case even LDAP + Samba related data) is not relevant for this issue.

@henning-gerhardt
Copy link
Collaborator Author

@markusweigelt : I have no favorite synchronize algorithm for storing this password inside the database (I never what nor would implement this). We already had a lot of discussions about what should be stored in which way inside the database or not. As every one does what he/she/it think is the best, this will be implemented as there are no rules and / or suggestions.

@markusweigelt
Copy link
Collaborator

I think we should distinguish between sensitive data that should not be directly visible in the frontend (e.g. passwords of any kind) and data that are so sensitive that they should be stored in encrypted form.

For keystore and manager password, I changed the input type from text to password. For the manager password,
we should change the storage behavior.

markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Sep 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Sep 15, 2021
…using secret key and add test prove implementation
@matthias-ronge
Copy link
Collaborator

matthias-ronge commented Sep 16, 2021

Matthias, you are wrong regarding to the manager password: the LDAP manager account with this password is needed to at least create and maybe needed to modify an existing LDAP entry. While using LDAP the used password (manager and even user passwords) are submitted as hash values to the LDAP server. The content of the LDAP entry (in our case even LDAP + Samba related data) is not relevant for this issue.

That was what I said: Except for the login, the manager password is required for all operations (creating a new user, changing a user's data, checking whether a username is already taken). But what I wanted to show: It is not needed for the login. It is important to emphasize that. I don't want to rule out that it will be sent around with the login, but I know from experience that the login works without this password.

What I wanted to say: For the most secure operation possible, LDAP should be operated in read-only mode, and the password is not stored here at all. To create or change users, establish a course of business, for example, the user goes to the registration desk in the reception hall. There, a secured application has write access to LDAP, nobody else has. Only after the user has entered the LDAP officially, create a user account in Kitodo and point the LDAP group to the existing user.

And, secondly, what I wanted to say is, that this is the standard way when you use Microsoft Active Directory for authentication, because you cannot write Samba schema fields in Microsoft Active Directory, so you cannot write there from Production even with a password, because Microsoft Active Directory will reject the record from Production because of these Samba fields.

It is not really relevant to this issue, but for security, it is relevant and a consideration whether or not one should not prefer this route anyway.

I just wanted to show that this option exists and is used when high security is required. It has disadvantages, for example you cannot change user details or password from Production, and not automatically set up a network drive (unless otherwise implemented).

@markusweigelt markusweigelt reopened this Sep 16, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Sep 17, 2021
…and migration and decrypt of manager passwords
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 6, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 6, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 6, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
…using secret key and add test prove implementation
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
…and migration and decrypt of manager passwords
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Oct 18, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
…using secret key and add test prove implementation
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
…and migration and decrypt of manager passwords
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
markusweigelt added a commit to markusweigelt/kitodo-production that referenced this issue Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants