Skip to content

Support redis user password auth and tls encryption - #27888

Merged
skjnldsv merged 3 commits into
masterfrom
fix/redis-auth
Jul 22, 2021
Merged

Support redis user password auth and tls encryption#27888
skjnldsv merged 3 commits into
masterfrom
fix/redis-auth

Conversation

@skjnldsv

@skjnldsvskjnldsv commented Jul 9, 2021

Copy link
Copy Markdown
Member
'memcache.locking' => '\OC\Memcache\Redis',
'memcache.distributed' => '\OC\Memcache\Redis',
'memcache.local' =>'\OC\Memcache\Redis' ,'redis' => [
'host' => 'tls://127.0.0.1',
'port' => 6379,
'user' => 'nextcloud',
'password' => 'password',
'ssl_context' => [
'local_cert' => '/certs/redis.crt',
'local_pk' => '/certs/redis.key',
'cafile' => '/certs/ca.crt',
'verify_peer_name' => false
]
]

(From https://redis.io/topics/encryption)

  1. Download redis source https://github.com/redis/redis/archive/refs/tags/6.2.4.tar.gz
  2. Generate certificates from ./utils/gen-test-certs.sh
  3. Create a users.acl config file containing user nextcloud on +@all ~* >password
  4. Start redis server
redis-server --tls-port 6379 --port 0 \
--tls-cert-file ./certs/redis.crt \
--tls-key-file ./certs/redis.key \
--tls-ca-cert-file ./certs/ca.crt \
--tls-auth-clients optional \
--aclfile users.acl \
--protected-mode no
  1. Setup nextcloud with above config
  2. Profit

Questions

  • How shall we handle the support and throw of old phpredis module version? Is there a way to say phpredis > 5.x.x for Nextcloud 23 ?

@skjnldsvskjnldsv added 2. developing Work in progress enhancement feature: caching Related to our caching system: scssCacher, jsCombiner... labels Jul 9, 2021
@kesselb

kesselb commented Jul 12, 2021

Copy link
Copy Markdown
Contributor

To fix Psalm: #27928

@kesselb

This comment has been minimized.

@skjnldsvskjnldsv changed the title Support redis user password authSupport redis user password auth and tls encryptionJul 20, 2021
@skjnldsv
skjnldsvforce-pushed the fix/redis-auth branch 2 times, most recently from 494b9dc to 99ed44cCompareJuly 20, 2021 08:36
@skjnldsvskjnldsv self-assigned this Jul 20, 2021
@skjnldsvskjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 20, 2021
@skjnldsvskjnldsv added this to the Nextcloud 23 milestone Jul 20, 2021
@skjnldsv

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@juliusknorr

This comment has been minimized.

@PVince81

This comment has been minimized.

@skjnldsv

This comment has been minimized.

@PVince81

Copy link
Copy Markdown
Member

I've tested this PR and it worked for me locally with the instructions.

@skjnldsv I think we talked about the possibility of adding/updating unit tests for the RedisFactory, is this still to do ?

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

@skjnldsv I think we talked about the possibility of adding/updating unit tests for the RedisFactory, is this still to do ?

I'm lacking the knowledge and time for that, wanna focus on it?

@PVince81

Copy link
Copy Markdown
Member

@skjnldsv I think we talked about the possibility of adding/updating unit tests for the RedisFactory, is this still to do ?

I'm lacking the knowledge and time for that, wanna focus on it?

I had a look and it would require some extra gymnastics (refactoring, injection) to be able to mock Redis and RedisCluster and considering the low complexity it's probably not worth the effort.

@PVince81

This comment has been minimized.

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Done, I upgraded and refactored everything for cleaner code.
Please review again @PVince81 and @juliushaertl

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

@PVince81PVince81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 code looks fine, I didn't retest though.

cool that you were able to also add the bits for RedisCluster

Comment threadconfig/config.sample.php Outdated
Comment threadlib/private/RedisFactory.php Outdated
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Okay, I went down the rabbit hole

Tests setups NO TLS

'memcache.locking' => '\OC\Memcache\Redis',
'memcache.distributed' => '\OC\Memcache\Redis',
'memcache.local' =>'\OC\Memcache\Redis' ,'redis.cluster' => array(
'seeds' => [
'redis-node-0:6379',
'redis-node-1:6379',
'redis-node-2:6379',
'redis-node-3:6379',
'redis-node-4:6379',
'redis-node-5:6379',
],
'failover_mode' => \RedisCluster::FAILOVER_ERROR,
'password' => 'password'# optional if you're using the protected config
),
Docker-compose unprotected cluster
redis-node-0:
image: docker.io/bitnami/redis-clusterenvironment:
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-1:
image: docker.io/bitnami/redis-clusterenvironment:
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-2:
image: docker.io/bitnami/redis-clusterenvironment:
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-3:
image: docker.io/bitnami/redis-clusterenvironment:
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-4:
image: docker.io/bitnami/redis-clusterenvironment:
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-5:
image: docker.io/bitnami/redis-clusterdepends_on:
- redis-node-0
- redis-node-1
- redis-node-2
- redis-node-3
- redis-node-4volumes:
- ./certs:/certsenvironment:
# - REDIS_TLS_ENABLED=yes# - REDIS_PORT_NUMBER=0# - REDIS_TLS_PORT=6379# - REDIS_TLS_CERT_FILE=/certs/redis.crt# - REDIS_TLS_KEY_FILE=/certs/redis.key# - REDIS_TLS_CA_FILE=/certs/ca.crt
- ALLOW_EMPTY_PASSWORD=yes
- REDIS_CLUSTER_REPLICAS=1
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5
- REDIS_CLUSTER_CREATOR=yes
- REDIS_CLUSTER_SLEEP_BEFORE_DNS_LOOKUP=5networks:
- nextcloud
Docker-compose password protected cluster
redis-node-0:
image: docker.io/bitnami/redis-clusterenvironment:
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-1:
image: docker.io/bitnami/redis-clusterenvironment:
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-2:
image: docker.io/bitnami/redis-clusterenvironment:
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-3:
image: docker.io/bitnami/redis-clusterenvironment:
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-4:
image: docker.io/bitnami/redis-clusterenvironment:
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-5:
image: docker.io/bitnami/redis-clusterdepends_on:
- redis-node-0
- redis-node-1
- redis-node-2
- redis-node-3
- redis-node-4volumes:
- ./certs:/certsenvironment:
# - REDIS_TLS_ENABLED=yes# - REDIS_PORT_NUMBER=0# - REDIS_TLS_PORT=6379# - REDIS_TLS_CERT_FILE=/certs/redis.crt# - REDIS_TLS_KEY_FILE=/certs/redis.key# - REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDISCLI_AUTH=password
- REDIS_CLUSTER_REPLICAS=1
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5
- REDIS_CLUSTER_CREATOR=yes
- REDIS_CLUSTER_SLEEP_BEFORE_DNS_LOOKUP=5networks:
- nextcloud

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Finally but not last: TLS CLUSTER PASSWORD PROTECTED

'memcache.locking' => '\OC\Memcache\Redis',
'memcache.distributed' => '\OC\Memcache\Redis',
'memcache.local' =>'\OC\Memcache\Redis' ,'redis.cluster' => array(
'seeds' => [
'redis-node-0:6379',
'redis-node-1:6379',
'redis-node-2:6379',
'redis-node-3:6379',
'redis-node-4:6379',
'redis-node-5:6379',
],
'failover_mode' => \RedisCluster::FAILOVER_ERROR,
'password' => 'password',
'ssl_context' => [
'local_cert' => '/certs/redis.crt',
'local_pk' => '/certs/redis.key',
'cafile' => '/certs/ca.crt',
'verify_peer_name' => false
]
),
Docker compose config
redis-node-0:
image: docker.io/bitnami/redis-clustervolumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-1:
image: docker.io/bitnami/redis-clustervolumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-2:
image: docker.io/bitnami/redis-clustervolumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-3:
image: docker.io/bitnami/redis-clustervolumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-4:
image: docker.io/bitnami/redis-clustervolumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5networks:
- nextcloudredis-node-5:
image: docker.io/bitnami/redis-clusterdepends_on:
- redis-node-0
- redis-node-1
- redis-node-2
- redis-node-3
- redis-node-4volumes:
- ./certs:/certsenvironment:
- REDIS_PORT_NUMBER=0
- REDIS_TLS_ENABLED=yes
- REDIS_TLS_AUTH_CLIENTS=yes
- REDIS_TLS_PORT=6379
- REDIS_TLS_CERT_FILE=/certs/redis.crt
- REDIS_TLS_KEY_FILE=/certs/redis.key
- REDIS_TLS_CA_FILE=/certs/ca.crt
- REDIS_PASSWORD=password
- REDISCLI_AUTH=password
- REDIS_CLUSTER_REPLICAS=1
- REDIS_NODES=redis-node-0 redis-node-1 redis-node-2 redis-node-3 redis-node-4 redis-node-5
- REDIS_CLUSTER_CREATOR=yes
- REDIS_CLUSTER_SLEEP_BEFORE_DNS_LOOKUP=5networks:
- nextcloud

@skjnldsv

Copy link
Copy Markdown
MemberAuthor

Just for info, all my local cluster tests were successful. 🚀 🎉

@juliusknorr

Copy link
Copy Markdown
Member

Hm failures seem related:

+ ./occ maintenance:install --admin-pass=admin --data-dir=/dev/shm/nc_int
Nextcloud was successfully installed
+ ./occ config:system:set redis host --value=cache
System config value redis => host set to string cache
+ ./occ config:system:set redis port --value=6379 --type=integer
System config value redis => port set to integer 6379
+ ./occ config:system:set redis timeout --value=0 --type=integer
System config value redis => timeout set to integer 0
+ ./occ config:system:set --type string --value "\OC\Memcache\Redis" memcache.local
System config value memcache.local set to string \OC\Memcache\Redis
+ ./occ config:system:set --type string --value "\OC\Memcache\Redis" memcache.distributed
PHP Warning: Redis::connect() expects at most 6 parameters, 7 given in /drone/src/lib/private/RedisFactory.php on line 118
An unhandled exception has been thrown:
RedisException: Redis server went away in /drone/src/lib/private/Memcache/Redis.php:55
Stack trace:
#0 /drone/src/lib/private/Memcache/Redis.php(55): Redis->get('63dfb9db467bd4b...')
#1 /drone/src/lib/private/App/InfoParser.php(58): OC\Memcache\Redis->get('/drone/src/apps...')
#2 /drone/src/lib/private/App/AppManager.php(502): OC\App\InfoParser->parse('/drone/src/apps...')
#3 /drone/src/lib/private/legacy/OC_App.php(593): OC\App\AppManager->getAppInfo('files', false, NULL)
#4 /drone/src/lib/private/AppFramework/App.php(69): OC_App::getAppInfo('files')
#5 /drone/src/lib/private/legacy/OC_App.php(278): OC\AppFramework\App::buildAppNamespace('files')
#6 /drone/src/lib/private/AppFramework/Bootstrap/Coordinator.php(108): OC_App::registerAutoloading('files', '/drone/src/apps...')
#7 /drone/src/lib/private/AppFramework/Bootstrap/Coordinator.php(82): OC\AppFramework\Bootstrap\Coordinator->registerApps(Array)
#8 /drone/src/lib/base.php(640): OC\AppFramework\Bootstrap\Coordinator->runInitialRegistration()
#9 /drone/src/lib/base.php(1083): OC::init()
#10 /drone/src/console.php(48): require_once('/drone/src/lib/...')
#11 /drone/src/occ(11): require_once('/drone/src/cons...')

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv

This comment has been minimized.

@juliusknorr

This comment has been minimized.

@juliusknorr

This comment has been minimized.

@skjnldsvskjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 22, 2021
@skjnldsv
skjnldsv merged commit f14b8aa into masterJul 22, 2021
@skjnldsv
skjnldsv deleted the fix/redis-auth branch July 22, 2021 10:47
@skjnldsv

Copy link
Copy Markdown
MemberAuthor

/backport to stable22

@PVince81

Copy link
Copy Markdown
Member

this introduced a little regression for those like me who relied on default values, PR here to fix it: #28129

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

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancementfeature: cachingRelated to our caching system: scssCacher, jsCombiner...

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@skjnldsv@kesselb@juliusknorr@PVince81