From 25329b4f06c06d4fe6f2777287f08babdaaebbfc Mon Sep 17 00:00:00 2001 From: Zeke Gabrielse Date: Tue, 12 Nov 2024 10:55:57 -0600 Subject: [PATCH] add jit v1 token rehashing --- app/models/concerns/tokenable.rb | 12 +++- app/services/license_key_lookup_service.rb | 11 ++++ app/services/token_lookup_service.rb | 11 ++++ .../license_key_lookup_service_spec.rb | 56 +++++++++++++++++ spec/services/token_lookup_service_spec.rb | 62 +++++++++++++++++++ 5 files changed, 150 insertions(+), 2 deletions(-) create mode 100644 spec/services/license_key_lookup_service_spec.rb create mode 100644 spec/services/token_lookup_service_spec.rb diff --git a/app/models/concerns/tokenable.rb b/app/models/concerns/tokenable.rb index ee795186ab..eb5b26d215 100644 --- a/app/models/concerns/tokenable.rb +++ b/app/models/concerns/tokenable.rb @@ -28,7 +28,7 @@ def generate_hashed_token(attribute, length: 64, version: ALGO_VERSION) # length, since the first 66 chars of our string consist of the account # and the bearer's UUID. This lets us use larger tokens (as seen here) # and avoid the nasty truncation. - res = BCrypt::Password.create Digest::SHA256.digest(raw) + res = BCrypt::Password.create Digest::SHA256.hexdigest(raw) when "v2" raw = SecureRandom.hex(length).gsub /.{#{version.length}}\z/, version raw = yield raw if block_given? @@ -53,6 +53,7 @@ def compare_hashed_token(attribute, token, version: ALGO_VERSION) a = self.send attribute b = nil + c = nil case version when "v1" @@ -64,6 +65,9 @@ def compare_hashed_token(attribute, token, version: ALGO_VERSION) end b = BCrypt::Engine.hash_secret digest, bcrypt.salt + + # FIXME(ezekg) support rehashing: https://github.com/bcrypt-ruby/bcrypt-ruby/pull/168 + c = BCrypt::Engine.hash_secret Digest::SHA256.hexdigest(token), bcrypt.salt when "v2" b = OpenSSL::HMAC.hexdigest "SHA512", account.private_key, token when "v3" @@ -72,7 +76,11 @@ def compare_hashed_token(attribute, token, version: ALGO_VERSION) raise NotImplementedError.new "token #{version} not implemented" end - secure_compare a, b + unless c.nil? + secure_compare(a, b) || secure_compare(a, c) + else + secure_compare(a, b) + end rescue false end diff --git a/app/services/license_key_lookup_service.rb b/app/services/license_key_lookup_service.rb index f4e05a53be..c7fffd0791 100644 --- a/app/services/license_key_lookup_service.rb +++ b/app/services/license_key_lookup_service.rb @@ -22,6 +22,17 @@ def call if license&.compare_hashed_token(:key, key, version: 'v1') Keygen.logger.warn { "[license-key-lookup-service] v1 keys are deprecated and must be regenerated: license_id=#{license.id.inspect}" } + # FIXME(ezekg) jit rehash key: https://github.com/bcrypt-ruby/bcrypt-ruby/pull/168 + digest = BCrypt::Engine.hash_secret( + Digest::SHA256.hexdigest(key), + BCrypt::Password.new(license.key).salt, # reuse salt + ) + unless license.send(:secure_compare, digest, license.key) + Keygen.logger.warn { "[license-key-lookup-service] rehashing key: license_id=#{license.id.inspect}" } + + license.update!(key: digest) + end + license else nil diff --git a/app/services/token_lookup_service.rb b/app/services/token_lookup_service.rb index 0874b71a42..8b60ae91b4 100644 --- a/app/services/token_lookup_service.rb +++ b/app/services/token_lookup_service.rb @@ -34,6 +34,17 @@ def call if instance&.compare_hashed_token(:digest, token, version: 'v1') Keygen.logger.warn { "[token-lookup-service] v1 tokens are deprecated and must be regenerated: bearer_type=#{instance.bearer.class.name.inspect} bearer_id=#{instance.bearer.id.inspect} token_id=#{instance.id.inspect}" } + # FIXME(ezekg) jit rehash token: https://github.com/bcrypt-ruby/bcrypt-ruby/pull/168 + digest = BCrypt::Engine.hash_secret( + Digest::SHA256.hexdigest(token), + BCrypt::Password.new(instance.digest).salt, # reuse salt + ) + unless instance.send(:secure_compare, digest, instance.digest) + Keygen.logger.warn { "[license-key-lookup-service] rehashing token: token_id=#{instance.id.inspect}" } + + instance.update!(digest:) + end + instance else nil diff --git a/spec/services/license_key_lookup_service_spec.rb b/spec/services/license_key_lookup_service_spec.rb new file mode 100644 index 0000000000..208ec8b2f1 --- /dev/null +++ b/spec/services/license_key_lookup_service_spec.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'spec_helper' + +describe LicenseKeyLookupService do + context 'when key is a legacy encrypted key' do + let(:key) { 'bdcecdc23c0f48229f77357151d1e67f-8e1ae3236636bfe57893b835871553eb-524ffec53ad05c14bbf3339eec741300-295512a0539cd0863abe42ddc29cf9v1' } + let(:license_id) { key.split('-').first } + let(:account) { create(:account) } + + context 'when key is a good key' do + let(:digest) { '$2a$12$kh33FHu.TWYMZlWldcOMKet4YiVNqMHq8/A343/GTfPz.NRCPq2Q.' } + + let!(:license) { + create(:license, id: license_id, key: digest, account:) + } + + it 'should support authenticating' do + l = described_class.call(key:, account:, legacy_encrypted: true) + + expect(l).to eq license + end + + it 'should not rehash' do + expect { described_class.call(key:, account:, legacy_encrypted: true) }.to_not( + change { license.reload.key }, + ) + end + end + + context 'when key is a bad key' do + let(:digest) { '$2a$12$kh33FHu.TWYMZlWldcOMKevM0Jj4VrN/8.EbfAj4cBEcS51A7hYUK' } + + let!(:license) { + create(:license, id: license_id, key: digest, account:) + } + + it 'should support authenticating' do + l = described_class.call(key:, account:, legacy_encrypted: true) + + expect(l).to eq license + end + + it 'should rehash' do + expect { described_class.call(key:, account:, legacy_encrypted: true) }.to( + change { license.reload.key }, + ) + + # sanity check + ok = license.compare_hashed_token(:key, key, version: 'v1') + expect(ok).to be true + end + end + end +end diff --git a/spec/services/token_lookup_service_spec.rb b/spec/services/token_lookup_service_spec.rb new file mode 100644 index 0000000000..c2650df770 --- /dev/null +++ b/spec/services/token_lookup_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'spec_helper' + +describe TokenLookupService do + context 'when token is a legacy v1 token' do + let(:token_value) { '1780aeccbbb14b4dad0cf8b8165faba2.52f61713fd0c49a2a6537e3142d043cb.3e2373c60b7df8514ff763dad707faad44956c2421fdb2588fff2a73099b07v1' } + let(:account_id) { token_value.split('.').first } + let(:token_id) { token_value.split('.').second } + let(:account) { create(:account, id: account_id) } + let(:product) { create(:product, account:) } + + context 'when token is a good token' do + let(:digest) { '$2a$12$7sgr9E9ZURyXzttcuIl/muYRww.wHxNgCpgUHsKODV2ZXpPWiqLkq' } + + let!(:token) { + t = create(:token, id: token_id, bearer: product, account:) + t.update!(digest:) + t + } + + it 'should support authenticating' do + t = described_class.call(token: token_value, account:) + + expect(t).to eq token + end + + it 'should not rehash' do + expect { described_class.call(token: token_value, account:) }.to_not( + change { token.reload.digest }, + ) + end + end + + context 'when token is a bad token' do + let(:digest) { '$2a$12$7sgr9E9ZURyXzttcuIl/muSl7JViEcBcytnbouQooX22PP43AzZ4C' } + + let!(:token) { + t = create(:token, id: token_id, bearer: product, account:) + t.update!(digest:) + t + } + + it 'should support authenticating' do + t = described_class.call(token: token_value, account:) + + expect(t).to eq token + end + + it 'should rehash' do + expect { described_class.call(token: token_value, account:) }.to( + change { token.reload.digest }, + ) + + # sanity check + ok = token.compare_hashed_token(:digest, token_value, version: 'v1') + expect(ok).to be true + end + end + end +end