From ea80608b15267f68365099c229d1ac37bd9a31fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= <mksionek@gitlab.com>
Date: Mon, 15 Feb 2021 16:56:37 +0100
Subject: [PATCH] Clean up active session file
From to-do items
Remove binding.pry
Fix warden to-do
Add warden guard clause
Add changelog entry
Change naming in changelog
---
.../profiles/active_sessions_controller.rb | 5 +-
app/helpers/active_sessions_helper.rb | 7 +-
app/models/active_session.rb | 50 +-------
.../security-clean-up-active-sessions.yml | 5 +
config/initializers/warden.rb | 3 +-
.../active_sessions_controller_spec.rb | 2 +-
spec/models/active_session_spec.rb | 115 +-----------------
7 files changed, 14 insertions(+), 173 deletions(-)
create mode 100644 changelogs/unreleased/security-clean-up-active-sessions.yml
diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb
index 1233c906406de..aafd7c2b65b18 100644
--- a/app/controllers/profiles/active_sessions_controller.rb
+++ b/app/controllers/profiles/active_sessions_controller.rb
@@ -8,9 +8,8 @@ def index
end
def destroy
- # params[:id] can be either an Rack::Session::SessionId#private_id
- # or an encrypted Rack::Session::SessionId#public_id
- ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id])
+ # params[:id] can be an Rack::Session::SessionId#private_id
+ ActiveSession.destroy_session(current_user, params[:id])
current_user.forget_me!
respond_to do |format|
diff --git a/app/helpers/active_sessions_helper.rb b/app/helpers/active_sessions_helper.rb
index 322c5b3b16d73..cfe0b747e78f4 100644
--- a/app/helpers/active_sessions_helper.rb
+++ b/app/helpers/active_sessions_helper.rb
@@ -24,11 +24,6 @@ def active_session_device_type_icon(active_session)
end
def revoke_session_path(active_session)
- if active_session.session_private_id
- profile_active_session_path(active_session.session_private_id)
- else
- # TODO: remove in 13.7
- profile_active_session_path(active_session.public_id)
- end
+ profile_active_session_path(active_session.session_private_id)
end
end
diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index 823685f78f49c..a0e74c7f48eaa 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -42,13 +42,6 @@ def human_device_type
device_type&.titleize
end
- # This is not the same as Rack::Session::SessionId#public_id, but we
- # need to preserve this for backwards compatibility.
- # TODO: remove in 13.7
- def public_id
- Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
- end
-
def self.set(user, request)
Gitlab::Redis::SharedState.with do |redis|
session_private_id = request.session.id.private_id
@@ -63,8 +56,6 @@ def self.set(user, request)
device_type: client.device_type,
created_at: user.current_sign_in_at || timestamp,
updated_at: timestamp,
- # TODO: remove in 13.7
- session_id: request.session.id.public_id,
session_private_id: session_private_id,
is_impersonated: request.session[:impersonator_id].present?
)
@@ -80,20 +71,10 @@ def self.set(user, request)
lookup_key_name(user.id),
session_private_id
)
-
- # We remove the ActiveSession stored by using public_id to avoid
- # duplicate entries
- remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id)
end
end
end
- # TODO: remove in 13.7
- private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id)
- redis.srem(lookup_key_name(user_id), rack_session_public_id)
- redis.del(key_name(user_id, rack_session_public_id))
- end
-
def self.list(user)
Gitlab::Redis::SharedState.with do |redis|
cleaned_up_lookup_entries(redis, user).map do |raw_session|
@@ -109,18 +90,6 @@ def self.cleanup(user)
end
end
- # TODO: remove in 13.7
- # After upgrade there might be a duplicate ActiveSessions:
- # - one with the public_id stored in #session_id
- # - another with private_id stored in #session_private_id
- def self.destroy_with_rack_session_id(user, rack_session_id)
- return unless rack_session_id
-
- Gitlab::Redis::SharedState.with do |redis|
- destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id])
- end
- end
-
def self.destroy_sessions(redis, user, session_ids)
key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
@@ -132,19 +101,11 @@ def self.destroy_sessions(redis, user, session_ids)
end
end
- # TODO: remove in 13.7
- # After upgrade, .destroy might be called with the session id encrypted
- # by .public_id.
- def self.destroy_with_deprecated_encryption(user, session_id)
+ def self.destroy_session(user, session_id)
return unless session_id
- decrypted_session_id = decrypt_public_id(session_id)
- rack_session_private_id = if decrypted_session_id
- Rack::Session::SessionId.new(decrypted_session_id).private_id
- end
-
Gitlab::Redis::SharedState.with do |redis|
- destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
+ destroy_sessions(redis, user, [session_id].compact)
end
end
@@ -275,11 +236,4 @@ def self.cleaned_up_lookup_entries(redis, user)
entries.compact
end
-
- # TODO: remove in 13.7
- private_class_method def self.decrypt_public_id(public_id)
- Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
- rescue
- nil
- end
end
diff --git a/changelogs/unreleased/security-clean-up-active-sessions.yml b/changelogs/unreleased/security-clean-up-active-sessions.yml
new file mode 100644
index 0000000000000..49d24584ddbb0
--- /dev/null
+++ b/changelogs/unreleased/security-clean-up-active-sessions.yml
@@ -0,0 +1,5 @@
+---
+title: Do not store marshalled sessions ids in Redis
+merge_request:
+author:
+type: security
diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb
index 2517c0cf5c29b..88f2a13df60c9 100644
--- a/config/initializers/warden.rb
+++ b/config/initializers/warden.rb
@@ -42,8 +42,7 @@
activity = Gitlab::Auth::Activity.new(opts)
tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth)
- # TODO: switch to `auth.request.session.id.private_id` in 13.7
- ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id)
+ ActiveSession.destroy_session(user, auth.request.session.id.private_id) if auth.request.session.id
activity.user_session_destroyed!
##
diff --git a/spec/controllers/profiles/active_sessions_controller_spec.rb b/spec/controllers/profiles/active_sessions_controller_spec.rb
index f54f69d853dba..12cf4f982e98a 100644
--- a/spec/controllers/profiles/active_sessions_controller_spec.rb
+++ b/spec/controllers/profiles/active_sessions_controller_spec.rb
@@ -12,7 +12,7 @@
it 'invalidates all remember user tokens' do
ActiveSession.set(user, request)
- session_id = request.session.id.public_id
+ session_id = request.session.id.private_id
user.remember_me!
delete :destroy, params: { id: session_id }
diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb
index 51435cc4342af..2fd7b127500e6 100644
--- a/spec/models/active_session_spec.rb
+++ b/spec/models/active_session_spec.rb
@@ -42,17 +42,6 @@
end
end
- describe '#public_id' do
- it 'returns an encrypted, url-encoded session id' do
- original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
- active_session = ActiveSession.new(session_id: original_session_id.public_id)
- encrypted_id = active_session.public_id
- derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
-
- expect(original_session_id.public_id).to eq derived_session_id
- end
- end
-
describe '.list' do
it 'returns all sessions by user' do
Gitlab::Redis::SharedState.with do |redis|
@@ -207,89 +196,9 @@
end
end
end
-
- context 'ActiveSession stored by deprecated rack_session_public_key' do
- let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
- let(:deprecated_active_session_lookup_key) { rack_session.public_id }
-
- before do
- Gitlab::Redis::SharedState.with do |redis|
- redis.set("session:user:gitlab:#{user.id}:#{deprecated_active_session_lookup_key}",
- '')
- redis.sadd(described_class.lookup_key_name(user.id),
- deprecated_active_session_lookup_key)
- end
- end
-
- it 'removes deprecated key and stores only new one' do
- expected_session_keys = ["session:user:gitlab:#{user.id}:#{rack_session.private_id}",
- "session:lookup:user:gitlab:#{user.id}"]
-
- ActiveSession.set(user, request)
-
- Gitlab::Redis::SharedState.with do |redis|
- actual_session_keys = redis.scan_each(match: 'session:*').to_a
- expect(actual_session_keys).to(match_array(expected_session_keys))
-
- expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq [rack_session.private_id]
- end
- end
- end
end
- describe '.destroy_with_rack_session_id' do
- it 'gracefully handles a nil session ID' do
- expect(described_class).not_to receive(:destroy_sessions)
-
- ActiveSession.destroy_with_rack_session_id(user, nil)
- end
-
- it 'removes the entry associated with the currently killed user session' do
- Gitlab::Redis::SharedState.with do |redis|
- redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
- redis.set("session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d", '')
- redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '')
- end
-
- ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
- Gitlab::Redis::SharedState.with do |redis|
- expect(redis.scan_each(match: "session:user:gitlab:*")).to match_array [
- "session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d",
- "session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358"
- ]
- end
- end
-
- it 'removes the lookup entry' do
- Gitlab::Redis::SharedState.with do |redis|
- redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
- redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d')
- end
-
- ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
- Gitlab::Redis::SharedState.with do |redis|
- expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty
- end
- end
-
- it 'removes the devise session' do
- Gitlab::Redis::SharedState.with do |redis|
- redis.set("session:user:gitlab:#{user.id}:#{rack_session.private_id}", '')
- # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
- redis.set("session:gitlab:#{rack_session.private_id}", '')
- end
-
- ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
- Gitlab::Redis::SharedState.with do |redis|
- expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
- end
- end
- end
-
- describe '.destroy_with_deprecated_encryption' do
+ describe '.destroy_session' do
shared_examples 'removes all session data' do
before do
Gitlab::Redis::SharedState.with do |redis|
@@ -330,7 +239,7 @@
end
context 'destroy called with Rack::Session::SessionId#private_id' do
- subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) }
+ subject { ActiveSession.destroy_session(user, rack_session.private_id) }
it 'calls .destroy_sessions' do
expect(ActiveSession).to(
@@ -347,26 +256,6 @@
include_examples 'removes all session data'
end
end
-
- context 'destroy called with ActiveSession#public_id (deprecated)' do
- let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
- let(:encrypted_active_session_id) { active_session.public_id }
- let(:active_session_lookup_key) { rack_session.public_id }
-
- subject { ActiveSession.destroy_with_deprecated_encryption(user, encrypted_active_session_id) }
-
- it 'calls .destroy_sessions' do
- expect(ActiveSession).to(
- receive(:destroy_sessions)
- .with(anything, user, [encrypted_active_session_id, rack_session.public_id, rack_session.private_id]))
-
- subject
- end
-
- context 'ActiveSession with session_id (deprecated)' do
- include_examples 'removes all session data'
- end
- end
end
describe '.destroy_all_but_current' do
--
GitLab