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..1de3385 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -152,16 +152,19 @@ 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. - -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. +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 +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..9865beb 100644 --- a/docs/kubelet-api.md +++ b/docs/kubelet-api.md @@ -46,20 +46,32 @@ 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: 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. +- **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 98739c2..d8e1aab 100644 --- a/pkg/vnode/kubelet_certificate.go +++ b/pkg/vnode/kubelet_certificate.go @@ -47,20 +47,20 @@ 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 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 } +// 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 { // nodeClient impersonates the virtual node and CREATES the request; ownClient is the @@ -72,7 +72,6 @@ type KubeletServingCertificateBootstrapper struct { nodeName string podName string podNamespace string - csrName string pollInterval time.Duration retryInterval time.Duration } @@ -82,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, @@ -118,7 +116,6 @@ func NewKubeletServingCertificateBootstrapper( nodeName: nodeName, podName: podName, podNamespace: podNamespace, - csrName: ServingCSRName(nodeName), pollInterval: kubeletServingPollInterval, retryInterval: kubeletServingRetryInterval, }, nil @@ -163,14 +160,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", @@ -184,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, @@ -191,7 +190,14 @@ 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: 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`", + 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 +207,27 @@ 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) + } + // 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", 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 +240,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 +277,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..c10f311 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") @@ -107,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) } @@ -185,12 +195,88 @@ 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") } +// 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. @@ -230,7 +316,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 +332,15 @@ 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, 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 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 { t.Fatalf("read role.yaml: %v", err) @@ -259,35 +350,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) } } }