Merge branch 'security-kubernetes-google-login-csrf' into 'master'

Validate session key when authorizing with GCP to create a cluster

Closes #2805

See merge request gitlab/gitlabhq!2902

app/controllers/google_api/authorizations_controller.rb

@@ -2,6 +2,10 @@
 
 module GoogleApi
   class AuthorizationsController < ApplicationController
+    include Gitlab::Utils::StrongMemoize
+
+    before_action :validate_session_key!
+
     def callback
       token, expires_at = GoogleApi::CloudPlatform::Client
         .new(nil, callback_google_api_auth_url)
@@ -11,21 +15,27 @@ module GoogleApi
       session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at] =
         expires_at.to_s
 
-      state_redirect_uri = redirect_uri_from_session_key(params[:state])
+      redirect_to redirect_uri_from_session
+    end
+
+    private
+
+    def validate_session_key!
+      access_denied! unless redirect_uri_from_session.present?
+    end
 
-      if state_redirect_uri
-        redirect_to state_redirect_uri
+    def redirect_uri_from_session
+      strong_memoize(:redirect_uri_from_session) do
+        if params[:state].present?
+          session[session_key_for_redirect_uri(params[:state])]
         else
-        redirect_to root_path
+          nil
+        end
       end
     end
 
-    private
-
-    def redirect_uri_from_session_key(state)
-      key = GoogleApi::CloudPlatform::Client
-        .session_key_for_redirect_uri(params[:state])
-      session[key] if key
+    def session_key_for_redirect_uri(state)
+      GoogleApi::CloudPlatform::Client.session_key_for_redirect_uri(state)
     end
   end
 end

changelogs/unreleased/security-kubernetes-google-login-csrf.yml

+---
+title: Validate session key when authorizing with GCP to create a cluster
+merge_request:
+author:
+type: security

spec/controllers/google_api/authorizations_controller_spec.rb

@@ -6,7 +6,7 @@ describe GoogleApi::AuthorizationsController do
     let(:token) { 'token' }
     let(:expires_at) { 1.hour.since.strftime('%s') }
 
-    subject { get :callback, params: { code: 'xxx', state: @state } }
+    subject { get :callback, params: { code: 'xxx', state: state } }
 
     before do
       sign_in(user)
@@ -15,24 +15,33 @@ describe GoogleApi::AuthorizationsController do
         .to receive(:get_token).and_return([token, expires_at])
     end
 
-    it 'sets token and expires_at in session' do
+    shared_examples_for 'access denied' do
+      it 'returns a 404' do
         subject
 
-      expect(session[GoogleApi::CloudPlatform::Client.session_key_for_token])
-        .to eq(token)
-      expect(session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at])
-        .to eq(expires_at)
+        expect(session[GoogleApi::CloudPlatform::Client.session_key_for_token]).to be_nil
+        expect(response).to have_http_status(:not_found)
+      end
     end
 
-    context 'when redirect uri key is stored in state' do
-      set(:project) { create(:project) }
-      let(:redirect_uri) { project_clusters_url(project).to_s }
+    context 'session key is present' do
+      let(:session_key) { 'session-key' }
+      let(:redirect_uri) { 'example.com' }
 
       before do
-        @state = GoogleApi::CloudPlatform::Client
-          .new_session_key_for_redirect_uri do |key|
-          session[key] = redirect_uri
+        session[GoogleApi::CloudPlatform::Client.session_key_for_redirect_uri(session_key)] = redirect_uri
       end
+
+      context 'session key matches state param' do
+        let(:state) { session_key }
+
+        it 'sets token and expires_at in session' do
+          subject
+
+          expect(session[GoogleApi::CloudPlatform::Client.session_key_for_token])
+            .to eq(token)
+          expect(session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at])
+            .to eq(expires_at)
         end
 
         it 'redirects to the URL stored in state param' do
@@ -40,10 +49,23 @@ describe GoogleApi::AuthorizationsController do
         end
       end
 
-    context 'when redirection url is not stored in state' do
-      it 'redirects to root_path' do
-        expect(subject).to redirect_to(root_path)
+      context 'session key does not match state param' do
+        let(:state) { 'bad-key' }
+
+        it_behaves_like 'access denied'
       end
+
+      context 'state param is blank' do
+        let(:state) { '' }
+
+        it_behaves_like 'access denied'
+      end
+    end
+
+    context 'state param is present, but session key is blank' do
+      let(:state) { 'session-key' }
+
+      it_behaves_like 'access denied'
     end
   end
 end