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

rewritten doc about jasypt password encryption and decryption #298

Closed
wants to merge 3 commits into from

Conversation

ravicm2
Copy link
Member

@ravicm2 ravicm2 commented Sep 24, 2020

No description provided.

@ravicm2
Copy link
Member Author

ravicm2 commented Sep 24, 2020

#71

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ravicm2 thanks for all your investigation to take this further and for creating this PR. 👍
IMHO this story is not so easy and I added some questions and review comments. Let us see what we can clarify easily and quickly. For all remaining aspects we might better do a call together...

A simple but reasonable approach is to configure the passwords encrypted with a master-password. The master-password should be a strong secret that is specific for each environment. It must never be committed to version-control.
In order to support encrypted passwords in spring-boot `application.properties` all you need to do is to add https://github.com/ulisesbocchio/jasypt-spring-boot#jasypt-spring-boot[jasypt-spring-boot] as dependency in your `pom.xml`(please check for recent version):
[source, xml]
Let us have a look how to encrypt password using `jasypt-springboot-starter` and `jasypt-springboot`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be jasypt-spring-boot-starter

</dependency>
----

Please check the latest version link:https://mvnrepository.com/artifact/com.github.ulisesbocchio/jasypt-spring-boot-starter[here] .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. Thanks 👍

Comment on lines -172 to +181
----ARGUMENTS-------------------
ARGUMENTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for clarification:
This was the raw output of jasypt CLI.
Has this been changed, as I produced invalid ASCIIDOC since the 5 dashes indicate the end of the source?
We could also simply change the 5 dashes opening and ending to 3 backticks instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also replace the example data to fit with the new encryption algorithm?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hohwille do you mean example provided here in the document or any other application already there for jasypt?

Copy link
Member

@hohwille hohwille Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only talking about the output of jasypt CLI where I annotated this review comment. With example I am talking about the used data

input: postgres
password: secret

and especially the produced result that IMHO will change as the suggested security algorithm changed with this PR. Also a question would be if the CLI changes or a different Main-Class needs to be used for this now.


jd5ZREpBqxuN9ok0IhnXabgw7V3EoG2p
----

The master-password can be configured on your target environment via the property `jasypt.encryptor.password`. As system properties given on the command-line are visible in the process list, we recommend to use an `config/application.yml` file only for this purpose (as we recommended to use `application.properties` for regular configs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why has this section been removed? It IMHO contains relevant information that is now missing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added this at line number 327

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you "removed" the info that the property is called jasypt.encryptor.password. I always welcome if people make our documentation more detailed/expressive, but we should avoid removing valuable info.

Comment on lines +251 to +277
New configuration class is required for `Jasypt-springboot` dependency and we need to add the annotation `@EncryptablePropertySource` at the class level.

Below is an example,

[source,java]
----
@Configuration
@EncryptablePropertySource("encrypted.properties")
public class AppJasyptConfig {
}
----

And a service class to map the property in encrypted.properties.
[source,java]
----
@Service
public class PropertyMapperJasypt {

@Value("${encrypted.property}")
private String property;

public String getProperty() {
return property;
}
}
----
Using the above service class and setting the secret key which we used for encryption, we can easily retrieve the encrypted.property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example that sounds more like a "tutorial to get started with jasypt" to me. IMHO we need a clear pattern/best practice how to do it properly.

Comment on lines +287 to +300
@Bean(name = "encryptorBean")
public StringEncryptor stringEncryptor() {
PooledPBEStringEncryptor encryptor = new PooledPBEStringEncryptor();
SimpleStringPBEConfig config = new SimpleStringPBEConfig();
config.setPassword("password");
config.setAlgorithm("PBEWithMD5AndDES");
config.setKeyObtentionIterations("1000");
config.setPoolSize("1");
config.setProviderName("SunJCE");
config.setSaltGeneratorClassName("org.jasypt.salt.RandomSaltGenerator");
config.setStringOutputType("base64");
encryptor.setConfig(config);
return encryptor;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are these the parameters that make the encrpytion secure according to state of the art?
Did you align this with @maybeec or where did you get this code from?

Copy link
Member Author

@ravicm2 ravicm2 Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No i have not discussed regarding this with @maybeec but this is an example i learned from a tutorial. This is one type of creating custom encryptor. we add change the parameters differently

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This config allows you to adapt the algorithm.
Anyhow the algorithm used here is the standard one which is quite insecure. Anyhow, at the time I tried making it more secure I was not able to get any other algorithm to work for any reason. Possibly this has been fixed meanwhile!?

@ravicm2
Copy link
Member Author

ravicm2 commented Sep 24, 2020

@ravicm2 thanks for all your investigation to take this further and for creating this PR. 👍
IMHO this story is not so easy and I added some questions and review comments. Let us see what we can clarify easily and quickly. For all remaining aspects we might better do a call together...

Sure @hohwille . May be tomorrow we can have a call regarding this. May be we can include @maybeec if he is free.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ravichandran M seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hohwille
Copy link
Member

This PR is IMHO replaced by PR #332.
I therefore close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants