THREESCALE 9308 HeadCrab Vulnerability (Redis) #968

valerymo · 2024-04-07T07:08:43Z

Jira: https://issues.redhat.com/browse/THREESCALE-9308

This PR is recreated from #954, that was closed because of significant changes in master

Current PR is intended to prevent redis replication and slaves creation to prevent HeadCrab Vulnerability.
for Internal Redis Only.
If Redis is Internal than following records are added to configuration to disable replicaoff and slaveoff redis cli commands:
rename-command REPLICAOF ""
rename-command SLAVEOF ""
If both or one of these rename-command records removed or not found in redis-config config map - controller will restore them.
PR does not prevent changes to other settings in the reids configuration (not REPLICAOF or SLAVEOF)

Tests Preparation - files

s3-creds-secret.yaml

kind: Secret
apiVersion: v1
metadata: 
  name: s3-credentials
  namespace: 3scale-test
data: 
  AWS_ACCESS_KEY_ID: QUtJQVY2SVcccc
  AWS_SECRET_ACCESS_KEY: aU5VbWdZY3hjSDFccc
  AWS_BUCKET: dm1vY2NzZjZxOGxyZWRoYXRyaGccccc
  AWS_REGION: ZXUtd2Vcccc
type: Opaque

apimanagerCR.yaml

apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: example-apimanager
spec:
  system: 
    fileStorage: 
      simpleStorageService: 
        configurationSecretRef: 
          name: s3-credentials
  wildcardDomain: apps.xxxxxx.axxxx.xxx.xxxxx.org

Validation

get this PR

cd .../3scale-operator and run 3scale installation:

make install
oc new-project 3scale-test
make download
make run

in another xterm:

oc apply -f s3-creds-secret.yaml
oc apply -f apimanagerCR.yaml

Check that redis-config ConfigMap has required changes:

$ oc describe cm redis-config |grep rename-command
rename-command REPLICAOF ""
rename-command SLAVEOF ""

Check that replicaof and slaveof commands are disabled.
Expected behavior as in example below

kubectl exec -ti $(kubectl get pods -l deployment=backend-redis -o name) -- /bin/bash
bash-4.2$ redis-cli 
127.0.0.1:6379> replicaof 10.10.10.10 6379
(error) ERR unknown command `replicaof`, with args beginning with: `10.10.10.10`, `6379`,
127.0.0.1:6379> replicaof
(error) ERR unknown command `replicaof`, with args beginning with: 
127.0.0.1:6379> slaveof
(error) ERR unknown command `slaveof`, with args beginning with: 
127.0.0.1:6379>

Notes: Internally, within redis system, the slaveof and replicaof commands will be disabled.
So the only way to enable it would be changing the configuration
See next item - that operator is controllilng redis-config config map and will restore records that prevening HeadCrab Vulnerability.

Check that redis-config config map changes are reverted

Edit config map - delete rename-command REPLICAOF "" or/and rename-command SLAVEOF ""

$ oc edit cm redis-config
......
......
// delete these lines
  rename-command REPLICAOF ""
  rename-command SLAVEOF ""

$ oc edit cm redis-config
configmap/redis-config edited

Check configMap and see that these lines are exists (reconsiler reverted configmap)

rename-command REPLICAOF ""
rename-command SLAVEOF ""
.....
Events:  <none>
$ oc describe cm redis-config

Check Upgrade

Following validation was done

Create Images/Indexes for master and for branch of PR
Install master ; check that redis-config CM does Not contain rename-command for REPLICAOF and SLAVEOF

$ oc describe cm redis-config |grep rename-com

Do upgrade and check that Redis configuration was updated, new records added:

$ oc describe cm redis-config |grep rename-com
rename-command REPLICAOF ""
rename-command SLAVEOF ""

Regression testing

PR does not prevent changes to other settings in the reids configuration (not REPLICAOF or SLAVEOF)
Make sure it works as expected:

edit the redis-config configuration map, edit one or more entries/definitions.
describe the redis-config configuration map and make sure the changes are in place.

Wrong formatting of redis configuration
In Redis, if the configuration file (redis.conf) contains a setting with an incorrect name or syntax, Redis will typically treat it as an error. Redis expects the configuration file to be correctly formatted with valid configuration directives.
If there's a typo or an unrecognized setting name in the configuration file, Redis will log an error when starting up. - -- try change one or rename-command or other settings name, or add something liketest in redis-config config map. Expected behavior:

$ oc edit cm redis-config
....
    dir /var/lib/redis/data
    xxxrename-command SLAVEOF ""
    xxxrename-command REPLICAOF ""
kind: ConfigMap
metadata:
.......
$ oc edit cm redis-config
configmap/redis-config edited

$ oc get pod |grep redis
backend-redis-6fcbd5964f-tnp7n           0/1     CrashLoopBackOff   1 (5s ago)    6s
system-redis-77bbb6f955-d9nxs            0/1     CrashLoopBackOff   1 (5s ago)    6s

$ oc logs backend-redis-6fcbd5964f-tnp7n
*** FATAL CONFIG FILE ERROR (Redis 6.2.7) ***
Reading the configuration file, at line 47
>>> 'xxxrename-command SLAVEOF ""'
Bad directive or wrong number of arguments
$

restore changed settings:

    rename-command SLAVEOF ""
    rename-command REPLICAOF ""
kind: ConfigMap
metadata:
  creationTimestamp: "2024-04-08T16:07:40Z"
$ oc edit cm redis-config
configmap/redis-config edited

$ oc get pod |grep redis
backend-redis-59bd88f884-4tvrk           1/1     Running            0             39s
system-redis-549598d55b-nk9lr            1/1     Running            0             38s

valerymo · 2024-04-07T10:16:56Z

Local Fresh install TEST

$ oc get pod  |grep redis
backend-redis-76c9c446cc-6hrsj           1/1     Running     0               4m35s
system-redis-68857df9c7-zgfr2            1/1     Running     0               4m33s

$ oc rsh backend-redis-76c9c446cc-6hrsj
sh-5.1$ redis-cli
127.0.0.1:6379> replicaof
(error) ERR unknown command `replicaof`, with args beginning with: 
127.0.0.1:6379> replicaof 10.10.10.10 6379
(error) ERR unknown command `replicaof`, with args beginning with: `10.10.10.10`, `6379`, 
127.0.0.1:6379> slaveof
(error) ERR unknown command `slaveof`, with args beginning with: 
127.0.0.1:6379> 
......

$ oc rsh system-redis-68857df9c7-zgfr2
sh-5.1$ redis-cli
127.0.0.1:6379> replicaof
(error) ERR unknown command `replicaof`, with args beginning with: 
127.0.0.1:6379> slaveof
(error) ERR unknown command `slaveof`, with args beginning with: 
127.0.0.1:6379>
.....

regarding comment in old PR 954 possible redeployment of redis pod in Local Fresh install

Looks like it happens for redis-system:
Below is log for fresh local install


$ oc rollout history deploy backend-redis
deployment.apps/backend-redis 
REVISION  CHANGE-CAUSE
1         <none>

$ oc rollout history deploy system-redis 
deployment.apps/system-redis 
REVISION  CHANGE-CAUSE
1         <none>
2         <none>

valerymo · 2024-04-07T16:25:12Z

Applied and tested: https://github.com/valerymo/3scale-operator/pull/5/files -
to avoid redeployment of redis pod in fresh installation.
(Changed applied manually; not git merge)

$ oc rollout history deploy backend-redis
deployment.apps/backend-redis 
REVISION  CHANGE-CAUSE
1         <none>

$ oc rollout history deploy system-redis
deployment.apps/system-redis 
REVISION  CHANGE-CAUSE
1         <none>

$ oc get pod |grep redis
backend-redis-6c69fd9bf9-925xg           1/1     Running     0               4m7s
system-redis-745b655b47-flk7r            1/1     Running     0               4m7s

$ oc rsh backend-redis-6c69fd9bf9-925xg
sh-5.1$ redis-cli
127.0.0.1:6379> slaveof
(error) ERR unknown command `slaveof`, with args beginning with: 
127.0.0.1:6379> replicaof 10.10.10.10 6379
(error) ERR unknown command `replicaof`, with args beginning with: `10.10.10.10`, `6379`, 
127.0.0.1:6379> replicaof
(error) ERR unknown command `replicaof`, with args beginning with: 
127.0.0.1:6379> slaveof
(error) ERR unknown command `slaveof`, with args beginning with: 
127.0.0.1:6379>

valerymo · 2024-04-07T16:27:29Z

TODO - retest Upgrade from 2.14 to PR

pkg/3scale/amp/component/redis.go

MStokluska · 2024-04-08T16:47:49Z

pkg/reconcilers/configmap.go

+
+		slaveOfString := "rename-command SLAVEOF \"\""
+		if !strings.Contains(existingString, slaveOfString) {
+			newString := existingString + "\n" + slaveOfString


I think this needs to be:
existingString = existingString + "\n" + <RENAME_COMMAND> in both, REPLICAOF and SLAVEOF.
Otherwise we are going to need 2 reconciles for both to be applied if both are missing.

bundle/manifests/3scale-operator.clusterserviceversion.yaml

eguzki · 2024-04-09T08:08:32Z

bundle/manifests/3scale-operator.clusterserviceversion.yaml

@@ -515,15 +515,15 @@ spec:
                - name: RELATED_IMAGE_SYSTEM_MEMCACHED
                  value: memcached:1.5
                - name: RELATED_IMAGE_BACKEND_REDIS
-                  value: quay.io/centos7/redis-6-centos7:latest


make these changes in another PR, please. They are not related to this PR issue

eguzki · 2024-04-09T08:09:51Z

pkg/3scale/amp/component/redis.go

+	}
+}
+
+func (redis *Redis) buildConfigMapTypeMeta() metav1.TypeMeta {


this is my own leftoever, can you please remove it? thnx

eguzki · 2024-04-09T08:16:47Z

pkg/reconcilers/configmap.go

+
+		replicaOfString := "rename-command REPLICAOF \"\""
+		if !strings.Contains(existingString, replicaOfString) {
+			newString := existingString + "\n" + replicaOfString


I will leave this up to you. I would not do this. I would not "modify" a file concatenating strings. It's error prone. If the rename-command REPLICAOF \"\" is not there, the file is invalid and hence, the operator updates it entirely.

the idea was to append to the existing config if not present rather than fully replace it. This allows for a custom configuration to continue. It's not clear from the documentation if edits to the configuration are allowed / supported but we don't prevent a user from doing so, so we can assume at least some customers have done so.

…uration

eguzki · 2024-04-09T13:14:27Z

pkg/reconcilers/configmap.go

+		existing.Data[fieldName] = desired.Data[fieldName]
+		updated = true
+	} else {
+		existingString := existingVal


out of interest, what is this doing?

THREESCALE 9308 HeadCrab Vulnerability (Redis)

cb4998a

valerymo requested a review from a team as a code owner April 7, 2024 07:08

openshift-ci bot added the do-not-merge/work-in-progress label Apr 7, 2024

valerymo mentioned this pull request Apr 7, 2024

THREESCALE - 9308 HeadCrab Vulnerability (Redis) #954

Closed

THREESCALE 9308 HeadCrab Vulnerability (Redis)- CM sync first

91c2d43

valerymo force-pushed the THREESCALE-9308-2 branch from c868d3f to 91c2d43 Compare April 8, 2024 06:01

valerymo changed the title ~~[WIP]THREESCALE 9308 HeadCrab Vulnerability (Redis)~~ THREESCALE 9308 HeadCrab Vulnerability (Redis) Apr 8, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 8, 2024

MStokluska reviewed Apr 8, 2024

View reviewed changes

pkg/3scale/amp/component/redis.go Show resolved Hide resolved

valerymo force-pushed the THREESCALE-9308-2 branch 4 times, most recently from 06790dc to 41b2e03 Compare April 8, 2024 15:24

MStokluska reviewed Apr 8, 2024

View reviewed changes

valerymo force-pushed the THREESCALE-9308-2 branch from 41b2e03 to 8143ef5 Compare April 9, 2024 05:53

MStokluska reviewed Apr 9, 2024

View reviewed changes

bundle/manifests/3scale-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

MStokluska reviewed Apr 9, 2024

View reviewed changes

bundle/manifests/3scale-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

eguzki reviewed Apr 9, 2024

View reviewed changes

valerymo force-pushed the THREESCALE-9308-2 branch 4 times, most recently from e520709 to 77d8642 Compare April 9, 2024 09:10

eguzki approved these changes Apr 9, 2024

View reviewed changes

THREESCALE 9308 HeadCrab Vulnerability - avoid impact existing config…

65cef0a

…uration

valerymo force-pushed the THREESCALE-9308-2 branch from 77d8642 to 65cef0a Compare April 9, 2024 11:59

valerymo merged commit f69d56b into 3scale:master Apr 9, 2024
13 checks passed

eguzki reviewed Apr 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE 9308 HeadCrab Vulnerability (Redis) #968

THREESCALE 9308 HeadCrab Vulnerability (Redis) #968

valerymo commented Apr 7, 2024 •

edited

valerymo commented Apr 7, 2024 •

edited

valerymo commented Apr 7, 2024

valerymo commented Apr 7, 2024 •

edited

MStokluska Apr 8, 2024

eguzki Apr 9, 2024

eguzki Apr 9, 2024

eguzki Apr 9, 2024

briangallagher Apr 9, 2024

eguzki Apr 9, 2024

eguzki Apr 9, 2024 •

edited

THREESCALE 9308 HeadCrab Vulnerability (Redis) #968

THREESCALE 9308 HeadCrab Vulnerability (Redis) #968

Conversation

valerymo commented Apr 7, 2024 • edited

Tests Preparation - files

Validation

Check Upgrade

Regression testing

valerymo commented Apr 7, 2024 • edited

valerymo commented Apr 7, 2024

valerymo commented Apr 7, 2024 • edited

MStokluska Apr 8, 2024

Choose a reason for hiding this comment

eguzki Apr 9, 2024

Choose a reason for hiding this comment

eguzki Apr 9, 2024

Choose a reason for hiding this comment

eguzki Apr 9, 2024

Choose a reason for hiding this comment

briangallagher Apr 9, 2024

Choose a reason for hiding this comment

eguzki Apr 9, 2024

Choose a reason for hiding this comment

eguzki Apr 9, 2024 • edited

Choose a reason for hiding this comment

valerymo commented Apr 7, 2024 •

edited

valerymo commented Apr 7, 2024 •

edited

valerymo commented Apr 7, 2024 •

edited

eguzki Apr 9, 2024 •

edited