Persist EKS External ID before presenting it to the user

If the External ID can be manipulated, it is possible to
impersonate a user that was has authenticated with AWS in
the past but has since been deleted (which defeats the
uniqueness constraint on role_external_id).

app/controllers/clusters/clusters_controller.rb

@@ -38,8 +38,7 @@ class Clusters::ClustersController < Clusters::BaseController
 
   def new
     if params[:provider] == 'aws'
-      @aws_role = current_user.aws_role || Aws::Role.new
-      @aws_role.ensure_role_external_id!
+      @aws_role = Aws::Role.create_or_find_by!(user: current_user)
       @instance_types = load_instance_types.to_json
 
     elsif params[:provider] == 'gcp'
@@ -273,7 +272,7 @@ class Clusters::ClustersController < Clusters::BaseController
   end
 
   def aws_role_params
-    params.require(:cluster).permit(:role_arn, :role_external_id)
+    params.require(:cluster).permit(:role_arn)
   end
 
   def generate_gcp_authorize_url

app/models/aws/role.rb

@@ -9,6 +9,7 @@ module Aws
     validates :role_external_id, uniqueness: true, length: { in: 1..64 }
     validates :role_arn,
       length: 1..2048,
+      allow_nil: true,
       format: {
         with: Gitlab::Regex.aws_arn_regex,
         message: Gitlab::Regex.aws_arn_regex_message

app/services/clusters/aws/authorize_role_service.rb

@@ -9,6 +9,7 @@ module Clusters
 
       ERRORS = [
         ActiveRecord::RecordInvalid,
+        ActiveRecord::RecordNotFound,
         Clusters::Aws::FetchCredentialsService::MissingRoleError,
         ::Aws::Errors::MissingCredentialsError,
         ::Aws::STS::Errors::ServiceError
@@ -20,7 +21,8 @@ module Clusters
       end
 
       def execute
-        @role = create_or_update_role!
+        ensure_role_exists!
+        update_role_arn!
 
         Response.new(:ok, credentials)
       rescue *ERRORS => e
@@ -33,14 +35,12 @@ module Clusters
 
       attr_reader :role, :params
 
-      def create_or_update_role!
-        if role = user.aws_role
-          role.update!(params)
+      def ensure_role_exists!
+        @role = ::Aws::Role.find_by_user_id!(user.id)
+      end
 
-          role
-        else
-          user.create_aws_role!(params)
-        end
+      def update_role_arn!
+        role.update!(params)
       end
 
       def credentials

changelogs/unreleased/security-prevent-aws-external-id-manipulation.yml

+---
+title: Persist EKS External ID before presenting it to the user
+merge_request:
+author:
+type: security

db/migrate/20200717040735_change_aws_roles_role_arn_null.rb

+# frozen_string_literal: true
+
+class ChangeAwsRolesRoleArnNull < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+  EXAMPLE_ARN = 'arn:aws:iam::000000000000:role/example-role'
+
+  def up
+    change_column_null :aws_roles, :role_arn, true
+  end
+
+  def down
+    # Records may now exist with nulls, so we must fill them with a dummy value
+    change_column_null :aws_roles, :role_arn, false, EXAMPLE_ARN
+  end
+end

db/schema_migrations/20200717040735

+6b8fa09c9700c494eeb5151f43064f1656eaaea804742629b7bd66483e2b04cb
\ No newline at end of file

db/structure.sql

@@ -9514,7 +9514,7 @@ CREATE TABLE public.aws_roles (
     user_id integer NOT NULL,
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
-    role_arn character varying(2048) NOT NULL,
+    role_arn character varying(2048),
     role_external_id character varying(64) NOT NULL
 );
 

spec/controllers/admin/clusters_controller_spec.rb

@@ -99,7 +99,9 @@ RSpec.describe Admin::ClustersController do
   end
 
   describe 'GET #new' do
-    def get_new(provider: 'gcp')
+    let(:user) { admin }
+
+    def go(provider: 'gcp')
       get :new, params: { provider: provider }
     end
 
@@ -112,7 +114,7 @@ RSpec.describe Admin::ClustersController do
 
         context 'when selected provider is gke and no valid gcp token exists' do
           it 'redirects to gcp authorize_url' do
-            get_new
+            go
 
             expect(response).to redirect_to(assigns(:authorize_url))
           end
@@ -125,7 +127,7 @@ RSpec.describe Admin::ClustersController do
         end
 
         it 'does not have authorize_url' do
-          get_new
+          go
 
           expect(assigns(:authorize_url)).to be_nil
         end
@@ -137,7 +139,7 @@ RSpec.describe Admin::ClustersController do
         end
 
         it 'has new object' do
-          get_new
+          go
 
           expect(assigns(:gcp_cluster)).to be_an_instance_of(Clusters::ClusterPresenter)
         end
@@ -158,16 +160,18 @@ RSpec.describe Admin::ClustersController do
 
     describe 'functionality for existing cluster' do
       it 'has new object' do
-        get_new
+        go
 
         expect(assigns(:user_cluster)).to be_an_instance_of(Clusters::ClusterPresenter)
       end
     end
 
+    include_examples 'GET new cluster shared examples'
+
     describe 'security' do
-      it { expect { get_new }.to be_allowed_for(:admin) }
-      it { expect { get_new }.to be_denied_for(:user) }
-      it { expect { get_new }.to be_denied_for(:external) }
+      it { expect { go }.to be_allowed_for(:admin) }
+      it { expect { go }.to be_denied_for(:user) }
+      it { expect { go }.to be_denied_for(:external) }
     end
   end
 
@@ -424,14 +428,13 @@ RSpec.describe Admin::ClustersController do
   end
 
   describe 'POST authorize AWS role for EKS cluster' do
-    let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' }
-    let(:role_external_id) { '12345' }
+    let!(:role) { create(:aws_role, user: admin) }
 
+    let(:role_arn) { 'arn:new-role' }
     let(:params) do
       {
         cluster: {
-          role_arn: role_arn,
-          role_external_id: role_external_id
+          role_arn: role_arn
         }
       }
     end
@@ -445,28 +448,32 @@ RSpec.describe Admin::ClustersController do
         .and_return(double(execute: double))
     end
 
-    it 'creates an Aws::Role record' do
-      expect { go }.to change { Aws::Role.count }
+    it 'updates the associated role with the supplied ARN' do
+      go
 
       expect(response).to have_gitlab_http_status(:ok)
-
-      role = Aws::Role.last
-      expect(role.user).to eq admin
-      expect(role.role_arn).to eq role_arn
-      expect(role.role_external_id).to eq role_external_id
+      expect(role.reload.role_arn).to eq(role_arn)
     end
 
-    context 'role cannot be created' do
+    context 'supplied role is invalid' do
       let(:role_arn) { 'invalid-role' }
 
-      it 'does not create a record' do
-        expect { go }.not_to change { Aws::Role.count }
+      it 'does not update the associated role' do
+        expect { go }.not_to change { role.role_arn }
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
     end
 
     describe 'security' do
+      before do
+        allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service|
+          response = double(status: :ok, body: double)
+
+          allow(service).to receive(:execute).and_return(response)
+        end
+      end
+
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_denied_for(:user) }
       it { expect { go }.to be_denied_for(:external) }

spec/controllers/groups/clusters_controller_spec.rb

@@ -180,6 +180,8 @@ RSpec.describe Groups::ClustersController do
       end
     end
 
+    include_examples 'GET new cluster shared examples'
+
     describe 'security' do
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(group) }
@@ -493,14 +495,13 @@ RSpec.describe Groups::ClustersController do
   end
 
   describe 'POST authorize AWS role for EKS cluster' do
-    let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' }
-    let(:role_external_id) { '12345' }
+    let!(:role) { create(:aws_role, user: user) }
 
+    let(:role_arn) { 'arn:new-role' }
     let(:params) do
       {
         cluster: {
-          role_arn: role_arn,
-          role_external_id: role_external_id
+          role_arn: role_arn
         }
       }
     end
@@ -514,28 +515,32 @@ RSpec.describe Groups::ClustersController do
         .and_return(double(execute: double))
     end
 
-    it 'creates an Aws::Role record' do
-      expect { go }.to change { Aws::Role.count }
+    it 'updates the associated role with the supplied ARN' do
+      go
 
       expect(response).to have_gitlab_http_status(:ok)
-
-      role = Aws::Role.last
-      expect(role.user).to eq user
-      expect(role.role_arn).to eq role_arn
-      expect(role.role_external_id).to eq role_external_id
+      expect(role.reload.role_arn).to eq(role_arn)
     end
 
-    context 'role cannot be created' do
+    context 'supplied role is invalid' do
       let(:role_arn) { 'invalid-role' }
 
-      it 'does not create a record' do
-        expect { go }.not_to change { Aws::Role.count }
+      it 'does not update the associated role' do
+        expect { go }.not_to change { role.role_arn }
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
     end
 
     describe 'security' do
+      before do
+        allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service|
+          response = double(status: :ok, body: double)
+
+          allow(service).to receive(:execute).and_return(response)
+        end
+      end
+
       it { expect { go }.to be_allowed_for(:admin) }
       it { expect { go }.to be_allowed_for(:owner).of(group) }
       it { expect { go }.to be_allowed_for(:maintainer).of(group) }

spec/controllers/projects/clusters_controller_spec.rb

@@ -183,6 +183,8 @@ RSpec.describe Projects::ClustersController do
       end
     end
 
+    include_examples 'GET new cluster shared examples'
+
     describe 'security' do
       it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
         expect { go }.to be_allowed_for(:admin)
@@ -521,14 +523,13 @@ RSpec.describe Projects::ClustersController do
   end
 
   describe 'POST authorize AWS role for EKS cluster' do
-    let(:role_arn) { 'arn:aws:iam::123456789012:role/role-name' }
-    let(:role_external_id) { '12345' }
+    let!(:role) { create(:aws_role, user: user) }
 
+    let(:role_arn) { 'arn:new-role' }
     let(:params) do
       {
         cluster: {
-          role_arn: role_arn,
-          role_external_id: role_external_id
+          role_arn: role_arn
         }
       }
     end
@@ -542,28 +543,32 @@ RSpec.describe Projects::ClustersController do
         .and_return(double(execute: double))
     end
 
-    it 'creates an Aws::Role record' do
-      expect { go }.to change { Aws::Role.count }
+    it 'updates the associated role with the supplied ARN' do
+      go
 
       expect(response).to have_gitlab_http_status(:ok)
-
-      role = Aws::Role.last
-      expect(role.user).to eq user
-      expect(role.role_arn).to eq role_arn
-      expect(role.role_external_id).to eq role_external_id
+      expect(role.reload.role_arn).to eq(role_arn)
     end
 
-    context 'role cannot be created' do
+    context 'supplied role is invalid' do
       let(:role_arn) { 'invalid-role' }
 
-      it 'does not create a record' do
-        expect { go }.not_to change { Aws::Role.count }
+      it 'does not update the associated role' do
+        expect { go }.not_to change { role.role_arn }
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
     end
 
     describe 'security' do
+      before do
+        allow_next_instance_of(Clusters::Aws::AuthorizeRoleService) do |service|
+          response = double(status: :ok, body: double)
+
+          allow(service).to receive(:execute).and_return(response)
+        end
+      end
+
       it 'is allowed for admin when admin mode enabled', :enable_admin_mode do
         expect { go }.to be_allowed_for(:admin)
       end

spec/models/aws/role_spec.rb

@@ -29,6 +29,12 @@ RSpec.describe Aws::Role do
 
         it { is_expected.to be_truthy }
       end
+
+      context 'ARN is nil' do
+        let(:role_arn) { }
+
+        it { is_expected.to be_truthy }
+      end
     end
   end
 

spec/services/clusters/aws/authorize_role_service_spec.rb

@@ -3,47 +3,34 @@
 require 'spec_helper'
 
 RSpec.describe Clusters::Aws::AuthorizeRoleService do
-  let(:user) { create(:user) }
+  subject { described_class.new(user, params: params).execute }
+
+  let(:role) { create(:aws_role) }
+  let(:user) { role.user }
   let(:credentials) { instance_double(Aws::Credentials) }
   let(:credentials_service) { instance_double(Clusters::Aws::FetchCredentialsService, execute: credentials) }
 
+  let(:role_arn) { 'arn:my-role' }
   let(:params) do
     params = ActionController::Parameters.new({
       cluster: {
-        role_arn: 'arn:my-role',
-        role_external_id: 'external-id'
+        role_arn: role_arn
       }
     })
 
-    params.require(:cluster).permit(:role_arn, :role_external_id)
+    params.require(:cluster).permit(:role_arn)
   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
 
-  context 'role does not exist' do
-    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
-  end
-
-  context 'role already exists' do
-    let(:role) { create(:aws_role, user: user) }
-
+  context 'role exists' do
     it 'updates the existing Aws::Role record and returns a set of credentials' do
-      expect(role).to receive(:update!)
-        .with(params).and_call_original
-
       expect(subject.status).to eq(:ok)
       expect(subject.body).to eq(credentials)
+      expect(role.reload.role_arn).to eq(role_arn)
     end
   end
 
@@ -61,11 +48,14 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do
       end
     end
 
-    context 'cannot create role' do
-      before do
-        allow(user).to receive(:create_aws_role!)
-          .and_raise(ActiveRecord::RecordInvalid.new(user))
-      end
+    context 'role does not exist' do
+      let(:user) { create(:user) }
+
+      include_examples 'bad request'
+    end
+
+    context 'supplied ARN is invalid' do
+      let(:role_arn) { 'invalid' }
 
       include_examples 'bad request'
     end

spec/support/shared_examples/controllers/clusters_controller_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'GET new cluster shared examples' do
+  describe 'EKS cluster' do
+    context 'user already has an associated AWS role' do
+      let!(:role) { create(:aws_role, user: user) }
+
+      it 'does not create an Aws::Role record' do
+        expect { go(provider: 'aws') }.not_to change { Aws::Role.count }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(assigns(:aws_role)).to eq(role)
+      end
+    end
+
+    context 'user does not have an associated AWS role' do
+      it 'creates an Aws::Role record' do
+        expect { go(provider: 'aws') }.to change { Aws::Role.count }
+
+        expect(response).to have_gitlab_http_status(:ok)
+
+        role = assigns(:aws_role)
+        expect(role.user).to eq(user)
+        expect(role.role_arn).to be_nil
+        expect(role.role_external_id).to be_present
+      end
+    end
+  end
+end