Merge branch 'cat-refactor-geo-signing-data' into 'master'

Refactor Geo signing data logic

See merge request gitlab-org/gitlab!83413

ee/app/models/geo_node.rb

@@ -53,6 +53,7 @@ class GeoNode < ApplicationRecord
   scope :secondary_nodes, -> { where(primary: false) }
   scope :name_in, -> (names) { where(name: names) }
   scope :ordered, -> { order(:id) }
+  scope :enabled, -> { where(enabled: true) }
 
   attr_encrypted :secret_access_key,
                  key: Settings.attr_encrypted_db_key_base_32,

ee/lib/gitlab/geo/base_request.rb

@@ -29,14 +29,9 @@ module Gitlab
       private
 
       def geo_auth_token(message)
-        geo_node = requesting_node
-        raise GeoNodeNotFoundError unless geo_node
+        signed_data = Gitlab::Geo::SignedData.new(geo_node: requesting_node).sign_and_encode_data(message)
 
-        token = JSONWebToken::HMACToken.new(geo_node.secret_access_key)
-        token.expire_time = Time.now + expiration_time
-        token[:data] = message.to_json
-
-        "#{GITLAB_GEO_AUTH_TOKEN_TYPE} #{geo_node.access_key}:#{token.encoded}"
+        "#{GITLAB_GEO_AUTH_TOKEN_TYPE} #{signed_data}"
       end
 
       def requesting_node

ee/lib/gitlab/geo/jwt_request_decoder.rb

@@ -6,8 +6,6 @@ module Gitlab
       include LogHelpers
       include ::Gitlab::Utils::StrongMemoize
 
-      IAT_LEEWAY = 60.seconds.to_i
-
       def self.geo_auth_attempt?(header)
         token_type, = header&.split(' ', 2)
         token_type == ::Gitlab::Geo::BaseRequest::GITLAB_GEO_AUTH_TOKEN_TYPE
@@ -54,60 +52,16 @@ module Gitlab
         # For example:
         # JWT payload = { "data": { "oid": "12345" }, iat: 123456 }
         #
-        begin
-          data = decode_auth_header
-        rescue OpenSSL::Cipher::CipherError
-          message = 'Error decrypting the Geo secret from the database. Check that the primary and secondary have the same db_key_base.'
-          log_error(message)
-          raise InvalidDecryptionKeyError, message
-        end
-
-        return unless data.present?
-
-        secret, encoded_message = data
-
-        begin
-          decoded = JWT.decode(
-            encoded_message,
-            secret,
-            true,
-            { leeway: IAT_LEEWAY, verify_iat: true, algorithm: 'HS256' }
-          )
-
-          message = decoded.first
-          data = Gitlab::Json.parse(message['data']) if message
-          data&.deep_symbolize_keys!
-          data
-        rescue JWT::ImmatureSignature, JWT::ExpiredSignature
-          message = "Signature not within leeway of #{IAT_LEEWAY} seconds. Check your system clocks!"
-          log_error(message)
-          raise InvalidSignatureTimeError, message
-        rescue JWT::DecodeError => e
-          log_error("Error decoding Geo request: #{e}")
-          nil
-        end
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def hmac_secret(access_key)
-        node_params = { access_key: access_key }
-
-        # By default, we fail authorization for requests from disabled nodes because
-        # it is a convenient place to block nearly all requests from disabled
-        # secondaries. The `include_disabled` option can safely override this
-        # check for `enabled`.
-        #
-        # A request is authorized if the access key in the Authorization header
-        # matches the access key of the requesting node, **and** the decoded data
-        # matches the requested resource.
-        node_params[:enabled] = true unless @include_disabled
+        signed_data = parse_auth_header
 
-        @geo_node ||= GeoNode.find_by(node_params)
-        @geo_node&.secret_access_key
+        Gitlab::Geo::SignedData.new(include_disabled_nodes: @include_disabled).decode_data(signed_data)
+      rescue OpenSSL::Cipher::CipherError
+        message = 'Error decrypting the Geo secret from the database. Check that the primary and secondary have the same db_key_base.'
+        log_error(message)
+        raise InvalidDecryptionKeyError, message
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
-      def decode_auth_header
+      def parse_auth_header
         return unless auth_header.present?
 
         tokens = auth_header.split(' ')
@@ -115,18 +69,7 @@ module Gitlab
         return unless tokens.count == 2
         return unless tokens[0] == Gitlab::Geo::BaseRequest::GITLAB_GEO_AUTH_TOKEN_TYPE
 
-        # Split at the first occurrence of a colon
-        geo_tokens = tokens[1].split(':', 2)
-
-        return unless geo_tokens.count == 2
-
-        access_key = geo_tokens[0]
-        encoded_message = geo_tokens[1]
-        secret = hmac_secret(access_key)
-
-        return unless secret.present?
-
-        [secret, encoded_message]
+        tokens[1]
       end
     end
   end

ee/lib/gitlab/geo/signed_data.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    class SignedData
+      include LogHelpers
+
+      VALIDITY_PERIOD = 1.minute
+      IAT_LEEWAY = 60.seconds.to_i
+
+      attr_reader :geo_node
+
+      def initialize(geo_node: nil, validity_period: VALIDITY_PERIOD, include_disabled_nodes: false)
+        @geo_node = geo_node
+        @validity_period = validity_period
+        @include_disabled_nodes = include_disabled_nodes
+      end
+
+      def sign_and_encode_data(data)
+        raise ::Gitlab::Geo::GeoNodeNotFoundError unless @geo_node
+
+        token = JSONWebToken::HMACToken.new(@geo_node.secret_access_key)
+        token.expire_time = Time.zone.now + @validity_period
+        token[:data] = data.to_json
+
+        "#{@geo_node.access_key}:#{token.encoded}"
+      end
+
+      def decode_data(signed_data)
+        return unless signed_data.present?
+
+        parse_data(signed_data)
+
+        return unless @secret_access_key.present? && @encoded_message.present?
+
+        decoded = JWT.decode(
+          @encoded_message,
+          @secret_access_key,
+          true,
+          { leeway: IAT_LEEWAY, verify_iat: true, algorithm: 'HS256' }
+        )
+
+        message = decoded.first
+        data = Gitlab::Json.parse(message['data']) if message
+        data&.deep_symbolize_keys!
+        data
+      rescue JWT::ImmatureSignature, JWT::ExpiredSignature
+        message = "Signature not within leeway of #{IAT_LEEWAY} seconds. Check your system clocks!"
+        log_error(message)
+        raise InvalidSignatureTimeError, message
+      rescue JWT::DecodeError => e
+        log_error("Error decoding Geo request: #{e}")
+        nil
+      end
+
+      private
+
+      def parse_data(signed_data)
+        geo_tokens = signed_data.split(':', 2)
+
+        return unless geo_tokens.count == 2
+
+        @access_key = geo_tokens[0]
+        @encoded_message = geo_tokens[1]
+        @secret_access_key = hmac_secret
+      end
+
+      def hmac_secret
+        # By default, we fail authorization for requests from disabled nodes because
+        # it is a convenient place to block nearly all requests from disabled
+        # secondaries. The `include_disabled_nodes` option can safely override this
+        # check for `enabled`.
+        #
+        # A request is authorized if the access key in the Authorization header
+        # matches the access key of the requesting node, **and** the decoded data
+        # matches the requested resource.
+        scoped_geo_nodes = if @include_disabled_nodes
+                             GeoNode.all
+                           else
+                             GeoNode.enabled
+                           end
+
+        @geo_node ||= scoped_geo_nodes.find_by_access_key(@access_key)
+        @geo_node&.secret_access_key
+      end
+    end
+  end
+end

ee/lib/system_check/geo/clocks_synchronization_check.rb

@@ -6,7 +6,7 @@ module SystemCheck
       set_name 'Machine clock is synchronized'
 
       def check?
-        Net::NTP.get.offset.abs < Gitlab::Geo::JwtRequestDecoder::IAT_LEEWAY
+        Net::NTP.get.offset.abs < Gitlab::Geo::SignedData::IAT_LEEWAY
       end
 
       def show_error

ee/spec/lib/ee/gitlab/auth/auth_finders_spec.rb

@@ -65,7 +65,7 @@ RSpec.describe EE::Gitlab::Auth::AuthFinders do
 
       it 'does not authenticate with invalid decryption key error' do
         allow_next_instance_of(::Gitlab::Geo::JwtRequestDecoder) do |instance|
-          expect(instance).to receive(:decode_auth_header).and_raise(Gitlab::Geo::InvalidDecryptionKeyError)
+          expect(instance).to receive(:decode).and_raise(Gitlab::Geo::InvalidDecryptionKeyError)
         end
 
         expect { subject }.to raise_error(::Gitlab::Auth::UnauthorizedError)

ee/spec/lib/gitlab/geo/jwt_request_decoder_spec.rb

@@ -60,12 +60,23 @@ RSpec.describe Gitlab::Geo::JwtRequestDecoder do
       travel_to(2.minutes.ago) { expect { subject.decode }.to raise_error(Gitlab::Geo::InvalidSignatureTimeError) }
     end
 
-    it 'raises invalid decryption key error' do
-      allow_next_instance_of(described_class) do |instance|
-        allow(instance).to receive(:decode_auth_header).and_raise(Gitlab::Geo::InvalidDecryptionKeyError)
+    context 'surfaces raised errors' do
+      where(:raised_error, :expected_error) do
+        [
+          [Gitlab::Geo::InvalidDecryptionKeyError, Gitlab::Geo::InvalidDecryptionKeyError],
+          [OpenSSL::Cipher::CipherError, Gitlab::Geo::InvalidDecryptionKeyError]
+        ]
       end
 
-      expect { subject.decode }.to raise_error(Gitlab::Geo::InvalidDecryptionKeyError)
+      with_them do
+        it 'raises expected error' do
+          allow_next_instance_of(Gitlab::Geo::SignedData) do |instance|
+            allow(instance).to receive(:decode_data).and_raise(raised_error)
+          end
+
+          expect { subject.decode }.to raise_error(expected_error)
+        end
+      end
     end
   end
 

ee/spec/lib/gitlab/geo/signed_data_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Geo::SignedData do
+  using RSpec::Parameterized::TableSyntax
+  include EE::GeoHelpers
+
+  let_it_be_with_reload(:geo_node) { create(:geo_node) }
+
+  let(:validity_period) { 42.minutes }
+  let(:include_disabled_nodes) { false }
+  let(:klass_args) { {} }
+  let(:data) { { input: 123, other_input: 'string value' } }
+  let(:signed_data) { described_class.new(geo_node: geo_node).sign_and_encode_data(data) }
+
+  before do
+    stub_current_geo_node(geo_node)
+  end
+
+  describe '#sign_and_encode_data' do
+    subject(:returned_data) { described_class.new(**klass_args).sign_and_encode_data(data) }
+
+    let(:klass_args) { { geo_node: geo_node } }
+    let(:parsed_access_key) { returned_data.split(':').first }
+    let(:jwt) { JWT.decode(returned_data.split(':').second, geo_node.secret_access_key) }
+    let(:decoded_data) { Gitlab::Json.parse(jwt.first['data']) }
+
+    context 'when data is not set' do
+      let(:data) { nil }
+
+      it 'does not set the data attribute' do
+        expect(decoded_data).to be_nil
+      end
+    end
+
+    context 'when geo_node is not set' do
+      let(:geo_node) { nil }
+
+      it 'raises a GeoNodeNotFoundError error' do
+        expect { subject }.to raise_error(::Gitlab::Geo::GeoNodeNotFoundError)
+      end
+    end
+
+    it 'formats the signed data properly' do
+      expect(parsed_access_key).to eq(geo_node.access_key)
+
+      decoded_data.deep_symbolize_keys!
+      expect(decoded_data).to eq(data)
+    end
+
+    it 'defaults to 1-minute expiration time' do
+      freeze_time do
+        expect(jwt.first['exp']).to eq((Time.zone.now + 1.minute).to_i)
+      end
+    end
+
+    context 'with custom validity period' do
+      let(:klass_args) { { geo_node: geo_node, validity_period: 42.minutes } }
+
+      it 'uses that expiration time' do
+        freeze_time do
+          expect(jwt.first['exp']).to eq((Time.zone.now + 42.minutes).to_i)
+        end
+      end
+    end
+  end
+
+  describe '#decode_data' do
+    subject(:returned_data) { described_class.new(**klass_args).decode_data(signed_data) }
+
+    it { is_expected.to eq(data) }
+
+    context 'when data is not set' do
+      let(:signed_data) { nil }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'for disabled nodes' do
+      before(:all) do
+        geo_node.update_attribute(:enabled, false)
+      end
+
+      after(:all) do
+        geo_node.update_attribute(:enabled, true)
+      end
+
+      context 'fails to decode for disabled nodes by default' do
+        it { is_expected.to be_nil }
+      end
+
+      context 'when include_disabled_nodes is set to false' do
+        let(:klass_args) { { include_disabled_nodes: false } }
+
+        it { is_expected.to be_nil }
+      end
+
+      context 'when include_disabled_nodes is set to true' do
+        let(:klass_args) { { include_disabled_nodes: true } }
+
+        it { is_expected.to eq(data) }
+      end
+    end
+
+    context 'with the wrong key' do
+      before do
+        # ensure this generated before changing the key
+        signed_data
+
+        geo_node.update_attribute(:secret_access_key, 'invalid')
+      end
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'time checks' do
+      before do
+        signed_data
+      end
+
+      it 'successfully decodes when clocks are off by IAT leeway' do
+        travel_to(30.seconds.ago) { expect(subject).to eq(data) }
+      end
+
+      it 'raises an error after expiring' do
+        travel_to(2.minutes.from_now) { expect { subject }.to raise_error(Gitlab::Geo::InvalidSignatureTimeError) }
+      end
+
+      it 'raises an error when clocks are not in sync' do
+        travel_to(2.minutes.ago) { expect { subject }.to raise_error(Gitlab::Geo::InvalidSignatureTimeError) }
+      end
+    end
+
+    context 'JWT raised errors' do
+      context 'surfaces expected errors' do
+        where(:raised_error, :expected_error) do
+          JWT::ImmatureSignature | Gitlab::Geo::InvalidSignatureTimeError
+          JWT::ExpiredSignature  | Gitlab::Geo::InvalidSignatureTimeError
+        end
+
+        with_them do
+          it 'raises expected error' do
+            expect(JWT).to receive(:decode).and_raise(raised_error)
+
+            expect { subject }.to raise_error(expected_error)
+          end
+        end
+      end
+
+      context 'for a decoding error' do
+        before do
+          allow(JWT).to receive(:decode).and_raise(JWT::DecodeError)
+        end
+
+        it { is_expected.to be_nil }
+      end
+    end
+  end
+end