Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perform challenge callbacks into a node #15125

Merged
merged 4 commits into from
May 7, 2023

Conversation

justinsb
Copy link
Member

In order to verify that the caller is running on the specified node, we source the expected IP address from the cloud, and require that the node set up a simple challenge/response server to answer requests.

Because the challenge server runs on a port outside of the nodePort range, this also makes it harder for pods to impersonate their host nodes - though we do combine this with TPM and similar functionality where it is available.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 11, 2023
@k8s-ci-robot k8s-ci-robot added area/kops-controller area/nodeup area/provider/aws Issues or PRs related to aws provider area/provider/gcp Issues or PRs related to gcp provider area/provider/hetzner Issues or PRs related to Hetzner provider area/provider/openstack Issues or PRs related to openstack provider labels Feb 11, 2023
@justinsb
Copy link
Member Author

/hold for discussion, I don't think we should merge this lightly :-)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2023
@zetaab
Copy link
Member

zetaab commented Feb 11, 2023

one notable thing: people can run pods in hostNetwork and then its possible to impersonate. But as I see this: why we should accept calls anymore from instances that are already members of kubernetes cluster? If we do not accept that, then its pretty easy to protect against normal pods.

One thing that is coming to my mind: if node lifetime is enough long it might be that kubelet certificates are going to expire and re-register is needed (kops-controller should accept in that case?)? That could be done like using kubectl delete node for node that is in NotReady state and after that it can request new certs, but its not pretty solution

@justinsb justinsb force-pushed the node_challenge branch 2 times, most recently from d98b755 to f24fde4 Compare April 26, 2023 10:25
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2023
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 4, 2023
@justinsb
Copy link
Member Author

justinsb commented May 4, 2023

one notable thing: people can run pods in hostNetwork and then its possible to impersonate. But as I see this: why we should accept calls anymore from instances that are already members of kubernetes cluster? If we do not accept that, then its pretty easy to protect against normal pods.

So I think we need to do this PR and prevent the node from re-registering. We need this so that we can have higher-confidence when the node joins (particularly on clouds where we don't get strong node-attestation, for example DigitalOcean). And we want to prevent the node from re-registering to avoid attacks where a pod tries to impersonate a node. We do want to allow some re-registration (as you've pointed out, where the node reboots, or where the cert expires) ... I am thinking we want a machine-key or similar, but I think we've agreed that we can iterate on that!

@justinsb justinsb force-pushed the node_challenge branch 4 times, most recently from e87a61a to ee53335 Compare May 4, 2023 14:12
@justinsb
Copy link
Member Author

justinsb commented May 4, 2023

/test pull-kops-e2e-cni-cilium-ipv6

@justinsb
Copy link
Member Author

justinsb commented May 5, 2023

/retest

Going to look into each of the failures, but they all appear to be unrelated

@justinsb
Copy link
Member Author

justinsb commented May 5, 2023

Test failures matched kubernetes/kubernetes#117363 , i.e. a data race in k8s

In order to verify that the caller is running on the specified node,
we source the expected IP address from the cloud, and require that the
node set up a simple challenge/response server to answer requests.

Because the challenge server runs on a port outside of the nodePort
range, this also makes it harder for pods to impersonate their host
nodes - though we do combine this with TPM and similar functionality
where it is available.
DigitalOcean (and others) will follow shortly.

Also create a method for CloudProvider, so that we are more ambivalent
towards bootstrapping methods.
@hakman
Copy link
Member

hakman commented May 6, 2023

/retest

@justinsb
Copy link
Member Author

justinsb commented May 7, 2023

I made the changes here to only run this on hetzner (and I'll rebase the digitalocean branch after this merges).

@hakman
Copy link
Member

hakman commented May 7, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 7, 2023
@k8s-ci-robot
Copy link
Contributor

[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:

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2023
@justinsb
Copy link
Member Author

justinsb commented May 7, 2023

Thanks for reviewing @hakman

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2023
@justinsb
Copy link
Member Author

justinsb commented May 7, 2023

/test pull-kops-e2e-cni-cilium

(It was the openapi data race again)

@k8s-ci-robot k8s-ci-robot merged commit 68dcc7a into kubernetes:master May 7, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api area/kops-controller area/nodeup area/provider/aws Issues or PRs related to aws provider area/provider/gcp Issues or PRs related to gcp provider area/provider/hetzner Issues or PRs related to Hetzner provider area/provider/openstack Issues or PRs related to openstack provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants