From 4bca00398351ae9243a8fea4f48f9f8bfe3465f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Wed, 4 Nov 2026 09:33:57 +0400 Subject: [PATCH] Hash remoteStorage access tokens at rest Store only SHA256(token) in a new token_digest column and use the digest for the Redis authorization keys instead of the plaintext token. The plaintext is now held in memory only for issuance. This is the expand step: the plaintext token column stays in place so the old code can keep running during the deploy. A follow-up migration will drop it once rs:rekey_tokens has re-keyed Redis and confirmed no plaintext keys remain. Refs #259 --- app/models/remote_storage_authorization.rb | 15 ++- .../remote_storage_manager/rekey_tokens.rb | 111 +++++++++++++++++ .../remote_storage_manager_service.rb | 2 + ...digest_to_remote_storage_authorizations.rb | 26 ++++ db/schema.rb | 4 +- lib/tasks/remote_storage.rake | 12 ++ spec/controllers/rs/oauth_controller_spec.rb | 6 +- ...e_storage_expire_authorization_job_spec.rb | 2 +- .../remote_storage_authorization_spec.rb | 30 ++++- .../rekey_tokens_spec.rb | 114 ++++++++++++++++++ 10 files changed, 313 insertions(+), 9 deletions(-) create mode 100644 app/services/remote_storage_manager/rekey_tokens.rb create mode 100644 app/services/remote_storage_manager_service.rb create mode 100644 db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb create mode 100644 lib/tasks/remote_storage.rake create mode 100644 spec/services/remote_storage_manager/rekey_tokens_spec.rb diff --git a/app/models/remote_storage_authorization.rb b/app/models/remote_storage_authorization.rb index 6c7ae6e..cd2c36d 100644 --- a/app/models/remote_storage_authorization.rb +++ b/app/models/remote_storage_authorization.rb @@ -2,6 +2,10 @@ class RemoteStorageAuthorization < ApplicationRecord belongs_to :user belongs_to :web_app, class_name: "AppCatalog::WebApp", optional: true + # The plaintext token is only ever held in memory, long enough to hand it to + # the client at issuance. At rest we store only its SHA256 digest. + attr_accessor :token + # Tokens issued before user agents were recorded may have been used from any # number of devices, so they cannot be attributed to a single one. UNKNOWN_DEVICE = "Unknown device(s)" @@ -77,8 +81,12 @@ class RemoteStorageAuthorization < ApplicationRecord end def delete_token_from_redis - key = "authorizations:#{user.cn}:#{token}" - redis.srem? key, redis.smembers(key) + value = token_digest.presence + value ||= read_attribute(:token) if self.class.column_names.include?("token") + return if value.blank? + + key = "authorizations:#{user.cn}:#{value}" + redis.del key rescue => e Rails.logger.error e Sentry.capture_exception(e) if Setting.sentry_enabled? @@ -92,10 +100,11 @@ class RemoteStorageAuthorization < ApplicationRecord def generate_token(length=16) self.token = SecureRandom.hex(length) if self.token.blank? + self.token_digest = Digest::SHA256.hexdigest(token) end def store_token_in_redis - redis.sadd "authorizations:#{user.cn}:#{token}", permissions + redis.sadd "authorizations:#{user.cn}:#{token_digest}", permissions end def schedule_token_expiry diff --git a/app/services/remote_storage_manager/rekey_tokens.rb b/app/services/remote_storage_manager/rekey_tokens.rb new file mode 100644 index 0000000..3f91125 --- /dev/null +++ b/app/services/remote_storage_manager/rekey_tokens.rb @@ -0,0 +1,111 @@ +require "digest" + +module RemoteStorageManager + # One-off, idempotent migration helper for issue #259. It backfills missing + # digests for authorizations created while the old plaintext code was still + # running, re-keys the Redis authorization sets from plaintext to digest + # keys, and removes any leftover non-digest keys. + # + # It only works while the plaintext `token` column still exists; after the + # follow-up contract migration drops it, `call` is a no-op. + class RekeyTokens < RemoteStorageManagerService + Result = Struct.new(:rekeyed, :legacy_removed, :skipped, keyword_init: true) + + def call + return Result.new(rekeyed: 0, legacy_removed: 0, skipped: true) unless token_column? + + rekeyed = 0 + + RemoteStorageAuthorization.find_each do |auth| + plaintext = persisted_token(auth) + next if plaintext.blank? && auth.token_digest.blank? + + digest = auth.token_digest.presence || Digest::SHA256.hexdigest(plaintext) + auth.update_columns(token_digest: digest) if auth.token_digest.blank? + + permissions = permissions_for(auth, plaintext) + + redis.sadd(redis_key(auth.user.cn, digest), permissions) if permissions.present? + redis.del(redis_key(auth.user.cn, plaintext)) if plaintext.present? + + rekeyed += 1 + end + + Result.new(rekeyed: rekeyed, legacy_removed: remove_legacy_keys, skipped: false) + end + + private + + def redis + @redis ||= Redis.new(url: Setting.rs_redis_url) + end + + def token_column? + RemoteStorageAuthorization.column_names.include?("token") + end + + # Reads the persisted plaintext directly, because the model's `token` + # accessor shadows the column after the token is moved into memory. + def persisted_token(auth) + auth.read_attribute(:token) if token_column? + end + + def redis_key(cn, value) + "authorizations:#{cn}:#{value}" + end + + def permissions_for(auth, plaintext) + permissions = redis.smembers(redis_key(auth.user.cn, plaintext)) if plaintext.present? + permissions.presence || permissions_from_db(auth) + end + + def permissions_from_db(auth) + case (permissions = auth.permissions) + when Array then permissions.map(&:to_s) + when String then parse_permissions(permissions) + else Array(permissions).map(&:to_s) + end + end + + def parse_permissions(raw) + parsed = parse_serialized(raw) + return parsed.map(&:to_s) if parsed.is_a?(Array) + + raw.to_s.gsub(/[{}]/, "").split(/[,\s]+/).reject(&:empty?) + end + + def parse_serialized(raw) + YAML.safe_load(raw) + rescue StandardError + begin + JSON.parse(raw) + rescue StandardError + nil + end + end + + # Removes leftover plaintext authorization keys. Legacy plaintext tokens are + # 32 hex chars (SecureRandom.hex(16)); digests are 64. Matching on shape + # instead of a snapshot of known keys means a digest issued concurrently + # during the sweep is never deleted. + def remove_legacy_keys + removed = 0 + cursor = "0" + + loop do + cursor, keys = redis.scan(cursor, match: "authorizations:*", count: 1000) + + keys.each do |key| + next if key.split(":", 3).last.match?(/\A[0-9a-f]{64}\z/) + + redis.del(key) + removed += 1 + end + + break if cursor == "0" + end + + removed + end + end +end diff --git a/app/services/remote_storage_manager_service.rb b/app/services/remote_storage_manager_service.rb new file mode 100644 index 0000000..9de9a97 --- /dev/null +++ b/app/services/remote_storage_manager_service.rb @@ -0,0 +1,2 @@ +class RemoteStorageManagerService < ApplicationService +end diff --git a/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb b/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb new file mode 100644 index 0000000..f196b02 --- /dev/null +++ b/db/migrate/20261104120000_add_token_digest_to_remote_storage_authorizations.rb @@ -0,0 +1,26 @@ +require "digest" + +class AddTokenDigestToRemoteStorageAuthorizations < ActiveRecord::Migration[8.1] + class MigrationRemoteStorageAuthorization < ActiveRecord::Base + self.table_name = "remote_storage_authorizations" + end + + def up + add_column :remote_storage_authorizations, :token_digest, :string + + MigrationRemoteStorageAuthorization.reset_column_information + + MigrationRemoteStorageAuthorization.find_each do |auth| + next if auth.token.blank? + + auth.update_columns(token_digest: Digest::SHA256.hexdigest(auth.token)) + end + + add_index :remote_storage_authorizations, :token_digest, unique: true + end + + def down + remove_index :remote_storage_authorizations, :token_digest + remove_column :remote_storage_authorizations, :token_digest + end +end diff --git a/db/schema.rb b/db/schema.rb index 4518582..b587aef 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do +ActiveRecord::Schema[8.1].define(version: 2026_11_04_120000) do create_table "active_storage_attachments", force: :cascade do |t| t.bigint "blob_id", null: false t.datetime "created_at", null: false @@ -109,11 +109,13 @@ ActiveRecord::Schema[8.1].define(version: 2026_11_02_130000) do t.text "permissions", default: "--- []\n" t.string "redirect_uri" t.string "token" + t.string "token_digest" t.datetime "updated_at", null: false t.text "user_agent" t.integer "user_id", null: false t.integer "web_app_id" t.index ["permissions"], name: "index_remote_storage_authorizations_on_permissions" + t.index ["token_digest"], name: "index_remote_storage_authorizations_on_token_digest", unique: true t.index ["user_id"], name: "index_remote_storage_authorizations_on_user_id" t.index ["web_app_id"], name: "index_remote_storage_authorizations_on_web_app_id" end diff --git a/lib/tasks/remote_storage.rake b/lib/tasks/remote_storage.rake new file mode 100644 index 0000000..3af410b --- /dev/null +++ b/lib/tasks/remote_storage.rake @@ -0,0 +1,12 @@ +namespace :rs do + desc "Backfill remoteStorage token digests and re-key Redis authorization keys (idempotent)" + task rekey_tokens: :environment do + result = RemoteStorageManager::RekeyTokens.call + + if result.skipped + puts "remote_storage_authorizations.token column is gone; nothing to re-key." + else + puts "Re-keyed #{result.rekeyed} authorization(s); removed #{result.legacy_removed} legacy Redis key(s)." + end + end +end diff --git a/spec/controllers/rs/oauth_controller_spec.rb b/spec/controllers/rs/oauth_controller_spec.rb index e06be1a..c7530a9 100644 --- a/spec/controllers/rs/oauth_controller_spec.rb +++ b/spec/controllers/rs/oauth_controller_spec.rb @@ -376,7 +376,9 @@ RSpec.describe Rs::OauthController, type: :controller do end it "redirects to the redirect_uri" do - expect(response).to redirect_to("https://example.com#access_token=#{@auth.token}&state=foobar123") + expect(response).to redirect_to( + %r{\Ahttps://example\.com#access_token=[a-f0-9]{32}&state=foobar123\z} + ) end end @@ -400,7 +402,7 @@ RSpec.describe Rs::OauthController, type: :controller do end it "updates the token" do - expect(@auth.reload.token).not_to be_nil + expect(@auth.reload.token_digest).not_to be_nil end end diff --git a/spec/jobs/remote_storage_expire_authorization_job_spec.rb b/spec/jobs/remote_storage_expire_authorization_job_spec.rb index afe0f3e..4bf458e 100644 --- a/spec/jobs/remote_storage_expire_authorization_job_spec.rb +++ b/spec/jobs/remote_storage_expire_authorization_job_spec.rb @@ -28,7 +28,7 @@ RSpec.describe RemoteStorageExpireAuthorizationJob, type: :job do } it "removes the RS authorization from redis" do - redis_key = "authorizations:#{@user.cn}:#{@rs_authorization.token}" + redis_key = "authorizations:#{@user.cn}:#{@rs_authorization.token_digest}" expect(redis.keys(redis_key)).to_not be_empty perform_enqueued_jobs { job } diff --git a/spec/models/remote_storage_authorization_spec.rb b/spec/models/remote_storage_authorization_spec.rb index 7616603..d38d25f 100644 --- a/spec/models/remote_storage_authorization_spec.rb +++ b/spec/models/remote_storage_authorization_spec.rb @@ -25,10 +25,22 @@ RSpec.describe RemoteStorageAuthorization, type: :model do expect(auth.token).to match(/[a-zA-Z0-9]+/) end + it "stores only the token digest" do + expect(auth.token_digest).to eq(Digest::SHA256.hexdigest(auth.token)) + end + + it "does not persist the plaintext token" do + plaintext = auth.token + auth.reload + + expect(auth.read_attribute(:token)).to be_nil + expect(auth.token_digest).to eq(Digest::SHA256.hexdigest(plaintext)) + end + it "stores a token in redis" do auth - authorizations = redis_rs.smembers("authorizations:#{user.cn}:#{auth.token}") + authorizations = redis_rs.smembers("authorizations:#{user.cn}:#{auth.token_digest}") expect(authorizations.sort).to eq(%w(documents photos contacts:rw videos:r tasks/work:r).sort) end @@ -56,7 +68,21 @@ RSpec.describe RemoteStorageAuthorization, type: :model do ) auth.destroy! - expect(redis_rs.exists?("authorizations:#{user.cn}:#{auth.token}")).to be(false) + expect(redis_rs.exists?("authorizations:#{user.cn}:#{auth.token_digest}")).to be(false) + end + + it "removes a legacy plaintext token from redis" do + auth = user.remote_storage_authorizations.create!( + permissions: %w(shares:rw), + client_id: "sharesome.5apps.com", + redirect_uri: "https://sharesome.5apps.com" + ) + auth.update_columns(token: "legacytoken", token_digest: nil) + redis_rs.sadd("authorizations:#{user.cn}:legacytoken", %w(shares:rw)) + + auth.destroy! + + expect(redis_rs.exists?("authorizations:#{user.cn}:legacytoken")).to be(false) end context "with expiry set" do diff --git a/spec/services/remote_storage_manager/rekey_tokens_spec.rb b/spec/services/remote_storage_manager/rekey_tokens_spec.rb new file mode 100644 index 0000000..3dbd3e9 --- /dev/null +++ b/spec/services/remote_storage_manager/rekey_tokens_spec.rb @@ -0,0 +1,114 @@ +require 'rails_helper' + +RSpec.describe RemoteStorageManager::RekeyTokens, type: :model do + let(:user) { create :user, cn: "ronald", ou: "kosmos.org" } + let(:redis) { redis_rs } + + before do + allow_any_instance_of(AppCatalog::WebApp).to receive(:update_metadata).and_return(true) + allow_any_instance_of(RemoteStorageAuthorization).to receive(:remove_token_expiry_job).and_return(nil) + + redis.keys("authorizations:*").each { |key| redis.del(key) } + end + + def authorization_keys + redis.keys("authorizations:#{user.cn}:*") + end + + # A row as it looked before #259: the plaintext token lives in the DB column + # and only the plaintext Redis key exists. + def legacy_authorization(token: "legacytoken", permissions: %w(documents:rw)) + auth = create :remote_storage_authorization, user: user, permissions: permissions + auth.update_columns(token: token, token_digest: nil) + + redis.keys("authorizations:#{user.cn}:*").each { |key| redis.del(key) } + redis.sadd("authorizations:#{user.cn}:#{token}", permissions) + + auth + end + + it "re-keys an authorization from the plaintext to the digest form" do + auth = legacy_authorization + digest = Digest::SHA256.hexdigest("legacytoken") + + result = described_class.call + + expect(result).to have_attributes(skipped: false, rekeyed: 1) + expect(redis.smembers("authorizations:#{user.cn}:#{digest}").sort).to eq(%w(documents:rw)) + expect(redis.exists?("authorizations:#{user.cn}:legacytoken")).to be(false) + expect(auth.reload.token_digest).to eq(digest) + end + + it "is idempotent" do + legacy_authorization + described_class.call + + expect { described_class.call }.not_to raise_error + expect(authorization_keys.length).to eq(1) + end + + it "backfills the digest for rows created while the old code was still running" do + auth = legacy_authorization + + described_class.call + + expect(auth.reload.token_digest).to eq(Digest::SHA256.hexdigest("legacytoken")) + end + + it "falls back to the stored permissions when the legacy Redis key is gone" do + auth = create :remote_storage_authorization, user: user, permissions: %w(photos:rw) + auth.update_columns(token: "legacytoken", token_digest: nil) + authorization_keys.each { |key| redis.del(key) } + + described_class.call + + digest = Digest::SHA256.hexdigest("legacytoken") + expect(redis.smembers("authorizations:#{user.cn}:#{digest}").sort).to eq(%w(photos:rw)) + end + + it "removes leftover non-digest authorization keys" do + create :remote_storage_authorization, user: user + redis.sadd("authorizations:#{user.cn}:leftovertoken", %w(photos)) + + result = described_class.call + + expect(result.legacy_removed).to eq(1) + expect(redis.exists?("authorizations:#{user.cn}:leftovertoken")).to be(false) + end + + it "keeps digest-shaped keys that are not in the database" do + fresh = Digest::SHA256.hexdigest("brand-new-token") + redis.sadd("authorizations:#{user.cn}:#{fresh}", %w(documents:rw)) + + result = described_class.call + + expect(redis.exists?("authorizations:#{user.cn}:#{fresh}")).to be(true) + expect(result.legacy_removed).to eq(0) + end + + it "is a no-op once the plaintext token column is gone" do + allow(RemoteStorageAuthorization).to receive(:column_names) + .and_return(RemoteStorageAuthorization.column_names - ["token"]) + + expect(described_class.call).to have_attributes(skipped: true, rekeyed: 0) + end + + describe "permissions parsing" do + subject(:service) { described_class.new } + + it "parses a JSON array string" do + expect(service.send(:parse_permissions, '["documents:rw", "photos"]')) + .to eq(%w(documents:rw photos)) + end + + it "parses a YAML array string" do + expect(service.send(:parse_permissions, "---\n- documents:rw\n- photos\n")) + .to eq(%w(documents:rw photos)) + end + + it "parses a Postgres brace array string" do + expect(service.send(:parse_permissions, "{documents:rw,photos}")) + .to eq(%w(documents:rw photos)) + end + end +end -- 2.50.1