summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--Gemfile.lock4
-rw-r--r--app/controllers/concerns/sessionless_authentication.rb6
-rw-r--r--app/controllers/graphql_controller.rb8
-rw-r--r--app/graphql/mutations/base_mutation.rb13
-rw-r--r--app/policies/global_policy.rb4
-rw-r--r--app/services/auth/dependency_proxy_authentication_service.rb5
-rw-r--r--changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml5
-rw-r--r--changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml5
-rw-r--r--changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml5
-rw-r--r--lib/gitlab/auth/scope_validator.rb24
-rw-r--r--lib/gitlab/graphql/authorize/object_authorization.rb19
-rw-r--r--spec/requests/api/graphql/mutations/notes/create/note_spec.rb2
-rw-r--r--spec/requests/jwt_controller_spec.rb10
-rw-r--r--spec/services/auth/dependency_proxy_authentication_service_spec.rb25
-rw-r--r--spec/services/projects/download_service_spec.rb5
-rw-r--r--spec/support/helpers/graphql_helpers.rb18
-rw-r--r--spec/support/shared_examples/requests/graphql_shared_examples.rb46
17 files changed, 168 insertions, 36 deletions
diff --git a/Gemfile.lock b/Gemfile.lock
index 19fad573b63..007ca87b465 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -171,10 +171,11 @@ GEM
capybara-screenshot (1.0.22)
capybara (>= 1.0, < 4)
launchy
- carrierwave (1.3.1)
+ carrierwave (1.3.2)
activemodel (>= 4.0.0)
activesupport (>= 4.0.0)
mime-types (>= 1.16)
+ ssrf_filter (~> 1.0)
cbor (0.5.9.6)
character_set (1.4.0)
charlock_holmes (0.7.7)
@@ -1205,6 +1206,7 @@ GEM
sprockets (>= 3.0.0)
sqlite3 (1.3.13)
sshkey (2.0.0)
+ ssrf_filter (1.0.7)
stackprof (0.2.15)
state_machines (0.5.0)
state_machines-activemodel (0.8.0)
diff --git a/app/controllers/concerns/sessionless_authentication.rb b/app/controllers/concerns/sessionless_authentication.rb
index 882fef7a342..3c8a683439a 100644
--- a/app/controllers/concerns/sessionless_authentication.rb
+++ b/app/controllers/concerns/sessionless_authentication.rb
@@ -7,11 +7,15 @@
module SessionlessAuthentication
# This filter handles personal access tokens, atom requests with rss tokens, and static object tokens
def authenticate_sessionless_user!(request_format)
- user = Gitlab::Auth::RequestAuthenticator.new(request).find_sessionless_user(request_format)
+ user = request_authenticator.find_sessionless_user(request_format)
sessionless_sign_in(user) if user
end
+ def request_authenticator
+ @request_authenticator ||= Gitlab::Auth::RequestAuthenticator.new(request)
+ end
+
def sessionless_user?
current_user && !session.key?('warden.user.user.key')
end
diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb
index a13ec1daddb..38bfb5ef2f8 100644
--- a/app/controllers/graphql_controller.rb
+++ b/app/controllers/graphql_controller.rb
@@ -110,7 +110,13 @@ class GraphqlController < ApplicationController
end
def context
- @context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user?, request: request }
+ api_user = !!sessionless_user?
+ @context ||= {
+ current_user: current_user,
+ is_sessionless_user: api_user,
+ request: request,
+ scope_validator: ::Gitlab::Auth::ScopeValidator.new(api_user, request_authenticator)
+ }
end
def build_variables(variable_info)
diff --git a/app/graphql/mutations/base_mutation.rb b/app/graphql/mutations/base_mutation.rb
index 1f18a37fcb9..da658e1f108 100644
--- a/app/graphql/mutations/base_mutation.rb
+++ b/app/graphql/mutations/base_mutation.rb
@@ -44,9 +44,18 @@ module Mutations
end
end
+ def self.authorizes_object?
+ true
+ end
+
def self.authorized?(object, context)
- # we never provide an object to mutations, but we do need to have a user.
- context[:current_user].present? && !context[:current_user].blocked?
+ auth = ::Gitlab::Graphql::Authorize::ObjectAuthorization.new(:execute_graphql_mutation, :api)
+
+ return true if auth.ok?(:global, context[:current_user],
+ scope_validator: context[:scope_validator])
+
+ # in our mutations we raise, rather than returning a null value.
+ raise_resource_not_available_error!
end
# See: AuthorizeResource#authorized_resource?
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 5ee34ebbb2f..d16c4734b2c 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -23,6 +23,7 @@ class GlobalPolicy < BasePolicy
prevent :receive_notifications
prevent :use_quick_actions
prevent :create_group
+ prevent :execute_graphql_mutation
end
rule { default }.policy do
@@ -32,6 +33,7 @@ class GlobalPolicy < BasePolicy
enable :receive_notifications
enable :use_quick_actions
enable :use_slash_commands
+ enable :execute_graphql_mutation
end
rule { inactive }.policy do
@@ -48,6 +50,8 @@ class GlobalPolicy < BasePolicy
prevent :use_slash_commands
end
+ rule { ~can?(:access_api) }.prevent :execute_graphql_mutation
+
rule { blocked | (internal & ~migration_bot & ~security_bot) }.policy do
prevent :access_git
end
diff --git a/app/services/auth/dependency_proxy_authentication_service.rb b/app/services/auth/dependency_proxy_authentication_service.rb
index 1b8c16b7c79..fab42e0ebb6 100644
--- a/app/services/auth/dependency_proxy_authentication_service.rb
+++ b/app/services/auth/dependency_proxy_authentication_service.rb
@@ -8,7 +8,10 @@ module Auth
def execute(authentication_abilities:)
return error('dependency proxy not enabled', 404) unless ::Gitlab.config.dependency_proxy.enabled
- return error('access forbidden', 403) unless current_user
+
+ # Because app/controllers/concerns/dependency_proxy/auth.rb consumes this
+ # JWT only as `User.find`, we currently only allow User (not DeployToken, etc)
+ return error('access forbidden', 403) unless current_user.is_a?(User)
{ token: authorized_token.encoded }
end
diff --git a/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml b/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml
new file mode 100644
index 00000000000..1758390a5cb
--- /dev/null
+++ b/changelogs/unreleased/327155-prevent-mutation-execution-with-read-api-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent tokens with only read_api scope from executing mutations
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml b/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml
new file mode 100644
index 00000000000..26137a42420
--- /dev/null
+++ b/changelogs/unreleased/security-10io-dependency-proxy-and-deploy-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Do not allow deploy tokens in the dependency proxy authentication service
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml b/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml
new file mode 100644
index 00000000000..4e31f9d855d
--- /dev/null
+++ b/changelogs/unreleased/security-bump-carrierwave-to-1-3-2.yml
@@ -0,0 +1,5 @@
+---
+title: Bump Carrierwave gem to v1.3.2
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/auth/scope_validator.rb b/lib/gitlab/auth/scope_validator.rb
new file mode 100644
index 00000000000..de4c36ad594
--- /dev/null
+++ b/lib/gitlab/auth/scope_validator.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+# Wrapper around a RequestAuthenticator to
+# perform authorization of scopes. Access is limited to
+# only those methods needed to validate that an API user
+# has at least one permitted scope.
+module Gitlab
+ module Auth
+ class ScopeValidator
+ def initialize(api_user, request_authenticator)
+ @api_user = api_user
+ @request_authenticator = request_authenticator
+ end
+
+ def valid_for?(permitted)
+ return true unless @api_user
+ return true if permitted.none?
+
+ scopes = permitted.map { |s| API::Scope.new(s) }
+ @request_authenticator.valid_access_token?(scopes: scopes)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/graphql/authorize/object_authorization.rb b/lib/gitlab/graphql/authorize/object_authorization.rb
index 0bc87108871..f13acc9ea27 100644
--- a/lib/gitlab/graphql/authorize/object_authorization.rb
+++ b/lib/gitlab/graphql/authorize/object_authorization.rb
@@ -4,10 +4,11 @@ module Gitlab
module Graphql
module Authorize
class ObjectAuthorization
- attr_reader :abilities
+ attr_reader :abilities, :permitted_scopes
- def initialize(abilities)
+ def initialize(abilities, scopes = %i[api read_api])
@abilities = Array.wrap(abilities).flatten
+ @permitted_scopes = Array.wrap(scopes)
end
def none?
@@ -18,7 +19,13 @@ module Gitlab
abilities.present?
end
- def ok?(object, current_user)
+ def ok?(object, current_user, scope_validator: nil)
+ scopes_ok?(scope_validator) && abilities_ok?(object, current_user)
+ end
+
+ private
+
+ def abilities_ok?(object, current_user)
return true if none?
subject = object.try(:declarative_policy_subject) || object
@@ -26,6 +33,12 @@ module Gitlab
Ability.allowed?(current_user, ability, subject)
end
end
+
+ def scopes_ok?(validator)
+ return true unless validator.present?
+
+ validator.valid_for?(permitted_scopes)
+ end
end
end
end
diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
index 1eed1c8e2ae..8dd8ed361ba 100644
--- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
+++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb
@@ -31,6 +31,8 @@ RSpec.describe 'Adding a Note' do
project.add_developer(current_user)
end
+ it_behaves_like 'a working GraphQL mutation'
+
it_behaves_like 'a Note mutation that creates a Note'
it_behaves_like 'a Note mutation when there are active record validation errors'
diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb
index 8be26784a3d..5b5658da97e 100644
--- a/spec/requests/jwt_controller_spec.rb
+++ b/spec/requests/jwt_controller_spec.rb
@@ -263,25 +263,21 @@ RSpec.describe JwtController do
let(:credential_user) { group_deploy_token.username }
let(:credential_password) { group_deploy_token.token }
- it_behaves_like 'with valid credentials'
+ it_behaves_like 'returning response status', :forbidden
end
context 'with project deploy token' do
let(:credential_user) { project_deploy_token.username }
let(:credential_password) { project_deploy_token.token }
- it_behaves_like 'with valid credentials'
+ it_behaves_like 'returning response status', :forbidden
end
context 'with invalid credentials' do
let(:credential_user) { 'foo' }
let(:credential_password) { 'bar' }
- it 'returns unauthorized' do
- subject
-
- expect(response).to have_gitlab_http_status(:unauthorized)
- end
+ it_behaves_like 'returning response status', :unauthorized
end
end
diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
index ba50149f53a..1fd1677c7da 100644
--- a/spec/services/auth/dependency_proxy_authentication_service_spec.rb
+++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb
@@ -13,28 +13,31 @@ RSpec.describe Auth::DependencyProxyAuthenticationService do
describe '#execute' do
subject { service.execute(authentication_abilities: nil) }
+ shared_examples 'returning' do |status:, message:|
+ it "returns #{message}", :aggregate_failures do
+ expect(subject[:http_status]).to eq(status)
+ expect(subject[:message]).to eq(message)
+ end
+ end
+
context 'dependency proxy is not enabled' do
before do
stub_config(dependency_proxy: { enabled: false })
end
- it 'returns not found' do
- result = subject
-
- expect(result[:http_status]).to eq(404)
- expect(result[:message]).to eq('dependency proxy not enabled')
- end
+ it_behaves_like 'returning', status: 404, message: 'dependency proxy not enabled'
end
context 'without a user' do
let(:user) { nil }
- it 'returns forbidden' do
- result = subject
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
+ end
+
+ context 'with a deploy token as user' do
+ let_it_be(:user) { create(:deploy_token) }
- expect(result[:http_status]).to eq(403)
- expect(result[:message]).to eq('access forbidden')
- end
+ it_behaves_like 'returning', status: 403, message: 'access forbidden'
end
context 'with a user' do
diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb
index 0f743eaa7f5..7d4fce814f5 100644
--- a/spec/services/projects/download_service_spec.rb
+++ b/spec/services/projects/download_service_spec.rb
@@ -20,8 +20,9 @@ RSpec.describe Projects::DownloadService do
context 'for URLs that are on the whitelist' do
before do
- stub_request(:get, 'http://mycompany.fogbugz.com/rails_sample.jpg').to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
- stub_request(:get, 'http://mycompany.fogbugz.com/doc_sample.txt').to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
+ # `ssrf_filter` resolves the hostname. See https://github.com/carrierwaveuploader/carrierwave/commit/91714adda998bc9e8decf5b1f5d260d808761304
+ stub_request(:get, %r{http://[\d\.]+/rails_sample.jpg}).to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg'))
+ stub_request(:get, %r{http://[\d\.]+/doc_sample.txt}).to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt'))
end
context 'an image file' do
diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb
index d714f04fbba..9d6c6ab93e4 100644
--- a/spec/support/helpers/graphql_helpers.rb
+++ b/spec/support/helpers/graphql_helpers.rb
@@ -396,17 +396,21 @@ module GraphqlHelpers
post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers
end
- def post_graphql(query, current_user: nil, variables: nil, headers: {})
+ def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {})
params = { query: query, variables: serialize_variables(variables) }
- post api('/', current_user, version: 'graphql'), params: params, headers: headers
+ post api('/', current_user, version: 'graphql', **token), params: params, headers: headers
- if graphql_errors # Errors are acceptable, but not this one:
- expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
- end
+ return unless graphql_errors
+
+ # Errors are acceptable, but not this one:
+ expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error'))
end
- def post_graphql_mutation(mutation, current_user: nil)
- post_graphql(mutation.query, current_user: current_user, variables: mutation.variables)
+ def post_graphql_mutation(mutation, current_user: nil, token: {})
+ post_graphql(mutation.query,
+ current_user: current_user,
+ variables: mutation.variables,
+ token: token)
end
def post_graphql_mutation_with_uploads(mutation, current_user: nil)
diff --git a/spec/support/shared_examples/requests/graphql_shared_examples.rb b/spec/support/shared_examples/requests/graphql_shared_examples.rb
index a66bc7112fe..d133c5ea641 100644
--- a/spec/support/shared_examples/requests/graphql_shared_examples.rb
+++ b/spec/support/shared_examples/requests/graphql_shared_examples.rb
@@ -10,6 +10,52 @@ RSpec.shared_examples 'a working graphql query' do
end
end
+RSpec.shared_examples 'a working GraphQL mutation' do
+ include GraphqlHelpers
+
+ before do
+ post_graphql_mutation(mutation, current_user: current_user, token: token)
+ end
+
+ shared_examples 'allows access to the mutation' do
+ let(:scopes) { ['api'] }
+
+ it_behaves_like 'a working graphql query' do
+ it 'returns data' do
+ expect(graphql_data.compact).not_to be_empty
+ end
+ end
+ end
+
+ shared_examples 'prevents access to the mutation' do
+ let(:scopes) { ['read_api'] }
+
+ it 'does not resolve the mutation' do
+ expect(graphql_data.compact).to be_empty
+ expect(graphql_errors).to be_present
+ end
+ end
+
+ context 'with a personal access token' do
+ let(:token) do
+ pat = create(:personal_access_token, user: current_user, scopes: scopes)
+ { personal_access_token: pat }
+ end
+
+ it_behaves_like 'prevents access to the mutation'
+ it_behaves_like 'allows access to the mutation'
+ end
+
+ context 'with an OAuth token' do
+ let(:token) do
+ { oauth_access_token: create(:oauth_access_token, resource_owner: current_user, scopes: scopes.join(' ')) }
+ end
+
+ it_behaves_like 'prevents access to the mutation'
+ it_behaves_like 'allows access to the mutation'
+ end
+end
+
RSpec.shared_examples 'a mutation on an unauthorized resource' do
it_behaves_like 'a mutation that returns top-level errors',
errors: [::Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR]