DEVOPS-2694-security-lightrun-installer-container-must-not-consume-secrets-as-env-vars #46
Conversation
yitzhaktal
force-pushed
the
DEVOPS-2694-security-lightrun-installer-container-must-not-consume-secrets-as-env-vars
branch
23 times, most recently
from
222dfc3 to
d3b55a9
Compare
…crets as files via volumes instead of exposing them as environment variables in containers.
yitzhaktal
force-pushed
the
DEVOPS-2694-security-lightrun-installer-container-must-not-consume-secrets-as-env-vars
branch
from
d3b55a9 to
d4efd4d
Compare
…ainer-must-not-consume-secrets-as-env-vars
| @@ -15,4 +15,4 @@ type: application | |||
| # This is the chart version. This version number should be incremented each time you make changes | |||
| # to the chart and its templates, including the app version. | |||
| # Versions are expected to follow Semantic Versioning (https://semver.org/) | |||
| version: 0.0.1 | |||
| version: 0.0.2 | |||
There was a problem hiding this comment.
you don't need to bump version directly on the Chart.yaml file. it automatically bumped as part of release pipeline.
| @@ -18,6 +18,7 @@ spec: | |||
| secretName: {{ .name }}-secret | |||
| {{- end }} | |||
| serverHostname: {{ .serverHostname }} | |||
| useSecretAsEnvVars: {{ .useSecretAsEnvVars | default true }} | |||
There was a problem hiding this comment.
Consider wrap it with if condition. because I might use newer version of lightrun-agents chart with an old values that not necessary has this field and I assume it will cause template error.
| useSecretAsEnvVars: {{ .useSecretAsEnvVars | default true }} | |
| {{- if .useSecretAsEnvVars }} | |
| useSecretAsEnvVars: {{ .useSecretAsEnvVars }} | |
| {{- end }} |
| @@ -72,19 +70,21 @@ func (r *LightrunJavaAgentReconciler) patchDeployment(lightrunJavaAgent *agentv1 | |||
| if err != nil { | |||
| return err | |||
| } | |||
| deploymentApplyConfig.Spec.Template.Spec.WithSecurityContext( | |||
| corev1ac.PodSecurityContext(). | |||
| WithFSGroup(1000), | |||
There was a problem hiding this comment.
Why we need this? it means we are overwriting customer's fsGroup on the pod level. if this is really needed why need to find an alternative way:
| WithReadOnlyRootFilesystem(true). | ||
| WithAllowPrivilegeEscalation(false). | ||
| WithRunAsNonRoot(true). | ||
| WithRunAsUser(1000), |
There was a problem hiding this comment.
why we had to change the entire securityContext?
before (old operator):
securityContext:
capabilities:
drop:
- ALL
runAsNonRoot: true
allowPrivilegeEscalation: false
seccompProfile:
type: RuntimeDefaultafter (this operator):
securityContext:
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true
runAsNonRoot: true
runAsUser: 1000In particular the drop capabilities and seccompProfile and hardcode the user id.
I assume it might not work on openshift clusters.
| WithReadOnlyRootFilesystem(true). | ||
| WithAllowPrivilegeEscalation(false). | ||
| WithRunAsNonRoot(true). | ||
| WithRunAsUser(1000), |
There was a problem hiding this comment.
same as my previous comment
| @@ -335,7 +392,7 @@ func (r *LightrunJavaAgentReconciler) patchStatefulSetAppContainers(lightrunJava | |||
| WithName(container.Name). | |||
| WithImage(container.Image). | |||
| WithVolumeMounts( | |||
| corev1ac.VolumeMount().WithMountPath(lightrunJavaAgent.Spec.InitContainer.SharedVolumeMountPath).WithName(lightrunJavaAgent.Spec.InitContainer.SharedVolumeName), | |||
| corev1ac.VolumeMount().WithName(lightrunJavaAgent.Spec.InitContainer.SharedVolumeName).WithMountPath(lightrunJavaAgent.Spec.InitContainer.SharedVolumeMountPath), | |||
There was a problem hiding this comment.
I see no reason for this change, you basically switch the position of WithName() and WithMountPath()
it creates inconsistency of the other function that patch deployment
Please revert this line
|
|
||
| COPY lightrun-init-agent/$FILE /tmp/$FILE |
There was a problem hiding this comment.
Why you removed the relative path? it probably won't work during building the image as part of release pipeline because of incorrect context
|
|
||
| USER 1000 | ||
| COPY lightrun-init-agent/update_config.sh /update_config.sh |
There was a problem hiding this comment.
Same as my previous comment
| # Copy and set permissions for update_config.sh before switching user | ||
| COPY update_config.sh /update_config.sh | ||
| RUN chmod 750 /update_config.sh && \ | ||
| chown 1000:1000 /update_config.sh |
There was a problem hiding this comment.
This comment is relevant to all lines above that modify permissions. I do not think it will work in openshift, please see comment -
No description provided.