Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/895 - helm chart security fixes for OasisPlatform #938

Closed
wants to merge 7 commits into from
Closed

Feature/895 - helm chart security fixes for OasisPlatform #938

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5527d92
modified files for Helm
slashme101 Dec 11, 2023
6144f14
added pv,pvc files and modified yml files
slashme101 Jan 2, 2024
c4752af
removed a commented line
slashme101 Jan 2, 2024
3f67d40
modified workers and oasis_server files
slashme101 Jan 8, 2024
534c45d
updated readonly on all files
slashme101 Jan 10, 2024
66542ac
modified volume mounts on these files
slashme101 Jan 15, 2024
95825cb
updated readonly
slashme101 Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion kubernetes/charts/oasis-models/templates/workers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ spec:
app: {{ $name }}
strategy:
type: Recreate
replicas: 0
replicas: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Set this back to the default of 0

Copy link
Author

Choose a reason for hiding this comment

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

Done

template:
metadata:
labels:
Expand All @@ -33,6 +33,8 @@ spec:
containers:
- image: {{ .image }}:{{ .version }}
{{- if .imagePullPolicy }}
# securityContext:
# readOnlyRootFilesystem: true
imagePullPolicy: {{ .imagePullPolicy }}
{{- end }}
name: worker
Expand Down
2 changes: 2 additions & 0 deletions kubernetes/charts/oasis-monitoring/templates/flower.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ spec:
containers:
- name: flower
image: {{ .Values.monitoringImages.flower.image }}:{{- .Values.monitoringImages.flower.version }}
securityContext:
readOnlyRootFilesystem: true
imagePullPolicy: IfNotPresent
command: [ "celery" ]
args: [ "flower", "--broker_api=http://guest:guest@broker:6379" ]
Expand Down
3 changes: 3 additions & 0 deletions kubernetes/charts/oasis-platform/templates/keycloak.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ spec:
containers:
- name: {{ .Values.keycloak.name }}
image: {{ .Values.images.keycloak.image }}:{{ .Values.images.keycloak.version }}
securityContext:
readOnlyRootFilesystem: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Keycloak fails with a readOnly Filesystem:

NAME                                                     READY   STATUS             RESTARTS        AGE
  ... 
keycloak-5f7cc8c7c7-ngpp2                                0/1     CrashLoopBackOff   5 (106s ago)    7m29s

ERROR: java.nio.file.FileSystemException: /opt/keycloak/lib/quarkus/quarkus-application.dat: Read-only file system
ERROR: /opt/keycloak/lib/quarkus/quarkus-application.dat: Read-only file system
For more details run the same command passing the '--verbose' option. Also you can use '--help' to see the details about the usage of the particular command.

See keycloak/keycloak#11286 for workaround

args: [
"start-dev",
"--import-realm",
Expand Down Expand Up @@ -174,6 +176,7 @@ spec:

volumes:
- name: realm-config
#readOnly: true
Copy link
Contributor

Choose a reason for hiding this comment

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

was this tested as ReadOnly, it should work since its not intended to be edited?

Copy link
Author

Choose a reason for hiding this comment

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

Done

configMap:
name: {{ $realmSecretName }}
{{- if (.Values.azure).secretProvider }}
Expand Down
2 changes: 2 additions & 0 deletions kubernetes/charts/oasis-platform/templates/oasis.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,8 @@ spec:
{{- include "h.initTcpAvailabilityCheckBySecret" (list . .Values.databases.oasis_db.name .Values.databases.celery_db.name .Values.databases.channel_layer.name) | nindent 8}}
containers:
- image: {{ .Values.images.oasis.platform.image }}:{{ .Values.images.oasis.platform.version }}
securityContext:
readOnlyRootFilesystem: true
Copy link
Contributor

Choose a reason for hiding this comment

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

celery beat fails because it needs access to a temp dir:

celery-beat-848fcc5cb-jlgbn                              0/1     CrashLoopBackOff   6 (3m54s ago)   11m

  File "/usr/lib/python3.10/tempfile.py", line 438, in gettempdir
    return _os.fsdecode(_gettempdir())
  File "/usr/lib/python3.10/tempfile.py", line 431, in _gettempdir
    tempdir = _get_default_tempdir()
  File "/usr/lib/python3.10/tempfile.py", line 362, in _get_default_tempdir
    raise FileNotFoundError(_errno.ENOENT,
FileNotFoundError: [Errno 2] No usable temporary directory found in ['/tmp', '/var/tmp', '/usr/tmp', '/var/www/oasis']

Try mounting a RW volume to allow writing tmp files (see keycloak link)

imagePullPolicy: {{ .Values.images.oasis.platform.imagePullPolicy }}
name: celery-beat
command: ["celery", "-A", "src.server.oasisapi.celery_app", "beat", "--loglevel=DEBUG"]
Expand Down
3 changes: 3 additions & 0 deletions kubernetes/charts/oasis-platform/templates/oasis_server.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ spec:
{{- include "h.initTcpAvailabilityCheckBySecret" (list . .Values.databases.oasis_db.name .Values.databases.celery_db.name .Values.keycloak.name .Values.databases.broker.name) | nindent 8}}
containers:
- image: {{ .Values.images.oasis.platform.image }}:{{ .Values.images.oasis.platform.version }}
securityContext:
readOnlyRootFilesystem: true
name: {{ .Values.oasisWebsocket.name }}
imagePullPolicy: {{ .Values.images.oasis.platform.imagePullPolicy }}
env:
Expand Down Expand Up @@ -147,6 +149,7 @@ spec:
volumes:
{{- if ((.Values.volumes.host).sharedFs) }}
- name: shared-fs-persistent-storage
readonly: true
persistentVolumeClaim:
claimName: {{ .Values.volumes.host.sharedFs.name }}
{{- else if ((.Values.volumes.azureFiles).sharedFs) }}
Expand Down
3 changes: 3 additions & 0 deletions kubernetes/charts/oasis-platform/templates/oasis_worker_controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,15 @@ spec:
annotations:
checksum/{{ .Values.oasisServer.name }}: {{ toJson .Values.oasisWebsocket | sha256sum }}
spec:
#automountServiceAccountToken: false
Copy link
Contributor

Choose a reason for hiding this comment

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

PR is missing this request from the ticket

Disable automounting API credentials (service account or pod level, automountServiceAccountToken: false). If Kubernetes API is needed than it should be explicitly mounted.

I think this line should read as true (explicitly allowed for just this pod) while the default at the ServiceAccount level should be automount disabled (so all other pods don't have access
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/
automountServiceAccountToken: True

Copy link
Contributor

Choose a reason for hiding this comment

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

OasisPlatform/kubernetes/charts/oasis-platform/templates/oasis.yaml

Lines 252 to 273 in 534c45d

app: oasis-task-controller
strategy:
type: Recreate
template:
metadata:
labels:
app: oasis-task-controller
{{- include "h.labels" . | nindent 8}}
annotations:
checksum/{{ .Values.databases.oasis_db.name }}: versions{{ toJson .Values.databases.oasis_db | sha256sum }}
checksum/{{ .Values.databases.celery_db.name }}: versions{{ toJson .Values.databases.celery_db | sha256sum }}
checksum/{{ .Values.databases.broker.name }}: versions{{ toJson .Values.databases.broker | sha256sum }}
checksum/{{ .Values.databases.channel_layer.name }}: versions{{ toJson .Values.databases.channel_layer | sha256sum }}
spec:
{{- include "h.affinity" . | nindent 6 }}
initContainers:
{{- include "h.initTcpAvailabilityCheckBySecret" (list . .Values.databases.oasis_db.name .Values.databases.celery_db.name .Values.databases.channel_layer.name .Values.databases.broker.name) | nindent 8}}
containers:
- image: {{ .Values.images.oasis.platform.image }}:{{ .Values.images.oasis.platform.version }}
imagePullPolicy: {{ .Values.images.oasis.platform.imagePullPolicy }}
name: main
command: ["celery", "-A", "src.server.oasisapi.celery_app", "worker", "--loglevel=INFO", "-Q", "task-controller"]

oasis-task-controller is also missing its ReadOnly mount, but that will also need a Read/Write mount to the /tmp directory, otherwise it will also crash, like with celery beat.

{{- include "h.affinity" . | nindent 6 }}
serviceAccountName: {{ $workerControllerServiceAccountName }}
initContainers:
{{- include "h.initTcpAvailabilityCheckBySecret" (list . .Values.oasisWebsocket.name) | nindent 8}}
containers:
- image: {{ .Values.images.oasis.worker_controller.image }}:{{ .Values.images.oasis.worker_controller.version }}
securityContext:
readOnlyRootFilesystem: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Error: INSTALLATION FAILED: YAML parse error on oasis/templates/oasis_worker_controller.yaml: error converting YAML to JSON: yaml: line 63: did not find expected '-' indicator

Indention error

Copy link
Author

Choose a reason for hiding this comment

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

Done

imagePullPolicy: {{ .Values.images.oasis.worker_controller.imagePullPolicy }}
name: main
env:
Expand Down
Loading