Merge branch '322594-create-a-new-way-of-encrypting-tokens-phase1-reader' into 'master'

Create new way of encrypting tokens - phase 1 - reader

See merge request gitlab-org/gitlab!58896

app/models/concerns/token_authenticatable_strategies/encrypted.rb

@@ -42,14 +42,14 @@ module TokenAuthenticatableStrategies
       return insecure_strategy.get_token(instance) if migrating?
 
       encrypted_token = instance.read_attribute(encrypted_field)
-      token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
+      token = EncryptionHelper.decrypt_token(encrypted_token)
       token || (insecure_strategy.get_token(instance) if optional?)
     end
 
     def set_token(instance, token)
       raise ArgumentError unless token.present?
 
-      instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      instance[encrypted_field] = EncryptionHelper.encrypt_token(token)
       instance[token_field] = token if migrating?
       instance[token_field] = nil if optional?
       token
@@ -85,10 +85,9 @@ module TokenAuthenticatableStrategies
     end
 
     def find_by_encrypted_token(token, unscoped)
-      nonce = Gitlab::CryptoHelper::AES256_GCM_IV_STATIC
-      encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token, nonce: nonce)
-
-      relation(unscoped).find_by(encrypted_field => encrypted_value)
+      encrypted_value = EncryptionHelper.encrypt_token(token)
+      token_encrypted_with_static_iv = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      relation(unscoped).find_by(encrypted_field => [encrypted_value, token_encrypted_with_static_iv])
     end
 
     def insecure_strategy

app/models/concerns/token_authenticatable_strategies/encryption_helper.rb

+# frozen_string_literal: true
+
+module TokenAuthenticatableStrategies
+  class EncryptionHelper
+    DYNAMIC_NONCE_IDENTIFIER = "|"
+    NONCE_SIZE = 12
+
+    def self.encrypt_token(plaintext_token)
+      Gitlab::CryptoHelper.aes256_gcm_encrypt(plaintext_token)
+    end
+
+    def self.decrypt_token(token)
+      return unless token
+
+      # The pattern of the token is "#{DYNAMIC_NONCE_IDENTIFIER}#{token}#{iv_of_12_characters}"
+      if token.start_with?(DYNAMIC_NONCE_IDENTIFIER) && token.size > NONCE_SIZE + DYNAMIC_NONCE_IDENTIFIER.size
+        token_to_decrypt = token[1...-NONCE_SIZE]
+        iv = token[-NONCE_SIZE..-1]
+
+        Gitlab::CryptoHelper.aes256_gcm_decrypt(token_to_decrypt, nonce: iv)
+      else
+        Gitlab::CryptoHelper.aes256_gcm_decrypt(token)
+      end
+    end
+  end
+end

ee/spec/lib/ee/gitlab/crypto_helper_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::CryptoHelper do
-  include ::EE::GeoHelpers
-
-  describe '.read_only?' do
-    context 'with Geo enabled' do
-      before do
-        allow(Gitlab::Geo).to receive(:enabled?) { true }
-        allow(Gitlab::Geo).to receive(:current_node) { geo_node }
-      end
-
-      context 'is Geo secondary node' do
-        let(:geo_node) { create(:geo_node) }
-
-        it 'returns true' do
-          expect(described_class.read_only?).to be_truthy
-        end
-      end
-
-      context 'is Geo primary node' do
-        let(:geo_node) { create(:geo_node, :primary) }
-
-        it 'returns false when is Geo primary node' do
-          expect(described_class.read_only?).to be_falsey
-        end
-      end
-    end
-  end
-end

lib/gitlab/crypto_helper.rb

@@ -16,30 +16,17 @@ module Gitlab
       ::Digest::SHA256.base64digest("#{value}#{salt}")
     end
 
-    def aes256_gcm_encrypt(value, nonce: nil)
-      aes256_gcm_encrypt_using_static_nonce(value)
+    def aes256_gcm_encrypt(value, nonce: AES256_GCM_IV_STATIC)
+      encrypted_token = Encryptor.encrypt(AES256_GCM_OPTIONS.merge(value: value, iv: nonce))
+      Base64.strict_encode64(encrypted_token)
     end
 
-    def aes256_gcm_decrypt(value)
+    def aes256_gcm_decrypt(value, nonce: AES256_GCM_IV_STATIC)
       return unless value
 
-      nonce = AES256_GCM_IV_STATIC
       encrypted_token = Base64.decode64(value)
       decrypted_token = Encryptor.decrypt(AES256_GCM_OPTIONS.merge(value: encrypted_token, iv: nonce))
       decrypted_token
     end
-
-    def aes256_gcm_encrypt_using_static_nonce(value)
-      create_encrypted_token(value, AES256_GCM_IV_STATIC)
-    end
-
-    def read_only?
-      Gitlab::Database.read_only?
-    end
-
-    def create_encrypted_token(value, iv)
-      encrypted_token = Encryptor.encrypt(AES256_GCM_OPTIONS.merge(value: value, iv: iv))
-      Base64.strict_encode64(encrypted_token)
-    end
   end
 end

spec/lib/gitlab/crypto_helper_spec.rb

@@ -20,15 +20,21 @@ RSpec.describe Gitlab::CryptoHelper do
       expect(encrypted).not_to include "\n"
     end
 
-    it 'does not save hashed token with iv value in database' do
-      expect { described_class.aes256_gcm_encrypt('some-value') }.not_to change { TokenWithIv.count }
-    end
-
     it 'encrypts using static iv' do
       expect(Encryptor).to receive(:encrypt).with(described_class::AES256_GCM_OPTIONS.merge(value: 'some-value', iv: described_class::AES256_GCM_IV_STATIC)).and_return('hashed_value')
 
       described_class.aes256_gcm_encrypt('some-value')
     end
+
+    context 'with provided iv' do
+      let(:iv) { create_nonce }
+
+      it 'encrypts using provided iv' do
+        expect(Encryptor).to receive(:encrypt).with(described_class::AES256_GCM_OPTIONS.merge(value: 'some-value', iv: iv)).and_return('hashed_value')
+
+        described_class.aes256_gcm_encrypt('some-value', nonce: iv)
+      end
+    end
   end
 
   describe '.aes256_gcm_decrypt' do
@@ -46,10 +52,22 @@ RSpec.describe Gitlab::CryptoHelper do
 
         expect(decrypted).to eq 'some-value'
       end
+    end
+
+    context 'when token was encrypted using random nonce' do
+      let(:value) { 'random-value' }
+      let(:iv) { create_nonce }
+      let(:encrypted) { described_class.aes256_gcm_encrypt(value, nonce: iv) }
+
+      it 'correctly decrypts encrypted string' do
+        decrypted = described_class.aes256_gcm_decrypt(encrypted, nonce: iv)
 
-      it 'does not save hashed token with iv value in database' do
-        expect { described_class.aes256_gcm_decrypt(encrypted) }.not_to change { TokenWithIv.count }
+        expect(decrypted).to eq value
       end
     end
   end
+
+  def create_nonce
+    ::Digest::SHA256.hexdigest('my-value').bytes.take(TokenAuthenticatableStrategies::EncryptionHelper::NONCE_SIZE).pack('c*')
+  end
 end

spec/models/concerns/token_authenticatable_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe ApplicationSetting, 'TokenAuthenticatable' do
         it 'persists new token as an encrypted string' do
           expect(subject).to eq settings.reload.runners_registration_token
           expect(settings.read_attribute('runners_registration_token_encrypted'))
-            .to eq Gitlab::CryptoHelper.aes256_gcm_encrypt(subject, nonce: Gitlab::CryptoHelper::AES256_GCM_IV_STATIC)
+            .to eq TokenAuthenticatableStrategies::EncryptionHelper.encrypt_token(subject)
           expect(settings).to be_persisted
         end
 
@@ -243,7 +243,7 @@ RSpec.describe Ci::Build, 'TokenAuthenticatable' do
         it 'persists new token as an encrypted string' do
           build.ensure_token!
 
-          encrypted = Gitlab::CryptoHelper.aes256_gcm_encrypt(build.token, nonce: Gitlab::CryptoHelper::AES256_GCM_IV_STATIC)
+          encrypted = TokenAuthenticatableStrategies::EncryptionHelper.encrypt_token(build.token)
 
           expect(build.read_attribute('token_encrypted')).to eq encrypted
         end

spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb

@@ -7,6 +7,10 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
   let(:instance) { double(:instance) }
 
   let(:encrypted) do
+    TokenAuthenticatableStrategies::EncryptionHelper.encrypt_token('my-value')
+  end
+
+  let(:encrypted_with_static_iv) do
     Gitlab::CryptoHelper.aes256_gcm_encrypt('my-value')
   end
 
@@ -15,12 +19,25 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#find_token_authenticatable' do
-    context 'when using optional strategy' do
+    context 'when encryption is required' do
+      let(:options) { { encrypted: :required } }
+
+      it 'finds the encrypted resource by cleartext' do
+        allow(model).to receive(:find_by)
+          .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv])
+          .and_return('encrypted resource')
+
+        expect(subject.find_token_authenticatable('my-value'))
+          .to eq 'encrypted resource'
+      end
+    end
+
+    context 'when encryption is optional' do
       let(:options) { { encrypted: :optional } }
 
       it 'finds the encrypted resource by cleartext' do
         allow(model).to receive(:find_by)
-          .with('some_field_encrypted' => encrypted)
+          .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv])
           .and_return('encrypted resource')
 
         expect(subject.find_token_authenticatable('my-value'))
@@ -33,7 +50,7 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
           .and_return('plaintext resource')
 
         allow(model).to receive(:find_by)
-          .with('some_field_encrypted' => encrypted)
+          .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv])
           .and_return(nil)
 
         expect(subject.find_token_authenticatable('my-value'))
@@ -41,7 +58,7 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
       end
     end
 
-    context 'when using migration strategy' do
+    context 'when encryption is migrating' do
       let(:options) { { encrypted: :migrating } }
 
       it 'finds the cleartext resource by cleartext' do
@@ -65,7 +82,27 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#get_token' do
-    context 'when using optional strategy' do
+    context 'when encryption is required' do
+      let(:options) { { encrypted: :required } }
+
+      it 'returns decrypted token when an encrypted with static iv token is present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(Gitlab::CryptoHelper.aes256_gcm_encrypt('my-test-value'))
+
+        expect(subject.get_token(instance)).to eq 'my-test-value'
+      end
+
+      it 'returns decrypted token when an encrypted token is present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(encrypted)
+
+        expect(subject.get_token(instance)).to eq 'my-value'
+      end
+    end
+
+    context 'when encryption is optional' do
       let(:options) { { encrypted: :optional } }
 
       it 'returns decrypted token when an encrypted token is present' do
@@ -76,6 +113,14 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
         expect(subject.get_token(instance)).to eq 'my-value'
       end
 
+      it 'returns decrypted token when an encrypted with static iv token is present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(Gitlab::CryptoHelper.aes256_gcm_encrypt('my-test-value'))
+
+        expect(subject.get_token(instance)).to eq 'my-test-value'
+      end
+
       it 'returns the plaintext token when encrypted token is not present' do
         allow(instance).to receive(:read_attribute)
           .with('some_field_encrypted')
@@ -89,7 +134,7 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
       end
     end
 
-    context 'when using migration strategy' do
+    context 'when encryption is migrating' do
       let(:options) { { encrypted: :migrating } }
 
       it 'returns cleartext token when an encrypted token is present' do
@@ -119,12 +164,22 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#set_token' do
-    context 'when using optional strategy' do
+    context 'when encryption is required' do
+      let(:options) { { encrypted: :required } }
+
+      it 'writes encrypted token and returns it' do
+        expect(instance).to receive(:[]=)
+          .with('some_field_encrypted', encrypted)
+
+        expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
+      end
+    end
+    context 'when encryption is optional' do
       let(:options) { { encrypted: :optional } }
 
       it 'writes encrypted token and removes plaintext token and returns it' do
         expect(instance).to receive(:[]=)
-          .with('some_field_encrypted', any_args)
+          .with('some_field_encrypted', encrypted)
         expect(instance).to receive(:[]=)
           .with('some_field', nil)
 
@@ -132,12 +187,12 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do
       end
     end
 
-    context 'when using migration strategy' do
+    context 'when encryption is migrating' do
       let(:options) { { encrypted: :migrating } }
 
       it 'writes encrypted token and writes plaintext token' do
         expect(instance).to receive(:[]=)
-          .with('some_field_encrypted', any_args)
+          .with('some_field_encrypted', encrypted)
         expect(instance).to receive(:[]=)
           .with('some_field', 'my-value')
 

spec/models/concerns/token_authenticatable_strategies/encryption_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe TokenAuthenticatableStrategies::EncryptionHelper do
+  let(:encrypted_token) { described_class.encrypt_token('my-value') }
+
+  describe '.encrypt_token' do
+    it 'encrypts token' do
+      expect(encrypted_token).not_to eq('my-value')
+    end
+  end
+
+  describe '.decrypt_token' do
+    it 'decrypts token with static iv' do
+      expect(described_class.decrypt_token(encrypted_token)).to eq('my-value')
+    end
+
+    it 'decrypts token with dynamic iv' do
+      iv = ::Digest::SHA256.hexdigest('my-value').bytes.take(described_class::NONCE_SIZE).pack('c*')
+      token = Gitlab::CryptoHelper.aes256_gcm_encrypt('my-value', nonce: iv)
+      encrypted_token = "#{described_class::DYNAMIC_NONCE_IDENTIFIER}#{token}#{iv}"
+
+      expect(described_class.decrypt_token(encrypted_token)).to eq('my-value')
+    end
+  end
+end