[AKS] Fix #34091: az aks get-credentials: Fix kubelogin conversion for explicit kubeconfig paths

[Aditya Pujara (a0x1ab)]

🤖 PR Validation — ️✔️ All clear

Breaking Changes	Tests
️✔️ None	️✔️ 130/130

Related command

az aks get-credentials

Description

Fixes #34091.

Implemented by X Engineering Agent using Azure AI Foundry. This is an AI-generated change, published by the configured operator.

Testing Guide

• Run az aks get-credentials.
• Focused regression coverage: src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py.
• Repository CI and the authorized live-test workflow validate the published revision.

History Notes

[AKS] az aks get-credentials: Fix kubelogin conversion for explicit kubeconfig paths

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• The PR title and description has followed the guideline in Submitting Pull Requests.

• I adhere to the Command Guidelines.

• I adhere to the Error Handling Guidelines.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot review overview

🟡 Changes recommended

Resolve the critical risk of launching an unintended kubelogin executable instead of the resolved PATH executable.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Use resolved kubelogin path to prevent unintended executable execution · New

What changed in this PR

Fixes az aks get-credentials kubelogin conversion when an explicit kubeconfig path is used.

Changes:

• Passes the selected kubeconfig path to kubelogin.
• Adds regression coverage for conversion, path handling, failures, and security cases.

File	Summary
src/​azure-cli/​azure/​cli/​command_modules/​acs/​tests/​latest/​test_custom.py	Adds focused regression coverage.
src/​azure-cli/​azure/​cli/​command_modules/​acs/​custom.py	Updates kubelogin conversion invocation.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[FumingZhang]

Aditya Pujara (@a0x1ab) Please take a look at this unresolved executable-resolution issue. The duplicate thread has been removed; please retain the resolved which("kubelogin") result, normalize it with os.path.abspath(), and use that absolute path as argv[0].

[Aditya Pujara (a0x1ab)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).

[Aditya Pujara (a0x1ab)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).

[FumingZhang]

don't forget to resolve the merge conflict too Aditya Pujara (@a0x1ab)

[Yong Zhang (yonzhan)]

Please fix CI issues

[FumingZhang]

There's still merge conflict Aditya Pujara (@a0x1ab)

[Yong Zhang (yonzhan)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).

[x-engineering-agent]

Live test results — azdev test --live --series (changed test files only)

✅ PASS

Selectors: acs.AksGetCredentialsTest (module)
PR head ref: agent-assist/azure-azure-cli-issue-34091-63d5031c5731
PR head sha: 63ce4617c2d7b65bdaf31d0633bbd077b17c665c
PR base ref: dev
New test files in PR: false

Changed test files run

src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py

Workflow run: https://github.com/Azure/issue-sentinel/actions/runs/38500187536

Last 80 lines of azdev output

=============
| Run Tests |
=============


test index found: /home/runner/.azdev/env_config/home/runner/work/issue-sentinel/issue-sentinel/.venv/test_index/latest.json

Test on modules: acs.AksGetCredentialsTest

WARNING: RUNNING TESTS LIVE
The tests are set to run against current profile "latest"
============================= test session starts ==============================
platform linux -- Python 3.12.15, pytest-9.1.1, pluggy-1.6.0 -- /home/runner/work/issue-sentinel/issue-sentinel/.venv/bin/python
cachedir: .pytest_cache
rootdir: /home/runner/work/issue-sentinel/issue-sentinel/azure-cli/src/azure-cli
configfile: pyproject.toml
plugins: platformdirs-4.13.0, forked-1.7.5, xdist-3.8.0
collecting ... collected 9 items

azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_conversion_failure_warns PASSED [ 11%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_conversion_paths PASSED [ 22%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_converts_selected_kubeconfig PASSED [ 33%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_missing_kubelogin_warns PASSED [ 44%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_nonzero_kubelogin_exit PASSED [ 55%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_preserves_overwrite_context_and_admin PASSED [ 66%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_rejects_symlink_before_conversion PASSED [ 77%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_stdout_skips_conversion PASSED [ 88%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_without_devicecode_skips_conversion PASSED [100%]

- generated xml file: /home/runner/work/issue-sentinel/issue-sentinel/test-output/results.xml -
============================== 9 passed in 1.13s ===============================

Posted by the X Engineering Agent live-test workflow.

[FumingZhang]

Azure/azure-cli#34099 and Azure/azure-cli#34104 implement the same runtime fix; only comments and tests differ. I suggest consolidating into #34099 for its broader test coverage, carrying over useful cases from #34104. Before merging, please also address the executable-resolution feedback: invoke the absolute path returned by which (normalized with os.path.abspath) instead of bare kubelogin, while keeping --kubeconfig path and no cwd override.

[x-engineering-agent]

Aditya Pujara (@a0x1ab)

Review

Upstream CI passed (52 checks), and the Agent live test passed for head 63ce4617c2d7b65bdaf31d0633bbd077b17c665c. One confirmed security finding remains.

Confirmed code finding

Executable selection (high): src/azure-cli/azure/cli/command_modules/acs/custom.py:2069 still passes the bare kubelogin name as argv[0] after discarding which("kubelogin"). On Windows, CreateProcess can select a current-directory executable before the executable found on PATH. Removing cwd also changes that search directory to the caller's working directory. Save the lookup result, normalize it with os.path.abspath, and pass that absolute executable as argv[0]. This addresses the outstanding human feedback.

Update src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py to assert the resolved executable path, including a relative PATH result and a Windows current-directory shadowing case. Existing subprocess-mock assertions require the bare name, and the real-executable tests skip Windows, so they do not catch this risk. Rerun the focused AksGetCredentialsTest cases through the approved validation workflow.

Test validation

• Live test: Passed. Workflow run
• Regression coverage: For acs: 1 focused test file(s) changed. Scenario coverage is unverified; review the issue's required conditions, test setup, and assertions before approval. No linked issue was found in the PR timeline; inspect the source issue and its required scenario manually.

The source issue #34091 requires conversion of the selected kubeconfig from devicecode to azurecli mode. The changed tests cover relative/absolute/spaced paths, KUBECONFIG selection, stdout bypass, missing converter and subprocess failure; their POSIX subprocess fixture asserts the converted file and unrelated-file preservation. No updated recording or actual Windows executable-selection coverage is included; the live workflow result does not establish that missing Windows scenario.

Risk assessment

65/100 - High - High confidence

• Evidence: The changed subprocess call uses an unresolved executable name on a credential-handling path; Windows coverage is absent. CI and POSIX conversion coverage reduce uncertainty for the path-selection fix, not the executable-selection issue.
• Scope: Two files in acs, one production file, with focused tests.
• Required review: Owning AKS squad and security review before merge; no approval is requested until executable resolution and its regression coverage are addressed.

Posted by x-engineering-agent (Reviewer)

[x-engineering-agent]

X Engineering Agent Report

⏳ In progress — X Engineering Agent is reviewing this PR at 28576f4. This comment will be updated with the results.

Handed to maintainers

Aditya Pujara (@a0x1ab)

I could not verify this PR's durable Foundry publication and managed head. I cannot safely apply the feedback at #34104 (comment); leaving this PR for manual follow-up.

Posted by x-engineering-agent (Reviewer)

Review — Comments

Aditya Pujara (@a0x1ab)

The explicit kubeconfig selection addresses #34091's empty-directory failure, but the executable-selection feedback remains unresolved on head 1b3bbad5e831. This is a corrective review, not an all-clear.

Upstream CI

52/52 checks passed in this round's snapshot; no failed or pending upstream checks.

Test validation

• Live test: Passed. 9 of 9 test(s) passed on 1b3bbad5e831. Workflow run
• Regression coverage: For acs: 1 focused test file(s) changed. Scenario coverage is unverified; review the issue's required conditions, test setup, and assertions before approval. No linked issue was found in the PR timeline; inspect the source issue and its required scenario manually.

The tests exercise aks_get_credentials directly with credential fixtures: relative/nested/absolute and space-containing paths, explicit-file precedence over KUBECONFIG, stdout, missing converter, conversion errors, overwrite/context/admin behavior and symlink rejection. A POSIX executable fixture verifies that the selected file changes from devicecode to azurecli while unrelated kubeconfigs remain unchanged. These nine tests passed in the Agent workflow; they do not exercise the real kubelogin binary, a live AKS service, or Windows executable search. No recordings changed; this local subprocess/file correction does not change ARM requests.

I manually inspected #34091: removing cwd=os.path.dirname(path) avoids the empty cwd for a bare filename, and passing --kubeconfig path targets the merged file. The generic coverage detector did not link this issue or recognize the direct-handler assertions; its uncertainty above must not be read as proof of full platform coverage.

Confirmed finding — [P1] Execute the resolved absolute kubelogin path.

src/azure-cli/azure/cli/command_modules/acs/custom.py:2068 checks which("kubelogin"), but the changed invocation at line 2072 still uses bare "kubelogin" as argv[0]. On Windows, executable search can select a current-directory kubelogin.exe instead of the executable found on PATH. The module already preserves and normalizes the resolved kubectl path at lines 364–367 and 2344–2347.

Keep the lookup result, normalize it with os.path.abspath(), and use that path as argv[0], preserving the explicit --kubeconfig argument and warning behavior. Update test_get_credentials_conversion_paths in acs/tests/latest/test_custom.py: it currently returns /bin/kubelogin from the lookup yet expects bare kubelogin, so it accepts this defect. Add assertions for absolute and relative lookup results and a Windows current-directory-shadowing regression where practical.

This confirms both human feedback and Copilot reviewer feedback; neither is resolved by the current head. No other correctness finding was confirmed by this review.

Next action: Maintainer follow-up is required under the existing provenance handoff. No implementation job was queued because durable publication/managed-head provenance remains unverified. The review request is retained; this does not approve the PR.

Focused verification after correction: Run azdev test acs.AksGetCredentialsTest, including the executable-path assertions, and exercise Windows shadowing without launching an untrusted executable. This review did not run a new test.

Risk assessment — 16/100 · Low · High confidence

The Low rating is driven by public CLI behavior.

• Change scope: 2 changed files, 246 changed lines (+242 / -4), including 1 production file.
• Affected components: acs
• Risk drivers: public CLI behavior (+18)
• Regression evidence: Changed regression tests are included, reducing risk.
• Confidence: High because changed-line patches were available for every production file.
• Required review: Owning-squad review is recommended for acs before merge.

Posted by x-engineering-agent (Reviewer)

Live test — Passed (9/9)

PASS

Selectors: acs.AksGetCredentialsTest (module)
PR head ref: agent-assist/azure-azure-cli-issue-34091-63d5031c5731
PR head sha: 28576f46cf06201b0fded19e29e2a91d8558cfdc
PR base ref: dev
New test files in PR: false

Changed test files run

src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py

Workflow run: https://github.com/Azure/issue-sentinel/actions/runs/38727066296

Last 80 lines of azdev output

=============
| Run Tests |
=============


test index found: /home/runner/.azdev/env_config/home/runner/work/issue-sentinel/issue-sentinel/.venv/test_index/latest.json

Test on modules: acs.AksGetCredentialsTest

WARNING: RUNNING TESTS LIVE
The tests are set to run against current profile "latest"
============================= test session starts ==============================
platform linux -- Python 3.12.15, pytest-9.1.1, pluggy-1.7.0 -- /home/runner/work/issue-sentinel/issue-sentinel/.venv/bin/python
cachedir: .pytest_cache
rootdir: /home/runner/work/issue-sentinel/issue-sentinel/azure-cli/src/azure-cli
configfile: pyproject.toml
plugins: platformdirs-4.13.0, forked-1.7.5, xdist-3.8.0
collecting ... collected 9 items

azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_conversion_failure_warns PASSED [ 11%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_conversion_paths PASSED [ 22%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_converts_selected_kubeconfig PASSED [ 33%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_missing_kubelogin_warns PASSED [ 44%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_nonzero_kubelogin_exit PASSED [ 55%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_preserves_overwrite_context_and_admin PASSED [ 66%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_rejects_symlink_before_conversion PASSED [ 77%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_stdout_skips_conversion PASSED [ 88%]
azure-cli/src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py::AksGetCredentialsTest::test_get_credentials_without_devicecode_skips_conversion PASSED [100%]

- generated xml file: /home/runner/work/issue-sentinel/issue-sentinel/test-output/results.xml -
============================== 9 passed in 1.02s ===============================

Workflow run

Posted by x-engineering-agent (Tester)