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.

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

Add default SecurityContext to every new ksvc #1821

Merged
merged 4 commits into from
Jul 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions docs/cmd/kn_container_add.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ kn container add NAME
--pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
--pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
--request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
--user int The user ID to run the container (e.g., 1001).
```
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_service_apply.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ kn service apply s0 --filename my-svc.yml
--scale-target int Recommendation for what metric value the PodAutoscaler should attempt to maintain. Use with --scale-metric flag to configure the metric name for which the target value should be maintained. Default metric name is concurrency. The flag defaults to --concurrency-limit when given.
--scale-utilization int Percentage of concurrent requests utilization before scaling up. (default 70)
--scale-window string Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
--timeout int Duration in seconds that the request routing layer will wait for a request delivered to a container to begin replying (default 300)
--user int The user ID to run the container (e.g., 1001).
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_service_create.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ kn service create NAME --image IMAGE
--scale-target int Recommendation for what metric value the PodAutoscaler should attempt to maintain. Use with --scale-metric flag to configure the metric name for which the target value should be maintained. Default metric name is concurrency. The flag defaults to --concurrency-limit when given.
--scale-utilization int Percentage of concurrent requests utilization before scaling up. (default 70)
--scale-window string Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
--tag strings Set tag (format: --tag revisionRef=tagName) where revisionRef can be a revision or '@latest' string representing latest ready revision. This flag can be specified multiple times.
--target string Work on local directory instead of a remote cluster (experimental)
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_service_update.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ kn service update NAME
--scale-target int Recommendation for what metric value the PodAutoscaler should attempt to maintain. Use with --scale-metric flag to configure the metric name for which the target value should be maintained. Default metric name is concurrency. The flag defaults to --concurrency-limit when given.
--scale-utilization int Percentage of concurrent requests utilization before scaling up. (default 70)
--scale-window string Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
--tag strings Set tag (format: --tag revisionRef=tagName) where revisionRef can be a revision or '@latest' string representing latest ready revision. This flag can be specified multiple times.
--target string Work on local directory instead of a remote cluster (experimental)
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_source_container_create.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ kn source container create NAME --image IMAGE --sink SINK
--pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
--pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
--request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
-s, --sink string Addressable sink for events. You can specify a broker, channel, Knative service or URI. Examples: '--sink broker:nest' for a broker 'nest', '--sink channel:pipe' for a channel 'pipe', '--sink ksvc:mysvc:mynamespace' for a Knative service 'mysvc' in another namespace 'mynamespace', '--sink https://event.receiver.uri' for an URI with an 'http://' or 'https://' schema, '--sink ksvc:receiver' or simply '--sink receiver' for a Knative service 'receiver' in the current namespace. If a prefix is not provided, it is considered as a Knative service in the current namespace. If referring to a Knative service in another namespace, 'ksvc:name:namespace' combination must be provided explicitly.
--user int The user ID to run the container (e.g., 1001).
Expand Down
1 change: 1 addition & 0 deletions docs/cmd/kn_source_container_update.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ kn source container update NAME --image IMAGE
--pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
--pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
--request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
--security-context string Security Context definition to be added the service. Accepted values: strict | none. (default "strict")
--service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
-s, --sink string Addressable sink for events. You can specify a broker, channel, Knative service or URI. Examples: '--sink broker:nest' for a broker 'nest', '--sink channel:pipe' for a channel 'pipe', '--sink ksvc:mysvc:mynamespace' for a Knative service 'mysvc' in another namespace 'mynamespace', '--sink https://event.receiver.uri' for an URI with an 'http://' or 'https://' schema, '--sink ksvc:receiver' or simply '--sink receiver' for a Knative service 'receiver' in the current namespace. If a prefix is not provided, it is considered as a Knative service in the current namespace. If referring to a Knative service in another namespace, 'ksvc:name:namespace' combination must be provided explicitly.
--user int The user ID to run the container (e.g., 1001).
Expand Down
3 changes: 2 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ require (
sigs.k8s.io/yaml v1.3.0
)

require k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2

require (
contrib.go.opencensus.io/exporter/ocagent v0.7.1-0.20200907061046-05415f1de66d // indirect
contrib.go.opencensus.io/exporter/prometheus v0.4.2 // indirect
Expand Down Expand Up @@ -120,7 +122,6 @@ require (
k8s.io/gengo v0.0.0-20221011193443-fad74ee6edd9 // indirect
k8s.io/klog/v2 v2.80.2-0.20221028030830-9ae4992afb54 // indirect
k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect
k8s.io/utils v0.0.0-20221108210102-8e77b1f39fe2 // indirect
sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
sigs.k8s.io/kustomize/api v0.12.1 // indirect
sigs.k8s.io/kustomize/kyaml v0.13.9 // indirect
Expand Down
2 changes: 2 additions & 0 deletions lib/test/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ func BuildServiceWithOptions(name string, so ...servingtest.ServiceOption) *serv
APIVersion: "serving.knative.dev/v1",
}
svc.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{}
svc.Spec.Template.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{}
return svc
}

Expand Down Expand Up @@ -301,6 +302,7 @@ func BuildRevision(name string, options ...servingtest.RevisionOption) *servingv
rev.ObjectMeta.UID = ""
rev.ObjectMeta.Generation = int64(0)
rev.Spec.PodSpec.Containers[0].Resources = corev1.ResourceRequirements{}
rev.Spec.PodSpec.Containers[0].SecurityContext = &corev1.SecurityContext{}
return rev
}

Expand Down
3 changes: 3 additions & 0 deletions pkg/kn/commands/service/create_mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"testing"
"time"

"knative.dev/client/pkg/kn/flags"

"knative.dev/serving/pkg/apis/autoscaling"

"gotest.tools/v3/assert"
Expand Down Expand Up @@ -477,6 +479,7 @@ func getService(name string) *servingv1.Service {
Limits: corev1.ResourceList{},
Requests: corev1.ResourceList{},
},
SecurityContext: flags.DefaultStrictSecCon(),
}}

return service
Expand Down
3 changes: 3 additions & 0 deletions pkg/kn/commands/source/container/container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"bytes"
"strings"

"knative.dev/client/pkg/kn/flags"

corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/clientcmd"
v1 "knative.dev/eventing/pkg/apis/sources/v1"
Expand Down Expand Up @@ -92,6 +94,7 @@ func createContainerSource(name, image string, sink duckv1.Destination, ceo map[
Limits: corev1.ResourceList{},
Requests: corev1.ResourceList{},
},
SecurityContext: flags.DefaultStrictSecCon(),
}}}).
Sink(sink).
Build()
Expand Down
16 changes: 16 additions & 0 deletions pkg/kn/flags/podspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ type PodSpecFlags struct {
ServiceAccountName string
ImagePullSecrets string
User int64

SecurityContext string
}

type ResourceFlags struct {
Expand Down Expand Up @@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.")

flagNames = append(flagNames, "security-context")

return flagNames
}

Expand Down Expand Up @@ -405,5 +411,15 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, flags *pflag.Flag
}
}

if flags.Changed("security-context") {
if err := UpdateSecurityContext(podSpec, p.SecurityContext); err != nil {
return err
}
} else {
if err := UpdateSecurityContext(podSpec, ""); err != nil {
return err
}
}

return nil
}
38 changes: 38 additions & 0 deletions pkg/kn/flags/podspec_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"strconv"
"strings"

"k8s.io/utils/pointer"

"k8s.io/apimachinery/pkg/util/intstr"

"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -378,6 +380,42 @@ func UpdateImagePullPolicy(spec *corev1.PodSpec, imagePullPolicy string) error {
return nil
}

// UpdateSecurityContext update the Security Context
func UpdateSecurityContext(spec *corev1.PodSpec, securityContext string) error {
container := containerOfPodSpec(spec)
switch strings.ToLower(securityContext) {
case "none":
// Blank any Security Context defined
container.SecurityContext = &corev1.SecurityContext{}
case "strict":
// Add or update Security Context to default strict
container.SecurityContext = DefaultStrictSecCon()
case "":
// Add default strict SC flag is not used, hence empty value
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if container.SecurityContext == nil {
container.SecurityContext = DefaultStrictSecCon()
}
//TODO(dsimansk): add parsing of SC options from the flag value
default:
return fmt.Errorf("invalid --security-context %s. Valid arguments: strict | none", securityContext)
}
return nil
}

// DefaultStrictSecCon helper function to get default strict Security Context
func DefaultStrictSecCon() *corev1.SecurityContext {
return &corev1.SecurityContext{
AllowPrivilegeEscalation: pointer.Bool(false),
RunAsNonRoot: pointer.Bool(true),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
SeccompProfile: &corev1.SeccompProfile{
Type: corev1.SeccompProfileTypeRuntimeDefault,
},
}
}

func getPolicy(policy string) v1.PullPolicy {
var ret v1.PullPolicy
switch strings.ToLower(policy) {
Expand Down
55 changes: 55 additions & 0 deletions pkg/kn/flags/podspec_helper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1487,3 +1487,58 @@ func TestResolveProbeOptions(t *testing.T) {
})
}
}

func TestUpdateSecurityContext(t *testing.T) {
testCases := []struct {
name string

expected *corev1.PodSpec
expectedError error
}{
{
name: "strict",
expected: &corev1.PodSpec{
Containers: []corev1.Container{
{SecurityContext: DefaultStrictSecCon()}},
},
expectedError: nil,
},
{
name: "none",
expected: &corev1.PodSpec{
Containers: []corev1.Container{{
SecurityContext: &corev1.SecurityContext{}}},
},
expectedError: nil,
},
{
name: "",
expected: &corev1.PodSpec{
Containers: []corev1.Container{
{SecurityContext: DefaultStrictSecCon()}},
},
expectedError: nil,
},
{
name: "unknown",
expected: &corev1.PodSpec{
Containers: []corev1.Container{
{SecurityContext: DefaultStrictSecCon()}},
},
expectedError: errors.New("invalid --security-context unknown. Valid arguments: strict | none"),
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual := &corev1.PodSpec{}
err := UpdateSecurityContext(actual, tc.name)
if tc.expectedError != nil {
assert.Error(t, err, tc.expectedError.Error())
} else {
assert.NilError(t, err)
assert.DeepEqual(t, actual, tc.expected)
}
})
}
}
30 changes: 16 additions & 14 deletions pkg/kn/flags/podspec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,22 @@ import (
"github.com/spf13/cobra"
"gotest.tools/v3/assert"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
"knative.dev/client/pkg/util"
"knative.dev/pkg/ptr"
)

func TestPodSpecFlags(t *testing.T) {
args := []string{"--image", "repo/user/imageID:tag", "--env", "b=c"}
wantedPod := &PodSpecFlags{
Image: "repo/user/imageID:tag",
Env: []string{"b=c"},
EnvFrom: []string{},
EnvValueFrom: []string{},
Mount: []string{},
Volume: []string{},
Arg: []string{},
Command: []string{},
Image: "repo/user/imageID:tag",
Env: []string{"b=c"},
EnvFrom: []string{},
EnvValueFrom: []string{},
Mount: []string{},
Volume: []string{},
Arg: []string{},
Command: []string{},
SecurityContext: "strict",
}
flags := &PodSpecFlags{}
testCmd := &cobra.Command{
Expand Down Expand Up @@ -86,12 +86,12 @@ func TestPodSpecResolve(t *testing.T) {
},
},
ReadinessProbe: &corev1.Probe{
ProbeHandler: v1.ProbeHandler{
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{Port: intstr.Parse("8080"), Path: "/path"},
},
},
LivenessProbe: &corev1.Probe{
ProbeHandler: v1.ProbeHandler{
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{Port: intstr.Parse("8080"), Path: "/path"},
},
},
Expand Down Expand Up @@ -223,6 +223,7 @@ containers:
Limits: corev1.ResourceList{},
Requests: corev1.ResourceList{},
},
SecurityContext: DefaultStrictSecCon(),
},
{
Name: "foo",
Expand Down Expand Up @@ -393,10 +394,11 @@ func TestPodSpecResolveWithEnvFile(t *testing.T) {
},
},
Env: []corev1.EnvVar{{Name: "svcOwner", Value: "James"}, {Name: "svcAuthor", Value: "James"}},
Resources: v1.ResourceRequirements{
Limits: v1.ResourceList{},
Requests: v1.ResourceList{},
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{},
Requests: corev1.ResourceList{},
},
SecurityContext: DefaultStrictSecCon(),
},
},
}
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/service_export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestServiceExport(t *testing.T) {
defer r.DumpIfFailed()

t.Log("create service with byo revision")
serviceCreateWithOptions(r, "hello", "--revision-name", "rev1")
serviceCreateWithOptions(r, "hello", "--revision-name", "rev1", "--security-context=none")

userImage := pkgtest.ImagePath("helloworld")
if strings.Contains(userImage, "@") {
Expand Down Expand Up @@ -233,7 +233,7 @@ func TestServiceExport(t *testing.T) {
), "--with-revisions", "--mode", "export", "-o", "yaml")

t.Log("create and export service 'foo' and verify that serviceUID and configurationUID labels are absent")
serviceCreateWithOptions(r, "foo")
serviceCreateWithOptions(r, "foo", "--security-context=none")
output := serviceExportOutput(r, "foo", "-o", "json")
actSvc := servingv1.Service{}
err = json.Unmarshal([]byte(output), &actSvc)
Expand Down