Merge branch 'hardening_prevent_encrypted_values_from_serializable_hash' into 'master'

Exclude sensitive attributes in serializable_hash by default

See merge request gitlab-org/gitlab!81773

app/models/application_record.rb

@@ -5,6 +5,7 @@ class ApplicationRecord < ActiveRecord::Base
   include Transactions
   include LegacyBulkInsert
   include CrossDatabaseModification
+  include SensitiveSerializableHash
 
   self.abstract_class = true
 

app/models/concerns/sensitive_serializable_hash.rb

+# frozen_string_literal: true
+
+module SensitiveSerializableHash
+  extend ActiveSupport::Concern
+
+  included do
+    class_attribute :attributes_exempt_from_serializable_hash, default: []
+  end
+
+  class_methods do
+    def prevent_from_serialization(*keys)
+      self.attributes_exempt_from_serializable_hash ||= []
+      self.attributes_exempt_from_serializable_hash.concat keys
+    end
+  end
+
+  # Override serializable_hash to exclude sensitive attributes by default
+  #
+  # In general, prefer NOT to use serializable_hash / to_json / as_json in favor
+  # of serializers / entities instead which has an allowlist of attributes
+  def serializable_hash(options = nil)
+    return super unless prevent_sensitive_fields_from_serializable_hash?
+    return super if options && options[:unsafe_serialization_hash]
+
+    options = options.try(:dup) || {}
+    options[:except] = Array(options[:except]).dup
+
+    options[:except].concat self.class.attributes_exempt_from_serializable_hash
+
+    if self.class.respond_to?(:encrypted_attributes)
+      options[:except].concat self.class.encrypted_attributes.keys
+
+      # Per https://github.com/attr-encrypted/attr_encrypted/blob/a96693e9a2a25f4f910bf915e29b0f364f277032/lib/attr_encrypted.rb#L413
+      options[:except].concat self.class.encrypted_attributes.values.map { |v| v[:attribute] }
+      options[:except].concat self.class.encrypted_attributes.values.map { |v| "#{v[:attribute]}_iv" }
+    end
+
+    super(options)
+  end
+
+  private
+
+  def prevent_sensitive_fields_from_serializable_hash?
+    Feature.enabled?(:prevent_sensitive_fields_from_serializable_hash, default_enabled: :yaml)
+  end
+end

app/models/concerns/token_authenticatable.rb

@@ -8,6 +8,10 @@ module TokenAuthenticatable
       @encrypted_token_authenticatable_fields ||= []
     end
 
+    def token_authenticatable_fields
+      @token_authenticatable_fields ||= []
+    end
+
     private
 
     def add_authentication_token_field(token_field, options = {})
@@ -23,6 +27,8 @@ module TokenAuthenticatable
       strategy = TokenAuthenticatableStrategies::Base
         .fabricate(self, token_field, options)
 
+      prevent_from_serialization(*strategy.token_fields) if respond_to?(:prevent_from_serialization)
+
       if options.fetch(:unique, true)
         define_singleton_method("find_by_#{token_field}") do |token|
           strategy.find_token_authenticatable(token)
@@ -82,9 +88,5 @@ module TokenAuthenticatable
       @token_authenticatable_module ||=
         const_set(:TokenAuthenticatable, Module.new).tap(&method(:include))
     end
-
-    def token_authenticatable_fields
-      @token_authenticatable_fields ||= []
-    end
   end
 end

app/models/concerns/token_authenticatable_strategies/base.rb

@@ -23,6 +23,14 @@ module TokenAuthenticatableStrategies
       raise NotImplementedError
     end
 
+    def token_fields
+      result = [token_field]
+
+      result << @expires_at_field if expirable?
+
+      result
+    end
+
     # Default implementation returns the token as-is
     def format_token(instance, token)
       instance.send("format_#{@token_field}", token) # rubocop:disable GitlabSecurity/PublicSend

app/models/concerns/token_authenticatable_strategies/digest.rb

@@ -2,6 +2,10 @@
 
 module TokenAuthenticatableStrategies
   class Digest < Base
+    def token_fields
+      super + [token_field_name]
+    end
+
     def find_token_authenticatable(token, unscoped = false)
       return unless token
 

app/models/concerns/token_authenticatable_strategies/encrypted.rb

@@ -2,6 +2,10 @@
 
 module TokenAuthenticatableStrategies
   class Encrypted < Base
+    def token_fields
+      super + [encrypted_field]
+    end
+
     def find_token_authenticatable(token, unscoped = false)
       return if token.blank?
 

config/feature_flags/development/prevent_sensitive_fields_from_serializable_hash.yml

+---
+name: prevent_sensitive_fields_from_serializable_hash
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/81773
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353878
+milestone: '14.9'
+type: development
+group: group::sharding
+default_enabled: false

lib/gitlab/json_cache.rb

@@ -43,7 +43,9 @@ module Gitlab
     end
 
     def write(key, value, options = nil)
-      backend.write(cache_key(key), value.to_json, options)
+      # As we use json as the serialization format, return everything from
+      # ActiveModel objects included encrypted values.
+      backend.write(cache_key(key), value.to_json(unsafe_serialization_hash: true), options)
     end
 
     def fetch(key, options = {}, &block)

spec/features/projects/jobs/user_triggers_manual_job_with_variables_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe 'User triggers manual job with variables', :js do
 
     wait_for_requests
 
-    expect(build.job_variables.as_json).to contain_exactly(
+    expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
       hash_including('key' => 'key_name', 'value' => 'key_value'))
   end
 end

spec/models/concerns/sensitive_serializable_hash_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SensitiveSerializableHash do
+  describe '.prevent_from_serialization' do
+    let(:test_class) do
+      Class.new do
+        include ActiveModel::Serialization
+        include SensitiveSerializableHash
+
+        attr_accessor :name, :super_secret
+
+        prevent_from_serialization :super_secret
+
+        def attributes
+          { 'name' => nil, 'super_secret' => nil }
+        end
+      end
+    end
+
+    let(:model) { test_class.new }
+
+    it 'does not include the field in serializable_hash' do
+      expect(model.serializable_hash).not_to include('super_secret')
+    end
+
+    context 'unsafe_serialization_hash option' do
+      it 'includes the field in serializable_hash' do
+        expect(model.serializable_hash(unsafe_serialization_hash: true)).to include('super_secret')
+      end
+    end
+
+    context 'when prevent_sensitive_fields_from_serializable_hash feature flag is disabled' do
+      before do
+        stub_feature_flags(prevent_sensitive_fields_from_serializable_hash: false)
+      end
+
+      it 'includes the field in serializable_hash' do
+        expect(model.serializable_hash).to include('super_secret')
+      end
+    end
+  end
+
+  describe '#serializable_hash' do
+    shared_examples "attr_encrypted attribute" do |klass, attribute_name|
+      context "#{klass.name}\##{attribute_name}" do
+        let(:attributes) { [attribute_name, "encrypted_#{attribute_name}", "encrypted_#{attribute_name}_iv"] }
+
+        it 'has a encrypted_attributes field' do
+          expect(klass.encrypted_attributes).to include(attribute_name.to_sym)
+        end
+
+        it 'does not include the attribute in serializable_hash', :aggregate_failures do
+          attributes.each do |attribute|
+            expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+            expect(model.serializable_hash).not_to include(attribute)
+            expect(model.to_json).not_to include(attribute)
+            expect(model.as_json).not_to include(attribute)
+          end
+        end
+
+        context 'unsafe_serialization_hash option' do
+          it 'includes the field in serializable_hash' do
+            attributes.each do |attribute|
+              expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+              expect(model.serializable_hash(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.to_json(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.as_json(unsafe_serialization_hash: true)).to include(attribute)
+            end
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'attr_encrypted attribute', WebHook, 'token' do
+      let_it_be(:model) { create(:system_hook) }
+    end
+
+    it_behaves_like 'attr_encrypted attribute', Ci::InstanceVariable, 'value' do
+      let_it_be(:model) { create(:ci_instance_variable) }
+    end
+
+    shared_examples "add_authentication_token_field attribute" do |klass, attribute_name, encrypted_attribute: true, digest_attribute: false|
+      context "#{klass.name}\##{attribute_name}" do
+        let(:attributes) do
+          if digest_attribute
+            ["#{attribute_name}_digest"]
+          elsif encrypted_attribute
+            [attribute_name, "#{attribute_name}_encrypted"]
+          else
+            [attribute_name]
+          end
+        end
+
+        it 'has a add_authentication_token_field field' do
+          expect(klass.token_authenticatable_fields).to include(attribute_name.to_sym)
+        end
+
+        it 'does not include the attribute in serializable_hash', :aggregate_failures do
+          attributes.each do |attribute|
+            expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+            expect(model.serializable_hash).not_to include(attribute)
+            expect(model.to_json).not_to include(attribute)
+            expect(model.as_json).not_to include(attribute)
+          end
+        end
+
+        context 'unsafe_serialization_hash option' do
+          it 'includes the field in serializable_hash' do
+            attributes.each do |attribute|
+              expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+              expect(model.serializable_hash(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.to_json(unsafe_serialization_hash: true)).to include(attribute)
+              expect(model.as_json(unsafe_serialization_hash: true)).to include(attribute)
+            end
+          end
+        end
+      end
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', Ci::Runner, 'token' do
+      let_it_be(:model) { create(:ci_runner) }
+
+      it 'does not include token_expires_at in serializable_hash' do
+        attribute = 'token_expires_at'
+
+        expect(model.attributes).to include(attribute) # double-check the attribute does exist
+
+        expect(model.serializable_hash).not_to include(attribute)
+        expect(model.to_json).not_to include(attribute)
+        expect(model.as_json).not_to include(attribute)
+      end
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', ApplicationSetting, 'health_check_access_token', encrypted_attribute: false do
+      # health_check_access_token_encrypted column does not exist
+      let_it_be(:model) { create(:application_setting) }
+    end
+
+    it_behaves_like 'add_authentication_token_field attribute', PersonalAccessToken, 'token', encrypted_attribute: false, digest_attribute: true do
+      # PersonalAccessToken only has token_digest column
+      let_it_be(:model) { create(:personal_access_token) }
+    end
+  end
+end

spec/models/concerns/token_authenticatable_spec.rb

@@ -9,6 +9,12 @@ RSpec.shared_examples 'TokenAuthenticatable' do
     it { is_expected.to respond_to("set_#{token_field}") }
     it { is_expected.to respond_to("reset_#{token_field}!") }
   end
+
+  describe 'SensitiveSerializableHash' do
+    it 'includes the token field in list of sensitive attributes prevented from serialization' do
+      expect(described_class.attributes_exempt_from_serializable_hash).to include(token_field)
+    end
+  end
 end
 
 RSpec.describe User, 'TokenAuthenticatable' do

spec/models/concerns/token_authenticatable_strategies/base_spec.rb

@@ -6,6 +6,24 @@ RSpec.describe TokenAuthenticatableStrategies::Base do
   let(:instance) { double(:instance) }
   let(:field) { double(:field) }
 
+  describe '#token_fields' do
+    let(:strategy) { described_class.new(instance, field, options) }
+    let(:field) { 'some_token' }
+    let(:options) { {} }
+
+    it 'includes the token field' do
+      expect(strategy.token_fields).to contain_exactly(field)
+    end
+
+    context 'with expires_at option' do
+      let(:options) { { expires_at: true } }
+
+      it 'includes the token_expires_at field' do
+        expect(strategy.token_fields).to contain_exactly(field, 'some_token_expires_at')
+      end
+    end
+  end
+
   describe '.fabricate' do
     context 'when digest stragegy is specified' do
       it 'fabricates digest strategy object' do

spec/models/concerns/token_authenticatable_strategies/digest_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TokenAuthenticatableStrategies::Digest do
+  let(:model) { class_double('Project') }
+  let(:options) { { digest: true } }
+
+  subject(:strategy) do
+    described_class.new(model, 'some_field', options)
+  end
+
+  describe '#token_fields' do
+    it 'includes the digest field' do
+      expect(strategy.token_fields).to contain_exactly('some_field', 'some_field_digest')
+    end
+  end
+end

spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb

@@ -14,10 +14,18 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
     Gitlab::CryptoHelper.aes256_gcm_encrypt('my-value')
   end
 
-  subject do
+  subject(:strategy) do
     described_class.new(model, 'some_field', options)
   end
 
+  describe '#token_fields' do
+    let(:options) { { encrypted: :required } }
+
+    it 'includes the encrypted field' do
+      expect(strategy.token_fields).to contain_exactly('some_field', 'some_field_encrypted')
+    end
+  end
+
   describe '#find_token_authenticatable' do
     context 'when encryption is required' do
       let(:options) { { encrypted: :required } }

spec/services/ci/job_artifacts/create_service_spec.rb

@@ -175,7 +175,7 @@ RSpec.describe Ci::JobArtifacts::CreateService do
         end
 
         expect(subject[:status]).to eq(:success)
-        expect(job.job_variables.as_json).to contain_exactly(
+        expect(job.job_variables.as_json(only: [:key, :value, :source])).to contain_exactly(
           hash_including('key' => 'KEY1', 'value' => 'VAR1', 'source' => 'dotenv'),
           hash_including('key' => 'KEY2', 'value' => 'VAR2', 'source' => 'dotenv'))
       end

spec/services/ci/parse_dotenv_artifact_service_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
       it 'parses the artifact' do
         expect(subject[:status]).to eq(:success)
 
-        expect(build.job_variables.as_json).to contain_exactly(
+        expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
           hash_including('key' => 'KEY1', 'value' => 'VAR1'),
           hash_including('key' => 'KEY2', 'value' => 'VAR2'))
       end
@@ -57,7 +57,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
 
           expect(subject[:status]).to eq(:success)
 
-          expect(build.job_variables.as_json).to contain_exactly(
+          expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
             hash_including('key' => 'KEY1', 'value' => 'VAR4'),
             hash_including('key' => 'KEY2', 'value' => 'VAR3'))
         end
@@ -101,7 +101,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'trims the trailing space' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1'))
           end
         end
@@ -112,7 +112,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY', 'value' => 'VARCONTAINING=EQLS'))
           end
         end
@@ -133,7 +133,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'skateboard', 'value' => '🛹'))
           end
         end
@@ -154,7 +154,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'V A R 1'))
           end
         end
@@ -165,7 +165,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => '"VAR1"'))
           end
         end
@@ -176,7 +176,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => "'VAR1'"))
           end
         end
@@ -187,7 +187,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the value as-is' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => '"  VAR1  "'))
           end
         end
@@ -208,7 +208,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'parses the dotenv data' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => ''))
           end
         end
@@ -250,7 +250,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'does not support variable expansion in dotenv parser' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1'),
               hash_including('key' => 'KEY2', 'value' => '${KEY1}_Test'))
           end
@@ -284,7 +284,7 @@ RSpec.describe Ci::ParseDotenvArtifactService do
           it 'does not support comment in dotenv parser' do
             subject
 
-            expect(build.job_variables.as_json).to contain_exactly(
+            expect(build.job_variables.as_json(only: [:key, :value])).to contain_exactly(
               hash_including('key' => 'KEY1', 'value' => 'VAR1         # This is variable'))
           end
         end