Skip to content

[1/3] Restrict Flex Node bootstrap token RBAC - #287

Open
Alex Huang (wenhug) wants to merge 3 commits into
mainfrom
wenhug/bootstrap-rbac-core
Open

[1/3] Restrict Flex Node bootstrap token RBAC#287
Alex Huang (wenhug) wants to merge 3 commits into
mainfrom
wenhug/bootstrap-rbac-core

Conversation

@wenhug

@wenhug Alex Huang (wenhug) commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • remove the bootstrap-group to system:node privilege escalation path
  • reconcile only the least-privilege CSR creation and approval bindings
  • add an explicit, fail-closed migration for the canonical legacy binding
  • inventory unsafe ClusterRoleBinding and namespaced RoleBinding edges
  • refuse new bootstrap tokens while any unsafe direct binding remains
  • verify the live agent version, certificate identity, stable process, and certificate-authenticated access to its exact Node before migration

Why this PR is focused

This is part 1 of the replacement for #286. It contains only the shipped RBAC behavior, its operator documentation, and direct regression coverage.

Stack:

  1. This PR
  2. [2/3] Harden E2E state, cleanup, and Arc diagnostics #288 — E2E state and Azure cleanup hardening
  3. [3/3] Test historical bootstrap RBAC migration #289 — historical v0.1.0 migration proof

Safety properties

  • Existing legacy agents are not broken by a silent deletion.
  • Customized, namespaced, owner-managed, finalized, or otherwise ambiguous bindings are preserved for manual review.
  • Automatic deletion is limited to the exact canonical cluster-scoped object and uses UID and resourceVersion preconditions.
  • The migration is idempotent and verifies its postconditions.
  • The operator probe proves baseline certificate authorization; feature-specific MachineOperation and service-proxy RBAC is called out separately.
  • Unsafe subject detection exactly matches system:bootstrappers and system:bootstrappers:aks-flex-node; unrelated bootstrap subgroups are not swept in.

Validation

  • make check
  • go test -race ./scripts
  • ShellCheck and bash -n for the changed E2E fixture
  • current-head native PR Checks and CodeQL passed on e7d517d
  • required native PR #287 E2E passed on e7d517d, including cleanup
  • focused historical v0.1.0 migration E2E passed on final stacked SHA 790f408, including evidence upload, cleanup, and cleanup diagnostics
  • independent blocker-only security and operability review found no remaining major issue

Deliberately deferred low-risk scope

This helper audits direct bootstrap-group-to-system:node bindings, not arbitrary custom or aggregated-role effective authorization. Proxy pipe buffering, long TMPDIR Unix-socket paths, and DELETE 404 races fail closed or have low practical exposure and are left for follow-up rather than expanding this security PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens the security posture of Flex Node bootstrap tokens by removing the legacy RBAC privilege-escalation path (bootstrapper group → system:node) and introducing an explicit, fail-closed migration flow that preserves ambiguous/customized operator RBAC for manual review.

Changes:

  • Replaces “apply a static RBAC manifest” with reconciliation that only manages least-privilege CSR create/auto-approve bindings and verifies postconditions.
  • Adds an explicit --remove-legacy-node-role-binding migration that only auto-deletes the canonical legacy binding using UID/resourceVersion preconditions, and refuses to mint new bootstrap tokens while any unsafe binding remains.
  • Adds regression coverage for reconciliation + migration semantics, and updates docs/E2E fixtures to align with the restricted bootstrap RBAC model.

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 Implements RBAC reconciliation, explicit legacy-binding migration with preconditions, and fail-closed bootstrap token generation when unsafe bindings remain.
scripts/aks_flex_config_test.go Adds comprehensive regression tests for safe RBAC reconciliation, migration behavior, concurrency preconditions, and repository-wide invariants.
hack/e2e/lib/node-join-kubeadm.sh Removes legacy system:node binding from the kubeadm E2E fixture and adds basic error handling for RBAC/ConfigMap applies; tightens config file permissions.
docs/usages/joining-nodes.md Updates bootstrap-token flow to reflect least-privilege RBAC and explicit migration requirements.
docs/usages/aks-flex-config.md Documents the explicit migration, certificate verification steps, and rollback considerations for removing the obsolete legacy binding.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wenhug

Copy link
Copy Markdown
Collaborator Author

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 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:412

  • The kubectl-proxy readiness detector only searches for the "Starting to serve on …" line in stdout. If kubectl emits this line on stderr (common for long-running command logging), this will always time out and prevent conditional deletion. Consider matching the readiness pattern in both streams.
                        if key.data == "stdout" and startup_pattern.search(stream_output):

Copilot AI review requested due to automatic review settings August 26, 2026 22:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants