Restrict Flex Node bootstrap token RBAC - #286
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens AKS Flex Node bootstrap-token RBAC by removing the legacy broad system:node binding for bootstrapper groups, adding migration/cleanup steps to reconcile existing clusters, and documenting the new behavior to ensure least-privilege node joins.
Changes:
- Removes the legacy
aks-flex-node-roleClusterRoleBinding from the Python helper RBAC manifest, and adds idempotent cleanup logic that runs after applying least-privilege RBAC (and before minting new bootstrap tokens). - Updates kubeadm E2E RBAC reconciliation to fail fast and explicitly propagate errors while running under a cluster lock that may disable
errexit. - Adds Go tests that assert the exact RBAC manifest content, ordering, idempotency, and a repository-wide invariant preventing bootstrap groups from being bound to
system:node, plus documentation updates describing migration and compatibility.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/aks-flex-config | Drops legacy system:node binding, adds legacy-binding cleanup after least-privilege RBAC and before token minting (fail-closed). |
| scripts/aks_flex_config_test.go | Adds regression tests for RBAC manifest contents, migration ordering/idempotency, fail-closed cleanup, kubeadm reconciliation behavior, and repo-wide invariant scanning. |
| hack/e2e/lib/node-join-kubeadm.sh | Removes legacy broad binding, adds explicit error propagation for RBAC and ConfigMap apply plus legacy-binding deletion. |
| docs/usages/joining-nodes.md | Documents least-privilege RBAC setup and legacy binding removal behavior when re-running setup-node-rbac. |
| docs/usages/aks-flex-config.md | Documents new required permissions and migration steps, including manual legacy binding removal command and fail-closed token behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
docs/usages/aks-flex-config.md:80
- The docs state the expected result of
kubectl get clusterrolebinding aks-flex-node-roleisNotFound, but the helper intentionally preserves a same-named binding when it is not the legacy unsafesystem:nodebinding. This verification step should instead instruct users to confirm the binding is absent or that it does not bind the bootstrap group tosystem:node, to avoid false alarms in clusters with a repurposed/safe same-named binding.
The expected result is `NotFound`. Once certificate issuance has been verified, both the kubelet and long-running Flex daemon use issued client certificates, so removing this binding does not interrupt joined nodes. New and in-progress joins retain the CSR permissions installed above.
Do not roll back a migrated host to an older or development-mode agent that still uses the bootstrap token for ordinary Kubernetes API requests. After this binding is removed, those requests correctly receive `403 Forbidden`. Restore a supported certificate-using agent instead of restoring the broad binding.
scripts/aks-flex-config:320
- The port-selection strategy has a TOCTOU race: the reservation socket binds to an ephemeral port and is then closed before starting
kubectl proxy, so another process can claim the port and cause intermittent proxy-start failures. Consider retrying proxy startup on bind failures (e.g., loop and pick a new port if the proxy exits immediately with an “address already in use” error), rather than assuming the reserved port will still be available.
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as reservation:
reservation.bind(("127.0.0.1", 0))
port = reservation.getsockname()[1]
proxy = subprocess.Popen(
[
Let kubectl bind port zero atomically, parse its reported port without buffered-pipe hangs, and bound process-group cleanup. Adds startup failure and fragmented-output coverage for review comment 3834577528.
Use privileged filesystem checks for managed binary links under the root-owned 0750 installation directory.
Keep the conditional Kubernetes delete on the loopback kubectl proxy even when enterprise proxy variables are set or NO_PROXY is empty.
Allow historical cleanup to use its saved subscription when the environment variable is absent, with a regression test for nounset execution.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/aks-flex-config:128
binding_has_bootstrap_groupraisesSystemExit("...subjects is not a list")without identifying which ClusterRoleBinding was malformed. If a cluster has drifted RBAC objects, this makes the failure hard to diagnose. Consider including the binding name (from metadata.name) in the error message, or passingnameinto this helper so the message can point at the exact object that needs manual review.
subjects = binding.get("subjects", [])
if subjects is None:
subjects = []
if not isinstance(subjects, list):
raise SystemExit("ERROR: managed ClusterRoleBinding subjects is not a list")
|
Superseded by a smaller, reviewable three-PR stack:
The tip of #289 is byte-for-byte identical to commit d677542 from this PR. Closing this monolithic PR so review can proceed in dependency order, starting with #287. |
Summary
system:nodeprivilege escalation path while retaining least-privilege CSR creation and approvalaks-flex-node-rolebinding, including optimistic concurrency checks and private local API transportv0.1.0, upgrades the same host to HEAD, migrates RBAC idempotently, revokes the token, restarts services, and verifies Node identity/readiness/Lease continuityCompatibility and migration
Existing joined nodes that use their issued kubelet/daemon certificates continue to work after migration. New and in-progress joins keep the narrowly scoped CSR permissions.
For a cluster configured by an older helper:
v0.1.1or later and verify its daemon certificate and service health;setup-node-rbac --remove-legacy-node-role-bindingexplicitly;Plain
setup-node-rbacinstalls/verifies the safe bindings but deliberately does not remove the legacy binding.generate-node-config --bootstrap-tokenfails closed while any bootstrap group still hassystem:node; it does not silently migrate an old node.Automatic removal is limited to the exact canonical legacy object and uses UID/resourceVersion preconditions. Ambiguous, customized, or concurrently changed bindings are preserved for manual review.
Validation
Local checks on the current branch:
make checkgo test -race -count=1 ./...go test -race -count=10 ./hack/e2ebash -nfor changed shell scriptsscripts/aks-flex-confighack/e2e/infra/main.bicepReal-cluster historical migration assertions passed on fresh Azure infrastructure at:
1.34.9: https://github.com/Azure/AKSFlexNode/actions/runs/325479371121.35.0: https://github.com/Azure/AKSFlexNode/actions/runs/32548662068In both runs, a real Ubuntu VM joined as a Ready node using the official
v0.1.0release, the same host upgraded to the PR build, the migration remained idempotent, bootstrap-token authorization changed from200to403and then401after revocation, and the same Node UID remained Ready while its Lease renewed after kubelet/daemon restarts. Those workflow runs are red only because an older cleanup implementation could not remove the default AKS node resource group; the functional test step passed. Final-SHA reruns including the cleanup fixes are in progress.This reproduces historical
v0.1.0node/config/RBAC state on newly created clusters. It is not evidence from a long-lived customer cluster with accumulated upgrade history or configuration drift. The repository E2E CSR approver is also not independent validation of the production AKS managed-ownership check; the test documents that boundary.Related work
This implements the backlog item Fix flex-node-bootstrap node group.
PR #284 must retain the same least-privilege manifest and explicit migration semantics when rebased or merged. A repository-wide invariant test prevents bootstrap groups from being rebound to
system:node.