Apply gosec checks to the codebase, except tests. #9489

markusthoemmes · 2020-09-21T11:44:52Z

Proposed Changes

I found the findings somewhat interesting and worth enabling the check if only to force us to think through decisions (like the TLS version for example) and document why we've chosen to disable security features.

Release Note

For security reasons, registries that are shipping image metadata on TLS version 1.0 or 1.1 are no longer supported.

/assign @julz @vagababov @mattmoor

/hold

Especially for the TLS Version bump on the image digest resolution. That needs some eyes for sure.

knative-prow-robot

@markusthoemmes: 0 warnings.

In response to this:

Proposed Changes

I found the findings somewhat interesting and worth enabling the check if only to force us to think through decisions (like the TLS version for example) and document why we've chosen to disable security features.

Release Note
NONE
/assign @julz @vagababov @mattmoor

/hold

Especially for the TLS Version bump on the image digest resolution. That needs some eyes for sure.

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

knative-prow-robot · 2020-09-21T11:45:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markusthoemmes]

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

knative-test-reporter-robot · 2020-09-21T11:54:11Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-unit-tests		0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

pkg/apis/serving.TestPodSpecValidation/with_volume_(ok)
pkg/apis/serving.TestPodSpecValidation/with_volume_name_collision
pkg/apis/serving.TestPodSpecValidation/with_volume_mount_path_collision
pkg/apis/serving.TestPodSpecValidation
pkg/apis/serving.TestPodSpecMultiContainerValidation/Volume_mounts_ok_with_single_container
pkg/apis/serving.TestPodSpecMultiContainerValidation/Volume_mounts_ok_when_having_multiple_containers
pkg/apis/serving.TestPodSpecMultiContainerValidation/Volume_not_mounted_when_having_multiple_containers
pkg/apis/serving.TestPodSpecMultiContainerValidation

and 19 more.

knative-metrics-robot · 2020-09-21T12:13:00Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/k8s_validation.go	99.3%	99.3%	0.0

codecov · 2020-09-21T12:13:05Z

Codecov Report

Merging #9489 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9489      +/-   ##
==========================================
- Coverage   88.61%   88.60%   -0.02%     
==========================================
  Files         209      209              
  Lines        9374     9378       +4     
==========================================
+ Hits         8307     8309       +2     
  Misses        814      814              
- Partials      253      255       +2

Impacted Files	Coverage Δ
pkg/activator/net/lb_policy.go	`91.89% <ø> (ø)`
pkg/queue/health/probe.go	`93.93% <ø> (ø)`
pkg/apis/serving/k8s_validation.go	`97.93% <100.00%> (+0.01%)`	⬆️
pkg/reconciler/revision/resolve.go	`94.87% <100.00%> (+0.13%)`	⬆️
pkg/reconciler/revision/resources/deploy.go	`100.00% <100.00%> (ø)`
pkg/reconciler/route/traffic/traffic.go	`93.33% <100.00%> (ø)`
pkg/reconciler/revision/reconcile_resources.go	`80.72% <0.00%> (-2.41%)`	⬇️
pkg/reconciler/autoscaling/kpa/scaler.go	`88.40% <0.00%> (-1.45%)`	⬇️
pkg/activator/net/revision_backends.go	`91.40% <0.00%> (+0.90%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7398ed7...9256e45. Read the comment docs.

mattmoor · 2020-09-21T14:23:22Z

pkg/apis/serving/k8s_validation.go

@@ -250,7 +250,8 @@ func validateEnv(ctx context.Context, envVars []corev1.EnvVar) *apis.FieldError

 func validateEnvFrom(envFromList []corev1.EnvFromSource) *apis.FieldError {
 	var errs *apis.FieldError
-	for i, envFrom := range envFromList {
+	for i := range envFromList {
+		envFrom := envFromList[i]


What's wrong with the iterator style?

https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable

I was trying to understand how we had so few, but I'm guessing this is limited to things where & is taken or it'd be everywhere. This is pleasantly small, and finally a check to keep us from making this common mistake 🤩

Note that this matters if the function stores the object somehow, but definitely no need to have a possible bug anyway :)

mattmoor

We should probably do this in PKG too.

markusthoemmes · 2020-09-21T14:34:53Z

We should probably do this in PKG too.

You bet I'm on it.

mattmoor

/lgtm

julz · 2020-09-21T14:45:21Z

can we put a release note on this re: the TLS version? I think IIUC that means we now require remote registries to have at least TLS 1.2 (which is probably fine, but worth making sure to call out in release notes).

julz

apart from that,
/lgtm

markusthoemmes · 2020-09-21T14:50:28Z

Done @julz

/hold cancel

knative-prow-robot assigned julz Sep 21, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2020

knative-prow-robot assigned mattmoor and vagababov Sep 21, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 21, 2020

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 21, 2020

knative-prow-robot reviewed Sep 21, 2020

View reviewed changes

knative-prow-robot requested review from nak3, tcnghia, vagababov and yanweiguo September 21, 2020 11:45

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking labels Sep 21, 2020

Apply gosec checks to the codebase, except tests.

9256e45

markusthoemmes force-pushed the introduce-gosec branch from 3745fc2 to 9256e45 Compare September 21, 2020 12:06

mattmoor reviewed Sep 21, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2020

julz reviewed Sep 21, 2020

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2020

knative-prow-robot merged commit 5b12982 into knative:master Sep 21, 2020

markusthoemmes mentioned this pull request Sep 21, 2020

Apply gosec checks to the codebase, except tests. knative/pkg#1725

Merged

julz mentioned this pull request Sep 21, 2020

Use http.Transport.Clone in preference to copy-pasting defaults #9488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Apply gosec checks to the codebase, except tests. #9489

Apply gosec checks to the codebase, except tests. #9489

Uh oh!

markusthoemmes commented Sep 21, 2020 •

edited

Loading

Uh oh!

knative-prow-robot left a comment

Uh oh!

knative-prow-robot commented Sep 21, 2020

Uh oh!

knative-test-reporter-robot commented Sep 21, 2020

Uh oh!

knative-metrics-robot commented Sep 21, 2020

Uh oh!

codecov bot commented Sep 21, 2020 •

edited

Loading

Uh oh!

mattmoor Sep 21, 2020

Uh oh!

markusthoemmes Sep 21, 2020

Uh oh!

mattmoor Sep 21, 2020

Uh oh!

vagababov Sep 21, 2020

Uh oh!

mattmoor left a comment

Uh oh!

markusthoemmes commented Sep 21, 2020

Uh oh!

mattmoor left a comment

Uh oh!

julz commented Sep 21, 2020

Uh oh!

julz left a comment

Uh oh!

markusthoemmes commented Sep 21, 2020

Uh oh!

Uh oh!

Apply gosec checks to the codebase, except tests. #9489

Apply gosec checks to the codebase, except tests. #9489

Uh oh!

Conversation

markusthoemmes commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow-robot commented Sep 21, 2020

Uh oh!

knative-test-reporter-robot commented Sep 21, 2020

Uh oh!

knative-metrics-robot commented Sep 21, 2020

Uh oh!

codecov bot commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattmoor Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

mattmoor Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

vagababov Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Sep 21, 2020

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

julz commented Sep 21, 2020

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Sep 21, 2020

Uh oh!

Uh oh!

markusthoemmes commented Sep 21, 2020 •

edited

Loading

codecov bot commented Sep 21, 2020 •

edited

Loading