Sentinel Auth

[zamu87]

Using sentinel configuration is not possibile to disable sentinel auth keeping master node auth enabled or to specify different passwords.

The problem is in Handler.php file because the same variable $pass si evaluated for $sentinelClient->auth and $redisMaster->auth

306   $sentinelClient = new \Credis_Client($server, NULL, $timeout, $persistent);
307   $sentinelClient->forceStandalone();
308   $sentinelClient->setMaxConnectRetries(0);
309   if ($pass) $sentinelClient->auth($pass);
310   
311   $sentinel = new \Credis_Sentinel($sentinelClient);
312   $sentinel
313      ->setClientTimeout($timeout)
314      ->setClientPersistent($persistent);
315   $redisMaster = $sentinel->getMasterClient($sentinelMaster);
316   if ($pass) $redisMaster->auth($pass);

[colinmollenhour]

Good observation, but is there a reason you are prevented from using the same password for both?

[zamu87]

Hi Colin, you’re right, it’s not a common practice to set different password for nodes and sentinel but I think it’s a common scenario to set a password for nodes and not for sentinel. I have 2 examples where this could happen:

• Magento 2.3.x or 2.4.x installation that currently requires colinmollenhour/cache-backend-redis version 1.11 (that does not support sentinel auth) and colinmollenhour/php-redis-session-abstract version 1.4.2 (that requires sentinel auth if node auth is enabled)
• using Redis in Kubernetes installed with redis-ha helm chart that does not support Sentinel Auth

I've found 2 possibile solutions for this problem:

• the most "elegant" one is to edit ConfigInterface to add the support for a sentinel_password field, but this could break a lot of installations
• the "quick and dirty" is to add a try catch on line 309 of file Handler.php to capture ERR Client sent AUTH, but no password is set Error

[colinmollenhour]

Thanks for the info, @zamu87 and the solutions. I have no objections to the "quick and dirty" so if you want to submit a PR for that one I can get it merged quickly.

For the "elegant" solution I think it definitely needs to be implemented to not break BC. This could probably be done by supporting a "sentinel_password" field that replaces the normal password if specified and if not specified reusing the normal password is assumed. Then if the sentinel_password has some special "empty" value it would still override the normal password but no auth would be performed. E.g. the "empty" value could be "-". It is not super elegant but I don't think this is worth breaking existing installations over.

[zamu87]

Thanks @colinmollenhour, i've submitted a PR for the "quick and dirty" solution #37

For the "elegant" solution the problem is that we have to edit Handler/ConfigInterface.php to support the new field "sentinel_password" and so we'll break all implementations of this interface.

[colinmollenhour]

Thanks for the PR @zamu87! Tagged as v1.4.3

Yes, the "elegant" solution would require a major version bump (v.1.5.0) as it would break BC.

[abramchenkoaa]

Hi @colinmollenhour

I faced a similar issue as @zamu87 and resolved it via composer patch by adding a new method to ConfigInterface called getSentinelPassword()
Another possible solution is to create a new interface, ConfigSentinelPasswordInterface, which includes the getSentinelPassword() method. This approach will not affect backward compatibility and will allow third-party developers to create a preference on \Cm\RedisSession\Handler\ConfigInterface inherits ConfigSentinelPasswordInterface and add the getSentinelPassword() method.

What about the following solution?

[colinmollenhour]

Thanks for the proposal, if you submit such a PR I will accept it.

[abramchenkoaa]

Hi @colinmollenhour , I have submitted a PR #55
Can you please take a moment to review it? Thank you!

[colinmollenhour]

Merged and released: https://github.com/colinmollenhour/php-redis-session-abstract/releases/tag/v1.5.2

Thanks!