Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(cli): require approval is a part of the CliIoHost #151

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export enum RequireApproval {
NEVER = 'never',

/**
* Prompt for approval for any type of change to the stack
* Prompt for approval for any type of change to the stack
*/
ANYCHANGE = 'any-change',

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/toolkit-lib/CODE_REGISTRY.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
| CDK_TOOLKIT_I5002 | Provides time for resource migration | info | n/a |
| CDK_TOOLKIT_I5031 | Informs about any log groups that are traced as part of the deployment | info | n/a |
| CDK_TOOLKIT_I5050 | Confirm rollback during deployment | info | n/a |
| CDK_TOOLKIT_I5060 | Confirm deploy security sensitive changes | info | n/a |
| CDK_TOOLKIT_I5060 | Confirm deploy security sensitive changes, depending on approval level | info | n/a |
| CDK_TOOLKIT_I5501 | Stack Monitoring: Start monitoring of a single stack | info | [StackMonitoringControlEvent](https://docs.aws.amazon.com/cdk/api/toolkit-lib/interfaces/StackMonitoringControlEvent.html) |
| CDK_TOOLKIT_I5502 | Stack Monitoring: Activity event for a single stack | info | [StackActivity](https://docs.aws.amazon.com/cdk/api/toolkit-lib/interfaces/StackActivity.html) |
| CDK_TOOLKIT_I5503 | Stack Monitoring: Finished monitoring of a single stack | info | [StackMonitoringControlEvent](https://docs.aws.amazon.com/cdk/api/toolkit-lib/interfaces/StackMonitoringControlEvent.html) |
Expand Down
3 changes: 1 addition & 2 deletions packages/@aws-cdk/toolkit-lib/lib/actions/deploy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,7 @@ export interface DeployOptions extends BaseDeployOptions {
* Require a confirmation for security relevant changes before continuing with the deployment
*
* @default RequireApproval.NEVER
* @deprecated in future a message containing the full diff will be emitted and a response requested.
* Approval workflows should be implemented in the `IIoHost`.
* @deprecated requireApproval is governed by the `IIoHost`. This property is no longer used.
*/
readonly requireApproval?: RequireApproval;

Expand Down
22 changes: 10 additions & 12 deletions packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
import { DescribeChangeSetOutput, fullDiff } from '@aws-cdk/cloudformation-diff';
import * as cxapi from '@aws-cdk/cx-api';
import { ToolkitError } from '../../../api/errors';
import { RequireApproval } from '../../deploy';

/**
* Return whether the diff has security-impacting changes that need confirmation
* Return whether the diff has security-impacting changes that need confirmation.
*/
export function diffRequiresApproval(
export function determinePermissionType(
oldTemplate: any,
newTemplate: cxapi.CloudFormationStackArtifact,
requireApproval: RequireApproval,
changeSet?: DescribeChangeSetOutput,
): boolean {
// @todo return or print the full diff.
): 'non-broadening' | 'broadening' | 'none' {
// @todo return a printable version of the full diff.
const diff = fullDiff(oldTemplate, newTemplate.template, changeSet);

switch (requireApproval) {
case RequireApproval.NEVER: return false;
case RequireApproval.ANY_CHANGE: return diff.permissionsAnyChanges;
case RequireApproval.BROADENING: return diff.permissionsBroadened;
default: throw new ToolkitError(`Unrecognized approval level: ${requireApproval}`);
if (diff.permissionsBroadened) {
return 'broadening';
} else if (diff.permissionsAnyChanges) {
return 'non-broadening';
} else {
return 'none';
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export const CODES = {
}),
CDK_TOOLKIT_I5060: codeInfo({
code: 'CDK_TOOLKIT_I5060',
description: 'Confirm deploy security sensitive changes',
description: 'Confirm deploy security sensitive changes, depending on approval level',
level: 'info',
}),

Expand Down
18 changes: 13 additions & 5 deletions packages/@aws-cdk/toolkit-lib/lib/api/io/private/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,28 @@ export const confirm = (
motivation: string,
defaultResponse: boolean,
concurrency?: number,
permissionChangeType?: string,
): ActionLessRequest<{
motivation: string;
concurrency?: number;
}, boolean> => {
return prompt(code, `${chalk.cyan(question)} (y/n)?`, defaultResponse, {
motivation,
concurrency,
});
return prompt(
code,
`${chalk.cyan(question)} (y/n)?`,
defaultResponse,
{ motivation, concurrency, approvalLevel: permissionChangeType },
);
};

/**
* Prompt for a response from the IoHost.
*/
export const prompt = <T, U>(code: CodeInfo, message: string, defaultResponse: U, payload?: T): ActionLessRequest<T, U> => {
export const prompt = <T, U>(
code: CodeInfo,
message: string,
defaultResponse: U,
payload?: T,
): ActionLessRequest<T, U> => {
return {
defaultResponse,
...formatMessage({
Expand Down
28 changes: 15 additions & 13 deletions packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { AssetBuildTime, type DeployOptions, RequireApproval } from '../actions/deploy';
import { type ExtendedDeployOptions, buildParameterMap, createHotswapPropertyOverrides, removePublishedAssets } from '../actions/deploy/private';
import { type DestroyOptions } from '../actions/destroy';
import { diffRequiresApproval } from '../actions/diff/private';
import { determinePermissionType } from '../actions/diff/private';
import { type ListOptions } from '../actions/list';
import { type RollbackOptions } from '../actions/rollback';
import { type SynthOptions } from '../actions/synth';
Expand Down Expand Up @@ -246,8 +246,6 @@

await migrator.tryMigrateResources(stackCollection, options);

const requireApproval = options.requireApproval ?? RequireApproval.NEVER;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this breaks the behavior of the requireApproval option but i don't have a good way or hooking it up to the requireApproval governed by the CliIoHost -- toolkit.ts only has a concept of an IIoHost


const parameterMap = buildParameterMap(options.parameters?.parameters);

const hotswapMode = options.hotswap ?? HotswapMode.FULL_DEPLOYMENT;
Expand Down Expand Up @@ -314,16 +312,20 @@
return;
}

if (requireApproval !== RequireApproval.NEVER) {
const currentTemplate = await deployments.readCurrentTemplate(stack);
if (diffRequiresApproval(currentTemplate, stack, requireApproval)) {
const motivation = '"--require-approval" is enabled and stack includes security-sensitive updates.';
const question = `${motivation}\nDo you wish to deploy these changes`;
const confirmed = await ioHost.requestResponse(confirm(CODES.CDK_TOOLKIT_I5060, question, motivation, true, concurrency));
if (!confirmed) {
throw new ToolkitError('Aborted by user');
}
}
const currentTemplate = await deployments.readCurrentTemplate(stack);
const permissionChangeType = determinePermissionType(currentTemplate, stack);
const motivation = '"--require-approval" is enabled and stack includes security-sensitive updates.';
const question = `${motivation}\nDo you wish to deploy these changes`;
const confirmed = await ioHost.requestResponse(confirm(
CODES.CDK_TOOLKIT_I5060,
question,
motivation,
true,
concurrency,
permissionChangeType,
));
if (!confirmed) {
throw new ToolkitError('Aborted by user');
}

// Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK)
Expand Down Expand Up @@ -388,15 +390,15 @@
break;

case 'failpaused-need-rollback-first': {
const motivation = r.reason === 'replacement'

Check failure on line 393 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'motivation' is already declared in the upper scope on line 317 column 13
? `Stack is in a paused fail state (${r.status}) and change includes a replacement which cannot be deployed with "--no-rollback"`
: `Stack is in a paused fail state (${r.status}) and command line arguments do not include "--no-rollback"`;
const question = `${motivation}. Perform a regular deployment`;

Check failure on line 396 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'question' is already declared in the upper scope on line 318 column 13

if (options.force) {
await ioHost.notify(warn(`${motivation}. Rolling back first (--force).`));
} else {
const confirmed = await ioHost.requestResponse(confirm(CODES.CDK_TOOLKIT_I5050, question, motivation, true, concurrency));

Check failure on line 401 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'confirmed' is already declared in the upper scope on line 319 column 13
if (!confirmed) {
throw new ToolkitError('Aborted by user');
}
Expand All @@ -414,14 +416,14 @@
}

case 'replacement-requires-rollback': {
const motivation = 'Change includes a replacement which cannot be deployed with "--no-rollback"';

Check failure on line 419 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'motivation' is already declared in the upper scope on line 317 column 13
const question = `${motivation}. Perform a regular deployment`;

Check failure on line 420 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'question' is already declared in the upper scope on line 318 column 13

// @todo no force here
if (options.force) {
await ioHost.notify(warn(`${motivation}. Proceeding with regular deployment (--force).`));
} else {
const confirmed = await ioHost.requestResponse(confirm(CODES.CDK_TOOLKIT_I5050, question, motivation, true, concurrency));

Check failure on line 426 in packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

View workflow job for this annotation

GitHub Actions / build

'confirmed' is already declared in the upper scope on line 319 column 13
if (!confirmed) {
throw new ToolkitError('Aborted by user');
}
Expand Down
26 changes: 23 additions & 3 deletions packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
import { IIoHost, IoMessage, IoMessageLevel, IoRequest } from '../../lib';
import { IIoHost, IoMessage, IoMessageLevel, IoRequest, RequireApproval } from '../../lib';
import { isMessageRelevantForLevel } from '../../lib/api/io/private/level-priority';

/**
* A test implementation of IIoHost that does nothing but can by spied on.
* Optional set a level to filter out all irrelevant messages.
* Optionally set a level to filter out all irrelevant messages.
* Optionally set a approval level.
*/
export class TestIoHost implements IIoHost {
public readonly notifySpy: jest.Mock<any, any, any>;
public readonly requestSpy: jest.Mock<any, any, any>;

public requireApproval: RequireApproval = RequireApproval.NEVER;

constructor(public level: IoMessageLevel = 'info') {
this.notifySpy = jest.fn();
this.requestSpy = jest.fn();
Expand All @@ -21,9 +24,26 @@ export class TestIoHost implements IIoHost {
}

public async requestResponse<T, U>(msg: IoRequest<T, U>): Promise<U> {
if (isMessageRelevantForLevel(msg, this.level)) {
if (isMessageRelevantForLevel(msg, this.level) && this.needsApproval(msg)) {
this.requestSpy(msg);
}
return msg.defaultResponse;
}

private needsApproval(msg: IoRequest<any, any>): boolean {
// Return true if the code is unrelated to approval
if (!['CDK_TOOLKIT_I5060'].includes(msg.code)) {
return true;
}
switch (this.requireApproval) {
case RequireApproval.NEVER:
return false;
case RequireApproval.ANY_CHANGE:
return true;
case RequireApproval.BROADENING:
return msg.data?.permissionChangeType === 'broadening';
default:
return true;
}
}
}
9 changes: 3 additions & 6 deletions packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,9 @@ describe('deploy', () => {

test('request response when require approval is set', async () => {
// WHEN
ioHost.requireApproval = RequireApproval.ANY_CHANGE;
const cx = await builderFixture(toolkit, 'stack-with-role');
await toolkit.deploy(cx, {
requireApproval: RequireApproval.ANY_CHANGE,
});
await toolkit.deploy(cx);

// THEN
expect(ioHost.requestSpy).toHaveBeenCalledWith(expect.objectContaining({
Expand All @@ -70,9 +69,7 @@ describe('deploy', () => {
test('skips response by default', async () => {
// WHEN
const cx = await builderFixture(toolkit, 'stack-with-role');
await toolkit.deploy(cx, {
requireApproval: RequireApproval.NEVER,
});
await toolkit.deploy(cx);

// THEN
expect(ioHost.requestSpy).not.toHaveBeenCalledWith(expect.objectContaining({
Expand Down
56 changes: 56 additions & 0 deletions packages/aws-cdk/lib/toolkit/cli-io-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as util from 'node:util';
import * as chalk from 'chalk';
import * as promptly from 'promptly';
import { ToolkitError } from './error';
import { RequireApproval } from '@aws-cdk/cloud-assembly-schema';
import { ActivityPrinterProps, CurrentActivityPrinter, HistoryActivityPrinter, IActivityPrinter } from '../cli/activity-printer';
import { StackActivityProgress } from '../commands/deploy';

Expand Down Expand Up @@ -163,6 +164,13 @@ export interface CliIoHostProps {
readonly isCI?: boolean;

/**
* In what scenarios should the CliIoHost ask for approval
*
* @default RequireApproval.BROADENING
*/
readonly requireApproval?: RequireApproval;

/*
* The initial Toolkit action the hosts starts with.
*
* @default StackActivityProgress.BAR
Expand Down Expand Up @@ -195,6 +203,7 @@ export class CliIoHost implements IIoHost {
private _isTTY: boolean;
private _logLevel: IoMessageLevel;
private _internalIoHost?: IIoHost;
private _requireApproval: RequireApproval;
private _progress: StackActivityProgress = StackActivityProgress.BAR;

// Stack Activity Printer
Expand All @@ -209,6 +218,7 @@ export class CliIoHost implements IIoHost {
this._isTTY = props.isTTY ?? process.stdout.isTTY ?? false;
this._logLevel = props.logLevel ?? 'info';
this._isCI = props.isCI ?? isCI();
this._requireApproval = props.requireApproval ?? RequireApproval.BROADENING;

this.stackProgress = props.stackProgress ?? StackActivityProgress.BAR;
}
Expand Down Expand Up @@ -297,6 +307,20 @@ export class CliIoHost implements IIoHost {
this._isTTY = value;
}

/**
* Return the conditions for requiring approval in this CliIoHost.
*/
public get requireApproval() {
return this._requireApproval;
}

/**
* Set the conditions for requiring approval in this CliIoHost.
*/
public set requireApproval(approval: RequireApproval) {
this._requireApproval = approval;
}

/**
* Whether the CliIoHost is running in CI mode. In CI mode, all non-error output goes to stdout instead of stderr.
*/
Expand Down Expand Up @@ -394,6 +418,30 @@ export class CliIoHost implements IIoHost {
].includes(msg.code);
}

/**
* Detect special messages encode information about whether or not
* they require approval
*/
private requiresApproval(msg: IoRequest<any, any>): boolean {
return [
'CDK_TOOLKIT_I5060',
].includes(msg.code);
}

private skipApprovalStep(msg: IoRequest<any, any>): boolean {
switch(this.requireApproval) {
// Never require approval
case RequireApproval.NEVER:
return true;
// Always require approval
case RequireApproval.ANYCHANGE:
return false;
// Require approval if changes include broadening permissions
case RequireApproval.BROADENING:
return ['none', 'non-broadening'].includes(msg.data?.permissionChangeType);
}
}

/**
* Determines the output stream, based on message level and configuration.
*/
Expand Down Expand Up @@ -454,6 +502,14 @@ export class CliIoHost implements IIoHost {
throw new ToolkitError(`${motivation}, but concurrency is greater than 1 so we are unable to get a confirmation from the user`);
}

// Special approval prompt
// Determine if the message needs approval. If it does, continue (it is a basic confirmation prompt)
// If it does not, return success (true). We only check messages with codes that we are aware
// are requires approval codes.
if (this.requiresApproval(msg) && this.skipApprovalStep(msg)) {
return true;
}

// Basic confirmation prompt
// We treat all requests with a boolean response as confirmation prompts
if (isConfirmationPrompt(msg)) {
Expand Down
Loading
Loading