erronppl commented 3 years ago

Connection discards user name

Reproduce the issue

The steps below assume bash, gpg, Docker and docker-compose are installed.

Starting from carmine root directory

git clone https://github.com/ptaoussanis/carmine.git
cd carmine

Set up a docker-compose file

cat << EOF > docker-compose.yml
---
services:
  redis:
    image: redis:6.2.2-buster
    container_name: carmine-test
    network_mode: "host"
    volumes:
      - type: bind
        source: ./redis.conf
        target: /usr/local/etc/redis/redis.conf
    command: [ "redis-server", "/usr/local/etc/redis/redis.conf" ]
EOF

Download Redis default configuration

curl -o redis.conf 'https://raw.githubusercontent.com/redis/redis/6.0/redis.conf'

Remove Redis protected mode

sed -i 's/^protected-mode yes$/protected-mode no' redis.conf

Add a password on user default. This user is created by Redis.

echo "user default on +@all ~* >$(gpg --gen-random --armor 2 32 | tr -d '=/:@' )" >> redis.conf

Add another user srv_usr with the same password

tail -n 1 redis.conf | sed 's/ default / srv_usr /' >> redis.conf

Put the password in redis-cli.env

echo "REDISCLI_AUTH=$(grep 'user srv_usr' redis.conf | grep -Eo '>.+' | sed 's/^>//')" > redis-cli.env

Start Redis Docker container

docker-compose up

Test the connection

Test connection with Redis CLI on user default without password

docker run --rm -it --net=host redis redis-cli -h localhost -p 6379 ping 
(error) NOAUTH Authentication required.

Test connection with Redis CLI on user default with password

docker run --rm -it --net=host --env-file redis-cli.env redis redis-cli -h localhost -p 6379 ping
PONG

Test connection with Redis CLI on user srv_usr without password

docker run --rm -it --net=host redis redis-cli -h localhost -p 6379 --user srv_usr ping 
(error) NOAUTH Authentication required.

Test connection with Redis CLI on user default with password

docker run --rm -it --net=host --env-file redis-cli.env redis redis-cli -h localhost -p 6379 --user srv_usr ping
PONG

Good !

Test carmine

Fire up REPL

Replace the password [REDACTED] with yours

$ lein repl
user=> (require '[taoensso.carmine :as car :refer (wcar)])
nil

user=> (def conn {:pool {} :spec {:uri "redis://localhost:6379/"}})
#'user/conn
user=> (car/wcar conn (car/ping))
Execution error (ExceptionInfo) at taoensso.carmine.protocol/get-unparsed-reply (protocol.clj:143).
NOAUTH Authentication required.

user=> (def conn {:pool {} :spec {:uri "redis://default:[REDACTED]@localhost:6379/"}})
#'user/conn
user=> (car/wcar conn (car/ping))
"PONG"

user=> (def conn {:pool {} :spec {:uri "redis://srv_usr:[REDACTED]@localhost:6379/"}})
#'user/conn
user=> (car/wcar conn (car/ping))
"PONG"

Good !

The problem

When I disable the user default, user srv_usr also stop working. However, redis-cli correctly handles srv_usr.

sed -i '/user default on/d' redis.conf
echo 'user default off' >> redis.conf

Restart carmine-test Docker container.

docker-compose down
docker-compose up

Test well-behave redis-CLI

docker run --rm -it --net=host --env-file redis-cli.env redis redis-cli -h localhost -p 6379 --user default ping
Warning: AUTH failed
(error) NOAUTH Authentication required.

docker run --rm -it --net=host --env-file redis-cli.env redis redis-cli -h localhost -p 6379 --user srv_usr ping
[sudo] password for root: 
PONG

VS carmine

lein repl
user=> (require '[taoensso.carmine :as car :refer (wcar)])
nil
user=> (def conn {:pool {} :spec {:uri "redis://default:[REDACTED]@localhost:6379/"}})
#'user/conn
user=> (car/wcar conn (car/ping))
Execution error (ExceptionInfo) at taoensso.carmine.protocol/get-unparsed-reply (protocol.clj:143).
WRONGPASS invalid username-password pair or user is disabled.

user=> (def conn {:pool {} :spec {:uri "redis://srv_usr:[REDACTED]@localhost:6379/"}})
#'user/conn
user=> (car/wcar conn (car/ping))
Execution error (ExceptionInfo) at taoensso.carmine.protocol/get-unparsed-reply (protocol.clj:143).
WRONGPASS invalid username-password pair or user is disabled.

Solution

The namespace taoensso.carmine.connections at commit 1ee8781 destructures the user name, but never uses it afterwards. Hence, Carmine always uses the user default made by Redis instead of the one provided.

At a quick glance, it seems it requires changes to 3 places to get it fixed:

src/taoensso/carmine/connections.clj:218:(def conn-spec
src/taoensso/carmine/connections.clj:77(defn make-new-connection
src/taoensso/carmine/commands.edn:99

I don't want to engage myself into this issue, but I will take a stab at it.

ptaoussanis commented 3 years ago

Hi there!

Redis support for auth [<username>] <password> was added to Redis in v6, and this isn't yet supported by Carmine. Would be happy to see a PR to add support. Otherwise will do this myself next time I'm working on Carmine.

At a quick glance, it seems it requires changes to 3 places to get it fixed

Sounds correct to me! (Though please don't modify the commands.edn, that'll be covered by https://github.com/ptaoussanis/carmine/pull/251)

ptaoussanis commented 1 year ago

Closing for now since #257 will be merged in forthcoming release 👍

taoensso / carmine