Merge branch 'feature/oidc-subject-claim' into 'master'

Don't hash user ID in OIDC subject claim

Closes #47791

See merge request gitlab-org/gitlab-ce!19784

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'

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)

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)

changelogs/unreleased/feature-oidc-subject-claim.yml

+---
+title: Don't hash user ID in OIDC subject claim
+merge_request: 19784
+author: Markus Koller
+type: changed

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  }

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<br><br>**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"

spec/requests/openid_connect_spec.rb

 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