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

SecurityContext added to ActiveGate container #597

Merged
merged 3 commits into from
Mar 14, 2022
Merged

SecurityContext added to ActiveGate container #597

merged 3 commits into from
Mar 14, 2022

Conversation

aorcholski
Copy link
Contributor

@aorcholski aorcholski commented Feb 23, 2022 •
edited

All changes related to ActiveGate container:

  1. the root filesystem is mounted ReadOnly.
  2. security context added:

privileged: false
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true *
runAsNonRoot: true
capabilities:
drop: ["all"]

  • enabled by "alpha.operator.dynatrace.com/feature-activegate-readonly-fs":"true" annotation
  1. seccompProfile is set to:

type: RuntimeDefault

  1. AppArmor is enabled via AG pod annotation *

container.apparmor.security.beta.kubernetes.io/activegate: runtime/default

  • enabled by "alpha.operator.dynatrace.com/feature-activegate-apparmor":"true" annotation

@0sewa0 0sewa0 added activegate Changes related to Activegate security Vulnerabilities or changes to Security labels Feb 23, 2022
@aorcholski aorcholski marked this pull request as ready for review February 23, 2022 15:37
@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch 2 times, most recently from 8177166 to 48b51a5 Compare February 23, 2022 17:27
chrismuellner
chrismuellner previously requested changes Feb 28, 2022
View reviewed changes
Copy link
Collaborator

@chrismuellner chrismuellner left a comment

Choose a reason for hiding this comment

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

Can't be merged for now as it would break older Activegate versions

@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch 3 times, most recently from 321fe2d to 1e45651 Compare March 2, 2022 12:42
@0sewa0
Copy link
Contributor

0sewa0 commented Mar 2, 2022

If you are brave and want to dive into the depts of hell helm, then you could make the dynakube have these annotations when apparmor: true in the helm values file. 😬

We do it like this for a deployment, different annotations, same check
https://github.com/Dynatrace/dynatrace-operator/blob/master/config/helm/chart/default/templates/Common/webhook/deployment-webhook.yaml#L36-L38

@chrismuellner chrismuellner dismissed their stale review March 2, 2022 15:12

Feature flags were added to make problematic changes opt-in

@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch 2 times, most recently from 60dbb46 to 73f47b8 Compare March 3, 2022 15:58
@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch from 73f47b8 to 98d142b Compare March 3, 2022 16:23
chrismuellner
chrismuellner requested changes Mar 4, 2022
View reviewed changes
src/kubeobjects/slice.go Show resolved Hide resolved
src/api/v1beta1/feature_flags.go Outdated Show resolved Hide resolved
config/helm/chart/default/templates/Common/customresource-dynakube.yaml Outdated Show resolved Hide resolved
src/controllers/activegate/reconciler/statefulset/statefulset.go Outdated Show resolved Hide resolved
src/kubeobjects/slice.go Outdated Show resolved Hide resolved
src/controllers/activegate/reconciler/statefulset/statefulset_test.go Outdated
@@ -475,3 +569,19 @@ func buildTestInstance() *dynatracev1beta1.DynaKube {
},
}
}

func buildActiveGateVolumeDirectories(readOnly bool) []string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this logic also exist for the implementation? It seems there we always add every volume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the logic no longer exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually logic is needed. Volumes created if needed (depend on StatsD and RO fs)

@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch 2 times, most recently from e7d7a9f to 956d763 Compare March 7, 2022 16:17
0sewa0
0sewa0 requested changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@0sewa0 0sewa0 left a comment

Choose a reason for hiding this comment

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

You need to add an SCC so these changes work on Openshift (openshift will complain if you have SecurityContext for a pod but no SCC for it)

Example: https://github.com/Dynatrace/dynatrace-operator/blob/master/config/helm/chart/default/templates/Openshift/operator/securitycontextconstraints.yaml

@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch from 956d763 to a74105a Compare March 10, 2022 10:41
@aorcholski aorcholski requested a review from 0sewa0 March 11, 2022 13:50
@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch from a74105a to ca6fc54 Compare March 11, 2022 13:53
0sewa0
0sewa0 previously approved these changes Mar 14, 2022
View reviewed changes
chrismuellner
chrismuellner previously approved these changes Mar 14, 2022
View reviewed changes
@aorcholski aorcholski dismissed stale reviews from chrismuellner and 0sewa0 via f6de289 March 14, 2022 12:21
@aorcholski aorcholski force-pushed the feature/secure-activegate-pod branch from ca6fc54 to f6de289 Compare March 14, 2022 12:21
@aorcholski aorcholski merged commit 51ca9f6 into master Mar 14, 2022
@aorcholski aorcholski deleted the feature/secure-activegate-pod branch March 14, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrismuellner chrismuellner chrismuellner approved these changes

@0sewa0 0sewa0 0sewa0 left review comments

@toszr toszr Awaiting requested review from toszr

@mjgrzybek mjgrzybek Awaiting requested review from mjgrzybek

Assignees
No one assigned
Labels
activegate Changes related to Activegate security Vulnerabilities or changes to Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aorcholski @0sewa0 @chrismuellner @mjgrzybek