diff --git a/Gemfile b/Gemfile index 283886b71874c079fda495c0cc334f44dac2eeaa..82559fa731cc41cce33bf04cf3c9e921466e6535 100644 --- a/Gemfile +++ b/Gemfile @@ -35,7 +35,7 @@ gem 'faraday', '~> 0.12' # Authentication libraries gem 'devise', '~> 4.4' gem 'doorkeeper', '~> 4.3' -gem 'doorkeeper-openid_connect', '~> 1.3' +gem 'doorkeeper-openid_connect', '~> 1.5' gem 'omniauth', '~> 1.8' gem 'omniauth-auth0', '~> 2.0.0' gem 'omniauth-azure-oauth2', '~> 0.0.9' diff --git a/Gemfile.lock b/Gemfile.lock index 13f9212c637c81900544bbc788179d42e4dff517..79e3888fa642509a182f1f66dd5f310f8a84e0b8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -171,7 +171,7 @@ GEM unf (>= 0.0.5, < 1.0.0) doorkeeper (4.3.2) railties (>= 4.2) - doorkeeper-openid_connect (1.4.0) + doorkeeper-openid_connect (1.5.0) doorkeeper (~> 4.3) json-jwt (~> 1.6) dropzonejs-rails (0.7.2) @@ -427,12 +427,10 @@ GEM oauth (~> 0.5, >= 0.5.0) jquery-atwho-rails (1.3.2) json (1.8.6) - json-jwt (1.9.2) + json-jwt (1.9.4) activesupport aes_key_wrap bindata - securecompare - url_safe_base64 json-schema (2.8.0) addressable (>= 2.4) jwt (1.5.6) @@ -512,7 +510,7 @@ GEM net-ldap (0.16.0) net-ssh (5.0.1) netrc (0.11.0) - nokogiri (1.8.2) + nokogiri (1.8.3) mini_portile2 (~> 2.3.0) nokogumbo (1.5.0) nokogiri @@ -827,7 +825,6 @@ GEM scss_lint (0.56.0) rake (>= 0.9, < 13) sass (~> 3.5.3) - securecompare (1.0.0) seed-fu (2.3.7) activerecord (>= 3.1) activesupport (>= 3.1) @@ -939,7 +936,6 @@ GEM equalizer (~> 0.0.9) parser (>= 2.3.1.2, < 2.6) procto (~> 0.0.2) - url_safe_base64 (0.2.2) validates_hostname (1.0.6) activerecord (>= 3.0) activesupport (>= 3.0) @@ -1013,7 +1009,7 @@ DEPENDENCIES devise-two-factor (~> 3.0.0) diffy (~> 3.1.0) doorkeeper (~> 4.3) - doorkeeper-openid_connect (~> 1.3) + doorkeeper-openid_connect (~> 1.5) dropzonejs-rails (~> 0.7.1) ed25519 (~> 1.2) email_reply_trimmer (~> 0.1) diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index 3161e4653ee2b3a3aacb988cee42d0fc6fc082a0..3159942b4c5eb78fd70036978bd980ce4ff5dc42 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -174,7 +174,7 @@ GEM unf (>= 0.0.5, < 1.0.0) doorkeeper (4.3.2) railties (>= 4.2) - doorkeeper-openid_connect (1.4.0) + doorkeeper-openid_connect (1.5.0) doorkeeper (~> 4.3) json-jwt (~> 1.6) dropzonejs-rails (0.7.2) @@ -430,12 +430,10 @@ GEM oauth (~> 0.5, >= 0.5.0) jquery-atwho-rails (1.3.2) json (1.8.6) - json-jwt (1.9.2) + json-jwt (1.9.4) activesupport aes_key_wrap bindata - securecompare - url_safe_base64 json-schema (2.8.0) addressable (>= 2.4) jwt (1.5.6) @@ -836,7 +834,6 @@ GEM scss_lint (0.56.0) rake (>= 0.9, < 13) sass (~> 3.5.3) - securecompare (1.0.0) seed-fu (2.3.7) activerecord (>= 3.1) activesupport (>= 3.1) @@ -946,7 +943,6 @@ GEM equalizer (~> 0.0.9) parser (>= 2.3.1.2, < 2.6) procto (~> 0.0.2) - url_safe_base64 (0.2.2) validates_hostname (1.0.6) activerecord (>= 3.0) activesupport (>= 3.0) @@ -1023,7 +1019,7 @@ DEPENDENCIES devise-two-factor (~> 3.0.0) diffy (~> 3.1.0) doorkeeper (~> 4.3) - doorkeeper-openid_connect (~> 1.3) + doorkeeper-openid_connect (~> 1.5) dropzonejs-rails (~> 0.7.1) ed25519 (~> 1.2) email_reply_trimmer (~> 0.1) diff --git a/changelogs/unreleased/feature-oidc-subject-claim.yml b/changelogs/unreleased/feature-oidc-subject-claim.yml new file mode 100644 index 0000000000000000000000000000000000000000..e995ca262342ccf7d91fca3e129089d885076bf4 --- /dev/null +++ b/changelogs/unreleased/feature-oidc-subject-claim.yml @@ -0,0 +1,5 @@ +--- +title: Don't hash user ID in OIDC subject claim +merge_request: 19784 +author: Markus Koller +type: changed diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb index 98e1f6e830f7e1e3971c3577e6d1f1d78fe3ba5a..ae5d834a02c8dfc78c43b96c520cdbb040eff35e 100644 --- a/config/initializers/doorkeeper_openid_connect.rb +++ b/config/initializers/doorkeeper_openid_connect.rb @@ -18,12 +18,17 @@ Doorkeeper::OpenidConnect.configure do end subject do |user| - # hash the user's ID with the Rails secret_key_base to avoid revealing it - Digest::SHA256.hexdigest "#{user.id}-#{Rails.application.secrets.secret_key_base}" + user.id end claims do with_options scope: :openid do |o| + o.claim(:sub_legacy, response: [:id_token, :user_info]) do |user| + # provide the previously hashed 'sub' claim to allow third-party apps + # to migrate to the new unhashed value + Digest::SHA256.hexdigest "#{user.id}-#{Rails.application.secrets.secret_key_base}" + end + o.claim(:name) { |user| user.name } o.claim(:nickname) { |user| user.username } o.claim(:email) { |user| user.public_email } diff --git a/doc/integration/openid_connect_provider.md b/doc/integration/openid_connect_provider.md index ad41be520454be0c768000b79f710559d2b87492..a7f907254a11bba37e69eec4188faca07df3e22b 100644 --- a/doc/integration/openid_connect_provider.md +++ b/doc/integration/openid_connect_provider.md @@ -5,11 +5,11 @@ to sign in to other services. ## Introduction to OpenID Connect -[OpenID Connect] \(OIC) is a simple identity layer on top of the +[OpenID Connect] \(OIDC) is a simple identity layer on top of the OAuth 2.0 protocol. It allows clients to verify the identity of the end-user based on the authentication performed by GitLab, as well as to obtain basic profile information about the end-user in an interoperable and -REST-like manner. OIC performs many of the same tasks as OpenID 2.0, +REST-like manner. OIDC performs many of the same tasks as OpenID 2.0, but does so in a way that is API-friendly, and usable by native and mobile applications. @@ -23,14 +23,17 @@ are supported. ## Enabling OpenID Connect for OAuth applications Refer to the [OAuth guide] for basic information on how to set up OAuth -applications in GitLab. To enable OIC for an application, all you have to do +applications in GitLab. To enable OIDC for an application, all you have to do is select the `openid` scope in the application settings. +## Shared information + Currently the following user information is shared with clients: | Claim | Type | Description | |:-----------------|:----------|:------------| -| `sub` | `string` | An opaque token that uniquely identifies the user +| `sub` | `string` | The ID of the user +| `sub_legacy` | `string` | An opaque token that uniquely identifies the user

**Deprecation notice:** this token isn't stable because it's tied to the Rails secret key base, and is provided only for migration to the new stable `sub` value available from GitLab 11.1 | `auth_time` | `integer` | The timestamp for the user's last authentication | `name` | `string` | The user's full name | `nickname` | `string` | The user's GitLab username @@ -41,6 +44,8 @@ Currently the following user information is shared with clients: | `picture` | `string` | URL for the user's GitLab avatar | `groups` | `array` | Names of the groups the user is a member of +Only the `sub` and `sub_legacy` claims are included in the ID token, all other claims are available from the `/oauth/userinfo` endpoint used by OIDC clients. + [OpenID Connect]: http://openid.net/connect/ "OpenID Connect website" [doorkeeper-openid_connect]: https://github.com/doorkeeper-gem/doorkeeper-openid_connect "Doorkeeper::OpenidConnect website" [OAuth guide]: oauth_provider.md "GitLab as OAuth2 authentication service provider" diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index bcb8d6c2bfcba89024b5d80ade0fa6c8c2811a27..b14d4b8fb6e3611dba57dfe86e8deacb775419d8 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -1,11 +1,49 @@ require 'spec_helper' describe 'OpenID Connect requests' do - let(:user) { create :user } + let(:user) do + create( + :user, + name: 'Alice', + username: 'alice', + email: 'private@example.com', + emails: [public_email], + public_email: public_email.email, + website_url: 'https://example.com', + avatar: fixture_file_upload('spec/fixtures/dk.png') + ) + end + + let(:public_email) { build :email, email: 'public@example.com' } + let(:access_grant) { create :oauth_access_grant, application: application, resource_owner_id: user.id } let(:access_token) { create :oauth_access_token, application: application, resource_owner_id: user.id } - def request_access_token + let(:hashed_subject) do + Digest::SHA256.hexdigest("#{user.id}-#{Rails.application.secrets.secret_key_base}") + end + + let(:id_token_claims) do + { + 'sub' => user.id.to_s, + 'sub_legacy' => hashed_subject + } + end + + let(:user_info_claims) do + { + 'name' => 'Alice', + 'nickname' => 'alice', + 'email' => 'public@example.com', + 'email_verified' => true, + 'website' => 'https://example.com', + 'profile' => 'http://localhost/alice', + 'picture' => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png", + 'groups' => kind_of(Array) + } + end + + def request_access_token! login_as user post '/oauth/token', @@ -16,26 +54,22 @@ describe 'OpenID Connect requests' do client_secret: application.secret end - def request_user_info + def request_user_info! get '/oauth/userinfo', nil, 'Authorization' => "Bearer #{access_token.token}" end - def hashed_subject - Digest::SHA256.hexdigest("#{user.id}-#{Rails.application.secrets.secret_key_base}") - end - context 'Application without OpenID scope' do let(:application) { create :oauth_application, scopes: 'api' } it 'token response does not include an ID token' do - request_access_token + request_access_token! expect(json_response).to include 'access_token' expect(json_response).not_to include 'id_token' end it 'userinfo response is unauthorized' do - request_user_info + request_user_info! expect(response).to have_gitlab_http_status 403 expect(response.body).to be_blank @@ -46,28 +80,12 @@ describe 'OpenID Connect requests' do let(:application) { create :oauth_application, scopes: 'openid' } it 'token response includes an ID token' do - request_access_token + request_access_token! expect(json_response).to include 'id_token' end context 'UserInfo payload' do - let(:user) do - create( - :user, - name: 'Alice', - username: 'alice', - emails: [private_email, public_email], - email: private_email.email, - public_email: public_email.email, - website_url: 'https://example.com', - avatar: fixture_file_upload('spec/fixtures/dk.png') - ) - end - - let!(:public_email) { build :email, email: 'public@example.com' } - let!(:private_email) { build :email, email: 'private@example.com' } - let!(:group1) { create :group } let!(:group2) { create :group } let!(:group3) { create :group, parent: group2 } @@ -76,41 +94,35 @@ describe 'OpenID Connect requests' do before do group1.add_user(user, GroupMember::OWNER) group3.add_user(user, Gitlab::Access::DEVELOPER) + + request_user_info! end it 'includes all user information and group memberships' do - request_user_info - - expect(json_response).to match(a_hash_including({ - 'sub' => hashed_subject, - 'name' => 'Alice', - 'nickname' => 'alice', - 'email' => 'public@example.com', - 'email_verified' => true, - 'website' => 'https://example.com', - 'profile' => 'http://localhost/alice', - 'picture' => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png", - 'groups' => anything - })) + expect(json_response).to match(id_token_claims.merge(user_info_claims)) expected_groups = [group1.full_path, group3.full_path] expected_groups << group4.full_path if Group.supports_nested_groups? expect(json_response['groups']).to match_array(expected_groups) end + + it 'does not include any unknown claims' do + expect(json_response.keys).to eq %w[sub sub_legacy] + user_info_claims.keys + end end context 'ID token payload' do before do - request_access_token + request_access_token! @payload = JSON::JWT.decode(json_response['id_token'], :skip_verification) end - it 'includes the Gitlab root URL' do - expect(@payload['iss']).to eq Gitlab.config.gitlab.url + it 'includes the subject claims' do + expect(@payload).to match(a_hash_including(id_token_claims)) end - it 'includes the hashed user ID' do - expect(@payload['sub']).to eq hashed_subject + it 'includes the Gitlab root URL' do + expect(@payload['iss']).to eq Gitlab.config.gitlab.url end it 'includes the time of the last authentication', :clean_gitlab_redis_shared_state do @@ -118,7 +130,7 @@ describe 'OpenID Connect requests' do end it 'does not include any unknown properties' do - expect(@payload.keys).to eq %w[iss sub aud exp iat auth_time] + expect(@payload.keys).to eq %w[iss sub aud exp iat auth_time sub_legacy] end end @@ -134,10 +146,10 @@ describe 'OpenID Connect requests' do context 'when user is blocked' do it 'returns authentication error' do access_grant - user.block + user.block! expect do - request_access_token + request_access_token! end.to raise_error UncaughtThrowError end end @@ -145,10 +157,10 @@ describe 'OpenID Connect requests' do context 'when user is ldap_blocked' do it 'returns authentication error' do access_grant - user.ldap_block + user.ldap_block! expect do - request_access_token + request_access_token! end.to raise_error UncaughtThrowError end end