fix(ec2): check for actions instead of policies. (#3694)

## Problem 
We currently check if certain policies are attached to the IAM role in
order to determine if the necessary permissions exist on the instance to
open an SSM session. However, this can be done more fine-grained.

## Solution 

https://github.com/aws/aws-toolkit-vscode/blob/a9d64f3a448480f8fed9ba96b3da33631c298265/src/ecs/util.ts#L33-L52
Leverage the approach done by ECS to check for specific actions rather
than policies.

- Restructure testing framework to look at actions instead of policies. 
- Refactor ECS code to pull out shared functionality to
`src/shared/remoteSession.ts`.
- Refactor existing code in the `src/ec2/model.ts` file to utilize the
approach demonstrated above.
- Add back `packages/core/src/shared/systemUtilities.ts` file since some
of it is used here.
<!---
    REMINDER:
    - Read CONTRIBUTING.md first.
    - Add test coverage for your changes.
    - Update the changelog using `npm run newChange`.
    - Link to related issues/commits.
    - Testing: how did you test your changes?
    - Screenshots
-->

## License

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: JadenSimon <[email protected]>
Co-authored-by: Justin M. Keyes <[email protected]>
Co-authored-by: Weinstock <[email protected]>

Author: 4 people

packages/core/src/awsService/ec2/model.ts

@@ -12,7 +12,12 @@ import { isCloud9 } from '../../shared/extensionUtilities'
 import { ToolkitError } from '../../shared/errors'
 import { SsmClient } from '../../shared/clients/ssmClient'
 import { Ec2Client } from '../../shared/clients/ec2Client'
-import { VscodeRemoteConnection, ensureDependencies, openRemoteTerminal } from '../../shared/remoteSession'
+import {
+    VscodeRemoteConnection,
+    ensureDependencies,
+    getDeniedSsmActions,
+    openRemoteTerminal,
+} from '../../shared/remoteSession'
 import { DefaultIamClient } from '../../shared/clients/iamClient'
 import { ErrorInformation } from '../../shared/errors'
 import { sshAgentSocketVariable, startSshAgent, startVscodeRemote } from '../../shared/extensions/ssh'
@@ -69,13 +74,10 @@ export class Ec2ConnectionManager {
         }
     }
 
-    public async hasProperPolicies(IamRoleArn: string): Promise<boolean> {
-        const attachedPolicies = (await this.iamClient.listAttachedRolePolicies(IamRoleArn)).map(
-            (policy) => policy.PolicyName!
-        )
-        const requiredPolicies = ['AmazonSSMManagedInstanceCore', 'AmazonSSMManagedEC2InstanceDefaultPolicy']
+    public async hasProperPermissions(IamRoleArn: string): Promise<boolean> {
+        const deniedActions = await getDeniedSsmActions(this.iamClient, IamRoleArn)
 
-        return requiredPolicies.length !== 0 && requiredPolicies.every((policy) => attachedPolicies.includes(policy))
+        return deniedActions.length === 0
     }
 
     public async isInstanceRunning(instanceId: string): Promise<boolean> {
@@ -102,12 +104,15 @@ export class Ec2ConnectionManager {
 
         if (!IamRole) {
             const message = `No IAM role attached to instance: ${selection.instanceId}`
-            this.throwConnectionError(message, selection, { code: 'EC2SSMPermission' })
+            this.throwConnectionError(message, selection, {
+                code: 'EC2SSMPermission',
+                documentationUri: this.policyDocumentationUri,
+            })
         }
 
-        const hasProperPolicies = await this.hasProperPolicies(IamRole!.Arn)
+        const hasPermission = await this.hasProperPermissions(IamRole!.Arn)
 
-        if (!hasProperPolicies) {
+        if (!hasPermission) {
             const message = `Ensure an IAM role with the required policies is attached to the instance. Found attached role: ${
                 IamRole!.Arn
             }`

packages/core/src/shared/clients/ec2Client.ts

@@ -61,7 +61,7 @@ export class Ec2Client {
 
         return safeInstances
             .map(async (instance) => {
-                return { ...instance, LastSeenStatus: await this.getInstanceStatus(instance.InstanceId!) }
+                return { ...instance, LastSeenStatus: await this.getInstanceStatus(instance.InstanceId) }
             })
             .map((instance) => {
                 return instanceHasName(instance!)

packages/core/src/shared/clients/iamClient.ts

@@ -104,4 +104,12 @@ export class DefaultIamClient {
         }
         return response.InstanceProfile.Roles[0]
     }
+
+    public async putRolePolicy(roleArn: string, policyName: string, policyDocument: string): Promise<void> {
+        const client = await this.createSdkClient()
+        const roleName = this.getFriendlyName(roleArn)
+        await client
+            .putRolePolicy({ RoleName: roleName, PolicyName: policyName, PolicyDocument: policyDocument })
+            .promise()
+    }
 }

packages/core/src/shared/clients/ssmClient.ts

@@ -64,7 +64,6 @@ export class SsmClient {
 
     public async getTargetPlatformName(target: string): Promise<string> {
         const instanceInformation = await this.describeInstance(target)
-
         return instanceInformation.PlatformName!
     }
 

packages/core/src/shared/remoteSession.ts

@@ -19,12 +19,21 @@ import { getOrInstallCli } from './utilities/cliUtils'
 import { pushIf } from './utilities/collectionUtils'
 import { ChildProcess } from './utilities/childProcess'
 import { findSshPath, getVscodeCliPath } from './utilities/pathFind'
+import { IamClient } from './clients/iamClient'
+import { IAM } from 'aws-sdk'
 
 export interface MissingTool {
     readonly name: 'code' | 'ssm' | 'ssh'
     readonly reason?: string
 }
 
+const minimumSsmActions = [
+    'ssmmessages:CreateControlChannel',
+    'ssmmessages:CreateDataChannel',
+    'ssmmessages:OpenControlChannel',
+    'ssmmessages:OpenDataChannel',
+]
+
 export async function openRemoteTerminal(options: vscode.TerminalOptions, onClose: () => void) {
     const timeout = new Timeout(60000)
 
@@ -165,3 +174,12 @@ export async function handleMissingTool(tools: Err<MissingTool[]>) {
         })
     )
 }
+
+export async function getDeniedSsmActions(client: IamClient, roleArn: string): Promise<IAM.EvaluationResult[]> {
+    const deniedActions = await client.getDeniedActions({
+        PolicySourceArn: roleArn,
+        ActionNames: minimumSsmActions,
+    })
+
+    return deniedActions
+}

packages/core/src/test/awsService/ec2/model.test.ts

@@ -42,36 +42,12 @@ describe('Ec2ConnectClient', function () {
         })
     })
 
-    describe('hasProperPolicies', async function () {
-        it('correctly determines if proper policies are included', async function () {
-            async function assertAcceptsPolicies(policies: IAM.Policy[], expectedResult: boolean) {
-                sinon.stub(DefaultIamClient.prototype, 'listAttachedRolePolicies').resolves(policies)
-
-                const result = await client.hasProperPolicies('')
-                assert.strictEqual(result, expectedResult)
-
-                sinon.restore()
-            }
-            await assertAcceptsPolicies(
-                [{ PolicyName: 'name' }, { PolicyName: 'name2' }, { PolicyName: 'name3' }],
-                false
-            )
-            await assertAcceptsPolicies(
-                [
-                    { PolicyName: 'AmazonSSMManagedInstanceCore' },
-                    { PolicyName: 'AmazonSSMManagedEC2InstanceDefaultPolicy' },
-                ],
-                true
-            )
-            await assertAcceptsPolicies([{ PolicyName: 'AmazonSSMManagedEC2InstanceDefaultPolicy' }], false)
-            await assertAcceptsPolicies([{ PolicyName: 'AmazonSSMManagedEC2InstanceDefaultPolicy' }], false)
-        })
-
+    describe('hasProperPermissions', async function () {
         it('throws error when sdk throws error', async function () {
             sinon.stub(DefaultIamClient.prototype, 'listAttachedRolePolicies').throws(new ToolkitError('error'))
 
             try {
-                await client.hasProperPolicies('')
+                await client.hasProperPermissions('')
                 assert.ok(false)
             } catch {
                 assert.ok(true)
@@ -131,7 +107,7 @@ describe('Ec2ConnectClient', function () {
         it('throws EC2SSMAgent error if instance is running and has IAM Role, but agent is not running', async function () {
             sinon.stub(Ec2ConnectionManager.prototype, 'isInstanceRunning').resolves(true)
             sinon.stub(Ec2ConnectionManager.prototype, 'getAttachedIamRole').resolves({ Arn: 'testRole' } as IAM.Role)
-            sinon.stub(Ec2ConnectionManager.prototype, 'hasProperPolicies').resolves(true)
+            sinon.stub(Ec2ConnectionManager.prototype, 'hasProperPermissions').resolves(true)
             sinon.stub(SsmClient.prototype, 'getInstanceAgentPingStatus').resolves('offline')
 
             try {
@@ -145,7 +121,7 @@ describe('Ec2ConnectClient', function () {
         it('does not throw an error if all checks pass', async function () {
             sinon.stub(Ec2ConnectionManager.prototype, 'isInstanceRunning').resolves(true)
             sinon.stub(Ec2ConnectionManager.prototype, 'getAttachedIamRole').resolves({ Arn: 'testRole' } as IAM.Role)
-            sinon.stub(Ec2ConnectionManager.prototype, 'hasProperPolicies').resolves(true)
+            sinon.stub(Ec2ConnectionManager.prototype, 'hasProperPermissions').resolves(true)
             sinon.stub(SsmClient.prototype, 'getInstanceAgentPingStatus').resolves('Online')
 
             assert.doesNotThrow(async () => await client.checkForStartSessionError(instanceSelection))

packages/core/src/test/shared/extensions/ssh.test.ts

@@ -2,7 +2,7 @@
  * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
  * SPDX-License-Identifier: Apache-2.0
  */
-import assert from 'assert'
+import * as assert from 'assert'
 import { ChildProcess } from '../../../shared/utilities/childProcess'
 import { startSshAgent } from '../../../shared/extensions/ssh'
 

packages/toolkit/.changes/next-release/Bug Fix-b6dbc82b-653c-4090-82c6-65972154ddf9.json

@@ -0,0 +1,4 @@
+{
+	"type": "Bug Fix",
+	"description": "when connecting to ec2 instance, check for IAM role permitted actions, rather than full policies"
+}