Commit fe5f7593 authored by Cindy Pallares 🦉's avatar Cindy Pallares 🦉

Merge branch 'security-fix-pat-web-access' into 'master'

[master] Resolve "Personal access token with only `read_user` scope can be used to authenticate any web request"

See merge request gitlab/gitlabhq!2583
parent e122e14a
...@@ -12,11 +12,11 @@ class ApplicationController < ActionController::Base ...@@ -12,11 +12,11 @@ class ApplicationController < ActionController::Base
include WorkhorseHelper include WorkhorseHelper
include EnforcesTwoFactorAuthentication include EnforcesTwoFactorAuthentication
include WithPerformanceBar include WithPerformanceBar
include SessionlessAuthentication
# this can be removed after switching to rails 5 # this can be removed after switching to rails 5
# https://gitlab.com/gitlab-org/gitlab-ce/issues/51908 # https://gitlab.com/gitlab-org/gitlab-ce/issues/51908
include InvalidUTF8ErrorHandler unless Gitlab.rails5? include InvalidUTF8ErrorHandler unless Gitlab.rails5?
before_action :authenticate_sessionless_user!
before_action :authenticate_user! before_action :authenticate_user!
before_action :enforce_terms!, if: :should_enforce_terms? before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket! before_action :validate_user_service_ticket!
...@@ -153,13 +153,6 @@ class ApplicationController < ActionController::Base ...@@ -153,13 +153,6 @@ class ApplicationController < ActionController::Base
end end
end end
# This filter handles personal access tokens, and atom requests with rss tokens
def authenticate_sessionless_user!
user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user
sessionless_sign_in(user) if user
end
def log_exception(exception) def log_exception(exception)
Raven.capture_exception(exception) if sentry_enabled? Raven.capture_exception(exception) if sentry_enabled?
...@@ -426,25 +419,11 @@ class ApplicationController < ActionController::Base ...@@ -426,25 +419,11 @@ class ApplicationController < ActionController::Base
Gitlab::I18n.with_user_locale(current_user, &block) Gitlab::I18n.with_user_locale(current_user, &block)
end end
def sessionless_sign_in(user)
if user && can?(user, :log_in)
# Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed
# for every request. If you want the token to work as a
# sign in token, you can simply remove store: false.
sign_in(user, store: false, message: :sessionless_sign_in)
end
end
def set_page_title_header def set_page_title_header
# Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8 # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8
response.headers['Page-Title'] = URI.escape(page_title('GitLab')) response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end end
def sessionless_user?
current_user && !session.keys.include?('warden.user.user.key')
end
def peek_request? def peek_request?
request.path.start_with?('/-/peek') request.path.start_with?('/-/peek')
end end
......
# frozen_string_literal: true
# == SessionlessAuthentication
#
# Controller concern to handle PAT and RSS token authentication methods
#
module SessionlessAuthentication
# This filter handles personal access tokens, and atom requests with rss tokens
def authenticate_sessionless_user!(request_format)
user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
sessionless_sign_in(user) if user
end
def sessionless_user?
current_user && !session.keys.include?('warden.user.user.key')
end
def sessionless_sign_in(user)
if user && can?(user, :log_in)
# Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed
# for every request. If you want the token to work as a
# sign in token, you can simply remove store: false.
sign_in(user, store: false, message: :sessionless_sign_in)
end
end
end
...@@ -4,6 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController ...@@ -4,6 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController
include ParamsBackwardCompatibility include ParamsBackwardCompatibility
include RendersMemberAccess include RendersMemberAccess
prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
before_action :set_non_archived_param before_action :set_non_archived_param
before_action :default_sorting before_action :default_sorting
skip_cross_project_access_check :index, :starred skip_cross_project_access_check :index, :starred
......
...@@ -4,6 +4,9 @@ class DashboardController < Dashboard::ApplicationController ...@@ -4,6 +4,9 @@ class DashboardController < Dashboard::ApplicationController
include IssuesAction include IssuesAction
include MergeRequestsAction include MergeRequestsAction
prepend_before_action(only: [:issues]) { authenticate_sessionless_user!(:rss) }
prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) }
before_action :event_filter, only: :activity before_action :event_filter, only: :activity
before_action :projects, only: [:issues, :merge_requests] before_action :projects, only: [:issues, :merge_requests]
before_action :set_show_full_reference, only: [:issues, :merge_requests] before_action :set_show_full_reference, only: [:issues, :merge_requests]
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
class GraphqlController < ApplicationController class GraphqlController < ApplicationController
# Unauthenticated users have access to the API for public data # Unauthenticated users have access to the API for public data
skip_before_action :authenticate_user! skip_before_action :authenticate_user!
prepend_before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
before_action :check_graphql_feature_flag! before_action :check_graphql_feature_flag!
......
...@@ -9,6 +9,9 @@ class GroupsController < Groups::ApplicationController ...@@ -9,6 +9,9 @@ class GroupsController < Groups::ApplicationController
respond_to :html respond_to :html
prepend_before_action(only: [:show, :issues]) { authenticate_sessionless_user!(:rss) }
prepend_before_action(only: [:issues_calendar]) { authenticate_sessionless_user!(:ics) }
before_action :authenticate_user!, only: [:new, :create] before_action :authenticate_user!, only: [:new, :create]
before_action :group, except: [:index, :new, :create] before_action :group, except: [:index, :new, :create]
......
...@@ -6,6 +6,7 @@ class Projects::CommitsController < Projects::ApplicationController ...@@ -6,6 +6,7 @@ class Projects::CommitsController < Projects::ApplicationController
include ExtractsPath include ExtractsPath
include RendersCommits include RendersCommits
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
before_action :whitelist_query_limiting, except: :commits_root before_action :whitelist_query_limiting, except: :commits_root
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :assign_ref_vars, except: :commits_root before_action :assign_ref_vars, except: :commits_root
......
...@@ -9,10 +9,6 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -9,10 +9,6 @@ class Projects::IssuesController < Projects::ApplicationController
include IssuesCalendar include IssuesCalendar
include SpammableActions include SpammableActions
def self.authenticate_user_only_actions
%i[new]
end
def self.issue_except_actions def self.issue_except_actions
%i[index calendar new create bulk_update] %i[index calendar new create bulk_update]
end end
...@@ -21,7 +17,10 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -21,7 +17,10 @@ class Projects::IssuesController < Projects::ApplicationController
%i[index calendar] %i[index calendar]
end end
prepend_before_action :authenticate_user!, only: authenticate_user_only_actions prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
prepend_before_action(only: [:calendar]) { authenticate_sessionless_user!(:ics) }
prepend_before_action :authenticate_new_issue!, only: [:new]
prepend_before_action :store_uri, only: [:new, :show]
before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
before_action :check_issues_available! before_action :check_issues_available!
...@@ -232,16 +231,18 @@ class Projects::IssuesController < Projects::ApplicationController ...@@ -232,16 +231,18 @@ class Projects::IssuesController < Projects::ApplicationController
] + [{ label_ids: [], assignee_ids: [] }] ] + [{ label_ids: [], assignee_ids: [] }]
end end
def authenticate_user! def authenticate_new_issue!
return if current_user return if current_user
notice = "Please sign in to create the new issue." notice = "Please sign in to create the new issue."
redirect_to new_user_session_path, notice: notice
end
def store_uri
if request.get? && !request.xhr? if request.get? && !request.xhr?
store_location_for :user, request.fullpath store_location_for :user, request.fullpath
end end
redirect_to new_user_session_path, notice: notice
end end
def serializer def serializer
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
class Projects::TagsController < Projects::ApplicationController class Projects::TagsController < Projects::ApplicationController
include SortingHelper include SortingHelper
prepend_before_action(only: [:index]) { authenticate_sessionless_user!(:rss) }
# Authorize # Authorize
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :authorize_download_code! before_action :authorize_download_code!
......
...@@ -7,6 +7,8 @@ class ProjectsController < Projects::ApplicationController ...@@ -7,6 +7,8 @@ class ProjectsController < Projects::ApplicationController
include PreviewMarkdown include PreviewMarkdown
include SendFileUpload include SendFileUpload
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
before_action :whitelist_query_limiting, only: [:create] before_action :whitelist_query_limiting, only: [:create]
before_action :authenticate_user!, except: [:index, :show, :activity, :refs] before_action :authenticate_user!, except: [:index, :show, :activity, :refs]
before_action :redirect_git_extension, only: [:show] before_action :redirect_git_extension, only: [:show]
......
...@@ -14,6 +14,7 @@ class UsersController < ApplicationController ...@@ -14,6 +14,7 @@ class UsersController < ApplicationController
calendar_activities: true calendar_activities: true
skip_before_action :authenticate_user! skip_before_action :authenticate_user!
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
before_action :user, except: [:exists] before_action :user, except: [:exists]
before_action :authorize_read_user_profile!, before_action :authorize_read_user_profile!,
only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :snippets] only: [:calendar, :calendar_activities, :groups, :projects, :contributed_projects, :snippets]
......
---
title: Restrict Personal Access Tokens to API scope on web requests
merge_request:
author:
type: security
...@@ -33,22 +33,22 @@ class Rack::Attack ...@@ -33,22 +33,22 @@ class Rack::Attack
throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req| throttle('throttle_authenticated_api', Gitlab::Throttle.authenticated_api_options) do |req|
Gitlab::Throttle.settings.throttle_authenticated_api_enabled && Gitlab::Throttle.settings.throttle_authenticated_api_enabled &&
req.api_request? && req.api_request? &&
req.authenticated_user_id req.authenticated_user_id([:api])
end end
throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req| throttle('throttle_authenticated_web', Gitlab::Throttle.authenticated_web_options) do |req|
Gitlab::Throttle.settings.throttle_authenticated_web_enabled && Gitlab::Throttle.settings.throttle_authenticated_web_enabled &&
req.web_request? && req.web_request? &&
req.authenticated_user_id req.authenticated_user_id([:api, :rss, :ics])
end end
class Request class Request
def unauthenticated? def unauthenticated?
!authenticated_user_id !authenticated_user_id([:api, :rss, :ics])
end end
def authenticated_user_id def authenticated_user_id(request_formats)
Gitlab::Auth::RequestAuthenticator.new(self).user&.id Gitlab::Auth::RequestAuthenticator.new(self).user(request_formats)&.id
end end
def api_request? def api_request?
......
...@@ -13,12 +13,18 @@ module Gitlab ...@@ -13,12 +13,18 @@ module Gitlab
@request = request @request = request
end end
def user def user(request_formats)
find_sessionless_user || find_user_from_warden request_formats.each do |format|
user = find_sessionless_user(format)
return user if user
end
find_user_from_warden
end end
def find_sessionless_user def find_sessionless_user(request_format)
find_user_from_access_token || find_user_from_feed_token find_user_from_web_access_token(request_format) || find_user_from_feed_token(request_format)
rescue Gitlab::Auth::AuthenticationError rescue Gitlab::Auth::AuthenticationError
nil nil
end end
......
...@@ -27,8 +27,8 @@ module Gitlab ...@@ -27,8 +27,8 @@ module Gitlab
current_request.env['warden']&.authenticate if verified_request? current_request.env['warden']&.authenticate if verified_request?
end end
def find_user_from_feed_token def find_user_from_feed_token(request_format)
return unless rss_request? || ics_request? return unless valid_rss_format?(request_format)
# NOTE: feed_token was renamed from rss_token but both needs to be supported because # NOTE: feed_token was renamed from rss_token but both needs to be supported because
# users might have already added the feed to their RSS reader before the rename # users might have already added the feed to their RSS reader before the rename
...@@ -38,6 +38,17 @@ module Gitlab ...@@ -38,6 +38,17 @@ module Gitlab
User.find_by_feed_token(token) || raise(UnauthorizedError) User.find_by_feed_token(token) || raise(UnauthorizedError)
end end
# We only allow Private Access Tokens with `api` scope to be used by web
# requests on RSS feeds or ICS files for backwards compatibility.
# It is also used by GraphQL/API requests.
def find_user_from_web_access_token(request_format)
return unless access_token && valid_web_access_format?(request_format)
validate_access_token!(scopes: [:api])
access_token.user || raise(UnauthorizedError)
end
def find_user_from_access_token def find_user_from_access_token
return unless access_token return unless access_token
...@@ -109,6 +120,26 @@ module Gitlab ...@@ -109,6 +120,26 @@ module Gitlab
@current_request ||= ensure_action_dispatch_request(request) @current_request ||= ensure_action_dispatch_request(request)
end end
def valid_web_access_format?(request_format)
case request_format
when :rss
rss_request?
when :ics
ics_request?
when :api
api_request?
end
end
def valid_rss_format?(request_format)
case request_format
when :rss
rss_request?
when :ics
ics_request?
end
end
def rss_request? def rss_request?
current_request.path.ends_with?('.atom') || current_request.format.atom? current_request.path.ends_with?('.atom') || current_request.format.atom?
end end
...@@ -116,6 +147,10 @@ module Gitlab ...@@ -116,6 +147,10 @@ module Gitlab
def ics_request? def ics_request?
current_request.path.ends_with?('.ics') || current_request.format.ics? current_request.path.ends_with?('.ics') || current_request.format.ics?
end end
def api_request?
current_request.path.starts_with?("/api/")
end
end end
end end
end end
...@@ -107,59 +107,6 @@ describe ApplicationController do ...@@ -107,59 +107,6 @@ describe ApplicationController do
end end
end end
describe "#authenticate_user_from_personal_access_token!" do
before do
stub_authentication_activity_metrics(debug: false)
end
controller(described_class) do
def index
render text: 'authenticated'
end
end
let(:personal_access_token) { create(:personal_access_token, user: user) }
context "when the 'personal_access_token' param is populated with the personal access token" do
it "logs the user in" do
expect(authentication_metrics)
.to increment(:user_authenticated_counter)
.and increment(:user_session_override_counter)
.and increment(:user_sessionless_authentication_counter)
get :index, private_token: personal_access_token.token
expect(response).to have_gitlab_http_status(200)
expect(response.body).to eq('authenticated')
end
end
context "when the 'PERSONAL_ACCESS_TOKEN' header is populated with the personal access token" do
it "logs the user in" do
expect(authentication_metrics)
.to increment(:user_authenticated_counter)
.and increment(:user_session_override_counter)
.and increment(:user_sessionless_authentication_counter)
@request.headers["PRIVATE-TOKEN"] = personal_access_token.token
get :index
expect(response).to have_gitlab_http_status(200)
expect(response.body).to eq('authenticated')
end
end
it "doesn't log the user in otherwise" do
expect(authentication_metrics)
.to increment(:user_unauthenticated_counter)
get :index, private_token: "token"
expect(response.status).not_to eq(200)
expect(response.body).not_to eq('authenticated')
end
end
describe 'session expiration' do describe 'session expiration' do
controller(described_class) do controller(described_class) do
# The anonymous controller will report 401 and fail to run any actions. # The anonymous controller will report 401 and fail to run any actions.
...@@ -224,74 +171,6 @@ describe ApplicationController do ...@@ -224,74 +171,6 @@ describe ApplicationController do
end end
end end
describe '#authenticate_sessionless_user!' do
before do
stub_authentication_activity_metrics(debug: false)
end
describe 'authenticating a user from a feed token' do
controller(described_class) do
def index
render text: 'authenticated'
end
end
context "when the 'feed_token' param is populated with the feed token" do
context 'when the request format is atom' do
it "logs the user in" do
expect(authentication_metrics)
.to increment(:user_authenticated_counter)
.and increment(:user_session_override_counter)
.and increment(:user_sessionless_authentication_counter)
get :index, feed_token: user.feed_token, format: :atom
expect(response).to have_gitlab_http_status 200
expect(response.body).to eq 'authenticated'
end
end
context 'when the request format is ics' do
it "logs the user in" do
expect(authentication_metrics)
.to increment(:user_authenticated_counter)
.and increment(:user_session_override_counter)
.and increment(:user_sessionless_authentication_counter)
get :index, feed_token: user.feed_token, format: :ics
expect(response).to have_gitlab_http_status 200
expect(response.body).to eq 'authenticated'
end
end
context 'when the request format is neither atom nor ics' do
it "doesn't log the user in" do
expect(authentication_metrics)
.to increment(:user_unauthenticated_counter)
get :index, feed_token: user.feed_token
expect(response.status).not_to have_gitlab_http_status 200
expect(response.body).not_to eq 'authenticated'
end
end
end
context "when the 'feed_token' param is populated with an invalid feed token" do
it "doesn't log the user" do
expect(authentication_metrics)
.to increment(:user_unauthenticated_counter)
get :index, feed_token: 'token', format: :atom
expect(response.status).not_to eq 200
expect(response.body).not_to eq 'authenticated'
end
end
end
end
describe '#route_not_found' do describe '#route_not_found' do
it 'renders 404 if authenticated' do it 'renders 404 if authenticated' do
allow(controller).to receive(:current_user).and_return(user) allow(controller).to receive(:current_user).and_return(user)
...@@ -557,36 +436,6 @@ describe ApplicationController do ...@@ -557,36 +436,6 @@ describe ApplicationController do
expect(response).to have_gitlab_http_status(200) expect(response).to have_gitlab_http_status(200)
end end
context 'for sessionless users' do
render_views
before do
sign_out user
end
it 'renders a 403 when the sessionless user did not accept the terms' do
get :index, feed_token: user.feed_token, format: :atom
expect(response).to have_gitlab_http_status(403)
end
it 'renders the error message when the format was html' do
get :index,
private_token: create(:personal_access_token, user: user).token,
format: :html
expect(response.body).to have_content /accept the terms of service/i
end
it 'renders a 200 when the sessionless user accepted the terms' do
accept_terms(user)
get :index, feed_token: user.feed_token, format: :atom
expect(response).to have_gitlab_http_status(200)
end
end
end end
end end
......
require 'spec_helper'
describe Dashboard::ProjectsController do
it_behaves_like 'authenticates sessionless user', :index, :atom
end
require 'spec_helper' require 'spec_helper'
describe DashboardController do describe DashboardController do
let(:user) { create(:user) } context 'signed in' do
let(:project) { create(:project) } let(:user) { create(:user) }
let(:project) { create(:project) }
before do before do
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
end end
describe 'GET issues' do describe 'GET issues' do
it_behaves_like 'issuables list meta-data', :issue, :issues it_behaves_like 'issuables list meta-data', :issue, :issues
it_behaves_like 'issuables requiring filter', :issues it_behaves_like 'issuables requiring filter', :issues
end end
describe 'GET merge requests' do describe 'GET merge requests' do
it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests it_behaves_like 'issuables list meta-data', :merge_request, :merge_requests
it_behaves_like 'issuables requiring filter', :merge_requests it_behaves_like 'issuables requiring filter', :merge_requests
end
end end
it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first
it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics
end end
...@@ -52,15 +52,58 @@ describe GraphqlController do ...@@ -52,15 +52,58 @@ describe GraphqlController do
end end
end end
context 'token authentication' do
before do
stub_authentication_activity_metrics(debug: false)
end
let(:user) { create(:user, username: 'Simon') }
let(:personal_access_token) { create(:personal_access_token, user: user) }
context "when the 'personal_access_token' param is populated with the personal access token" do
it 'logs the user in' do
expect(authentication_metrics)
.to increment(:user_authenticated_counter)
.and increment(:user_session_override_counter)
.and increment(:user_sessionless_authentication_counter)
run_test_query!(private_token: personal_access_token.token)
expect(response).to have_gitlab_http_status(200)
expect(query_response).to eq('echo' => '"Simon" says: test success')
end
end
context 'when the personal access token has no api scope' do
it 'does not log the user in' do
personal_access_token.update(scopes: [:read_user])