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

Protect keystorePassword in case https is used #164

Closed
nevenr opened this Issue Oct 16, 2014 · 6 comments

Comments

Projects
None yet
2 participants
@nevenr
Contributor

nevenr commented Oct 16, 2014

Hi,

Currently keystore password is plain text in configuration file.
Maybe it should be:

  • encrypted
  • placed in separate file that can be additionally protected (only specific users can read)
  • protected on some other way

Regards,
N.

@rhuss rhuss added this to the 1.3.1 milestone Nov 8, 2014

@rhuss

This comment has been minimized.

Owner

rhuss commented Jul 13, 2015

I think this could be a good solution:

  • Add a command option 'encrypt' to the JVM agent which does an encryption with an code-internal key
  • Select some indicators like "{{...}}" to distinguish between encrypted/unnecrypted passwords.
  • If such a pattern for an encrypted password is found, decrypt it and use that one opening the keystore.

This is of course not perfect (the symmteric key is stored within the agent jar), but at least one can not open the keystore directly with this password (so it provides a somewhat intermediate level security).

In implementing that, try to avoid external libs in order to keep the size footprint small.

@rhuss rhuss modified the milestones: 1.3.3, 1.3.1 Sep 11, 2015

nevenr added a commit to nevenr/jolokia that referenced this issue Sep 12, 2015

nevenr added a commit to nevenr/jolokia that referenced this issue Sep 12, 2015

nevenr added a commit to nevenr/jolokia that referenced this issue Sep 12, 2015

@nevenr

This comment has been minimized.

Contributor

nevenr commented Sep 12, 2015

Hi Roland,

for this purpose i reuse (copy) approach and code from Apache Maven , JolokiaCipher.java.
Also new command is added EncryptCommand.java and password from plain text is encrypted to bytes and encoded back to text with Base64.
But Base64 has characters (e.g =) that bother us during parameters parsing/handling in JvmAgentConfig (maybe also on some other places). What you would recommend to use instead Base64? Hex? Some other encoding?

Cheers,
Neven

@rhuss

This comment has been minimized.

Owner

rhuss commented Sep 13, 2015

Hi Neven,

thanks for your work !

I had the change to have a look at your code. Looks good, have some comments, though:

  • I already have a Base64 decoding in AuthorizationHeaderParser in the OSGi package. I extracted this, added a encode() and put it into a Base64Util on master, so you can easily reuse it.
  • For the CLI API: I would make encode a pure command, not an option. So its on the same footstep as start,stop,toggle. This would also help with the parsing problem, because no parsing is required at all, if we use delimiters which are not starting with -. I suggest to use {{ ... }} as delimiters (in order to allow people to have plain passwords starting with {). Makes it bit more safe and cleare that its a encoded/crypted password.
  • It would be cool, when the command encode is given and no further argument that then also the standard input is read so that it could be put into a shell pipeline. E.g.
java -jar jolokia.jar encode s3cr3t
curl https://pw.intranet/jolokia-pwd | java -jar jolokia.jar encode

I think that would give this feature a nice touch (but of course is not mandatory)

  • Personally I wouldn't put the jolokia 'master' password into a textfile below /META-INF. The reason is, that this encryption feature obviously only helps in avoiding accidental spoofing. Since Jolokia needs to decrypt the keystore password, it must have the symmetrically keysomewhere in its jar. It shouldn't be to easy to read this password, putting it into a textfile needs only a jar xvf to get to this master password. Since it is also only used internally I would keep it into a constant within the code. So you would need in addition a decompiler to get to the code. Yes, nothing is really secure here and we need to take care to keep this master password stable across releases. It's only about not to make it too easy. Ok, if you want to individualize the master password, what about checking for this property file and, if not existant, take the internal, default one. And by default we don't deliver such a pw file.
@nevenr

This comment has been minimized.

Contributor

nevenr commented Sep 14, 2015

Hi Roland,

  • I reused org.jolokia.util.Base64Util and removed org.jolokia.util.Base64 added by me.
  • 'pure command' approach is questionable because after command pid (numeric value) is expected or string that represent regexp pattern. Meaning that org.jolokia.jvmagent.client.util.OptionsAndArgs#initPid should be modified to consider org.jolokia.jvmagent.client.util.OptionsAndArgs#COMMANDS_WITHOUT_PID.
  • Also the issue (with = char in encrypted password) is not only in OptionsAndArgs but also in org.jolokia.jvmagent.JvmAgentConfig/JolokiaServerConfig if arguments glued together as provided on the commandline for an agent parameter
  • i will add 'standard input read' at the end
  • i will put jolokia master password somewhere in code on some semi secure way

Cheers,
Neven

@rhuss

This comment has been minimized.

Owner

rhuss commented Sep 15, 2015

'pure command' approach is questionable because after command pid (numeric value) is expected or string that represent regexp pattern. Meaning that org.jolokia.jvmagent.client.util.OptionsAndArgs#initPid should be modified to consider org.jolokia.jvmagent.client.util.OptionsAndArgs#COMMANDS_WITHOUT_PID.

Ok, I see. Let's do it like this: Simply continue as agreed, submit a PR and I will take care about how to best integrate it into the CLI.

Ok ?

@nevenr

This comment has been minimized.

Contributor

nevenr commented Oct 2, 2015

OK.

nevenr added a commit to nevenr/jolokia that referenced this issue Oct 3, 2015

rhuss added a commit that referenced this issue Jan 18, 2016

#164: Refactored encryption handling
* Added documentation
* Switched from an option '--encrypt' to a dedicated agent commad "encrypt"
* Switched markers from "{ ... } " to "[[ .... ]]" for consistency with jmx4perl encryption markers
* Simplified cypher which is sufficient for our needs (especially the salt generation)

rhuss added a commit that referenced this issue Jan 18, 2016

@rhuss rhuss closed this Jan 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment