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

Jump to bottom

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

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

wants to merge 7 commits into from

Conversation

slashme101
Copy link

@slashme101 slashme101 commented Jan 2, 2024 •
edited by sambles
Loading

Feature/895

**IMPORTANT: Please attach or create an issue after submitting a Pull Request.

Helm chart security fixes for OasisPlatform

Modified files for Helm

@sambles sambles linked an issue Jan 11, 2024 that may be closed by this pull request
helm chart security fixes for Oasis LMF #895
Open
@sambles sambles changed the title Feature/895 Feature/895 - helm chart security fixes for Oasis LMF Jan 11, 2024
@sambles sambles changed the title Feature/895 - helm chart security fixes for Oasis LMF Feature/895 - helm chart security fixes for OasisPlatform Jan 11, 2024
sambles
sambles requested changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@sambles sambles left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Task needs some more work, see comments

kubernetes/charts/oasis-models/templates/workers.yaml Outdated Show resolved Hide resolved
kubernetes/charts/oasis-platform/templates/keycloak.yaml Outdated
@@ -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

kubernetes/charts/oasis-models/templates/workers.yaml Outdated
@@ -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

kubernetes/charts/oasis-platform/templates/oasis_worker_controller.yaml Outdated
Comment on lines 60 to 61
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

kubernetes/charts/oasis-platform/templates/keycloak.yaml Outdated
Comment on lines 97 to 98
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

kubernetes/charts/oasis-platform/templates/oasis.yaml
@@ -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)

kubernetes/charts/oasis-platform/templates/oasis_worker_controller.yaml Outdated
@@ -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.

@sambles sambles marked this pull request as draft July 31, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sambles sambles sambles requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Oasis Dev Team Tasks
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

helm chart security fixes for Oasis LMF
2 participants
@slashme101 @sambles