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

improve documentation on password encryption for strong encryption #71

Closed
maybeec opened this issue Mar 26, 2019 · 11 comments · Fixed by #332, #381 or #391
Closed

improve documentation on password encryption for strong encryption #71

maybeec opened this issue Mar 26, 2019 · 11 comments · Fixed by #332, #381 or #391
Assignees
Labels
configuration config files (application.properties, etc.) documentation Guides, tutorials, readmes, etc. enhancement New feature or request security

Comments

@maybeec
Copy link
Member

maybeec commented Mar 26, 2019

I made use of jasypt password encryption in my property files.
One of my first step was to choose a strong cipher algorithm, so I was taking PBEWITHHMACSHA512ANDAES_256 as this was the best provided by jasypt at this moment of time. Interestingly, running the application, you get login issues as of non-provided password.
During debugging I found out, that the password is decrypted to empty string for some reason. I haven't found the cause yet.

Anyhow, googling further, it seems, that jasypt is very instable in regards to AES encryption:
https://sourceforge.net/p/jasypt/bugs/32/

Are there alternatives right now? Do we wait until jasypt is released in a proper version?

@maybeec
Copy link
Member Author

maybeec commented Mar 26, 2019

also see ulisesbocchio/jasypt-spring-boot#124

@hohwille hohwille added the documentation Guides, tutorials, readmes, etc. label May 9, 2019
@hohwille
Copy link
Member

hohwille commented May 9, 2019

This is only related to the documentation. Fixes need to be applied here:
https://github.com/devonfw/devon4j/blob/develop/documentation/guide-configuration.asciidoc#password-encryption

@hohwille hohwille added easyfix Good for newcomers bug Something isn't working and removed easyfix Good for newcomers labels May 9, 2019
@ssarmokadam ssarmokadam added this to the release:2020.12.001 milestone Nov 10, 2020
@hohwille
Copy link
Member

hohwille commented Dec 8, 2020

It is a pitty: We had this story almost done with PR #298 but as the author of the PR left without reworking the last review feedback and nobody took over, I have to move this issue to the next release :(

@sujith-mn sujith-mn self-assigned this Dec 31, 2020
@hohwille hohwille modified the milestones: release:2020.12.002, release:2020.12.003 Jan 21, 2021
@hohwille hohwille modified the milestones: release:2020.12.003, release:2021.04.001 Apr 8, 2021
@hohwille hohwille linked a pull request Apr 12, 2021 that will close this issue
@hohwille
Copy link
Member

@maybeec I have merged #332. Can you check if that now matches your expectation and the new recommended algorithm is addressing your concerns of this issue so we can close it?

@hohwille hohwille added configuration config files (application.properties, etc.) security labels Apr 12, 2021
@hohwille
Copy link
Member

hohwille commented Apr 12, 2021

What still confuses me is why PR #332 documented the algorithm PBEWITHHMACSHA512ANDAES_256 as @maybeec suggested but also added this config suggestion (where algorithm does not match what seems wrong to me):

jasypt.encryptor.algorithm=PBEWITHMD5ANDTRIPLEDES

BTW: Should that algorithm better go to application.yml or to application.properties? If it goes to application.properties and this is our best practice - why dont we already ship this in the main application.properties of our archetype?

@hohwille hohwille added enhancement New feature or request and removed bug Something isn't working labels Apr 16, 2021
@hohwille hohwille changed the title password encryption buggy improve documentation on password encryption for strong encryption Apr 16, 2021
@maybeec
Copy link
Member Author

maybeec commented Apr 16, 2021

I am quite confused that we have both. Having both next to each other ´application.ymlandapplication.properties` does not make sense to me.

Has encryption and running the application been tested with PBEWITHHMACSHA512ANDAES_256? just asking as I had some issues in the past to get it running.

@hohwille
Copy link
Member

hohwille commented Apr 16, 2021

I am quite confused that we have both. Having both next to each other ´application.ymlandapplication.properties` does not make sense to me.

@maybeec You are IMHO missing the point: The master-password has to be configured somewhere per environment. To keep this separate from other configs (e.g. managed in git repos) containing encrypted passwords helps to keep the approach meaningful. If the masterpassword is contained in the same config file that also contains the enctypted passwords, we can also drop the encryption and leave the passwords in plain text. The benefit is that if a config file with encrypted passwords for whatever reason goes into the wrong hands (e.g. due to a human mistake it is send in an email or whatever) an attacker still can not get the unencrypted password. Therefore we introduced application.yml that is supposed to ONLY contain the master-password and nothing else (except maybe as asked by myself the algorithm).

Has encryption and running the application been tested with PBEWITHHMACSHA512ANDAES_256? just asking as I had some issues in the past to get it running.

That is exactly what I instructed @sujith-mn to do and what he has tested and documented.

@maybeec
Copy link
Member Author

maybeec commented Apr 16, 2021

OK, fine for me. Anyhow I would even leave it out and not enable encryption by default. Even here we could have also thought about simply providing a tutorial and a minimal sample to show how to do it.

In a could environment you would most probably provide the file as a config map or even as an environment variable.
So I am fine then.

@hohwille
Copy link
Member

Anyhow I would even leave it out and not enable encryption by default.

We never had jasypt encryption enforced and I do not even see how this could be archieved. It was always optional and will remain such. After all it is just a documentation feature. The only thing I was suggesting, is to pre-configure the algorithm to a secure one by default in our app-template to avoid that projects start with the jasypt default algorithm which seems to be insecure as you claimed when opening this issue.

@hohwille
Copy link
Member

hohwille commented Apr 19, 2021

To make it cristal clear: There is one remaining question to clarify before we can close this story and we are already overdue with the release:

PBEWITHHMACSHA512ANDAES_256 != PBEWITHMD5ANDTRIPLEDES

So our documentation is IMHO inconsistent.
@sujith-mn can you fix this or can you explaing why this difference of algorithms should be correct?

@hohwille
Copy link
Member

OK, with this algorithm there was more or less a missunderstanding on my end:

  • @sujith-mn did properly adapt the documentation and explain how to do it. The thing with the second algorith (PBEWITHMD5ANDTRIPLEDES) was just an optional section explaining how to manually changing the algorithm so in case in the future the current default algorithm turns weak again and there is still a better algorithm option available that someone wants to use instead.
  • The main thing was to update the version of the jasypt-spring-boot-starter dependency to 3.0.3. This changes the default algorithm to PBEWITHHMACSHA512ANDAES_256 that is considered secure also is the one suggested by @maybeec

Sorry, that I did not get it initially. However, I have discussed with @sujith-mn and we took this missunderstanding as an indicator to further improve the doucmentation. So @sujith-mn will create another PR updating the doc to make this even more obvious and avoid such potential confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration config files (application.properties, etc.) documentation Guides, tutorials, readmes, etc. enhancement New feature or request security
Projects
None yet
4 participants