From 0b81b5ace0dd7c5ba3362238d8be41ce178e1ecc Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 31 May 2017 15:55:12 +0200 Subject: [PATCH 1/2] Create read_registry scope with JWT auth This is the first commit doing mainly 3 things: 1. create a new scope and allow users to use it 2. Have the JWTController respond correctly on this 3. Updates documentation to suggest usage of PATs There is one gotcha, there will be no support for impersonation tokens, as this seems not needed. Fixes gitlab-org/gitlab-ce#19219 --- app/controllers/jwt_controller.rb | 8 ++-- .../personal_access_tokens_controller.rb | 2 +- app/models/personal_access_token.rb | 11 +++-- .../unreleased/zj-read-registry-pat.yml | 4 ++ doc/user/project/container_registry.md | 13 +++--- lib/gitlab/auth.rb | 42 ++++++++++++------- lib/gitlab/auth/result.rb | 4 ++ spec/lib/gitlab/auth_spec.rb | 7 ++++ spec/models/personal_access_token_spec.rb | 20 ++++++++- spec/requests/jwt_controller_spec.rb | 15 ++++++- 10 files changed, 93 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/zj-read-registry-pat.yml diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb index 1c01be06451..2648b901f01 100644 --- a/app/controllers/jwt_controller.rb +++ b/app/controllers/jwt_controller.rb @@ -20,13 +20,15 @@ class JwtController < ApplicationController private def authenticate_project_or_user - @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities) + @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_api_abilities) authenticate_with_http_basic do |login, password| @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip) - render_unauthorized unless @authentication_result.success? && - (@authentication_result.actor.nil? || @authentication_result.actor.is_a?(User)) + if @authentication_result.failed? || + (@authentication_result.actor.present? && !@authentication_result.actor.is_a?(User)) + render_unauthorized + end end rescue Gitlab::Auth::MissingPersonalTokenError render_missing_personal_token diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb index 0abe7ea3c9b..f748d191ef4 100644 --- a/app/controllers/profiles/personal_access_tokens_controller.rb +++ b/app/controllers/profiles/personal_access_tokens_controller.rb @@ -38,7 +38,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController end def set_index_vars - @scopes = Gitlab::Auth::API_SCOPES + @scopes = Gitlab::Auth::AVAILABLE_SCOPES @personal_access_token = finder.build @inactive_personal_access_tokens = finder(state: 'inactive').execute diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index e8b000ddad6..0aee696bed3 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -15,11 +15,10 @@ class PersonalAccessToken < ActiveRecord::Base scope :without_impersonation, -> { where(impersonation: false) } validates :scopes, presence: true - validate :validate_api_scopes + validate :validate_scopes def revoke! - self.revoked = true - self.save + update!(revoked: true) end def active? @@ -28,9 +27,9 @@ class PersonalAccessToken < ActiveRecord::Base protected - def validate_api_scopes - unless scopes.all? { |scope| Gitlab::Auth::API_SCOPES.include?(scope.to_sym) } - errors.add :scopes, "can only contain API scopes" + def validate_scopes + unless scopes.all? { |scope| Gitlab::Auth::AVAILABLE_SCOPES.include?(scope.to_sym) } + errors.add :scopes, "can only contain available scopes" end end end diff --git a/changelogs/unreleased/zj-read-registry-pat.yml b/changelogs/unreleased/zj-read-registry-pat.yml new file mode 100644 index 00000000000..d36159bbdf5 --- /dev/null +++ b/changelogs/unreleased/zj-read-registry-pat.yml @@ -0,0 +1,4 @@ +--- +title: Allow pulling of container images using personal access tokens +merge_request: 11845 +author: diff --git a/doc/user/project/container_registry.md b/doc/user/project/container_registry.md index 6a2ca7fb428..b2eca9ef809 100644 --- a/doc/user/project/container_registry.md +++ b/doc/user/project/container_registry.md @@ -106,12 +106,14 @@ Make sure that your GitLab Runner is configured to allow building Docker images following the [Using Docker Build](../../ci/docker/using_docker_build.md) and [Using the GitLab Container Registry documentation](../../ci/docker/using_docker_build.md#using-the-gitlab-container-registry). -## Limitations +## Using with private projects -In order to use a container image from your private project as an `image:` in -your `.gitlab-ci.yml`, you have to follow the -[Using a private Docker Registry][private-docker] -documentation. This workflow will be simplified in the future. +If a project is private, credentials will need to be provided for authorization. +The preferred way to do this, is by using personal access tokens, which can be +created under `/profile/personal_access_tokens`. The minimal scope needed is: +`read_registry`. + +This feature was introduced in GitLab 9.3. ## Troubleshooting the GitLab Container Registry @@ -257,4 +259,3 @@ Once the right permissions were set, the error will go away. [ce-4040]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4040 [docker-docs]: https://docs.docker.com/engine/userguide/intro/ -[private-docker]: https://docs.gitlab.com/runner/configuration/advanced-configuration.html#using-a-private-container-registry diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 099c45dcfb7..f461d0f97f1 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -2,6 +2,8 @@ module Gitlab module Auth MissingPersonalTokenError = Class.new(StandardError) + REGISTRY_SCOPES = [:read_registry].freeze + # Scopes used for GitLab API access API_SCOPES = [:api, :read_user].freeze @@ -11,8 +13,10 @@ module Gitlab # Default scopes for OAuth applications that don't define their own DEFAULT_SCOPES = [:api].freeze + AVAILABLE_SCOPES = (API_SCOPES + REGISTRY_SCOPES).freeze + # Other available scopes - OPTIONAL_SCOPES = (API_SCOPES + OPENID_SCOPES - DEFAULT_SCOPES).freeze + OPTIONAL_SCOPES = (AVAILABLE_SCOPES + OPENID_SCOPES - DEFAULT_SCOPES).freeze class << self def find_for_git_client(login, password, project:, ip:) @@ -26,8 +30,8 @@ module Gitlab build_access_token_check(login, password) || lfs_token_check(login, password) || oauth_access_token_check(login, password) || - user_with_password_for_git(login, password) || personal_access_token_check(password) || + user_with_password_for_git(login, password) || Gitlab::Auth::Result.new rate_limit!(ip, success: result.success?, login: login) @@ -103,15 +107,16 @@ module Gitlab raise Gitlab::Auth::MissingPersonalTokenError if user.two_factor_enabled? - Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities) + Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_api_abilities) end def oauth_access_token_check(login, password) if login == "oauth2" && password.present? token = Doorkeeper::AccessToken.by_token(password) + if valid_oauth_token?(token) user = User.find_by(id: token.resource_owner_id) - Gitlab::Auth::Result.new(user, nil, :oauth, full_authentication_abilities) + Gitlab::Auth::Result.new(user, nil, :oauth, full_api_abilities) end end end @@ -121,17 +126,26 @@ module Gitlab token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password) - if token && valid_api_token?(token) - Gitlab::Auth::Result.new(token.user, nil, :personal_token, full_authentication_abilities) + if token && valid_scoped_token?(token, scopes: AVAILABLE_SCOPES.map(&:to_s)) + Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes)) end end def valid_oauth_token?(token) - token && token.accessible? && valid_api_token?(token) + token && token.accessible? && valid_scoped_token?(token) end - def valid_api_token?(token) - AccessTokenValidationService.new(token).include_any_scope?(['api']) + def valid_scoped_token?(token, scopes: %w[api]) + AccessTokenValidationService.new(token).include_any_scope?(scopes) + end + + def abilities_for_scope(scopes) + abilities = Set.new + + abilities.merge(full_api_abilities) if scopes.include?("api") + abilities << :read_container_image if scopes.include?("read_registry") + + abilities.to_a end def lfs_token_check(login, password) @@ -150,9 +164,9 @@ module Gitlab authentication_abilities = if token_handler.user? - full_authentication_abilities + full_api_abilities else - read_authentication_abilities + read_api_abilities end if Devise.secure_compare(token_handler.token, password) @@ -188,7 +202,7 @@ module Gitlab ] end - def read_authentication_abilities + def read_api_abilities [ :read_project, :download_code, @@ -196,8 +210,8 @@ module Gitlab ] end - def full_authentication_abilities - read_authentication_abilities + [ + def full_api_abilities + read_api_abilities + [ :push_code, :create_container_image ] diff --git a/lib/gitlab/auth/result.rb b/lib/gitlab/auth/result.rb index 39b86c61a18..75451cf8aa9 100644 --- a/lib/gitlab/auth/result.rb +++ b/lib/gitlab/auth/result.rb @@ -15,6 +15,10 @@ module Gitlab def success? actor.present? || type == :ci end + + def failed? + !success? + end end end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 50bc3ef1b7c..6574e6d0087 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -143,6 +143,13 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_token, full_authentication_abilities)) end + it 'succeeds for personal access tokens with the `read_registry` scope' do + personal_access_token = create(:personal_access_token, scopes: ['read_registry']) + + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '') + expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_token, [:read_container_image])) + end + it 'succeeds if it is an impersonation token' do impersonation_token = create(:personal_access_token, :impersonation, scopes: ['api']) diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 823623d96fa..fa781195608 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -35,6 +35,16 @@ describe PersonalAccessToken, models: true do end end + describe 'revoke!' do + let(:active_personal_access_token) { create(:personal_access_token) } + + it 'revokes the token' do + active_personal_access_token.revoke! + + expect(active_personal_access_token.revoked?).to be true + end + end + context "validations" do let(:personal_access_token) { build(:personal_access_token) } @@ -51,11 +61,17 @@ describe PersonalAccessToken, models: true do expect(personal_access_token).to be_valid end - it "rejects creating a token with non-API scopes" do + it "allows creating a token with read_registry scope" do + personal_access_token.scopes = [:read_registry] + + expect(personal_access_token).to be_valid + end + + it "rejects creating a token with unavailable scopes" do personal_access_token.scopes = [:openid, :api] expect(personal_access_token).not_to be_valid - expect(personal_access_token.errors[:scopes].first).to eq "can only contain API scopes" + expect(personal_access_token.errors[:scopes].first).to eq "can only contain available scopes" end end end diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb index a3e7844b2f3..8ddae9f6b89 100644 --- a/spec/requests/jwt_controller_spec.rb +++ b/spec/requests/jwt_controller_spec.rb @@ -41,6 +41,19 @@ describe JwtController do it { expect(response).to have_http_status(401) } end + + context 'using personal access tokens' do + let(:user) { create(:user) } + let(:pat) { create(:personal_access_token, user: user, scopes: ['read_registry']) } + let(:headers) { { authorization: credentials('personal_access_token', pat.token) } } + + subject! { get '/jwt/auth', parameters, headers } + + it 'authenticates correctly' do + expect(response).to have_http_status(200) + expect(service_class).to have_received(:new).with(nil, user, parameters) + end + end end context 'using User login' do @@ -89,7 +102,7 @@ describe JwtController do end it 'allows read access' do - expect(service).to receive(:execute).with(authentication_abilities: Gitlab::Auth.read_authentication_abilities) + expect(service).to receive(:execute).with(authentication_abilities: Gitlab::Auth.read_api_abilities) get '/jwt/auth', parameters end From 9fcc3e5982311a380681c822df72fe470a5ea1ca Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 6 Jun 2017 13:18:01 +0200 Subject: [PATCH 2/2] Fix test failures --- app/controllers/jwt_controller.rb | 2 +- lib/gitlab/auth.rb | 39 +++++++++++-------- .../profiles/personal_access_tokens_spec.rb | 6 ++- spec/lib/gitlab/auth_spec.rb | 19 ++++----- spec/requests/jwt_controller_spec.rb | 2 +- 5 files changed, 38 insertions(+), 30 deletions(-) diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb index 2648b901f01..c585d26df77 100644 --- a/app/controllers/jwt_controller.rb +++ b/app/controllers/jwt_controller.rb @@ -20,7 +20,7 @@ class JwtController < ApplicationController private def authenticate_project_or_user - @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_api_abilities) + @authentication_result = Gitlab::Auth::Result.new(nil, nil, :none, Gitlab::Auth.read_authentication_abilities) authenticate_with_http_basic do |login, password| @authentication_result = Gitlab::Auth.find_for_git_client(login, password, project: nil, ip: request.ip) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index f461d0f97f1..da07ba2f2a3 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -107,7 +107,7 @@ module Gitlab raise Gitlab::Auth::MissingPersonalTokenError if user.two_factor_enabled? - Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_api_abilities) + Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities) end def oauth_access_token_check(login, password) @@ -116,7 +116,7 @@ module Gitlab if valid_oauth_token?(token) user = User.find_by(id: token.resource_owner_id) - Gitlab::Auth::Result.new(user, nil, :oauth, full_api_abilities) + Gitlab::Auth::Result.new(user, nil, :oauth, full_authentication_abilities) end end end @@ -126,26 +126,23 @@ module Gitlab token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password) - if token && valid_scoped_token?(token, scopes: AVAILABLE_SCOPES.map(&:to_s)) + if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map(&:to_s)) Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes)) end end def valid_oauth_token?(token) - token && token.accessible? && valid_scoped_token?(token) + token && token.accessible? && valid_scoped_token?(token, ["api"]) end - def valid_scoped_token?(token, scopes: %w[api]) + def valid_scoped_token?(token, scopes) AccessTokenValidationService.new(token).include_any_scope?(scopes) end def abilities_for_scope(scopes) - abilities = Set.new - - abilities.merge(full_api_abilities) if scopes.include?("api") - abilities << :read_container_image if scopes.include?("read_registry") - - abilities.to_a + scopes.map do |scope| + self.public_send(:"#{scope}_scope_authentication_abilities") + end.flatten.uniq end def lfs_token_check(login, password) @@ -164,9 +161,9 @@ module Gitlab authentication_abilities = if token_handler.user? - full_api_abilities + full_authentication_abilities else - read_api_abilities + read_authentication_abilities end if Devise.secure_compare(token_handler.token, password) @@ -202,7 +199,7 @@ module Gitlab ] end - def read_api_abilities + def read_authentication_abilities [ :read_project, :download_code, @@ -210,12 +207,22 @@ module Gitlab ] end - def full_api_abilities - read_api_abilities + [ + def full_authentication_abilities + read_authentication_abilities + [ :push_code, :create_container_image ] end + alias_method :api_scope_authentication_abilities, :full_authentication_abilities + + def read_registry_scope_authentication_abilities + [:read_container_image] + end + + # The currently used auth method doesn't allow any actions for this scope + def read_user_scope_authentication_abilities + [] + end end end end diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb index 27a20e78a43..7e2e685df26 100644 --- a/spec/features/profiles/personal_access_tokens_spec.rb +++ b/spec/features/profiles/personal_access_tokens_spec.rb @@ -17,6 +17,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do def disallow_personal_access_token_saves! allow_any_instance_of(PersonalAccessToken).to receive(:save).and_return(false) + errors = ActiveModel::Errors.new(PersonalAccessToken.new).tap { |e| e.add(:name, "cannot be nil") } allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors) end @@ -91,8 +92,11 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do context "when revocation fails" do it "displays an error message" do - disallow_personal_access_token_saves! visit profile_personal_access_tokens_path + allow_any_instance_of(PersonalAccessToken).to receive(:update!).and_return(false) + + errors = ActiveModel::Errors.new(PersonalAccessToken.new).tap { |e| e.add(:name, "cannot be nil") } + allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors) click_on "Revoke" expect(active_personal_access_tokens).to have_text(personal_access_token.name) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 6574e6d0087..d6006eab0c9 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -17,7 +17,11 @@ describe Gitlab::Auth, lib: true do end it 'OPTIONAL_SCOPES contains all non-default scopes' do - expect(subject::OPTIONAL_SCOPES).to eq [:read_user, :openid] + expect(subject::OPTIONAL_SCOPES).to eq %i[read_user read_registry openid] + end + + it 'REGISTRY_SCOPES contains all registry related scopes' do + expect(subject::REGISTRY_SCOPES).to eq %i[read_registry] end end @@ -157,18 +161,11 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(impersonation_token.user, nil, :personal_token, full_authentication_abilities)) end - it 'fails for personal access tokens with other scopes' do + it 'limits abilities based on scope' do personal_access_token = create(:personal_access_token, scopes: ['read_user']) - expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') - expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) - end - - it 'fails for impersonation token with other scopes' do - impersonation_token = create(:personal_access_token, scopes: ['read_user']) - - expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') - expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '') + expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_token, [])) end it 'fails if password is nil' do diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb index 8ddae9f6b89..e056353fa6f 100644 --- a/spec/requests/jwt_controller_spec.rb +++ b/spec/requests/jwt_controller_spec.rb @@ -102,7 +102,7 @@ describe JwtController do end it 'allows read access' do - expect(service).to receive(:execute).with(authentication_abilities: Gitlab::Auth.read_api_abilities) + expect(service).to receive(:execute).with(authentication_abilities: Gitlab::Auth.read_authentication_abilities) get '/jwt/auth', parameters end