Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

[v0.16.0] Creating a switch to disable ICMP Ping instead of the default allow 0.0.0/0 #1854

Merged

Conversation

HarryStericker
Copy link
Contributor

Currently kube-aws includes a rule in its security group to allow ping from all traffic (0.0.0.0/0). Unless this is necessary for a reason I have not foreseen, it is an inherent security risk, violates CIS compliance, and is probably superfluous if SG's are managed appropriately.

Therefore, I have created a switch in cluster.yaml to allow removal of this rule. I have left it enabled by default but am conscious that anyone upgrading needs to ensure this is in their cluster.yaml, so am happy to adjust the logic so that there's no impact unless you explicitly ask for it to be disabled.

I don't think I've missed any other spots where this rule is used.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign redbaron
You can assign the PR to them by writing /assign @redbaron in a comment when ready.

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

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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 18, 2020
@HarryStericker
Copy link
Contributor Author

Hey @dominicgunn, can i get a few pointers when you have time. Not sure what needs modifying to fix the build errors.

Copy link
Contributor

@dominicgunn dominicgunn left a comment

Choose a reason for hiding this comment

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

Left a comment for you @HarryStericker

Comment on lines 223 to 228
{
"CidrIp": "0.0.0.0/0",
"FromPort": -1,
"IpProtocol": "icmp",
"ToPort": -1
}
Copy link
Contributor

@dominicgunn dominicgunn May 26, 2020

Choose a reason for hiding this comment

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

I think this might be your issue. Now that this is optional ({{ if .... }}) the block above is going to leave a trailing , if this is disabled, like this.

....
    "ToPort": 22
  },
]

We can't have that trailing , if openICMP is disabled.

This will also need changes to the go code, so we can add OpenICMP as a struct. Do a search for cloudFormationStreaming.

@dominicgunn dominicgunn changed the title Creating a switch to disable ICMP Ping instead of the default allow 0.0.0/0 [v0.16.0] Creating a switch to disable ICMP Ping instead of the default allow 0.0.0/0 May 26, 2020
@dominicgunn dominicgunn added this to the v0.16.0 milestone May 26, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 27, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #1854 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1854   +/-   ##
=======================================
  Coverage   24.40%   24.40%           
=======================================
  Files          98       98           
  Lines        5117     5117           
=======================================
  Hits         1249     1249           
  Misses       3728     3728           
  Partials      140      140           
Impacted Files Coverage Δ
pkg/api/cluster.go 0.00% <ø> (ø)

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 bd0633d...4aad063. Read the comment docs.

@dominicgunn
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@dominicgunn dominicgunn merged commit 21a0c48 into kubernetes-retired:master May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants