DefaultJedisClientConfig password exposure issue

[ivanfrias]

The class DefaultJedisClientConfig overrides the getPassword method that returns a String.
Returning a string might be considered a potential security issue since an attacker might inspect the heap and find the value in plaintext.
Ideally we should just pass-through the value supplied by the provider here and not create a String based on the char[] array.

Expected behavior

Return a char[] instead of String

Actual behavior

A string is returned.

Steps to reproduce:

N/A

Redis / Jedis Configuration

N/A

Jedis version:

N/A

Redis version:

Java version:

N/A

[sazzad16]

String getPassword() is still there to support legacy applications without breaking. We may remove it at some point in favor of getCredentialsProvider().

[sazzad16]

Tagged 6.0.0 just so it stays in front of eyes more.

[wickdynex]

If possible, I would like to take ownership of this issue. In my opinion, I can replace String with char[] and modify the DefaultJedisClientConfig class and DefaultRedisCredentials methods. However, I have three questions:

1. Using char[] instead of String is for easier password clearing. When exactly should we clear the password? Or is it not necessary to clear it?
2. I couldn't find any unit tests for getPassword. Do I need to write a unit test for it to follow TDD development practices?
3. Which branch should I work on?

Looking forward to ur reply.

[sazzad16]

@wickdynex Thank you for your interest.

I would ask you not to work on this specific issue right now. I have already mentioned in my earlier #4021 (comment),

We may remove it (String getPassword()) at some point in favor of getCredentialsProvider().

The concerned users should use RedisCredentials/RedisCredentialsProvider.

[wickdynex]

@wickdynex Thank you for your interest.

I would ask you not to work on this specific issue right now. I have already mentioned in my earlier #4021 (comment),

We may remove it (String getPassword()) at some point in favor of getCredentialsProvider().

The concerned users should use RedisCredentials/RedisCredentialsProvider.

Thanks for your reply, I missed this #4021 (comment). So is this issue have assigned to your teammate? Otherwise, I can also fix this issue🥰.

[sazzad16]

So is this issue have assigned to your teammate?

No. No one.

[wickdynex]

So is this issue have assigned to your teammate?

No. No one.

If u'ld like to assign to me, I’d be happy to take on this issue.
Here’s the approach I plan to take to fix it:

• Use DefaultJedisClientConfig.getCredentialsProvider()method to replace DefaultJedisClientConfig .getPassword() where it used at. And refactor the usage.

Would you agree with this approach? Please let me know if you have any feedback or suggestions.

Followed Questions:

1. Should I write unit tests for this method? I noticed that the project may be lacking tests in some areas, and it could be a good idea to add tests if necessary.
2. Which branch should I work on?

Looking forward to ur reply.

[sazzad16]

Should I write unit tests for this method?

Ideally, yes.

Which branch should I work on?

master.

[wickdynex]

Should I write unit tests for this method?

Ideally, yes.

Which branch should I work on?

master.

Thanks. Please assign this issue to me.

[sazzad16]

Please assign this issue to me.

@wickdynex It's not necessary. You're welcome to craft a PR about any improvement even without the assignment. Thanks.

[wickdynex]

Please assign this issue to me.

@wickdynex It's not necessary. You're welcome to craft a PR about any improvement even without the assignment. Thanks.

Thank u. I'll try craft PR.☺️

[wickdynex]

Should I write unit tests for this method?

Ideally, yes.

Which branch should I work on?

master.

I’ve found that not only the DefaultJedisClientConfig class, but also the JedisCluster, JedisFactory, JedisPool, and JedisPooled classes are using String instead of char[] to store the password, along with related tests. So terrible! This means that refactoring this issue will require a rather large PR. Do I need to refactor all instances of this password-related bug, or should I just focus on refactoring DefaultJedisClientConfig and modify the related code to use password.toCharArray() instead?
Please give me some advice, thanks. Looking forward to ur reply.