Add default SecurityContext to every new ksvc #1821

dsimansk · 2023-06-26T11:30:48Z

Description

This's handling a warning from API-server validation about SecurityContext being empty for kn service create and related cmds.

I'll look into a few further improvements. E.g. either adding SecurityContext builder pattern or at least plain --security-context seccon.yaml flag to make it customizable.

/cc @rhuss @vyasgun

Changes

Add default SecurityContext to every new ksvc

Reference

Fixes #

Release Note

Add default SecurityContext to every new ksvc

knative-prow

@dsimansk: 2 warnings.

In response to this:

Description

This's handling a warning from API-server validation about SecurityContext being empty for kn service create and related cmds.

I'll look into a few further improvements. E.g. either adding SecurityContext builder pattern or at least plain --security-context seccon.yaml flag to make it customizable.

/cc @rhuss @vyasgun

Changes

Add default SecurityContext to every new ksvc

Reference

Fixes #

Release Note
Add default SecurityContext to every new ksvc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow · 2023-06-26T11:31:00Z

pkg/kn/flags/podspec_helper.go

@@ -378,6 +380,26 @@ func UpdateImagePullPolicy(spec *corev1.PodSpec, imagePullPolicy string) error {
 	return nil
 }

+func UpdateDefaultSecurityContext(spec *corev1.PodSpec) {


Golint comments: exported function UpdateDefaultSecurityContext should have comment or be unexported. More info.

knative-prow · 2023-06-26T11:31:00Z

pkg/kn/flags/podspec_helper.go

+	}
+}
+
+func DefaultSecCon() *corev1.SecurityContext {


Golint comments: exported function DefaultSecCon should have comment or be unexported. More info.

codecov · 2023-06-26T11:38:39Z

Codecov Report

Patch coverage: 82.92% and project coverage change: +0.03 🎉

Comparison is base (00e38dc) 79.70% compared to head (009b2e7) 79.74%.

❗ Current head 009b2e7 differs from pull request most recent head c96dc6c. Consider uploading reports for the commit c96dc6c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1821      +/-   ##
==========================================
+ Coverage   79.70%   79.74%   +0.03%     
==========================================
  Files         179      179              
  Lines       13850    13891      +41     
==========================================
+ Hits        11039    11077      +38     
- Misses       2050     2052       +2     
- Partials      761      762       +1

Impacted Files	Coverage Δ
pkg/kn/flags/podspec.go	`74.33% <46.15%> (-1.28%)`	⬇️
pkg/kn/flags/podspec_helper.go	`85.53% <100.00%> (+1.03%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rhuss · 2023-06-26T13:06:22Z

pkg/kn/flags/podspec_helper.go

+func DefaultSecCon() *corev1.SecurityContext {
+	return &corev1.SecurityContext{
+		AllowPrivilegeEscalation: pointer.Bool(false),
+		RunAsNonRoot:             pointer.Bool(true),


My question would be if this does not break the existing setup and whether this is not too restrictive. I mean, there are still tons of container images that can only run as root and/or need certain capabilities. Having such a strict default (that currently can't be changed), is quite drastic. I think we should only add this if there is at least an option to get back the previous behaviour, i.e. having no SecurityContext at all. This then could be documented as a change (which is still not backwards compatible). This option could be called --no-security-context or maybe even better something like --security-context=none (with the alternatives like --security-context=strict (the default) and --security-context=privilegeEscalation:allow,runAsNonRoot:true,dropCapabilities:NET for fine grained security context configuration (this we can add later but strict and none should be already along with this PR.

Yep, I was actually thinking about --no-security-context to be added immediately. I like strict & none combination, with the further enhancement to towards fine grained options.

That's at least ready with on/off switch. 0fcf800

rhuss · 2023-06-26T13:07:26Z

Please see my comment https://github.com/knative/client/pull/1821/files#r1242163943 that we should already add basic option handling here. Otherwise, I think it would break too much.

Also note the linter warnings.

dsimansk · 2023-06-28T10:35:32Z

/retest

rhuss

Looks good to me, with some minor nits.

rhuss · 2023-07-13T07:23:47Z

pkg/kn/flags/podspec.go

@@ -234,6 +236,10 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string {
 	flagNames = append(flagNames, "pull-secret")
 	flagset.Int64VarP(&p.User, "user", "", 0, "The user ID to run the container (e.g., 1001).")
 	flagNames = append(flagNames, "user")
+
+	flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Security Context definition to be added the service. Accepted values: strict | none.")


Suggested change

flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Security Context definition to be added the service. Accepted values: strict | none.")

flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Predefined security context for the service. Accepted values: 'none' for no security context and 'strict' for dropping all capabilities, running as non-root, and no privilege escalation.")

rhuss · 2023-07-13T07:28:07Z

pkg/kn/flags/podspec_helper.go

+		// Add or update Security Context to default strict
+		container.SecurityContext = DefaultStrictSecCon()
+	case "":
+		// Add default strict SC flag is not used, hence empty value


Can this be the case if a default value is set to "strict" ?

knative-prow · 2023-07-13T07:28:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add default SecurityContext to every new ksvc

686b083

knative-prow bot requested a review from itsmurugappan June 26, 2023 11:30

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2023

knative-prow bot requested review from rhuss and vyasgun June 26, 2023 11:30

knative-prow bot reviewed Jun 26, 2023

View reviewed changes

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2023

rhuss reviewed Jun 26, 2023

View reviewed changes

Add basic seccon config flag

009b2e7

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2023

dsimansk added 2 commits June 27, 2023 17:38

Fix export e2e tests

0fcf800

Fix unit test

c96dc6c

rhuss approved these changes Jul 13, 2023

View reviewed changes

knative-prow bot assigned rhuss Jul 13, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2023

knative-prow bot merged commit 75d62a3 into knative:main Jul 13, 2023
23 checks passed

dsimansk linked an issue Jul 26, 2023 that may be closed by this pull request

Feature suggestions for offline mode (kn service --target) #1195

Open

dsimansk added the kind/feature New feature or request label Jul 26, 2023

dsimansk added this to the Knative 1.11 milestone Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default SecurityContext to every new ksvc #1821

Add default SecurityContext to every new ksvc #1821

dsimansk commented Jun 26, 2023

knative-prow bot left a comment

knative-prow bot Jun 26, 2023

knative-prow bot Jun 26, 2023

codecov bot commented Jun 26, 2023 •

edited

Loading

rhuss Jun 26, 2023

dsimansk Jun 26, 2023

dsimansk Jun 27, 2023

rhuss commented Jun 26, 2023

dsimansk commented Jun 28, 2023

rhuss left a comment

rhuss Jul 13, 2023

rhuss Jul 13, 2023

knative-prow bot commented Jul 13, 2023

	flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Security Context definition to be added the service. Accepted values: strict \| none.")
	flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Predefined security context for the service. Accepted values: 'none' for no security context and 'strict' for dropping all capabilities, running as non-root, and no privilege escalation.")

Add default SecurityContext to every new ksvc #1821

Add default SecurityContext to every new ksvc #1821

Conversation

dsimansk commented Jun 26, 2023

Description

Changes

Reference

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

knative-prow bot Jun 26, 2023

Choose a reason for hiding this comment

knative-prow bot Jun 26, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 26, 2023 • edited Loading

Codecov Report

rhuss Jun 26, 2023

Choose a reason for hiding this comment

dsimansk Jun 26, 2023

Choose a reason for hiding this comment

dsimansk Jun 27, 2023

Choose a reason for hiding this comment

rhuss commented Jun 26, 2023

dsimansk commented Jun 28, 2023

rhuss left a comment

Choose a reason for hiding this comment

rhuss Jul 13, 2023

Choose a reason for hiding this comment

rhuss Jul 13, 2023

Choose a reason for hiding this comment

knative-prow bot commented Jul 13, 2023

codecov bot commented Jun 26, 2023 •

edited

Loading