KEP-4872: Harden Kubelet serving cert validation #4911
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
kind/kep
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: g-gaston 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 |
k8s-ci-robot
added
the
size/L
g-gaston
changed the title
| # The following PRR answers are required at alpha release | ||
| # List the feature gate name and the components for which it must be enabled | ||
| feature-gates: | ||
| - name: KubeletCertCNValidation |
There was a problem hiding this comment.
How about: NodeCertificateNameValidation?
The API server is verifying a certificate presented by a node, whatever software that node is running (which, OK, is very likely to be kubelet).
Co-authored-by: Tim Bannister <[email protected]>
|
|
||
| Provided an actor with control of a node can impersonate another node, the impact would be: | ||
|
|
||
| * Break confidentiality of the requests sent by the Kube-API server to the kubelet (e.g kubectl exec/logs).These are usually user-driven requests. That gives the threat actor the possibility of producing incorrect or mis-leading feedback. In the exec case, it could allow a threat actor to issue prompts for credentials. In addition, the exec commands might contain user secrets. |
There was a problem hiding this comment.
if this actor is already owning a node in the cluster then exec and logs does not need to impersonate a node, they just target pods in a node, and require the kubelet to terminate the connection
| #### Metrics | ||
|
|
||
| In order to help cluster administrators determine if it's safe to enable the feature, we propose to add a new metric `kube_apiserver_validation_kubelet_cert_cn_errors` that will track the number of errors due to the new CN validation. | ||
| If the feature gate is disabled, we will still add the validation code to the HTTP transport, however, if the validation fails we won't return an error, we will just increment the metric counter. |
There was a problem hiding this comment.
Hmm... normally we want all code for the new feature to be completely inert when the gate is disabled. That way if there are bugs or non-functional regressions (like performance issues, etc), disabling the feature gate gives a way to mitigate that.
I like the idea of surfacing the metric of how many CNs would fail the validation, but I would only do the check and publish the metric if the feature gate is enabled. Someone wanting to dry run could enable the gate to active the code but pass --disable-kubelet-cert-cn-validation=false to avoid failing requests if the validation fails.
|
|
||
| ##### e2e tests | ||
|
|
||
| End-to-end tests won't be needed as unit and integration tests will cover all the scenarios. |
There was a problem hiding this comment.
I don't know if you will be able to do everything with integrations , but since a lot of e2e test use logs, exec and portforward, I assume it will be implicitly covered by those test once the feature gate is enabled
| This vulnerability can be exploited through ARP poisoning or other routing attacks, allowing a rogue node to obtain a certificate for an IP it does not own and reroute traffic to itself. | ||
|
|
||
| When the Kube API server connects to a kubelet, it verifies that the serving certificate is signed by a trusted CA and that the IP or hostname it’s connecting to is included in the certificate's SANs. | ||
| If a rogue node obtained a certificate for an IP it does not own and reroute traffic to itself, it would be able to impersonate a Node that reports that IP. |
There was a problem hiding this comment.
can you expand on the "reroute traffic to itself" problem?
I may not get all the full details of the scenario, nodeA with IP1 is removed but somehow actorB manages to get its certificate, spawn a nodeB with nodeA name? IP2 or IP1??? , join it to the cluster and set the address IP1 on node.status.addresses ?
|
|
||
| We will remove the metric once the feature is GA. | ||
|
|
||
| > TODO: let's discuss this in the review. We could consider adding the node name to the metric or even keeping the metric post GA if it's valuable. |
There was a problem hiding this comment.
node name has the cardinality problem, but leaving the metric sounds like a good thing to me, it can help to detect issues with the node certificates due to bugs per example
| #### Alpha | ||
|
|
||
| * Add feature flag for gating usage, off by default | ||
| * Add flag to disable extra validation |
There was a problem hiding this comment.
remember you need to add the flag only if the feature flag is enabled, otherwise if the feature does not progress removing the flag can not be possible without breaking the users that are already setting it
|
+1 sounds a great addition, simple and very effective |
TODOs with things I think can use a discussion.