From be81952b38465d5a75c4b2edebea7f895a598202 Mon Sep 17 00:00:00 2001 From: Miguel Campos Date: Mon, 25 Nov 2024 14:33:59 -0800 Subject: [PATCH 1/2] add sudo propagation check for azure --- src/plugins/azure/ssh.ts | 32 ++++++++++++++++++++++++++---- src/plugins/ssh/index.ts | 42 ++++++++++++++++++++++++++-------------- src/types/ssh.ts | 23 +++++++++++++++------- 3 files changed, 72 insertions(+), 25 deletions(-) diff --git a/src/plugins/azure/ssh.ts b/src/plugins/azure/ssh.ts index 9a037ae..a173e27 100644 --- a/src/plugins/azure/ssh.ts +++ b/src/plugins/azure/ssh.ts @@ -8,6 +8,7 @@ This file is part of @p0security/cli You should have received a copy of the GNU General Public License along with @p0security/cli. If not, see . **/ +import { isSudoCommand } from "../../commands/shared/ssh"; import { SshProvider } from "../../types/ssh"; import { azAccountSetCommand, azLogin, azLoginCommand } from "./auth"; import { ensureAzInstall } from "./install"; @@ -26,6 +27,19 @@ import { } from "./types"; import path from "node:path"; +const unprovisionedAccessPatterns = [ + { + // The output of `sudo -v` when the user is not allowed to run sudo + pattern: /Sorry, user .+ may not run sudo on .+/, + }, +] as const; + +const validPreTestAccessPatterns = [ + { + pattern: /sudo: a password is required/, + }, +] as const; + // TODO: Determine what this value should be for Azure const PROPAGATION_TIMEOUT_LIMIT_MS = 2 * 60 * 1000; @@ -55,8 +69,18 @@ export const azureSshProvider: SshProvider< propagationTimeoutMs: PROPAGATION_TIMEOUT_LIMIT_MS, - // TODO(ENG-3149): Implement sudo access checks here - preTestAccessPropagationArgs: () => undefined, + preTestAccessPropagationArgs: (cmdArgs) => { + if (isSudoCommand(cmdArgs)) { + return { + ...cmdArgs, + // `sudo -v` prints `Sorry, user may not run sudo on .` to stderr when user is not a sudoer. + // we have to use `-n` flag to avoid the oauth prompt on azure cli. + command: "sudo", + arguments: ["-nv"], + }; + } + return undefined; + }, // Azure doesn't support ProxyCommand, as nice as that would be. Yet. proxyCommand: () => [], @@ -161,8 +185,8 @@ export const azureSshProvider: SshProvider< bastionId: request.permission.bastionHostId, }), - // TODO: Implement - unprovisionedAccessPatterns: [], + unprovisionedAccessPatterns, + validPreTestAccessPatterns, toCliRequest: async (request) => { return { diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index 198e386..a3578dd 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -57,19 +57,28 @@ const accessPropagationGuard = ( ) => { let isEphemeralAccessDeniedException = false; let isLoginException = false; + let isValidPreTestError = false; child.stderr.on("data", (chunk) => { const chunkString: string = chunk.toString("utf-8"); parseAndPrintSshOutputToStderr(chunkString, options); - const match = provider.unprovisionedAccessPatterns.find((message) => - chunkString.match(message.pattern) + const matchUnprovisionedPattern = provider.unprovisionedAccessPatterns.find( + (message) => chunkString.match(message.pattern) ); - if (match) { + const matchPreTestPattern = provider.validPreTestAccessPatterns?.find( + (message) => chunkString.match(message.pattern) + ); + + if (matchUnprovisionedPattern) { isEphemeralAccessDeniedException = true; } + if (matchPreTestPattern && !matchUnprovisionedPattern) { + isValidPreTestError = true; + } + if (provider.loginRequiredPattern) { const loginMatch = chunkString.match(provider.loginRequiredPattern); isLoginException = isLoginException || !!loginMatch; // once true, always true @@ -83,6 +92,7 @@ const accessPropagationGuard = ( return { isAccessPropagated: () => !isEphemeralAccessDeniedException, isLoginException: () => isLoginException, + isValidPreTestError: () => isValidPreTestError, }; }; @@ -170,11 +180,8 @@ async function spawnSshNode( ); // TODO ENG-2284 support login with Google Cloud: currently return a boolean to indicate if the exception was a Google login error. - const { isAccessPropagated, isLoginException } = accessPropagationGuard( - provider, - child, - options - ); + const { isAccessPropagated, isLoginException, isValidPreTestError } = + accessPropagationGuard(provider, child, options); const exitListener = child.on("exit", (code) => { exitListener.unref(); @@ -203,6 +210,15 @@ async function spawnSshNode( options.abortController?.abort(code); if (!options.isAccessPropagationPreTest) print2(`SSH session terminated`); + if ( + options.isAccessPropagationPreTest && + provider.validPreTestAccessPatterns && + isValidPreTestError() + ) { + // override the exit code to 0 if the expected error was found, this means access is ready. + resolve(0); + return; + } resolve(code); }); }); @@ -355,6 +371,7 @@ const preTestAccessPropagationIfNeeded = async < credential: P extends SshProvider ? C : undefined, + setupData: SshAdditionalSetup | undefined, endTime: number ) => { const testCmdArgs = sshProvider.preTestAccessPropagationArgs(cmdArgs); @@ -365,7 +382,7 @@ const preTestAccessPropagationIfNeeded = async < const { command, args } = createCommand( request, testCmdArgs, - undefined, // No need to re-apply SSH options from setupData + setupData, proxyCommand ); // Assumes that this is a non-interactive ssh command that exits automatically @@ -427,8 +444,6 @@ export const sshOrScp = async (args: { const endTime = Date.now() + sshProvider.propagationTimeoutMs; - let sshNodeExit; - try { const exitCode = await preTestAccessPropagationIfNeeded( sshProvider, @@ -436,13 +451,14 @@ export const sshOrScp = async (args: { cmdArgs, proxyCommand, credential, + setupData, endTime ); if (exitCode && exitCode !== 0) { return exitCode; // Only exit if there was an error when pre-testing } - sshNodeExit = await spawnSshNode({ + return await spawnSshNode({ credential, abortController: new AbortController(), command, @@ -455,6 +471,4 @@ export const sshOrScp = async (args: { } finally { await setupData?.teardown(); } - - return sshNodeExit; }; diff --git a/src/types/ssh.ts b/src/types/ssh.ts index 33bbefc..233b8a7 100644 --- a/src/types/ssh.ts +++ b/src/types/ssh.ts @@ -44,6 +44,13 @@ export type CliPermissionSpec< export const SupportedSshProviders = ["aws", "azure", "gcloud"] as const; export type SupportedSshProvider = (typeof SupportedSshProviders)[number]; +export type AccessPattern = { + /** If the error matches this string, indicates that access is not provisioned */ + readonly pattern: RegExp; + /** Maximum amount of time to wait for provisioning after encountering this error */ + readonly validationWindowMs?: number; +}; + export type SshProvider< PR extends PluginSshRequest = PluginSshRequest, O extends object | undefined = undefined, @@ -102,13 +109,15 @@ export type SshProvider< /** Unwraps this provider's types */ requestToSsh: (request: CliPermissionSpec) => SR; - /** Regex matches for error strings indicating that the provider has not yet fully provisioned node acces */ - unprovisionedAccessPatterns: readonly { - /** If the error matches this string, indicates that access is not provisioned */ - readonly pattern: RegExp; - /** Maximum amount of time to wait for provisioning after encountering this error */ - readonly validationWindowMs?: number; - }[]; + /** Regex matches for error strings indicating that the provider has not yet fully provisioned node access */ + unprovisionedAccessPatterns: readonly AccessPattern[]; + + /** Regex matches for error strings indicating that the provider is ready for node access. + * Used to override error codes during access propagation testing. + */ + validPreTestAccessPatterns?: readonly AccessPattern[]; + + /** Regex matches for error strings indicating that the provider has fully provisioned */ /** Converts a backend request to a CLI request */ toCliRequest: ( From e7d5d7cbf8b9904d3256e313f971bdf47e34f691 Mon Sep 17 00:00:00 2001 From: Miguel Campos Date: Mon, 2 Dec 2024 17:23:38 -0800 Subject: [PATCH 2/2] remove isPretest from propagation guard --- src/plugins/azure/ssh.ts | 4 ++-- src/plugins/ssh/index.ts | 47 ++++++++++++++++++++++++---------------- src/types/ssh.ts | 2 +- 3 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/plugins/azure/ssh.ts b/src/plugins/azure/ssh.ts index a173e27..541c6fb 100644 --- a/src/plugins/azure/ssh.ts +++ b/src/plugins/azure/ssh.ts @@ -34,7 +34,7 @@ const unprovisionedAccessPatterns = [ }, ] as const; -const validPreTestAccessPatterns = [ +const provisionedAccessPatterns = [ { pattern: /sudo: a password is required/, }, @@ -186,7 +186,7 @@ export const azureSshProvider: SshProvider< }), unprovisionedAccessPatterns, - validPreTestAccessPatterns, + provisionedAccessPatterns, toCliRequest: async (request) => { return { diff --git a/src/plugins/ssh/index.ts b/src/plugins/ssh/index.ts index a3578dd..cea1626 100644 --- a/src/plugins/ssh/index.ts +++ b/src/plugins/ssh/index.ts @@ -16,7 +16,12 @@ import { import { PRIVATE_KEY_PATH } from "../../common/keys"; import { print2 } from "../../drivers/stdio"; import { Authn } from "../../types/identity"; -import { SshProvider, SshRequest, SupportedSshProvider } from "../../types/ssh"; +import { + AccessPattern, + SshProvider, + SshRequest, + SupportedSshProvider, +} from "../../types/ssh"; import { delay } from "../../util"; import { AwsCredentials } from "../aws/types"; import { @@ -51,24 +56,26 @@ const RETRY_DELAY_MS = 5000; * do not capture stderr emitted from the wrapped shell session. */ const accessPropagationGuard = ( - provider: SshProvider, + invalidAccessPatterns: readonly AccessPattern[], + validAccessPatterns: readonly AccessPattern[] | undefined, + loginRequiredPattern: RegExp | undefined, child: ChildProcessByStdio, options: SpawnSshNodeOptions ) => { let isEphemeralAccessDeniedException = false; let isLoginException = false; - let isValidPreTestError = false; + let isValidError = false; child.stderr.on("data", (chunk) => { const chunkString: string = chunk.toString("utf-8"); parseAndPrintSshOutputToStderr(chunkString, options); - const matchUnprovisionedPattern = provider.unprovisionedAccessPatterns.find( - (message) => chunkString.match(message.pattern) + const matchUnprovisionedPattern = invalidAccessPatterns.find((message) => + chunkString.match(message.pattern) ); - const matchPreTestPattern = provider.validPreTestAccessPatterns?.find( - (message) => chunkString.match(message.pattern) + const matchPreTestPattern = validAccessPatterns?.find((message) => + chunkString.match(message.pattern) ); if (matchUnprovisionedPattern) { @@ -76,11 +83,11 @@ const accessPropagationGuard = ( } if (matchPreTestPattern && !matchUnprovisionedPattern) { - isValidPreTestError = true; + isValidError = true; } - if (provider.loginRequiredPattern) { - const loginMatch = chunkString.match(provider.loginRequiredPattern); + if (loginRequiredPattern) { + const loginMatch = chunkString.match(loginRequiredPattern); isLoginException = isLoginException || !!loginMatch; // once true, always true } @@ -90,9 +97,10 @@ const accessPropagationGuard = ( }); return { - isAccessPropagated: () => !isEphemeralAccessDeniedException, + isAccessPropagated: () => + !isEphemeralAccessDeniedException && + (!validAccessPatterns || isValidError), isLoginException: () => isLoginException, - isValidPreTestError: () => isValidPreTestError, }; }; @@ -180,8 +188,13 @@ async function spawnSshNode( ); // TODO ENG-2284 support login with Google Cloud: currently return a boolean to indicate if the exception was a Google login error. - const { isAccessPropagated, isLoginException, isValidPreTestError } = - accessPropagationGuard(provider, child, options); + const { isAccessPropagated, isLoginException } = accessPropagationGuard( + provider.unprovisionedAccessPatterns, + provider.provisionedAccessPatterns, + provider.loginRequiredPattern, + child, + options + ); const exitListener = child.on("exit", (code) => { exitListener.unref(); @@ -210,11 +223,7 @@ async function spawnSshNode( options.abortController?.abort(code); if (!options.isAccessPropagationPreTest) print2(`SSH session terminated`); - if ( - options.isAccessPropagationPreTest && - provider.validPreTestAccessPatterns && - isValidPreTestError() - ) { + if (options.isAccessPropagationPreTest && isAccessPropagated()) { // override the exit code to 0 if the expected error was found, this means access is ready. resolve(0); return; diff --git a/src/types/ssh.ts b/src/types/ssh.ts index 233b8a7..39f1a9a 100644 --- a/src/types/ssh.ts +++ b/src/types/ssh.ts @@ -115,7 +115,7 @@ export type SshProvider< /** Regex matches for error strings indicating that the provider is ready for node access. * Used to override error codes during access propagation testing. */ - validPreTestAccessPatterns?: readonly AccessPattern[]; + provisionedAccessPatterns?: readonly AccessPattern[]; /** Regex matches for error strings indicating that the provider has fully provisioned */