From feddc4a887a0ec528b36adfbc173fcabe74d825d Mon Sep 17 00:00:00 2001 From: kerthcet Date: Mon, 14 Sep 2026 11:59:54 +0100 Subject: [PATCH 1/3] use a fixed cert name Signed-off-by: kerthcet --- cmd/main.go | 9 +-- config/manager/manager.yaml | 3 +- config/rbac/role.yaml | 8 +-- docs/deploy.md | 10 ++-- docs/kubelet-api.md | 13 +++-- pkg/vnode/kubelet_certificate.go | 45 +++++++++------ pkg/vnode/kubelet_certificate_test.go | 80 +++++++++++++++++---------- 7 files changed, 101 insertions(+), 67 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index dcf2859..caa132a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -525,11 +525,12 @@ func setupKubeletServer(mgr ctrl.Manager, addr, clientCA string, servingTLSBoots // declaration's doc. A resourceName containing a colon must be QUOTED, or the marker fails to // parse and takes every other rbac rule in the package with it. // -// Keep the CSR names in step with vnode.ServingCSRName, and the users with the providers that -// can register. Only `create` cannot be scoped by name. +// Keep the CSR name in step with vnode.ServingCSRName, and the users with the providers that +// can register — that list is the one part still keyed to a runtime-chosen name. Only `create` +// cannot be scoped by name. // +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,verbs=create -// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,resourceNames={nebula-kubelet-serving-nebula-aws,nebula-kubelet-serving-nebula-modal,nebula-kubelet-serving-nebula-fake},verbs=delete;get -// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests/approval,resourceNames={nebula-kubelet-serving-nebula-aws,nebula-kubelet-serving-nebula-modal,nebula-kubelet-serving-nebula-fake},verbs=update +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,resourceNames=nebula-kubelet-serving,verbs=delete;get +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests/approval,resourceNames=nebula-kubelet-serving,verbs=update // +kubebuilder:rbac:groups=certificates.k8s.io,resources=signers,resourceNames=kubernetes.io/kubelet-serving,verbs=approve // +kubebuilder:rbac:groups="",resources=users,resourceNames={"system:node:nebula-aws","system:node:nebula-modal","system:node:nebula-fake"},verbs=impersonate // +kubebuilder:rbac:groups="",resources=groups,resourceNames="system:nodes",verbs=impersonate diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index de892f5..404f25f 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -98,7 +98,8 @@ spec: fieldPath: status.podIP # Recorded as annotations on the kubelet-serving CSR, so an operator looking # at a stuck request can tell which manager Pod asked for it. The CSR's name - # comes from the node, not from here (see vnode.ServingCSRName). + # is fixed and carries no Pod identity (see vnode.ServingCSRName), which is + # why these annotations are the only way to tell. - name: POD_NAME valueFrom: fieldRef: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 33dc22e..4f4153d 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -88,9 +88,7 @@ rules: - apiGroups: - certificates.k8s.io resourceNames: - - nebula-kubelet-serving-nebula-aws - - nebula-kubelet-serving-nebula-fake - - nebula-kubelet-serving-nebula-modal + - nebula-kubelet-serving resources: - certificatesigningrequests verbs: @@ -99,9 +97,7 @@ rules: - apiGroups: - certificates.k8s.io resourceNames: - - nebula-kubelet-serving-nebula-aws - - nebula-kubelet-serving-nebula-fake - - nebula-kubelet-serving-nebula-modal + - nebula-kubelet-serving resources: - certificatesigningrequests/approval verbs: diff --git a/docs/deploy.md b/docs/deploy.md index b785b62..4c9b102 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -157,11 +157,11 @@ everything else — the stale delete, the polling, the approval — goes out as ServiceAccount, because a node identity may create and get its own CSRs and nothing more. A single-identity version fails on the delete and never creates a CSR at all. -The request is named `nebula-kubelet-serving-`, one per virtual node for the life of the -cluster, which is what lets `config/rbac/role.yaml` scope delete, get and approval to those -names by `resourceNames`. Only `create` is cluster-wide. An external approver, if you run one, -should match on that node identity, the `system:nodes` organization, and the current manager -Pod IP as the sole IP SAN. +The request is named `nebula-kubelet-serving` — one for the whole cluster, because every virtual +node advertises this Pod's IP and the API server verifies the address it dialed. A fixed name is +what lets `config/rbac/role.yaml` scope delete, get and approval to it by `resourceNames`. Only +`create` is cluster-wide. An external approver, if you run one, should match on that node +identity, the `system:nodes` organization, and the current manager Pod IP as the sole IP SAN. --- diff --git a/docs/kubelet-api.md b/docs/kubelet-api.md index 8a574aa..4b39d4f 100644 --- a/docs/kubelet-api.md +++ b/docs/kubelet-api.md @@ -54,12 +54,13 @@ The mechanics that are easy to get wrong: ServiceAccount does the delete, the polling and the approval, because a node identity may create and get its own CSRs and nothing more. Requester and approver differing is ordinary: the signer cares only who asked. -- **One certificate covers every virtual node.** All of them advertise the same address — this - Pod's IP — and the API server verifies against the address it dialed, not the node name. So - one request, under the first registered provider's node name, serves the whole set. -- **One CSR per node, named `nebula-kubelet-serving-`.** Stable rather than generated, so - `config/rbac/role.yaml` can scope delete, get and approval to those names by `resourceNames`; - only `create` is cluster-wide. +- **One certificate covers every virtual node, so there is one CSR.** All of them advertise the + same address — this Pod's IP — and the API server verifies against the address it dialed, not + the node name. So a single request, submitted under the first registered provider's node + identity, serves the whole set. Its name is the fixed `nebula-kubelet-serving`, which is what + lets `config/rbac/role.yaml` scope delete, get and approval to that one name by + `resourceNames`; only `create` is cluster-wide. The node identity still varies, and the + `users` impersonate grant has to list every provider that can register. - **Renewal is unattended.** 30 days requested, re-requested 24h before expiry with a fresh ECDSA key that never leaves memory. A failed attempt retains the current certificate and retries in 30s; a failed *approval* is retried in place, so a transient API error costs a poll diff --git a/pkg/vnode/kubelet_certificate.go b/pkg/vnode/kubelet_certificate.go index 98739c2..167c18a 100644 --- a/pkg/vnode/kubelet_certificate.go +++ b/pkg/vnode/kubelet_certificate.go @@ -55,12 +55,16 @@ const ( // CSR stays Approved and unsigned, with no condition to notice. func NodeIdentity(nodeName string) string { return "system:node:" + nodeName } -// ServingCSRName is the CSR one virtual node reuses for the life of the cluster. +// ServingCSRName is the single CSR the kubelet endpoint reuses for the life of the cluster. // -// Derived from the node name and nothing per-process, so RBAC can scope delete, get and -// approval to exactly these names (see the markers in cmd/main.go). Changing the format means -// changing that list too, or the manager loses access to its own CSR. -func ServingCSRName(nodeName string) string { return "nebula-kubelet-serving-" + nodeName } +// A constant, not a per-node name, because one certificate serves every virtual node: they all +// advertise this Pod's address and the API server verifies the address it dialed. A per-node +// name would claim otherwise, and RBAC would have to enumerate names only known at runtime. +// +// If virtual nodes ever stop sharing one address — provider-sharded replicas, each with its own +// Pod IP — each needs its own certificate and this must go back to a per-node name, or two +// replicas will delete each other's request. +const ServingCSRName = "nebula-kubelet-serving" type KubeletServingCertificateBootstrapper struct { // nodeClient impersonates the virtual node and CREATES the request; ownClient is the @@ -72,7 +76,6 @@ type KubeletServingCertificateBootstrapper struct { nodeName string podName string podNamespace string - csrName string pollInterval time.Duration retryInterval time.Duration } @@ -118,7 +121,6 @@ func NewKubeletServingCertificateBootstrapper( nodeName: nodeName, podName: podName, podNamespace: podNamespace, - csrName: ServingCSRName(nodeName), pollInterval: kubeletServingPollInterval, retryInterval: kubeletServingRetryInterval, }, nil @@ -163,14 +165,14 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte // A CSR left by an earlier attempt is unusable: its certificate would be for a key we no // longer hold. Usually a no-op — the cleaner drops an issued CSR an hour after approval. - if err := b.ownClient.Delete(ctx, b.csrName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { - return time.Time{}, fmt.Errorf("delete stale CSR %s: %w", b.csrName, err) + if err := b.ownClient.Delete(ctx, ServingCSRName, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) { + return time.Time{}, fmt.Errorf("delete stale CSR %s: %w", ServingCSRName, err) } expirationSeconds := int32(kubeletServingCertificateLifetime / time.Second) // The one call whose IDENTITY matters (see the constructor). csr, err := b.nodeClient.Create(ctx, &certificatesv1.CertificateSigningRequest{ ObjectMeta: metav1.ObjectMeta{ - Name: b.csrName, + Name: ServingCSRName, Labels: map[string]string{ "app.kubernetes.io/name": "nebula", "app.kubernetes.io/component": "kubelet-serving-certificate", @@ -191,7 +193,16 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte }, }, metav1.CreateOptions{}) if err != nil { - return time.Time{}, fmt.Errorf("create CSR %s: %w", b.csrName, err) + // Forbidden here is almost always the impersonate grant, not the create: the identity is + // the node name, which a new provider changes, while the RBAC list naming it is static + // YAML. Saying so turns the eventual symptom — the endpoint keeps its self-signed + // certificate and `kubectl exec` fails x509 on EKS — into the fix. + if apierrors.IsForbidden(err) { + return time.Time{}, fmt.Errorf("create CSR %s as %s: %w; add that name to the users "+ + "impersonate grant in cmd/main.go and run `make manifests`", + ServingCSRName, NodeIdentity(b.nodeName), err) + } + return time.Time{}, fmt.Errorf("create CSR %s: %w", ServingCSRName, err) } log := logf.FromContext(ctx).WithName("kubelet-serving-certificate") @@ -201,19 +212,19 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte ticker := time.NewTicker(b.pollInterval) defer ticker.Stop() for { - current, err := b.ownClient.Get(ctx, b.csrName, metav1.GetOptions{}) + current, err := b.ownClient.Get(ctx, ServingCSRName, metav1.GetOptions{}) if err != nil { - return time.Time{}, fmt.Errorf("get CSR %s: %w", b.csrName, err) + return time.Time{}, fmt.Errorf("get CSR %s: %w", ServingCSRName, err) } for _, condition := range current.Status.Conditions { if condition.Type == certificatesv1.CertificateDenied || condition.Type == certificatesv1.CertificateFailed { - return time.Time{}, fmt.Errorf("CSR %s ended with %s: %s", b.csrName, condition.Type, condition.Message) + return time.Time{}, fmt.Errorf("CSR %s ended with %s: %s", ServingCSRName, condition.Type, condition.Message) } } if len(current.Status.Certificate) > 0 { cert, notAfter, err := servingCertificate(current.Status.Certificate, keyPEM, b.nodeIP) if err != nil { - return time.Time{}, fmt.Errorf("load certificate from CSR %s: %w", b.csrName, err) + return time.Time{}, fmt.Errorf("load certificate from CSR %s: %w", ServingCSRName, err) } b.server.SetServingCertificate(cert) return notAfter, nil @@ -226,7 +237,7 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte if err := b.approve(ctx, current); err != nil { log.Error(err, "could not self-approve the serving certificate request; "+ "approve it by hand or the endpoint keeps its self-signed certificate", - "csr", b.csrName, "approveCommand", "kubectl certificate approve "+b.csrName) + "csr", ServingCSRName, "approveCommand", "kubectl certificate approve "+ServingCSRName) } } @@ -263,7 +274,7 @@ func (b *KubeletServingCertificateBootstrapper) approve( Message: "approved by the Nebula manager for its own kubelet serving endpoint", LastUpdateTime: metav1.Now(), }) - _, err := b.ownClient.UpdateApproval(ctx, b.csrName, csr, metav1.UpdateOptions{}) + _, err := b.ownClient.UpdateApproval(ctx, ServingCSRName, csr, metav1.UpdateOptions{}) return err } diff --git a/pkg/vnode/kubelet_certificate_test.go b/pkg/vnode/kubelet_certificate_test.go index 476dfd3..6ef7b33 100644 --- a/pkg/vnode/kubelet_certificate_test.go +++ b/pkg/vnode/kubelet_certificate_test.go @@ -43,6 +43,9 @@ import ( "k8s.io/client-go/kubernetes/fake" k8stesting "k8s.io/client-go/testing" "sigs.k8s.io/yaml" + + "github.com/InftyAI/Nebula/pkg/provider" + fakeprovider "github.com/InftyAI/Nebula/pkg/provider/fake" ) func TestKubeletServingCertificateBootstrapperInstallsIssuedCertificate(t *testing.T) { @@ -99,7 +102,7 @@ func TestKubeletServingCertificateBootstrapperInstallsIssuedCertificate(t *testi var csr *certificatesv1.CertificateSigningRequest waitFor(t, func() bool { csr, err = client.CertificatesV1().CertificateSigningRequests().Get( - context.Background(), bootstrapper.csrName, metav1.GetOptions{}, + context.Background(), ServingCSRName, metav1.GetOptions{}, ) return err == nil && isApproved(csr) }, "self-approved kubelet-serving CSR") @@ -185,7 +188,7 @@ func TestKubeletServingCertificateBootstrapperRoutesVerbsByIdentity(t *testing.T // create as the node. waitFor(t, func() bool { csr, getErr := nodeFake.CertificatesV1().CertificateSigningRequests().Get( - ctx, bootstrapper.csrName, metav1.GetOptions{}, + ctx, ServingCSRName, metav1.GetOptions{}, ) return getErr == nil && isApproved(csr) }, "CSR created as the node and approved as the manager") @@ -230,7 +233,7 @@ func TestKubeletServingCertificateBootstrapperRetriesApproval(t *testing.T) { waitFor(t, func() bool { csr, getErr := client.CertificatesV1().CertificateSigningRequests().Get( - ctx, bootstrapper.csrName, metav1.GetOptions{}, + ctx, ServingCSRName, metav1.GetOptions{}, ) return getErr == nil && isApproved(csr) }, "approval retried after a transient failure") @@ -246,10 +249,20 @@ func TestKubeletServingCertificateBootstrapperRetriesApproval(t *testing.T) { } } -// TestServingCSRNameIsScopedByRBAC guards the coupling the narrow grant rests on: the name is -// computed in Go, the resourceNames list is written by hand, and drift between them is silent -// in CI and surfaces only as a Forbidden on a real cluster. -func TestServingCSRNameIsScopedByRBAC(t *testing.T) { +// registrableProviders is every provider whose adapter can register, and so every node identity +// addServingCertificateBootstrap might impersonate — it takes the first REGISTERED name, and +// which one that is depends on what has credentials at startup. Constants rather than literals +// so a rename breaks the build; a new adapter has to be added here by hand. +var registrableProviders = []string{provider.ProviderAWS, provider.ProviderModal, fakeprovider.ProviderName} + +// TestKubeletServingRBACGrants guards the two couplings between names computed in Go and the +// hand-written resourceNames lists. Drift is silent in CI and surfaces on a real cluster as a +// Forbidden, three layers from the eventual symptom (a self-signed certificate, so `kubectl +// exec` fails x509 on EKS). +// +// The impersonate list is the fragile one: it is keyed to a node name chosen at runtime, so +// landing a provider adapter without touching RBAC breaks the endpoint. +func TestKubeletServingRBACGrants(t *testing.T) { raw, err := os.ReadFile(filepath.Join("..", "..", "config", "rbac", "role.yaml")) if err != nil { t.Fatalf("read role.yaml: %v", err) @@ -259,35 +272,46 @@ func TestServingCSRNameIsScopedByRBAC(t *testing.T) { t.Fatalf("parse role.yaml: %v", err) } + // Keyed by group/resource, because "users" lives in the core group and the CSR rules do not. scoped := map[string]map[string]bool{} for _, rule := range role.Rules { - if !slices.Contains(rule.APIGroups, certificatesv1.GroupName) { - continue - } - for _, resource := range rule.Resources { - // An unscoped rule may only create: that verb cannot be scoped by name, while - // deleting or approving someone else's CSR is what the scoping exists to prevent. - if len(rule.ResourceNames) == 0 { - if !slices.Equal(rule.Verbs, []string{"create"}) { - t.Errorf("cluster-wide rule on %s grants %v, want [create] alone", resource, rule.Verbs) + for _, group := range rule.APIGroups { + for _, resource := range rule.Resources { + // An unscoped CSR rule may only create: that verb cannot be scoped by name, while + // deleting or approving someone else's request is what the scoping exists to prevent. + if len(rule.ResourceNames) == 0 { + if group == certificatesv1.GroupName && !slices.Equal(rule.Verbs, []string{"create"}) { + t.Errorf("cluster-wide rule on %s grants %v, want [create] alone", resource, rule.Verbs) + } + continue + } + key := group + "/" + resource + if scoped[key] == nil { + scoped[key] = map[string]bool{} + } + for _, name := range rule.ResourceNames { + scoped[key][name] = true } - continue - } - if scoped[resource] == nil { - scoped[resource] = map[string]bool{} - } - for _, name := range rule.ResourceNames { - scoped[resource][name] = true } } } + remediate := "run `make manifests` after changing the markers in cmd/main.go" for _, resource := range []string{"certificatesigningrequests", "certificatesigningrequests/approval"} { - for _, provider := range []string{"aws", "modal", "fake"} { - if want := ServingCSRName(NodeName(provider)); !scoped[resource][want] { - t.Errorf("role.yaml does not scope %s to %q; run `make manifests` after changing "+ - "ServingCSRName or the markers in cmd/main.go", resource, want) - } + key := certificatesv1.GroupName + "/" + resource + if !scoped[key][ServingCSRName] { + t.Errorf("role.yaml does not scope %s to %q; %s", resource, ServingCSRName, remediate) + } + // One certificate serves every node, so exactly one name: an extra is either a leftover + // per-node grant or a widening nobody asked for. + if got := len(scoped[key]); got != 1 { + t.Errorf("%s is scoped to %d names, want only %q", resource, got, ServingCSRName) + } + } + for _, name := range registrableProviders { + if want := NodeIdentity(NodeName(name)); !scoped["/users"][want] { + t.Errorf("role.yaml does not grant impersonate on user %q, so provider %q cannot request "+ + "a serving certificate; %s", want, name, remediate) } } } From 4228f288d7e495e85c83cd907d376d69e1e2428d Mon Sep 17 00:00:00 2001 From: kerthcet Date: Mon, 14 Sep 2026 13:17:40 +0100 Subject: [PATCH 2/3] update comment Signed-off-by: kerthcet --- docs/deploy.md | 11 +++++--- docs/kubelet-api.md | 14 +++++++--- pkg/vnode/kubelet_certificate.go | 39 ++++++++++++--------------- pkg/vnode/kubelet_certificate_test.go | 28 ++++++++++--------- 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index 4c9b102..1de3385 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -152,10 +152,13 @@ kubectl -n nebula-system logs deploy/nebula-controller-manager \ ``` Two identities are involved, and the split is not cosmetic. The CSR is **created** while -impersonating `system:node:nebula-`, because the signer signs for nobody else; -everything else — the stale delete, the polling, the approval — goes out as the manager's -ServiceAccount, because a node identity may create and get its own CSRs and nothing more. A -single-identity version fails on the delete and never creates a CSR at all. +impersonating `system:node:nebula-`, because EKS silently declines to sign a request from +the manager's own ServiceAccount — upstream Kubernetes does not check the requester at all, so this +is EKS-specific and measured rather than inferred (see +[kubelet-api.md](kubelet-api.md#the-serving-certificate)); everything else — the stale delete, the polling, +the approval — goes out as the manager's ServiceAccount, because a node identity may create and +get its own CSRs and nothing more. A single-identity version fails on the delete and never +creates a CSR at all. The request is named `nebula-kubelet-serving` — one for the whole cluster, because every virtual node advertises this Pod's IP and the API server verifies the address it dialed. A fixed name is diff --git a/docs/kubelet-api.md b/docs/kubelet-api.md index 4b39d4f..a888264 100644 --- a/docs/kubelet-api.md +++ b/docs/kubelet-api.md @@ -46,10 +46,16 @@ fallback, so nothing depends on the request succeeding. The mechanics that are easy to get wrong: -- **The requester is a node, and it is checked.** The CSR is created while impersonating - `system:node:nebula-`, with that same name as its CN. The signer signs for the node - that asks and for nobody else, and it reports a mismatch **nowhere** — the CSR sits - `Approved` with no certificate. `Approved,Issued` is the only healthy state. +- **EKS checks the requester; upstream does not.** The CSR is created while impersonating + `system:node:nebula-`, with that same name as its CN. Send the identical request as the + manager's own ServiceAccount and EKS approves it and then never signs it — no certificate, and + **no condition** to notice, so `Approved,Issued` is the only healthy state. Both ways were + measured on EKS 1.35 with identical CSR bytes, differing only in the creating identity; check that + again before believing any claim that the impersonation is removable. None of this is upstream + behavior: + `ValidateKubeletServingCSR` never sees the requester and checks only the CN prefix and a + `system:nodes` organization, and a validation failure there writes `CertificateFailed` rather + than going quiet. - **Two identities, not one.** Only the create is impersonated. The manager's own ServiceAccount does the delete, the polling and the approval, because a node identity may create and get its own CSRs and nothing more. Requester and approver differing is ordinary: diff --git a/pkg/vnode/kubelet_certificate.go b/pkg/vnode/kubelet_certificate.go index 167c18a..191b10b 100644 --- a/pkg/vnode/kubelet_certificate.go +++ b/pkg/vnode/kubelet_certificate.go @@ -47,23 +47,19 @@ const ( kubeletServingPollInterval = 2 * time.Second ) -// NodeIdentity is the username the kubernetes.io/kubelet-serving signer expects on a request -// for a node's serving certificate. +// NodeIdentity is the CN a serving certificate request must carry, and the username the client +// impersonates to submit it — one function because the two must agree. // -// One function because two places must agree: the request's CN, and the identity the client -// impersonates to submit it. The signer compares them and ignores a mismatch in silence — the -// CSR stays Approved and unsigned, with no condition to notice. +// EKS signs only for a system:node: creator, and refuses in silence: the same request from the +// manager's ServiceAccount is approved and then never signed, with no condition to notice. That +// is EKS-specific, not upstream behavior. func NodeIdentity(nodeName string) string { return "system:node:" + nodeName } // ServingCSRName is the single CSR the kubelet endpoint reuses for the life of the cluster. // -// A constant, not a per-node name, because one certificate serves every virtual node: they all -// advertise this Pod's address and the API server verifies the address it dialed. A per-node -// name would claim otherwise, and RBAC would have to enumerate names only known at runtime. -// -// If virtual nodes ever stop sharing one address — provider-sharded replicas, each with its own -// Pod IP — each needs its own certificate and this must go back to a per-node name, or two -// replicas will delete each other's request. +// One name because one certificate serves every virtual node: they all advertise this Pod's +// address, and the API server verifies the address it dialed. Provider-sharded replicas, each +// with its own Pod IP, would need a name per node again. const ServingCSRName = "nebula-kubelet-serving" type KubeletServingCertificateBootstrapper struct { @@ -85,13 +81,12 @@ var _ manager.Runnable = (*KubeletServingCertificateBootstrapper)(nil) // NewKubeletServingCertificateBootstrapper builds the CSR loop for one virtual node, over TWO // clients because no single identity can do the whole job: // -// - nodeClient impersonates NodeIdentity(nodeName) and creates the request; the signer -// refuses one submitted by anything else. -// - ownClient is the manager's ServiceAccount and does the rest. A node may create and get -// its own CSRs and nothing more — on EKS it `cannot delete resource -// "certificatesigningrequests"`, and approving is an approver's job anyway. +// - nodeClient impersonates NodeIdentity(nodeName) and creates the request; see there. +// - ownClient is the manager's ServiceAccount and does the rest. A node may create and get its +// own CSRs and nothing more — on EKS it `cannot delete resource +// "certificatesigningrequests"`. // -// Requester and approver differing is the ordinary arrangement: the signer checks who ASKED. +// Only the CREATE's identity matters, so approving as the manager is not a workaround. func NewKubeletServingCertificateBootstrapper( nodeClient, ownClient kubernetes.Interface, server *KubeletServer, @@ -186,6 +181,8 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte Request: requestPEM, SignerName: certificatesv1.KubeletServingSignerName, ExpirationSeconds: &expirationSeconds, + // Two, not three: keyEncipherment is for an RSA key and this one is ECDSA. The signer + // accepts either set, so adding it would still sign — and still be wrong. Usages: []certificatesv1.KeyUsage{ certificatesv1.UsageDigitalSignature, certificatesv1.UsageServerAuth, @@ -193,10 +190,8 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte }, }, metav1.CreateOptions{}) if err != nil { - // Forbidden here is almost always the impersonate grant, not the create: the identity is - // the node name, which a new provider changes, while the RBAC list naming it is static - // YAML. Saying so turns the eventual symptom — the endpoint keeps its self-signed - // certificate and `kubectl exec` fails x509 on EKS — into the fix. + // Forbidden here is almost always the impersonate grant: the identity changes with the + // provider, the RBAC list naming it does not. if apierrors.IsForbidden(err) { return time.Time{}, fmt.Errorf("create CSR %s as %s: %w; add that name to the users "+ "impersonate grant in cmd/main.go and run `make manifests`", diff --git a/pkg/vnode/kubelet_certificate_test.go b/pkg/vnode/kubelet_certificate_test.go index 6ef7b33..c214dcf 100644 --- a/pkg/vnode/kubelet_certificate_test.go +++ b/pkg/vnode/kubelet_certificate_test.go @@ -110,9 +110,16 @@ func TestKubeletServingCertificateBootstrapperInstallsIssuedCertificate(t *testi if csr.Spec.SignerName != certificatesv1.KubeletServingSignerName { t.Fatalf("signer = %q, want %q", csr.Spec.SignerName, certificatesv1.KubeletServingSignerName) } + // Exact set equality, and the three-usage set is also accepted — so a stray keyEncipherment + // still signs, and only this assertion catches it. + if want := []certificatesv1.KeyUsage{ + certificatesv1.UsageDigitalSignature, + certificatesv1.UsageServerAuth, + }; !slices.Equal(csr.Spec.Usages, want) { + t.Fatalf("usages = %v, want %v", csr.Spec.Usages, want) + } request := parseCertificateRequest(t, csr.Spec.Request) - // Must be the node identity the client impersonates, not the Pod: the signer compares the - // two and ignores a mismatch without any condition to notice (see NodeIdentity). + // The node identity, not the Pod: it has to match the impersonated user (see NodeIdentity). if request.Subject.CommonName != "system:node:nebula-modal" { t.Fatalf("common name = %q", request.Subject.CommonName) } @@ -249,19 +256,14 @@ func TestKubeletServingCertificateBootstrapperRetriesApproval(t *testing.T) { } } -// registrableProviders is every provider whose adapter can register, and so every node identity -// addServingCertificateBootstrap might impersonate — it takes the first REGISTERED name, and -// which one that is depends on what has credentials at startup. Constants rather than literals -// so a rename breaks the build; a new adapter has to be added here by hand. +// registrableProviders is every provider whose adapter can register, so every identity +// addServingCertificateBootstrap might impersonate — it takes the first REGISTERED name, which +// depends on what has credentials at startup. A new adapter has to be added here by hand. var registrableProviders = []string{provider.ProviderAWS, provider.ProviderModal, fakeprovider.ProviderName} -// TestKubeletServingRBACGrants guards the two couplings between names computed in Go and the -// hand-written resourceNames lists. Drift is silent in CI and surfaces on a real cluster as a -// Forbidden, three layers from the eventual symptom (a self-signed certificate, so `kubectl -// exec` fails x509 on EKS). -// -// The impersonate list is the fragile one: it is keyed to a node name chosen at runtime, so -// landing a provider adapter without touching RBAC breaks the endpoint. +// TestKubeletServingRBACGrants pins names computed in Go against the hand-written resourceNames +// lists. Drift passes CI and surfaces only on a real cluster, as a Forbidden nowhere near the +// symptom it eventually causes. func TestKubeletServingRBACGrants(t *testing.T) { raw, err := os.ReadFile(filepath.Join("..", "..", "config", "rbac", "role.yaml")) if err != nil { From 6cf25e36aea1fe13dd82b0a88bb20a1ee53fe778 Mon Sep 17 00:00:00 2001 From: kerthcet Date: Mon, 14 Sep 2026 13:30:49 +0100 Subject: [PATCH 3/3] address comments Signed-off-by: kerthcet --- docs/kubelet-api.md | 5 ++ pkg/vnode/kubelet_certificate.go | 8 +++ pkg/vnode/kubelet_certificate_test.go | 76 +++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/docs/kubelet-api.md b/docs/kubelet-api.md index a888264..9865beb 100644 --- a/docs/kubelet-api.md +++ b/docs/kubelet-api.md @@ -67,6 +67,11 @@ The mechanics that are easy to get wrong: lets `config/rbac/role.yaml` scope delete, get and approval to that one name by `resourceNames`; only `create` is cluster-wide. The node identity still varies, and the `users` impersonate grant has to list every provider that can register. +- **That name is global, so the object under it is checked.** A fetch by name can return a CSR + someone else recreated, and approving it would sign a key and SANs the manager does not control — + the name is the only thing its approval grant is scoped by. So each poll compares the object's UID + with the one it created and restarts the attempt on a mismatch. Two Nebula installations in one + cluster will therefore log a replaced CSR at each other indefinitely rather than converge. - **Renewal is unattended.** 30 days requested, re-requested 24h before expiry with a fresh ECDSA key that never leaves memory. A failed attempt retains the current certificate and retries in 30s; a failed *approval* is retried in place, so a transient API error costs a poll diff --git a/pkg/vnode/kubelet_certificate.go b/pkg/vnode/kubelet_certificate.go index 191b10b..d8e1aab 100644 --- a/pkg/vnode/kubelet_certificate.go +++ b/pkg/vnode/kubelet_certificate.go @@ -211,6 +211,14 @@ func (b *KubeletServingCertificateBootstrapper) requestAndWait(ctx context.Conte if err != nil { return time.Time{}, fmt.Errorf("get CSR %s: %w", ServingCSRName, err) } + // The name is cluster-global, so a get can return an object we did not create. Approving + // that would sign a key and SANs we do not control, and our approval grant is scoped by + // this name alone. + if current.UID != csr.UID { + return time.Time{}, fmt.Errorf("CSR %s was replaced (uid %s, created %s); "+ + "another Nebula installation sharing the cluster would do this", + ServingCSRName, current.UID, csr.UID) + } for _, condition := range current.Status.Conditions { if condition.Type == certificatesv1.CertificateDenied || condition.Type == certificatesv1.CertificateFailed { return time.Time{}, fmt.Errorf("CSR %s ended with %s: %s", ServingCSRName, condition.Type, condition.Message) diff --git a/pkg/vnode/kubelet_certificate_test.go b/pkg/vnode/kubelet_certificate_test.go index c214dcf..c10f311 100644 --- a/pkg/vnode/kubelet_certificate_test.go +++ b/pkg/vnode/kubelet_certificate_test.go @@ -201,6 +201,82 @@ func TestKubeletServingCertificateBootstrapperRoutesVerbsByIdentity(t *testing.T }, "CSR created as the node and approved as the manager") } +// TestKubeletServingCertificateBootstrapperRejectsReplacedCSR covers the cluster-global name: an +// object recreated under it belongs to someone else, and approving it would sign SANs we do not +// control. The attempt must restart instead, and recover once the name is ours again. +func TestKubeletServingCertificateBootstrapperRejectsReplacedCSR(t *testing.T) { + client := fake.NewSimpleClientset() + var mu sync.Mutex + replaced := true + var creates, approvals int + // The fake tracker assigns no UID, so ours is empty and only the imposter's differs. + client.PrependReactor("get", "certificatesigningrequests", + func(k8stesting.Action) (bool, runtime.Object, error) { + mu.Lock() + defer mu.Unlock() + if !replaced { + return false, nil, nil + } + return true, &certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{Name: ServingCSRName, UID: "someone-else"}, + }, nil + }) + client.PrependReactor("create", "certificatesigningrequests", + func(k8stesting.Action) (bool, runtime.Object, error) { + mu.Lock() + creates++ + mu.Unlock() + return false, nil, nil + }) + client.PrependReactor("update", "certificatesigningrequests", + func(k8stesting.Action) (bool, runtime.Object, error) { + mu.Lock() + approvals++ + mu.Unlock() + return false, nil, nil + }) + + server, err := NewKubeletServer("10.20.18.154", ":10250", "") + if err != nil { + t.Fatalf("NewKubeletServer: %v", err) + } + bootstrapper, err := NewKubeletServingCertificateBootstrapper( + client, client, server, + "10.20.18.154", "nebula-modal", "nebula-system", + "nebula-controller-manager-abc", + ) + if err != nil { + t.Fatalf("NewKubeletServingCertificateBootstrapper: %v", err) + } + bootstrapper.pollInterval = 5 * time.Millisecond + bootstrapper.retryInterval = 5 * time.Millisecond + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { _ = bootstrapper.Start(ctx) }() + + // A second create proves the guard ended the first attempt; an approval is the failure this + // test exists for, so stop on either and let the assertion below name which happened. + waitFor(t, func() bool { + mu.Lock() + defer mu.Unlock() + return creates >= 2 || approvals > 0 + }, "the attempt to restart on a replaced CSR") + mu.Lock() + if approvals != 0 { + t.Errorf("approved a CSR created by someone else (%d times)", approvals) + } + replaced = false + mu.Unlock() + + waitFor(t, func() bool { + csr, getErr := client.CertificatesV1().CertificateSigningRequests().Get( + ctx, ServingCSRName, metav1.GetOptions{}, + ) + return getErr == nil && isApproved(csr) + }, "approval once the name is ours again") +} + // TestKubeletServingCertificateBootstrapperRetriesApproval covers a transient UpdateApproval. // Approving from outside the poll left nothing to try again, so the loop watched a CSR that // could not be signed until the cleaner removed it a day later.