Set IMDSv2 on by default for nodes and apiservers #11329

olemarkus · 2021-04-27T08:56:59Z

This should mitigate security concerns around things like e.g enabling lifecycle hooks by default

rifelpet · 2021-04-27T12:39:46Z

I recall we initially defaulted this to enabled but reverted it in #10655. Can we address the issues that lead to the revert?

olemarkus · 2021-04-27T12:46:34Z

The revert was because it caused any pod that piggybacked on the instance's IAM role to fail. This should not happen on API server or worker nodes though. I hope it is rare that people add extra privileges to normal nodes and rather rely on KIAM (who should have their servers running on the control plane).

It also caused breakage on EBS CSI nodes because they rely on talking to the metadata service. This breakage was technically caused by me by removing hostnetworking: true from the DS manifest. We have added this back in now. Turns out it was exactly because of this they ran with hostNetworking.

I.e we should have addressed the issues now.

rifelpet · 2021-04-27T23:15:04Z

Typically changes like this we only enable for new clusters (and sometimes only for new clusters with k8s minor versions >= the kops minor version)

Any reason not to follow that same pattern here?

olemarkus · 2021-04-28T04:55:41Z

I want to play the security card here. But I am fine if we constrain this to a given k8s version too. But I think we really need this on by default on normal nodes. @johngmyers what do you think?

hakman · 2021-05-10T17:12:23Z

After thinking about this for some time, I think it it would be better to set this only when warm pool is involved.
For example, Flatcar will break completely when IMDSv2 is set and probably there are some other corner cases.
What do you think @olemarkus?

olemarkus · 2021-05-10T17:43:01Z

What makes flatcar break? Does it handle routing differently than other distros?

johngmyers · 2021-05-10T17:48:51Z

We should be having secure defaults, at least for new clusters.

hakman · 2021-05-10T18:13:08Z

Flatcar breaks because of the outdated AWS SDK: flatcar/ignition#19.
I agree that we should have secure defaults, but seems that this will require too many exceptions. Maybe it's not yet time for it to be a default.

olemarkus · 2021-05-10T18:45:38Z

Honestly, to me it looks like in this case we should use IMDSv2 by default and flatcar users should explicitly disable it. That this hasn't been fixed in flatcar sounds odd and I would prefer this not holding every other distro back from secure by default.

We can add an additinal callout here: https://kops.sigs.k8s.io/operations/images/#flatcar

hakman · 2021-05-10T19:30:25Z

Same story with Debian 9. It will most likely break many obscure workflows or setups where people were lazy to update.
Security minded people have the option to enable IMDSv2 quite easily. They can also debug any issues that might appear, much more than the others that won't know what hit them.

rifelpet · 2021-05-10T20:19:05Z

I agree that we should create secure clusters by default.

Does debian 10 work? I'm fine with documenting that using certain distros (particularly older ones) on new clusters will need to have an instancegroup field overridden. We would need to do something similar for userdata generation for bottlerocket and windows anyways, so it seems like a reasonable pattern to follow.

johngmyers · 2021-05-11T01:58:14Z

Corner cases don't detract from changing the default for new clusters to something more secure. They do detract from changing the default for existing clusters.

If there is an upcoming change, such as ServiceAccountIAM, which would significantly reduce the number of affected corner cases, that can argue to deferring the change of defaults.

hakman · 2021-05-11T03:52:06Z

docs/releases/1.21-NOTES.md

+    httpTokens: optional
+```
+
+This change does not affect control plane nodes.


Maybe best moved in main message.

hakman · 2021-05-11T04:01:15Z

Debian 10 does work. I can't say that I really agree that these are corner cases, but I agree that the vote is to go forward like this.
A problem that I see is that there is no easy way to keep automated tests working for Flatcar, as there is no flag to override this.
Also, as Peter was saying earlier, it would apply for older k8s versions, that we still support and for which the default image is Debian 9.

johngmyers · 2021-05-11T04:45:21Z

I could accept exempting new clusters that are on old k8s versions.

Do we need to resurrect #8531?

rifelpet · 2021-05-13T13:21:49Z

In office hours we decided that we can enable this for new k8s 1.22 clusters. This address the Debian 9 issue because it is already EOL, leaving flatcar compatibility unknown.

all node roles, token requred, hop limit 1

olemarkus · 2021-05-25T18:09:47Z

Implemented accordingly. Do we have any tests that uses k8s 1.22 yet?

rifelpet · 2021-05-25T18:30:35Z

We have a k8s-latest job but apparently its history is too large for testgrid and we need to reduce the retention on it. We also have an arm64 job that you could observe

olemarkus · 2021-05-25T18:41:56Z

Both of those cilium tests use latest. Should be good enough to see if this breaks anything.

johngmyers · 2021-05-27T06:03:23Z

docs/releases/1.22-NOTES.md

+```
+spec:
+  instanceMetadata:
+    httpPutResponseHopLimit: 1


Is this line necessary?

No. Removed.

hakman · 2021-05-27T06:12:14Z

docs/releases/1.22-NOTES.md

+    httpTokens: optional
+```
+
+This change does not affect control plane nodes.


I think this change will affect control plane nodes also.

Changed the wording to it is more clear.

The problem is not the wording but the intention.
As @rifelpet was saying above, conclusion from office hours was:

all node roles, token requred, hop limit 1

We cannot implement this on master roles though. Not until IRSA is used for all components and enabled by default.

upup/pkg/fi/cloudup/new_cluster.go

hakman · 2021-06-03T12:19:27Z

upup/pkg/fi/cloudup/new_cluster.go

+
+			if cluster.IsKubernetesGTE("1.22") {
+				bastionGroup.Spec.InstanceMetadata = &api.InstanceMetadataOptions{
+					HTTPPutResponseHopLimit: fi.Int64(3),


Any special reason for using 3 as hop limit for Bastions?

No. Seems like a mistake.

Bastion, nodes, and api servers get limit of 1 API servers tend to run pods requiring metadata access. The hop limit depends on CNI, but all should work with a limit of 3.

k8s-ci-robot · 2021-06-05T08:13:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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~~ [hakman]

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

rifelpet · 2021-06-05T14:19:15Z

docs/releases/1.22-NOTES.md

+ On AWS, kOps will enable [Instance Metadata Service Version 2](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html) by default with a max-hop-limit of 1 on new clusters that use Kubernetes 1.22. This means that any container running in the cluster will be unable to connect to the instance metadata _unless_ the container is running with `hostNetworking: true`. This will increase security by default, but may break some types of workloads. In order to revert to old behavior, add the following to the InstanceGroup:
+
+```
+spec:
+  instanceMetadata:
+    httpTokens: optional
+```
+
+This change only affects dedicated API server nodes and worker nodes. It does not affect control plane nodes.


I think this release note needs updating to match the final behavior in this PR. It does affect control plane nodes but with a hop limit of 3 (along with apiserver nodes)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/documentation labels Apr 27, 2021

k8s-ci-robot requested review from hakman and johngmyers April 27, 2021 08:57

johngmyers added the kind/office-hours label Apr 29, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2021

johngmyers added this to the v1.22 milestone May 7, 2021

hakman reviewed May 11, 2021

View reviewed changes

rifelpet removed the kind/office-hours label May 13, 2021

olemarkus force-pushed the ig-max-hop-limit branch from 25304db to 2a9b5a9 Compare May 25, 2021 18:08

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2021

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

johngmyers reviewed May 27, 2021

View reviewed changes

hakman reviewed May 27, 2021

View reviewed changes

olemarkus force-pushed the ig-max-hop-limit branch from 2a9b5a9 to 839e8c3 Compare May 27, 2021 06:55

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2021

olemarkus force-pushed the ig-max-hop-limit branch from 839e8c3 to fb12c6a Compare May 27, 2021 06:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2021

hakman reviewed May 27, 2021

View reviewed changes

upup/pkg/fi/cloudup/new_cluster.go Outdated Show resolved Hide resolved

olemarkus force-pushed the ig-max-hop-limit branch from fb12c6a to 7eea8f8 Compare May 30, 2021 17:54

olemarkus requested review from johngmyers and hakman May 31, 2021 07:35

hakman reviewed Jun 3, 2021

View reviewed changes

Set IMDSv2 on by default for nodes

b3a60d3

Bastion, nodes, and api servers get limit of 1 API servers tend to run pods requiring metadata access. The hop limit depends on CNI, but all should work with a limit of 3.

olemarkus force-pushed the ig-max-hop-limit branch from 7eea8f8 to b3a60d3 Compare June 5, 2021 06:18

olemarkus requested a review from hakman June 5, 2021 06:18

hakman approved these changes Jun 5, 2021

View reviewed changes

k8s-ci-robot assigned hakman Jun 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2021

k8s-ci-robot merged commit 83cd195 into kubernetes:master Jun 5, 2021

rifelpet reviewed Jun 5, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set IMDSv2 on by default for nodes and apiservers #11329

Set IMDSv2 on by default for nodes and apiservers #11329

olemarkus commented Apr 27, 2021

rifelpet commented Apr 27, 2021

olemarkus commented Apr 27, 2021

rifelpet commented Apr 27, 2021

olemarkus commented Apr 28, 2021

hakman commented May 10, 2021

olemarkus commented May 10, 2021

johngmyers commented May 10, 2021

hakman commented May 10, 2021

olemarkus commented May 10, 2021

hakman commented May 10, 2021

rifelpet commented May 10, 2021

johngmyers commented May 11, 2021

hakman May 11, 2021

hakman commented May 11, 2021

johngmyers commented May 11, 2021 •

edited

rifelpet commented May 13, 2021 •

edited

olemarkus commented May 25, 2021

rifelpet commented May 25, 2021

olemarkus commented May 25, 2021

johngmyers May 27, 2021

olemarkus May 27, 2021

hakman May 27, 2021

olemarkus May 27, 2021

hakman May 27, 2021

olemarkus May 27, 2021

hakman Jun 3, 2021

olemarkus Jun 5, 2021

k8s-ci-robot commented Jun 5, 2021

rifelpet Jun 5, 2021

Set IMDSv2 on by default for nodes and apiservers #11329

Set IMDSv2 on by default for nodes and apiservers #11329

Conversation

olemarkus commented Apr 27, 2021

rifelpet commented Apr 27, 2021

olemarkus commented Apr 27, 2021

rifelpet commented Apr 27, 2021

olemarkus commented Apr 28, 2021

hakman commented May 10, 2021

olemarkus commented May 10, 2021

johngmyers commented May 10, 2021

hakman commented May 10, 2021

olemarkus commented May 10, 2021

hakman commented May 10, 2021

rifelpet commented May 10, 2021

johngmyers commented May 11, 2021

Choose a reason for hiding this comment

hakman commented May 11, 2021

johngmyers commented May 11, 2021 • edited

rifelpet commented May 13, 2021 • edited

olemarkus commented May 25, 2021

rifelpet commented May 25, 2021

olemarkus commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 5, 2021

Choose a reason for hiding this comment

johngmyers commented May 11, 2021 •

edited

rifelpet commented May 13, 2021 •

edited