fix(ec2): openTerminal fails on Cloud9 instances #3677

Problem:
Steps to reproduce:
- create a cloud9 ec2 instance
- try to use "open terminal" feature on the instance
- command failed error :(

Questions:
- Why did the toolkit try to use the `AWSCloud9SSMInstanceProfile` role if it's not valid?
    - In order to check if an EC2 instance has the proper permissions, we use
      the EC2 SDK to get the role name associated with the instance. Then, we
      use the IAM SDK to list the policies associated with that role. Therefore,
      the toolkit got the `AWSCloud9SSMInstanceProfile` role from the EC2 SDK,
      but then the IAM threw an error saying the role does not exist. Currently
      unsure why the role is invalid. 
- or is it valid, but we need to pass the full ARN to whatever API is throwing the error?
    - The docs:
      https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/IAM.html#listAttachedRolePolicies-property,
      only accept the `friendly name` which is the role name and not the ARN.
      Once we know why the role doesn't exist (despite being attached to an EC2
      instance), that will likely lead us to how we can request more info on it
      via SDK. 
- or do we need to find a different role and ignore this one?
    - Only one IAM role can be attached to each EC2 instance. Since this is the
      role we get from the EC2 SDK, there is no other role we can use. 
- or can we tell the user something more meaningful about what the problem
  actually is, instead of just "NoSuchEntity ... the role cannot be found"?

Solution:
- Show a specific error about what happened. Rather than throwing the general
  lacking permission error, mention that there was a failure in retrieving the
  policies attached to a given instance.
- Log the ARN of the role we receive from the EC2 SDK.

Note: Because we check for policies before executing `startSession`, it is
possible that the proper policies do exist on the instance, but we fail the
pre-check. Thus, we don't connect despite having the proper polices. One way
around this could be to attempt connection despite a missing role, however this
presents another challenge of what error message to throw if that connection
fails.

Author: Hweinstock

src/ec2/model.ts

@@ -8,7 +8,7 @@ import { IAM } from 'aws-sdk'
 import { Ec2Selection } from './utils'
 import { getOrInstallCli } from '../shared/utilities/cliUtils'
 import { isCloud9 } from '../shared/extensionUtilities'
-import { ToolkitError } from '../shared/errors'
+import { ToolkitError, isAwsError } from '../shared/errors'
 import { SsmClient } from '../shared/clients/ssmClient'
 import { Ec2Client } from '../shared/clients/ec2Client'
 
@@ -17,12 +17,17 @@ export type Ec2ConnectErrorCode = 'EC2SSMStatus' | 'EC2SSMPermission' | 'EC2SSMC
 import { openRemoteTerminal } from '../shared/remoteSession'
 import { DefaultIamClient } from '../shared/clients/iamClient'
 import { ErrorInformation } from '../shared/errors'
+import { getLogger } from '../shared/logger'
 
 export class Ec2ConnectionManager {
     private ssmClient: SsmClient
     private ec2Client: Ec2Client
     private iamClient: DefaultIamClient
 
+    private policyDocumentationUri = vscode.Uri.parse(
+        'https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-getting-started-instance-profile.html'
+    )
+
     public constructor(readonly regionCode: string) {
         this.ssmClient = this.createSsmSdkClient()
         this.ec2Client = this.createEc2SdkClient()
@@ -41,14 +46,27 @@ export class Ec2ConnectionManager {
         return new DefaultIamClient(this.regionCode)
     }
 
-    protected async getAttachedPolicies(instanceId: string): Promise<IAM.attachedPoliciesListType> {
+    protected async getAttachedPolicies(instanceId: string): Promise<IAM.AttachedPolicy[]> {
         const IamRole = await this.ec2Client.getAttachedIamRole(instanceId)
-        if (!IamRole) {
+        if (!IamRole?.Arn) {
             return []
         }
-        const iamResponse = await this.iamClient.listAttachedRolePolicies(IamRole!.Arn!)
-
-        return iamResponse.AttachedPolicies ?? []
+        try {
+            const attachedPolicies = await this.iamClient.listAttachedRolePolicies(IamRole.Arn)
+            return attachedPolicies
+        } catch (e) {
+            if (isAwsError(e) && e.code == 'NoSuchEntity') {
+                const errorMessage = `Attached role does not exist in IAM: ${IamRole.Arn}.`
+                getLogger().error(`ec2: ${errorMessage}`)
+                throw ToolkitError.chain(e, errorMessage, {
+                    code: e.code,
+                    documentationUri: this.policyDocumentationUri,
+                })
+            }
+            throw ToolkitError.chain(e as Error, `Failed to check policies for EC2 instance: ${instanceId}`, {
+                code: 'PolicyCheck',
+            })
+        }
     }
 
     public async hasProperPolicies(instanceId: string): Promise<boolean> {
@@ -63,11 +81,27 @@ export class Ec2ConnectionManager {
         return instanceStatus == 'running'
     }
 
-    private throwConnectionError(message: string, selection: Ec2Selection, errorInfo: ErrorInformation) {
+    protected throwConnectionError(message: string, selection: Ec2Selection, errorInfo: ErrorInformation) {
         const generalErrorMessage = `Unable to connect to target instance ${selection.instanceId} on region ${selection.region}. `
         throw new ToolkitError(generalErrorMessage + message, errorInfo)
     }
 
+    protected async throwPolicyError(selection: Ec2Selection) {
+        const role = await this.ec2Client.getAttachedIamRole(selection.instanceId)
+
+        const baseMessage = 'Ensure an IAM role with the required policies is attached to the instance.'
+        const messageExtension =
+            role && role.Arn
+                ? `Found attached role ${role.Arn}.`
+                : `Failed to find role attached to ${selection.instanceId}`
+        const fullMessage = `${baseMessage} ${messageExtension}`
+
+        this.throwConnectionError(fullMessage, selection, {
+            code: 'EC2SSMPermission',
+            documentationUri: this.policyDocumentationUri,
+        })
+    }
+
     public async checkForStartSessionError(selection: Ec2Selection): Promise<void> {
         const isInstanceRunning = await this.isInstanceRunning(selection.instanceId)
         const hasProperPolicies = await this.hasProperPolicies(selection.instanceId)
@@ -79,14 +113,7 @@ export class Ec2ConnectionManager {
         }
 
         if (!hasProperPolicies) {
-            const message = 'Ensure the IAM role attached to the instance has the required policies.'
-            const documentationUri = vscode.Uri.parse(
-                'https://docs.aws.amazon.com/systems-manager/latest/userguide/session-manager-getting-started-instance-profile.html'
-            )
-            this.throwConnectionError(message, selection, {
-                code: 'EC2SSMPermission',
-                documentationUri: documentationUri,
-            })
+            await this.throwPolicyError(selection)
         }
 
         if (!isSsmAgentRunning) {

src/shared/clients/iamClient.ts

@@ -77,11 +77,20 @@ export class DefaultIamClient {
         return tokens[tokens.length - 1]
     }
 
-    public async listAttachedRolePolicies(arn: string): Promise<IAM.ListAttachedRolePoliciesResponse> {
+    public async listAttachedRolePolicies(arn: string): Promise<IAM.AttachedPolicy[]> {
         const client = await this.createSdkClient()
         const roleName = this.getFriendlyName(arn)
-        const response = await client.listAttachedRolePolicies({ RoleName: roleName }).promise()
 
-        return response
+        const requester = async (request: IAM.ListAttachedRolePoliciesRequest) =>
+            client.listAttachedRolePolicies(request).promise()
+
+        const collection = pageableToCollection(requester, { RoleName: roleName }, 'Marker', 'AttachedPolicies')
+            .flatten()
+            .filter(p => p !== undefined)
+            .map(p => p!)
+
+        const policies = await collection.promise()
+
+        return policies
     }
 }

src/test/ec2/model.test.ts

@@ -4,13 +4,15 @@
  */
 
 import * as assert from 'assert'
+import * as sinon from 'sinon'
 import { Ec2ConnectErrorCode, Ec2ConnectionManager } from '../../ec2/model'
 import { SsmClient } from '../../shared/clients/ssmClient'
 import { Ec2Client } from '../../shared/clients/ec2Client'
 import { attachedPoliciesListType } from 'aws-sdk/clients/iam'
 import { Ec2Selection } from '../../ec2/utils'
 import { ToolkitError } from '../../shared/errors'
-import { EC2 } from 'aws-sdk'
+import { EC2, IAM } from 'aws-sdk'
+import { DefaultIamClient } from '../../shared/clients/iamClient'
 
 describe('Ec2ConnectClient', function () {
     class MockSsmClient extends SsmClient {
@@ -45,6 +47,16 @@ describe('Ec2ConnectClient', function () {
         protected override createEc2SdkClient(): Ec2Client {
             return new MockEc2Client()
         }
+
+        public async testGetAttachedPolicies(instanceId: string): Promise<IAM.attachedPoliciesListType> {
+            return await this.getAttachedPolicies(instanceId)
+        }
+
+        protected override async throwPolicyError(selection: Ec2Selection): Promise<void> {
+            this.throwConnectionError('', selection, {
+                code: 'EC2SSMPermission',
+            })
+        }
     }
 
     describe('isInstanceRunning', async function () {
@@ -164,6 +176,8 @@ describe('Ec2ConnectClient', function () {
                                 PolicyName: 'AmazonSSMManagedEC2InstanceDefaultPolicy',
                             },
                         ]
+                    case 'toolkitErrorInstance':
+                        throw new ToolkitError('', { code: 'NoSuchEntity' })
                     default:
                         return []
                 }
@@ -177,16 +191,54 @@ describe('Ec2ConnectClient', function () {
             let result: boolean
 
             result = await client.hasProperPolicies('firstInstance')
-            assert.strictEqual(false, result)
+            assert.strictEqual(result, false)
 
             result = await client.hasProperPolicies('secondInstance')
-            assert.strictEqual(true, result)
+            assert.strictEqual(result, true)
 
             result = await client.hasProperPolicies('thirdInstance')
-            assert.strictEqual(false, result)
+            assert.strictEqual(result, false)
 
             result = await client.hasProperPolicies('fourthInstance')
-            assert.strictEqual(false, result)
+            assert.strictEqual(result, false)
+        })
+
+        it('throws error when sdk throws error', async function () {
+            try {
+                await client.hasProperPolicies('toolkitErrorInstance')
+                assert.ok(false)
+            } catch {
+                assert.ok(true)
+            }
+        })
+    })
+
+    describe('getAttachedPolicies', async function () {
+        let client: MockEc2ConnectClient
+
+        before(async function () {
+            client = new MockEc2ConnectClient()
+        })
+
+        it('returns empty when IamRole not found', async function () {
+            sinon.stub(Ec2Client.prototype, 'getAttachedIamRole').resolves(undefined)
+            const response = await client.testGetAttachedPolicies('test-instance')
+            assert.deepStrictEqual(response, [])
+
+            sinon.restore()
+        })
+
+        it('throws error if IamRole is found but invalid', async function () {
+            sinon.stub(Ec2Client.prototype, 'getAttachedIamRole').resolves({ Arn: 'some-fake-role' })
+            sinon.stub(DefaultIamClient.prototype, 'listAttachedRolePolicies').throws('NoSuchEntity')
+            try {
+                await client.testGetAttachedPolicies('test-instance')
+                assert.ok(false)
+            } catch {
+                assert.ok(true)
+            } finally {
+                sinon.restore()
+            }
         })
     })
 })