Remove AWS proxy from EKS cluster form

Instead we can let the frontend handle these requests
by passing a set of temporary credentials with
restricted (read only) permissions.

app/controllers/application_controller.rb

@@ -228,10 +228,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def respond_201
-    head :created
-  end
-
   def respond_422
     head :unprocessable_entity
   end

app/controllers/clusters/clusters_controller.rb

@@ -8,7 +8,7 @@ class Clusters::ClustersController < Clusters::BaseController
   before_action :validate_gcp_token, only: [:new]
   before_action :gcp_cluster, only: [:new]
   before_action :user_cluster, only: [:new]
-  before_action :authorize_create_cluster!, only: [:new, :authorize_aws_role, :revoke_aws_role, :aws_proxy]
+  before_action :authorize_create_cluster!, only: [:new, :authorize_aws_role, :revoke_aws_role]
   before_action :authorize_update_cluster!, only: [:update]
   before_action :authorize_admin_cluster!, only: [:destroy, :clear_cache]
   before_action :update_applications_status, only: [:cluster_status]
@@ -145,9 +145,12 @@ class Clusters::ClustersController < Clusters::BaseController
   end
 
   def authorize_aws_role
-    role = current_user.build_aws_role(create_role_params)
+    response = Clusters::Aws::AuthorizeRoleService.new(
+      current_user,
+      params: create_role_params
+    ).execute
 
-    role.save ? respond_201 : respond_422
+    render json: response.body, status: response.status
   end
 
   def revoke_aws_role
@@ -156,15 +159,6 @@ class Clusters::ClustersController < Clusters::BaseController
     head :no_content
   end
 
-  def aws_proxy
-    response = Clusters::Aws::ProxyService.new(
-      current_user.aws_role,
-      params: params
-    ).execute
-
-    render json: response.body, status: response.status
-  end
-
   def clear_cache
     cluster.delete_cached_resources!
 

app/models/clusters/providers/aws.rb

@@ -8,9 +8,11 @@ module Clusters
 
       self.table_name = 'cluster_providers_aws'
 
+      DEFAULT_REGION = 'us-east-1'
+
       belongs_to :cluster, inverse_of: :provider_aws, class_name: 'Clusters::Cluster'
 
-      default_value_for :region, 'us-east-1'
+      default_value_for :region, DEFAULT_REGION
       default_value_for :num_nodes, 3
       default_value_for :instance_type, 'm5.large'
 

app/presenters/clusterable_presenter.rb

@@ -29,10 +29,6 @@ class ClusterablePresenter < Gitlab::View::Presenter::Delegated
     new_polymorphic_path([clusterable, :cluster], options)
   end
 
-  def aws_api_proxy_path(resource)
-    polymorphic_path([clusterable, :clusters], action: :aws_proxy, resource: resource)
-  end
-
   def authorize_aws_role_path
     polymorphic_path([clusterable, :clusters], action: :authorize_aws_role)
   end

app/presenters/instance_clusterable_presenter.rb

@@ -72,11 +72,6 @@ class InstanceClusterablePresenter < ClusterablePresenter
     revoke_aws_role_admin_clusters_path
   end
 
-  override :aws_api_proxy_path
-  def aws_api_proxy_path(resource)
-    aws_proxy_admin_clusters_path(resource: resource)
-  end
-
   override :empty_state_help_text
   def empty_state_help_text
     s_('ClusterIntegration|Adding an integration will share the cluster across all projects.')

app/services/clusters/aws/authorize_role_service.rb

+# frozen_string_literal: true
+
+module Clusters
+  module Aws
+    class AuthorizeRoleService
+      attr_reader :user
+
+      Response = Struct.new(:status, :body)
+
+      ERRORS = [
+        ActiveRecord::RecordInvalid,
+        Clusters::Aws::FetchCredentialsService::MissingRoleError,
+        ::Aws::Errors::MissingCredentialsError,
+        ::Aws::STS::Errors::ServiceError
+      ].freeze
+
+      def initialize(user, params:)
+        @user = user
+        @params = params
+      end
+
+      def execute
+        @role = create_role!
+
+        Response.new(:ok, credentials)
+      rescue *ERRORS
+        Response.new(:unprocessable_entity, {})
+      end
+
+      private
+
+      attr_reader :role, :params
+
+      def create_role!
+        user.create_aws_role!(params)
+      end
+
+      def credentials
+        Clusters::Aws::FetchCredentialsService.new(role).execute
+      end
+    end
+  end
+end

app/services/clusters/aws/fetch_credentials_service.rb

@@ -7,9 +7,8 @@ module Clusters
 
       MissingRoleError = Class.new(StandardError)
 
-      def initialize(provision_role, region:, provider: nil)
+      def initialize(provision_role, provider: nil)
         @provision_role = provision_role
-        @region = region
         @provider = provider
       end
 
@@ -20,13 +19,14 @@ module Clusters
           client: client,
           role_arn: provision_role.role_arn,
           role_session_name: session_name,
-          external_id: provision_role.role_external_id
+          external_id: provision_role.role_external_id,
+          policy: session_policy
         ).credentials
       end
 
       private
 
-      attr_reader :provider, :region
+      attr_reader :provider
 
       def client
         ::Aws::STS::Client.new(credentials: gitlab_credentials, region: region)
@@ -44,6 +44,26 @@ module Clusters
         Gitlab::CurrentSettings.eks_secret_access_key
       end
 
+      def region
+        provider&.region || Clusters::Providers::Aws::DEFAULT_REGION
+      end
+
+      ##
+      # If we haven't created a provider record yet,
+      # we restrict ourselves to read only access so
+      # that we can safely expose credentials to the
+      # frontend (to be used when populating the
+      # creation form).
+      def session_policy
+        if provider.nil?
+          File.read(read_only_policy)
+        end
+      end
+
+      def read_only_policy
+        Rails.root.join('vendor', 'aws', 'iam', "eks_cluster_read_only_policy.json")
+      end
+
       def session_name
         if provider.present?
           "gitlab-eks-cluster-#{provider.cluster_id}-user-#{provision_role.user_id}"

app/services/clusters/aws/proxy_service.rb

-# frozen_string_literal: true
-
-module Clusters
-  module Aws
-    class ProxyService
-      DEFAULT_REGION = 'us-east-1'
-
-      BadRequest = Class.new(StandardError)
-      Response = Struct.new(:status, :body)
-
-      def initialize(role, params:)
-        @role = role
-        @params = params
-      end
-
-      def execute
-        api_response = request_from_api!
-
-        Response.new(:ok, api_response.to_hash)
-      rescue *service_errors
-        Response.new(:bad_request, {})
-      end
-
-      private
-
-      attr_reader :role, :params
-
-      def request_from_api!
-        case requested_resource
-        when 'key_pairs'
-          ec2_client.describe_key_pairs
-
-        when 'instance_types'
-          instance_types
-
-        when 'roles'
-          iam_client.list_roles
-
-        when 'regions'
-          ec2_client.describe_regions
-
-        when 'security_groups'
-          raise BadRequest unless vpc_id.present?
-
-          ec2_client.describe_security_groups(vpc_filter)
-
-        when 'subnets'
-          raise BadRequest unless vpc_id.present?
-
-          ec2_client.describe_subnets(vpc_filter)
-
-        when 'vpcs'
-          ec2_client.describe_vpcs
-
-        else
-          raise BadRequest
-        end
-      end
-
-      def requested_resource
-        params[:resource]
-      end
-
-      def vpc_id
-        params[:vpc_id]
-      end
-
-      def region
-        params[:region] || DEFAULT_REGION
-      end
-
-      def vpc_filter
-        {
-          filters: [{
-            name: "vpc-id",
-            values: [vpc_id]
-          }]
-        }
-      end
-
-      ##
-      # Unfortunately the EC2 API doesn't provide a list of
-      # possible instance types. There is a workaround, using
-      # the Pricing API, but instead of requiring the
-      # user to grant extra permissions for this we use the
-      # values that validate the CloudFormation template.
-      def instance_types
-        {
-          instance_types: cluster_stack_instance_types.map { |type| Hash(instance_type_name: type) }
-        }
-      end
-
-      def cluster_stack_instance_types
-        YAML.safe_load(stack_template).dig('Parameters', 'NodeInstanceType', 'AllowedValues')
-      end
-
-      def stack_template
-        File.read(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml'))
-      end
-
-      def ec2_client
-        ::Aws::EC2::Client.new(client_options)
-      end
-
-      def iam_client
-        ::Aws::IAM::Client.new(client_options)
-      end
-
-      def credentials
-        Clusters::Aws::FetchCredentialsService.new(role, region: region).execute
-      end
-
-      def client_options
-        {
-          credentials: credentials,
-          region: region,
-          http_open_timeout: 5,
-          http_read_timeout: 10
-        }
-      end
-
-      def service_errors
-        [
-          BadRequest,
-          Clusters::Aws::FetchCredentialsService::MissingRoleError,
-          ::Aws::Errors::MissingCredentialsError,
-          ::Aws::EC2::Errors::ServiceError,
-          ::Aws::IAM::Errors::ServiceError,
-          ::Aws::STS::Errors::ServiceError
-        ]
-      end
-    end
-  end
-end

app/views/clusters/clusters/aws/_new.html.haml

@@ -7,13 +7,6 @@
     'create-role-path' => clusterable.authorize_aws_role_path,
     'sign-out-path' => clusterable.revoke_aws_role_path,
     'create-cluster-path' => clusterable.create_aws_clusters_path,
-    'get-roles-path' => clusterable.aws_api_proxy_path('roles'),
-    'get-regions-path' => clusterable.aws_api_proxy_path('regions'),
-    'get-key-pairs-path' => clusterable.aws_api_proxy_path('key_pairs'),
-    'get-vpcs-path' => clusterable.aws_api_proxy_path('vpcs'),
-    'get-subnets-path' => clusterable.aws_api_proxy_path('subnets'),
-    'get-security-groups-path' => clusterable.aws_api_proxy_path('security_groups'),
-    'get-instance-types-path' => clusterable.aws_api_proxy_path('instance_types'),
     'account-id' => Gitlab::CurrentSettings.eks_account_id,
     'external-id' => @aws_role.role_external_id,
     'kubernetes-integration-help-path' => help_page_path('user/project/clusters/index'),

config/routes.rb

@@ -146,10 +146,6 @@ Rails.application.routes.draw do
         post :create_aws
         post :authorize_aws_role
         delete :revoke_aws_role
-
-        scope :aws do
-          get 'api/:resource', to: 'clusters#aws_proxy', as: :aws_proxy
-        end
       end
 
       member do

spec/controllers/admin/clusters_controller_spec.rb

@@ -381,10 +381,15 @@ describe Admin::ClustersController do
       post :authorize_aws_role, params: params
     end
 
+    before do
+      allow(Clusters::Aws::FetchCredentialsService).to receive(:new)
+        .and_return(double(execute: double))
+    end
+
     it 'creates an Aws::Role record' do
       expect { go }.to change { Aws::Role.count }
 
-      expect(response.status).to eq 201
+      expect(response.status).to eq 200
 
       role = Aws::Role.last
       expect(role.user).to eq admin

spec/controllers/groups/clusters_controller_spec.rb

@@ -443,10 +443,15 @@ describe Groups::ClustersController do
       post :authorize_aws_role, params: params.merge(group_id: group)
     end
 
+    before do
+      allow(Clusters::Aws::FetchCredentialsService).to receive(:new)
+        .and_return(double(execute: double))
+    end
+
     it 'creates an Aws::Role record' do
       expect { go }.to change { Aws::Role.count }
 
-      expect(response.status).to eq 201
+      expect(response.status).to eq 200
 
       role = Aws::Role.last
       expect(role.user).to eq user

spec/controllers/projects/clusters_controller_spec.rb

@@ -444,10 +444,15 @@ describe Projects::ClustersController do
       post :authorize_aws_role, params: params.merge(namespace_id: project.namespace, project_id: project)
     end
 
+    before do
+      allow(Clusters::Aws::FetchCredentialsService).to receive(:new)
+        .and_return(double(execute: double))
+    end
+
     it 'creates an Aws::Role record' do
       expect { go }.to change { Aws::Role.count }
 
-      expect(response.status).to eq 201
+      expect(response.status).to eq 200
 
       role = Aws::Role.last
       expect(role.user).to eq user

spec/presenters/instance_clusterable_presenter_spec.rb

@@ -27,14 +27,6 @@ describe InstanceClusterablePresenter do
     it { is_expected.to eq(revoke_aws_role_admin_clusters_path) }
   end
 
-  describe '#aws_api_proxy_path' do
-    let(:resource) { 'resource' }
-
-    subject { described_class.new(instance).aws_api_proxy_path(resource) }
-
-    it { is_expected.to eq(aws_proxy_admin_clusters_path(resource: resource)) }
-  end
-
   describe '#clear_cluster_cache_path' do
     subject { presenter.clear_cluster_cache_path(cluster) }
 

spec/services/clusters/aws/authorize_role_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Clusters::Aws::AuthorizeRoleService do
+  let(:user) { create(:user) }
+  let(:credentials) { instance_double(Aws::Credentials) }
+  let(:credentials_service) { instance_double(Clusters::Aws::FetchCredentialsService, execute: credentials) }
+
+  let(:params) do
+    params = ActionController::Parameters.new({
+      cluster: {
+        role_arn: 'arn:my-role',
+        role_external_id: 'external-id'
+      }
+    })
+
+    params.require(:cluster).permit(:role_arn, :role_external_id)
+  end
+
+  subject { described_class.new(user, params: params).execute }
+
+  before do
+    allow(Clusters::Aws::FetchCredentialsService).to receive(:new)
+      .with(instance_of(Aws::Role)).and_return(credentials_service)
+  end
+
+  it 'creates an Aws::Role record and returns a set of credentials' do
+    expect(user).to receive(:create_aws_role!)
+      .with(params).and_call_original
+
+    expect(subject.status).to eq(:ok)
+    expect(subject.body).to eq(credentials)
+  end
+
+  context 'errors' do
+    shared_examples 'bad request' do
+      it 'returns an empty hash' do
+        expect(subject.status).to eq(:unprocessable_entity)
+        expect(subject.body).to eq({})
+      end
+    end
+
+    context 'cannot create role' do
+      before do
+        allow(user).to receive(:create_aws_role!)
+          .and_raise(ActiveRecord::RecordInvalid.new(user))
+      end
+
+      include_examples 'bad request'
+    end
+
+    context 'client errors' do
+      before do
+        allow(credentials_service).to receive(:execute).and_raise(error)
+      end
+
+      context 'error fetching credentials' do
+        let(:error) { Aws::STS::Errors::ServiceError.new(nil, 'error message') }
+
+        include_examples 'bad request'
+      end
+
+      context 'credentials not configured' do
+        let(:error) { Aws::Errors::MissingCredentialsError.new('error message') }
+
+        include_examples 'bad request'
+      end
+
+      context 'role not configured' do
+        let(:error) { Clusters::Aws::FetchCredentialsService::MissingRoleError.new('error message') }
+
+        include_examples 'bad request'
+      end
+    end
+  end
+end

spec/services/clusters/aws/fetch_credentials_service_spec.rb

@@ -5,19 +5,18 @@ require 'spec_helper'
 describe Clusters::Aws::FetchCredentialsService do
   describe '#execute' do
     let(:user) { create(:user) }
-    let(:provider) { create(:cluster_provider_aws) }
+    let(:provider) { create(:cluster_provider_aws, region: 'ap-southeast-2') }
 
     let(:gitlab_access_key_id) { 'gitlab-access-key-id' }
     let(:gitlab_secret_access_key) { 'gitlab-secret-access-key' }
 
-    let(:region) { 'us-east-1' }
     let(:gitlab_credentials) { Aws::Credentials.new(gitlab_access_key_id, gitlab_secret_access_key) }
     let(:sts_client) { Aws::STS::Client.new(credentials: gitlab_credentials, region: region) }
     let(:assumed_role) { instance_double(Aws::AssumeRoleCredentials, credentials: assumed_role_credentials) }
 
     let(:assumed_role_credentials) { double }
 
-    subject { described_class.new(provision_role, region: region, provider: provider).execute }
+    subject { described_class.new(provision_role, provider: provider).execute }
 
     context 'provision role is configured' do
       let(:provision_role) { create(:aws_role, user: user) }
@@ -39,19 +38,30 @@ describe Clusters::Aws::FetchCredentialsService do
             client: sts_client,
             role_arn: provision_role.role_arn,
             role_session_name: session_name,
-            external_id: provision_role.role_external_id
+            external_id: provision_role.role_external_id,
+            policy: session_policy
           ).and_return(assumed_role)
       end
 
       context 'provider is specified' do
+        let(:region) { provider.region }
         let(:session_name) { "gitlab-eks-cluster-#{provider.cluster_id}-user-#{user.id}" }
+        let(:session_policy) { nil }
 
         it { is_expected.to eq assumed_role_credentials }
       end
 
       context 'provider is not specifed' do
         let(:provider) { nil }
+        let(:region) { Clusters::Providers::Aws::DEFAULT_REGION }
         let(:session_name) { "gitlab-eks-autofill-user-#{user.id}" }
+        let(:session_policy) { 'policy-document' }
+
+        before do
+          allow(File).to receive(:read)
+            .with(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json'))
+            .and_return(session_policy)
+        end
 
         it { is_expected.to eq assumed_role_credentials }
       end

spec/services/clusters/aws/proxy_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Clusters::Aws::ProxyService do
-  let(:role) { create(:aws_role) }
-  let(:credentials) { instance_double(Aws::Credentials) }
-  let(:client_instance) { instance_double(client) }
-
-  let(:region) { 'region' }
-  let(:vpc_id) { }
-  let(:params) do
-    ActionController::Parameters.new({
-      resource: resource,
-      region: region,
-      vpc_id: vpc_id
-    })
-  end
-
-  subject { described_class.new(role, params: params).execute }
-
-  context 'external resources' do
-    before do
-      allow(Clusters::Aws::FetchCredentialsService).to receive(:new) do
-        double(execute: credentials)
-      end
-
-      allow(client).to receive(:new)
-        .with(
-          credentials: credentials, region: region,
-          http_open_timeout: 5, http_read_timeout: 10)
-        .and_return(client_instance)
-    end
-
-    shared_examples 'bad request' do
-      it 'returns an empty hash' do
-        expect(subject.status).to eq :bad_request
-        expect(subject.body).to eq({})
-      end
-    end
-
-    describe 'key_pairs' do
-      let(:client) { Aws::EC2::Client }
-      let(:resource) { 'key_pairs' }
-      let(:response) { double(to_hash: :key_pairs) }
-
-      it 'requests a list of key pairs' do
-        expect(client_instance).to receive(:describe_key_pairs).once.and_return(response)
-        expect(subject.status).to eq :ok
-        expect(subject.body).to eq :key_pairs
-      end
-    end
-
-    describe 'roles' do
-      let(:client) { Aws::IAM::Client }
-      let(:resource) { 'roles' }
-      let(:response) { double(to_hash: :roles) }
-
-      it 'requests a list of roles' do
-        expect(client_instance).to receive(:list_roles).once.and_return(response)
-        expect(subject.status).to eq :ok
-        expect(subject.body).to eq :roles
-      end
-    end
-
-    describe 'regions' do
-      let(:client) { Aws::EC2::Client }
-      let(:resource) { 'regions' }
-      let(:response) { double(to_hash: :regions) }
-
-      it 'requests a list of regions' do
-        expect(client_instance).to receive(:describe_regions).once.and_return(response)
-        expect(subject.status).to eq :ok
-        expect(subject.body).to eq :regions
-      end
-    end
-
-    describe 'security_groups' do
-      let(:client) { Aws::EC2::Client }
-      let(:resource) { 'security_groups' }
-      let(:response) { double(to_hash: :security_groups) }
-
-      include_examples 'bad request'
-
-      context 'VPC is specified' do
-        let(:vpc_id) { 'vpc-1' }
-
-        it 'requests a list of security groups for a VPC' do
-          expect(client_instance).to receive(:describe_security_groups).once
-            .with(filters: [{ name: 'vpc-id', values: [vpc_id] }])
-            .and_return(response)
-          expect(subject.status).to eq :ok
-          expect(subject.body).to eq :security_groups
-        end
-      end
-    end
-
-    describe 'subnets' do
-      let(:client) { Aws::EC2::Client }
-      let(:resource) { 'subnets' }
-      let(:response) { double(to_hash: :subnets) }
-
-      include_examples 'bad request'
-
-      context 'VPC is specified' do
-        let(:vpc_id) { 'vpc-1' }
-
-        it 'requests a list of subnets for a VPC' do
-          expect(client_instance).to receive(:describe_subnets).once
-            .with(filters: [{ name: 'vpc-id', values: [vpc_id] }])
-            .and_return(response)
-          expect(subject.status).to eq :ok
-          expect(subject.body).to eq :subnets
-        end
-      end
-    end
-
-    describe 'vpcs' do
-      let(:client) { Aws::EC2::Client }
-      let(:resource) { 'vpcs' }
-      let(:response) { double(to_hash: :vpcs) }
-
-      it 'requests a list of VPCs' do
-        expect(client_instance).to receive(:describe_vpcs).once.and_return(response)
-        expect(subject.status).to eq :ok
-        expect(subject.body).to eq :vpcs
-      end
-    end
-
-    context 'errors' do
-      let(:client) { Aws::EC2::Client }
-
-      context 'unknown resource' do
-        let(:resource) { 'instances' }
-
-        include_examples 'bad request'
-      end
-
-      context 'client and configuration errors' do
-        let(:resource) { 'vpcs' }
-
-        before do
-          allow(client_instance).to receive(:describe_vpcs).and_raise(error)
-        end
-
-        context 'error fetching credentials' do
-          let(:error) { Aws::STS::Errors::ServiceError.new(nil, 'error message') }
-
-          include_examples 'bad request'
-        end
-
-        context 'credentials not configured' do
-          let(:error) { Aws::Errors::MissingCredentialsError.new('error message') }
-
-          include_examples 'bad request'
-        end
-
-        context 'role not configured' do
-          let(:error) { Clusters::Aws::FetchCredentialsService::MissingRoleError.new('error message') }
-
-          include_examples 'bad request'
-        end
-
-        context 'EC2 error' do
-          let(:error) { Aws::EC2::Errors::ServiceError.new(nil, 'error message') }
-
-          include_examples 'bad request'
-        end
-
-        context 'IAM error' do
-          let(:error) { Aws::IAM::Errors::ServiceError.new(nil, 'error message') }
-
-          include_examples 'bad request'
-        end
-
-        context 'STS error' do
-          let(:error) { Aws::STS::Errors::ServiceError.new(nil, 'error message') }
-
-          include_examples 'bad request'
-        end
-      end
-    end
-  end
-
-  context 'local resources' do
-    describe 'instance_types' do
-      let(:resource) { 'instance_types' }
-      let(:cloudformation_template) { double }
-      let(:instance_types) { double(dig: %w(t3.small)) }
-
-      before do
-        allow(File).to receive(:read)
-          .with(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml'))
-          .and_return(cloudformation_template)
-
-        allow(YAML).to receive(:safe_load)
-          .with(cloudformation_template)
-          .and_return(instance_types)
-      end
-
-      it 'returns a list of instance types' do
-        expect(subject.status).to eq :ok
-        expect(subject.body).to have_key(:instance_types)
-        expect(subject.body[:instance_types]).to match_array([
-          instance_type_name: 't3.small'
-        ])
-      end
-    end
-  end
-end

vendor/aws/iam/eks_cluster_read_only_policy.json

+{
+  "Version": "2012-10-17",
+  "Statement": [
+      {
+          "Effect": "Allow",
+          "Action": [
+              "iam:ListRoles",
+              "ec2:DescribeKeyPairs",
+              "ec2:DescribeRegions",
+              "ec2:DescribeSecurityGroups",
+              "ec2:DescribeSubnets",
+              "ec2:DescribeVpcs"
+          ],
+          "Resource": "*"
+      }
+  ]
+}