Add ip addresses into kubelet certs #12188
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
area/kops-controller
area/nodeup
area/provider/aws
|
/retest |
olemarkus
force-pushed
the
kubelet-ip-altnames
branch
from
0a6b1a9
to
4dc67d8
Compare
olemarkus
changed the title
|
@johngmyers Could you take a look at this PR, please? |
|
What is the reason for adding the IP addresses to the kubelet-server cert? The concern I have is that when there are two services with the same IP in their respective SANs, a client using that IP to talk to one can be tricked into talking to the other. There are clients that use IP addresses to talk to kube-apiserver, so they could then be redirected to talk to kubelet instead. |
|
Sorry, for the lack of explanation. I put it in the commit, but it didn't make it into the PR. The challenge is that there is no host DNS record for ipv6. So if you have an ipv6-only pod (say metrics-server) that wants to talk to kubelet, it cannot verify the TLS cert. So either such Pods would need host networking or we would need to put IPs in the certificate. I find the latter more safe than the former. Upstream is also fond of using IPs before hostname. https://github.com/kubernetes-sigs/metrics-server/blob/master/manifests/base/deployment.yaml#L26 |
| @@ -232,8 +232,14 @@ func (a awsVerifier) VerifyToken(token string, body []byte) (*fi.VerifyResult, e | |||
|
|
|||
| instance := instances.Reservations[0].Instances[0] | |||
|
|
|||
| addrs, err := GetInstanceCertificateNames(a.ec2, instanceID) | |||
There was a problem hiding this comment.
This new function calls DescribeInstances even though DescribeInstances was just called a few lines above. Maybe it could be refactored to provide the DescribeInstancesOutput so it only needs to be called once here? and then kubeletNames can call DescribeInstances separately.
Since AWS does not resolve instance hostnames to ipv6, ipv6-only pods that talk to kubelet API has to use node IP, not hostname. Thus we need to add IPs to kubelet server cert.
olemarkus
force-pushed
the
kubelet-ip-altnames
branch
from
4dc67d8
to
ad16042
Compare
|
this LGTM pending @johngmyers's feedback |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers 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 |
k8s-ci-robot
added
the
approved
| return nil, err | ||
| } | ||
|
|
||
| addrs, _ := net.LookupHost(name) |
There was a problem hiding this comment.
This is probably putting too much trust in the DNS. It would be much better to get the IP addresses from cloud APIs, like we do for AWS.
No description provided.