Add validation to warn about insecure SecurityContext defaults - #13399

Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged

Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom
evankanderson:warning-warning-insecure-defaults-for-3-years

Conversation

@evankanderson

@evankandersonevankanderson commented Oct 16, 2022

Copy link
Copy Markdown
Member

Fixes#13398

It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which
is "following current Pod hardening best practices", but they do not, presumably for historical reasons.

Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"
(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and
default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.

Proposed Changes

  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.

Release Note

Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot added area/API API objects and controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2022
@knative-prow

knative-prowBot commented Oct 16, 2022

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@knative-prowknative-prowBot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2022
@evankandersonevankanderson mentioned this pull request Oct 16, 2022
Merged
@evankanderson

evankanderson commented Oct 16, 2022

Copy link
Copy Markdown
MemberAuthor

I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1

@evankanderson

evankanderson commented Oct 16, 2022

Copy link
Copy Markdown
MemberAuthor

@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82

 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor

mattmoor commented Oct 16, 2022

Copy link
Copy Markdown
Member

Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71

@mattmoor

mattmoor commented Oct 16, 2022

Copy link
Copy Markdown
Member

tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()

@evankanderson

evankanderson commented Oct 16, 2022

Copy link
Copy Markdown
MemberAuthor

Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.

@mattmoor

mattmoor commented Oct 16, 2022

Copy link
Copy Markdown
Member

I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
Merged
Merged
@codecov

codecovBot commented Dec 17, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️

Coverage data is based on head (749cf68) compared to base (35abde0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

evankanderson
evankanderson commented Dec 30, 2022
View reviewed changes

@evankandersonevankanderson left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso

I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.

@evankanderson

evankanderson commented Dec 30, 2022

Copy link
Copy Markdown
MemberAuthor

/retest

psschwei
psschwei reviewed Jan 19, 2023
View reviewed changes

@psschweipsschwei left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM

Comment threadpkg/apis/serving/k8s_validation.go Outdated
Comment on lines +886 to +890
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.

@psschweipsschweiJan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)

dprotaso
dprotaso reviewed Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go Outdated
for i := range ps.Containers {
errs = errs.Also(
warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
ViaFieldIndex("containers", 1).ViaField("securityContext"))

@dprotasodprotasoJan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change
ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.

Thanks!

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.

Comment threadpkg/apis/serving/k8s_validation.go Outdated
for i := range ps.InitContainers {
errs = errs.Also(
warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
ViaFieldIndex("initContainers", 1).ViaField("securityContext"))

@dprotasodprotasoJan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change
ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done

Comment threadpkg/apis/serving/k8s_validation_test.go Outdated
ps corev1.PodSpec
cfgOpts []configOption
want *apis.FieldError
errLevel apis.DiagnosticLevel

@dprotasodprotasoJan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel

looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38

@dprotasodprotasoJan 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case

got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.

I think I was trying to be consistent here, but I can roll all these back if you prefer.

dprotaso
dprotaso reviewed Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
errs = errs.Also(insecureDefault("capabilities"))
} else {
if sc.Capabilities.Drop == nil {
errs = errs.Also(insecureDefault("capabilities.drop"))

@dprotasodprotasoJan 19, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even

Knative made this mistake and it was only recently fixed - #13327

@evankandersonevankandersonJan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.

@evankanderson

evankanderson commented Jan 20, 2023

Copy link
Copy Markdown
MemberAuthor

(Still rolling back the validation_test changes, but wanted to get other bits pushed.)

@evankanderson

evankanderson commented Jan 20, 2023

Copy link
Copy Markdown
MemberAuthor

Okay, this should be ready for another review.

@dprotaso

dprotaso commented Jan 20, 2023

Copy link
Copy Markdown
Member

/lgtm

@knative-prowknative-prowBot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@knative-prow
knative-prowBot merged commit e05aa3a into knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments
+1 more reviewer
@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso

@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Add validation to warn about insecure SecurityContext defaults - #13399
Merged
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Jan 20, 2023
Merged
Add validation to warn about insecure SecurityContext defaults#13399
knative-prow[bot] merged 8 commits into
knative:mainfrom 
evankanderson:warning-warning-insecure-defaults-for-3-years
Conversation
@evankanderson
@evankandersonevankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Fixes#13398
It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which

is "following current Pod hardening best practices", but they do not, presumably for historical reasons.
Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"

(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and

default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.
Proposed Changes
  • Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
    
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
    
may not know the default values have higher privilege than necessary.
  • Mention in the warning that future Knative versions may default to safer values.
Release Note
Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.
These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

@knative-prowknative-prowBot
added

area/API
API objects and controllers
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Oct 16, 2022
@knative-prow
knative-prowBot
commented
Oct 16, 2022

Copy link


Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: evankanderson
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Details
Needs approval from an approver in each of these files:

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

Approvers can cancel approval by writing /approve cancel in a comment
@knative-prowknative-prowBot
added
the 
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Oct 16, 2022
@evankandersonevankanderson
mentioned this pull request
Oct 16, 2022
 Merged

@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82
 configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
&v1.Configuration{
TypeMeta: {},
ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
Spec: {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
Status: v1.ConfigurationStatus{
Status: v1.Status{
ObservedGeneration: 1234,
Conditions: v1.Conditions{
{
Type: "Ready",
- Status: "Unknown",
+ Status: "False",
Severity: "",
... // 1 ignored field
- Reason: "",
+ Reason: "RevisionFailed",
- Message: "",
+ Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
},
},
Annotations: nil,
},
ConfigurationStatusFields: v1.ConfigurationStatusFields{
LatestReadyRevisionName: "",
- LatestCreatedRevisionName: "no-revisions-yet-01234",
+ LatestCreatedRevisionName: "",
},
},
}
configuration_test.go:570: Unexpected event(-want, +got):
strings.Join({
- `Normal Created Created Revision "no-revisions-yet-01234"`,
+ "Warning CreationFailed Failed to create Revision: Kubernetes def",
+ "ault value is insecure, Knative may default this to secure in a ",
+ "future release: spec.securityContext.containers[1].allowPrivileg",
+ "eEscalation, spec.securityContext.containers[1].capabilities, sp",
+ "ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
+ "ext.containers[1].seccompProfile",
}, "")

@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()
@evankanderson
evankanderson
commented
Oct 16, 2022

Copy link


Copy Markdown

MemberAuthor
Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.
@mattmoor
mattmoor
commented
Oct 16, 2022

Copy link


Copy Markdown

Member
I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

This was referenced Oct 17, 2022
 Merged

 Merged

@codecov
codecovBot
commented
Dec 17, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26%⚠️
Coverage data is based on head (749cf68) compared to base (35abde0).

Patch coverage: 100.00% of modified lines in pull request are covered.
Additional details and impacted files
@@ Coverage Diff @@## main #13399 +/- ##
==========================================
- Coverage 86.43% 86.17% -0.26% 
==========================================
Files 196 197 +1 Lines 14600 14723 +123 ==========================================
+ Hits 12619 12688 +69 - Misses 1680 1733 +53 - Partials 301 302 +1 
Impacted FilesCoverage Δ
pkg/apis/serving/k8s_validation.go94.58% <100.00%> (+0.37%)⬆️
pkg/autoscaler/statforwarder/processor.go88.00% <0.00%> (-6.00%)⬇️
pkg/autoscaler/statforwarder/forwarder.go90.66% <0.00%> (-5.34%)⬇️
cmd/autoscaler/main.go9.47% <0.00%> (-0.42%)⬇️
pkg/apis/serving/v1/service_defaults.go100.00% <0.00%> (ø)
pkg/apis/serving/v1/configuration_defaults.go100.00% <0.00%> (ø)
pkg/reconciler/route/resources/certificate.go100.00% <0.00%> (ø)
.../reconciler/domainmapping/resources/certificate.go100.00% <0.00%> (ø)
cmd/autoscaler/leaderelection.go0.00% <0.00%> (ø)
pkg/reconciler/route/route.go80.30% <0.00%> (+0.24%)⬆️
... and 2 more
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.

📢 Do you have feedback about the report comment? Let us know in this issue.
evankanderson
evankanderson
commented
Dec 30, 2022
View reviewed changes
@evankandersonevankanderson
left a comment


Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

/assign @psschwei@dprotaso
I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.
@evankanderson
evankanderson
commented
Dec 30, 2022

Copy link


Copy Markdown

MemberAuthor
/retest
psschwei
psschwei
reviewed
Jan 19, 2023
View reviewed changes
@psschweipsschwei
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

other than a nit on the comment, LGTM
Comment threadpkg/apis/serving/k8s_validation.go
Outdated


Comment on lines

+886
 to 
+890

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@psschweipsschweiJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
// warnDefaultContainerSecurityContext warns about Kubernetes default
// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile
// forbids these values). Because securityContext values may also be set at
// the Pod level, the container-level settings need to be considered alongside
// the Pod-level settings.
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done (and rewrapped, just 'cuz)
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.Containers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
 ViaFieldIndex("containers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))
ViaFieldIndex("containers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.
Thanks!
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.
Comment threadpkg/apis/serving/k8s_validation.go
Outdated

	for i := range ps.InitContainers {
 errs = errs.Also(
 warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
 ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
ViaFieldIndex("initContainers", i).ViaField("securityContext"))
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done
Comment threadpkg/apis/serving/k8s_validation_test.go
Outdated

 ps corev1.PodSpec
 cfgOpts []configOption
 want *apis.FieldError
 errLevel apis.DiagnosticLevel
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

question: is the default value (0) == apis.ErrorLevel
looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38
@dprotasodprotasoJan 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case
got = got.Filter(apis.ErrorLevel)

@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes, the default value is error for compatibility with all the existing code.
I think I was trying to be consistent here, but I can roll all these back if you prefer.
dprotaso
dprotaso
reviewed
Jan 19, 2023
View reviewed changes
Comment threadpkg/apis/serving/k8s_validation.go
 errs = errs.Also(insecureDefault("capabilities"))
	} else {
 if sc.Capabilities.Drop == nil {
 errs = errs.Also(insecureDefault("capabilities.drop"))
@dprotasodprotasoJan 19, 2023•

edited

Loading

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

You might want to add a warning that all needs to be ALL - or error out on this even
Knative made this mistake and it was only recently fixed - #13327
@evankandersonevankandersonJan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Added a warning if arg 0 is "all", I think that should be sufficient for now.
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
(Still rolling back the validation_test changes, but wanted to get other bits pushed.)
@evankanderson
evankanderson
commented
Jan 20, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this should be ready for another review.
@dprotaso
dprotaso
commented
Jan 20, 2023

Copy link


Copy Markdown

Member
/lgtm
@knative-prowknative-prowBot
added
the 
lgtm
Indicates that a PR is ready to be merged.
label
Jan 20, 2023
@knative-prow
knative-prowBot
merged commit e05aa3a
into
knative:mainJan 20, 2023
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dprotasodprotasodprotaso left review comments

+1 more reviewer

@psschweipsschweipsschwei left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@dprotasodprotaso
@psschweipsschwei

Labels

approvedIndicates a PR has been approved by an approver from all required OWNERS files.area/APIAPI objects and controllerslgtmIndicates that a PR is ready to be merged.size/LDenotes a PR that changes 100-499 lines, ignoring generated files.

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@evankanderson@mattmoor@dprotaso@psschwei