Merge branch '214607-ci-jwt-signing-key/jwks-take-3' into 'master'

Use dedicated signing key for CI_JOB_JWT (take 3)

See merge request gitlab-org/gitlab!43950

app/controllers/jwks_controller.rb

+# frozen_string_literal: true
+
+class JwksController < ActionController::Base # rubocop:disable Rails/ApplicationController
+  def index
+    render json: { keys: keys }
+  end
+
+  private
+
+  def keys
+    [
+      # We keep openid_connect_signing_key so that we can seamlessly
+      # replace it with ci_jwt_signing_key and remove it on the next release.
+      # TODO: Remove openid_connect_signing_key in 13.7
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/221031
+      Rails.application.secrets.openid_connect_signing_key,
+      Gitlab::CurrentSettings.ci_jwt_signing_key
+    ].compact.map do |key_data|
+      OpenSSL::PKey::RSA.new(key_data)
+        .public_key
+        .to_jwk
+        .slice(:kty, :kid, :e, :n)
+        .merge(use: 'sig', alg: 'RS256')
+    end
+  end
+end

app/models/application_setting.rb

@@ -384,6 +384,9 @@ class ApplicationSetting < ApplicationRecord
   validates :raw_blob_request_limit,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
 
+  validates :ci_jwt_signing_key,
+            rsa_key: true, allow_nil: true
+
   attr_encrypted :asset_proxy_secret_key,
                  mode: :per_attribute_iv,
                  key: Settings.attr_encrypted_db_key_base_truncated,
@@ -409,6 +412,7 @@ class ApplicationSetting < ApplicationRecord
   attr_encrypted :recaptcha_site_key, encryption_options_base_truncated_aes_256_gcm
   attr_encrypted :slack_app_secret, encryption_options_base_truncated_aes_256_gcm
   attr_encrypted :slack_app_verification_token, encryption_options_base_truncated_aes_256_gcm
+  attr_encrypted :ci_jwt_signing_key, encryption_options_base_truncated_aes_256_gcm
 
   before_validation :ensure_uuid!
 

app/models/ci/build.rb

@@ -1059,7 +1059,7 @@ module Ci
 
         jwt = Gitlab::Ci::Jwt.for_build(self)
         variables.append(key: 'CI_JOB_JWT', value: jwt, public: false, masked: true)
-      rescue OpenSSL::PKey::RSAError => e
+      rescue OpenSSL::PKey::RSAError, Gitlab::Ci::Jwt::NoSigningKeyError => e
         Gitlab::ErrorTracking.track_exception(e)
       end
     end

app/validators/rsa_key_validator.rb

+# frozen_string_literal: true
+
+# RsaKeyValidator
+#
+# Custom validator for RSA private keys.
+#
+#   class Project < ActiveRecord::Base
+#     validates :signing_key, rsa_key: true
+#   end
+#
+class RsaKeyValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    unless valid_rsa_key?(value)
+      record.errors.add(attribute, "is not a valid RSA key")
+    end
+  end
+
+  private
+
+  def valid_rsa_key?(value)
+    return false unless value
+
+    OpenSSL::PKey::RSA.new(value)
+  rescue OpenSSL::PKey::RSAError
+    false
+  end
+end

changelogs/unreleased/214607-ci-jwt-signing-key-jwks-take-3.yml

+---
+title: Add CI JWT signing key to application_setings
+merge_request: 43950
+author:
+type: added

config/feature_flags/development/ci_jwt_signing_key.yml

+---
+name: ci_jwt_signing_key
+introduced_by_url:
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/258546
+type: development
+group: group::release management
+default_enabled: false

config/routes.rb

@@ -175,9 +175,8 @@ Rails.application.routes.draw do
     resources :abuse_reports, only: [:new, :create]
 
     # JWKS (JSON Web Key Set) endpoint
-    # Used by third parties to verify CI_JOB_JWT, placeholder route
-    # in case we decide to move away from doorkeeper-openid_connect
-    get 'jwks' => 'doorkeeper/openid_connect/discovery#keys'
+    # Used by third parties to verify CI_JOB_JWT
+    get 'jwks' => 'jwks#index'
 
     draw :snippets
     draw :profile

db/fixtures/development/02_application_settings.rb

@@ -7,4 +7,7 @@ ApplicationSetting.create_from_defaults
 puts "Enable hashed storage for every new projects.".color(:green)
 ApplicationSetting.current_without_cache.update!(hashed_storage_enabled: true)
 
+puts "Generate CI JWT signing key".color(:green)
+ApplicationSetting.current_without_cache.update!(ci_jwt_signing_key: OpenSSL::PKey::RSA.new(2048).to_pem)
+
 print '.'

db/fixtures/production/010_settings.rb

@@ -24,3 +24,7 @@ if ENV['GITLAB_PROMETHEUS_METRICS_ENABLED'].present?
   settings.prometheus_metrics_enabled = value
   save(settings, 'Prometheus metrics enabled flag')
 end
+
+settings = Gitlab::CurrentSettings.current_application_settings
+settings.ci_jwt_signing_key = OpenSSL::PKey::RSA.new(2048).to_pem
+save(settings, 'CI JWT signing key')

db/migrate/20201008011523_add_ci_jwt_signing_key_to_application_settings.rb

+# frozen_string_literal: true
+
+class AddCiJwtSigningKeyToApplicationSettings < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  # rubocop:disable Migration/AddLimitToTextColumns
+  # limit is added in 20201001011937_add_text_limit_to_application_settings_encrypted_ci_jwt_signing_key_iv
+  def change
+    add_column :application_settings, :encrypted_ci_jwt_signing_key, :text
+    add_column :application_settings, :encrypted_ci_jwt_signing_key_iv, :text
+  end
+  # rubocop:enable Migration/AddLimitToTextColumns
+end

db/migrate/20201008011937_add_text_limit_to_application_settings_encrypted_ci_jwt_signing_key_iv.rb

+# frozen_string_literal: true
+
+class AddTextLimitToApplicationSettingsEncryptedCiJwtSigningKeyIv < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :application_settings, :encrypted_ci_jwt_signing_key_iv, 255
+  end
+
+  def down
+    remove_text_limit :application_settings, :encrypted_ci_jwt_signing_key_iv
+  end
+end

db/migrate/20201008013434_generate_ci_jwt_signing_key.rb

+# frozen_string_literal: true
+
+class GenerateCiJwtSigningKey < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+
+    attr_encrypted :ci_jwt_signing_key, {
+      mode: :per_attribute_iv,
+      key: Rails.application.secrets.db_key_base[0..31],
+      algorithm: 'aes-256-gcm',
+      encode: true
+    }
+  end
+
+  def up
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.find_each do |application_setting|
+      application_setting.update(ci_jwt_signing_key: OpenSSL::PKey::RSA.new(2048).to_pem)
+    end
+  end
+
+  def down
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.find_each do |application_setting|
+      application_setting.update_columns(encrypted_ci_jwt_signing_key: nil, encrypted_ci_jwt_signing_key_iv: nil)
+    end
+  end
+end

db/schema_migrations/20201008011523

+b7b49ca4c021b7caa9f8612ad9b69d4ec6d79894db2e43266bfe26f2e0bffe08
\ No newline at end of file

db/schema_migrations/20201008011937

+8af89bb3e63bfca24cee8fdf6f0dd587fae7d81bfeaf6d427f84c7b37c9664ba
\ No newline at end of file

db/schema_migrations/20201008013434

+966f6e95189b551cba0ef548cb410911c0beee30d0a265ae21d90321ecbb2a00
\ No newline at end of file

db/structure.sql

@@ -9294,9 +9294,12 @@ CREATE TABLE application_settings (
     require_admin_approval_after_user_signup boolean DEFAULT false NOT NULL,
     help_page_documentation_base_url text,
     automatic_purchased_storage_allocation boolean DEFAULT false NOT NULL,
+    encrypted_ci_jwt_signing_key text,
+    encrypted_ci_jwt_signing_key_iv text,
     CONSTRAINT check_2dba05b802 CHECK ((char_length(gitpod_url) <= 255)),
     CONSTRAINT check_51700b31b5 CHECK ((char_length(default_branch_name) <= 255)),
     CONSTRAINT check_57123c9593 CHECK ((char_length(help_page_documentation_base_url) <= 255)),
+    CONSTRAINT check_85a39b68ff CHECK ((char_length(encrypted_ci_jwt_signing_key_iv) <= 255)),
     CONSTRAINT check_9c6c447a13 CHECK ((char_length(maintenance_mode_message) <= 255)),
     CONSTRAINT check_d03919528d CHECK ((char_length(container_registry_vendor) <= 255)),
     CONSTRAINT check_d820146492 CHECK ((char_length(spam_check_endpoint_url) <= 255)),

lib/gitlab/ci/jwt.rb

@@ -6,6 +6,8 @@ module Gitlab
       NOT_BEFORE_TIME = 5
       DEFAULT_EXPIRE_TIME = 60 * 5
 
+      NoSigningKeyError = Class.new(StandardError)
+
       def self.for_build(build)
         self.new(build, ttl: build.metadata_timeout).encoded
       end
@@ -27,7 +29,7 @@ module Gitlab
 
       private
 
-      attr_reader :build, :ttl, :key_data
+      attr_reader :build, :ttl
 
       def reserved_claims
         now = Time.now.to_i
@@ -60,7 +62,17 @@ module Gitlab
       end
 
       def key
-        @key ||= OpenSSL::PKey::RSA.new(Rails.application.secrets.openid_connect_signing_key)
+        @key ||= begin
+                   key_data = if Feature.enabled?(:ci_jwt_signing_key, build.project)
+                                Gitlab::CurrentSettings.ci_jwt_signing_key
+                              else
+                                Rails.application.secrets.openid_connect_signing_key
+                              end
+
+                   raise NoSigningKeyError unless key_data
+
+                   OpenSSL::PKey::RSA.new(key_data)
+                 end
       end
 
       def public_key

spec/controllers/jwks_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe JwksController do
+  describe 'GET #index' do
+    let(:ci_jwt_signing_key) { OpenSSL::PKey::RSA.generate(1024) }
+    let(:ci_jwk) { ci_jwt_signing_key.to_jwk }
+    let(:oidc_jwk) { OpenSSL::PKey::RSA.new(Rails.application.secrets.openid_connect_signing_key).to_jwk }
+
+    before do
+      stub_application_setting(ci_jwt_signing_key: ci_jwt_signing_key.to_s)
+    end
+
+    it 'returns signing keys used to sign CI_JOB_JWT' do
+      get :index
+
+      expect(response).to have_gitlab_http_status(:ok)
+
+      ids = json_response['keys'].map { |jwk| jwk['kid'] }
+      expect(ids).to contain_exactly(ci_jwk['kid'], oidc_jwk['kid'])
+    end
+
+    it 'does not leak private key data' do
+      get :index
+
+      aggregate_failures do
+        json_response['keys'].each do |jwk|
+          expect(jwk.keys).to contain_exactly('kty', 'kid', 'e', 'n', 'use', 'alg')
+          expect(jwk['use']).to eq('sig')
+          expect(jwk['alg']).to eq('RS256')
+        end
+      end
+    end
+  end
+end

spec/db/production/settings_spec.rb

@@ -62,4 +62,11 @@ RSpec.describe 'seed production settings' do
       end
     end
   end
+
+  context 'CI JWT signing key' do
+    it 'writes valid RSA key to the database' do
+      expect { load(settings_file) }.to change { settings.reload.ci_jwt_signing_key }.from(nil)
+      expect { OpenSSL::PKey::RSA.new(settings.ci_jwt_signing_key) }.not_to raise_error
+    end
+  end
 end

spec/lib/gitlab/ci/jwt_spec.rb

@@ -93,32 +93,65 @@ RSpec.describe Gitlab::Ci::Jwt do
   end
 
   describe '.for_build' do
-    let(:rsa_key) { OpenSSL::PKey::RSA.new(Rails.application.secrets.openid_connect_signing_key) }
+    shared_examples 'generating JWT for build' do
+      context 'when signing key is present' do
+        let(:rsa_key) { OpenSSL::PKey::RSA.generate(1024) }
+        let(:rsa_key_data) { rsa_key.to_s }
 
-    subject(:jwt) { described_class.for_build(build) }
+        it 'generates JWT with key id' do
+          _payload, headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
+
+          expect(headers['kid']).to eq(rsa_key.public_key.to_jwk['kid'])
+        end
+
+        it 'generates JWT for the given job with ttl equal to build timeout' do
+          expect(build).to receive(:metadata_timeout).and_return(3_600)
+
+          payload, _headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
+          ttl = payload["exp"] - payload["iat"]
+
+          expect(ttl).to eq(3_600)
+        end
+
+        it 'generates JWT for the given job with default ttl if build timeout is not set' do
+          expect(build).to receive(:metadata_timeout).and_return(nil)
+
+          payload, _headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
+          ttl = payload["exp"] - payload["iat"]
 
-    it 'generates JWT with key id' do
-      _payload, headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
+          expect(ttl).to eq(5.minutes.to_i)
+        end
+      end
+
+      context 'when signing key is missing' do
+        let(:rsa_key_data) { nil }
 
-      expect(headers['kid']).to eq(rsa_key.public_key.to_jwk['kid'])
+        it 'raises NoSigningKeyError' do
+          expect { jwt }.to raise_error described_class::NoSigningKeyError
+        end
+      end
     end
 
-    it 'generates JWT for the given job with ttl equal to build timeout' do
-      expect(build).to receive(:metadata_timeout).and_return(3_600)
+    subject(:jwt) { described_class.for_build(build) }
+
+    context 'when ci_jwt_signing_key feature flag is disabled' do
+      before do
+        stub_feature_flags(ci_jwt_signing_key: false)
 
-      payload, _headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
-      ttl = payload["exp"] - payload["iat"]
+        allow(Rails.application.secrets).to receive(:openid_connect_signing_key).and_return(rsa_key_data)
+      end
 
-      expect(ttl).to eq(3_600)
+      it_behaves_like 'generating JWT for build'
     end
 
-    it 'generates JWT for the given job with default ttl if build timeout is not set' do
-      expect(build).to receive(:metadata_timeout).and_return(nil)
+    context 'when ci_jwt_signing_key feature flag is enabled' do
+      before do
+        stub_feature_flags(ci_jwt_signing_key: true)
 
-      payload, _headers = JWT.decode(jwt, rsa_key.public_key, true, { algorithm: 'RS256' })
-      ttl = payload["exp"] - payload["iat"]
+        stub_application_setting(ci_jwt_signing_key: rsa_key_data)
+      end
 
-      expect(ttl).to eq(5.minutes.to_i)
+      it_behaves_like 'generating JWT for build'
     end
   end
 end

spec/migrations/generate_ci_jwt_signing_key_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'migrate', '20201008013434_generate_ci_jwt_signing_key.rb')
+
+RSpec.describe GenerateCiJwtSigningKey do
+  let(:application_settings) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'application_settings'
+
+      attr_encrypted :ci_jwt_signing_key, {
+        mode: :per_attribute_iv,
+        key: Rails.application.secrets.db_key_base[0..31],
+        algorithm: 'aes-256-gcm',
+        encode: true
+      }
+    end
+  end
+
+  it 'generates JWT signing key' do
+    application_settings.create!
+
+    reversible_migration do |migration|
+      migration.before -> {
+        settings = application_settings.first
+
+        expect(settings.ci_jwt_signing_key).to be_nil
+        expect(settings.encrypted_ci_jwt_signing_key).to be_nil
+        expect(settings.encrypted_ci_jwt_signing_key_iv).to be_nil
+      }
+
+      migration.after -> {
+        settings = application_settings.first
+
+        expect(settings.encrypted_ci_jwt_signing_key).to be_present
+        expect(settings.encrypted_ci_jwt_signing_key_iv).to be_present
+        expect { OpenSSL::PKey::RSA.new(settings.ci_jwt_signing_key) }.not_to raise_error
+      }
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -647,6 +647,23 @@ RSpec.describe ApplicationSetting do
           end
         end
       end
+
+      describe '#ci_jwt_signing_key' do
+        it { is_expected.not_to allow_value('').for(:ci_jwt_signing_key) }
+        it { is_expected.not_to allow_value('invalid RSA key').for(:ci_jwt_signing_key) }
+        it { is_expected.to allow_value(nil).for(:ci_jwt_signing_key) }
+        it { is_expected.to allow_value(OpenSSL::PKey::RSA.new(1024).to_pem).for(:ci_jwt_signing_key) }
+
+        it 'is encrypted' do
+          subject.ci_jwt_signing_key = OpenSSL::PKey::RSA.new(1024).to_pem
+
+          aggregate_failures do
+            expect(subject.encrypted_ci_jwt_signing_key).to be_present
+            expect(subject.encrypted_ci_jwt_signing_key_iv).to be_present
+            expect(subject.encrypted_ci_jwt_signing_key).not_to eq(subject.ci_jwt_signing_key)
+          end
+        end
+      end
     end
 
     context 'static objects external storage' do

spec/models/ci/build_spec.rb

@@ -2464,7 +2464,7 @@ RSpec.describe Ci::Build do
       end
 
       before do
-        allow(Gitlab::Ci::Jwt).to receive(:for_build).with(build).and_return('ci.job.jwt')
+        allow(Gitlab::Ci::Jwt).to receive(:for_build).and_return('ci.job.jwt')
         build.set_token('my-token')
         build.yaml_variables = []
       end
@@ -2482,12 +2482,17 @@ RSpec.describe Ci::Build do
       end
 
       context 'when CI_JOB_JWT generation fails' do
-        it 'CI_JOB_JWT is not included' do
-          expect(Gitlab::Ci::Jwt).to receive(:for_build).and_raise(OpenSSL::PKey::RSAError, 'Neither PUB key nor PRIV key: not enough data')
-          expect(Gitlab::ErrorTracking).to receive(:track_exception)
-
-          expect { subject }.not_to raise_error
-          expect(subject.pluck(:key)).not_to include('CI_JOB_JWT')
+        [
+          OpenSSL::PKey::RSAError,
+          Gitlab::Ci::Jwt::NoSigningKeyError
+        ].each do |reason_to_fail|
+          it 'CI_JOB_JWT is not included' do
+            expect(Gitlab::Ci::Jwt).to receive(:for_build).and_raise(reason_to_fail)
+            expect(Gitlab::ErrorTracking).to receive(:track_exception)
+
+            expect { subject }.not_to raise_error
+            expect(subject.pluck(:key)).not_to include('CI_JOB_JWT')
+          end
         end
       end
 

spec/routing/openid_connect_spec.rb

@@ -3,7 +3,6 @@
 require 'spec_helper'
 
 # oauth_discovery_keys      GET /oauth/discovery/keys(.:format)             doorkeeper/openid_connect/discovery#keys
-# jwks                      GET /-/jwks(.:format)                           doorkeeper/openid_connect/discovery#keys
 # oauth_discovery_provider  GET /.well-known/openid-configuration(.:format) doorkeeper/openid_connect/discovery#provider
 # oauth_discovery_webfinger GET /.well-known/webfinger(.:format)            doorkeeper/openid_connect/discovery#webfinger
 RSpec.describe Doorkeeper::OpenidConnect::DiscoveryController, 'routing' do
@@ -18,10 +17,6 @@ RSpec.describe Doorkeeper::OpenidConnect::DiscoveryController, 'routing' do
   it "to #keys" do
     expect(get('/oauth/discovery/keys')).to route_to('doorkeeper/openid_connect/discovery#keys')
   end
-
-  it "/-/jwks" do
-    expect(get('/-/jwks')).to route_to('doorkeeper/openid_connect/discovery#keys')
-  end
 end
 
 # oauth_userinfo GET  /oauth/userinfo(.:format) doorkeeper/openid_connect/userinfo#show

spec/routing/routing_spec.rb

@@ -369,3 +369,10 @@ RSpec.describe RunnerSetupController, 'routing' do
     expect(get("/-/runner_setup/platforms")).to route_to('runner_setup#platforms')
   end
 end
+
+# jwks  GET /-/jwks(.:format)  jwks#index
+RSpec.describe JwksController, "routing" do
+  it "to #index" do
+    expect(get('/-/jwks')).to route_to('jwks#index')
+  end
+end

spec/validators/rsa_key_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RsaKeyValidator do
+  let(:validatable) do
+    Class.new do
+      include ActiveModel::Validations
+
+      attr_accessor :signing_key
+
+      validates :signing_key, rsa_key: true
+
+      def initialize(signing_key)
+        @signing_key = signing_key
+      end
+    end
+  end
+
+  subject(:validator) { described_class.new(attributes: [:signing_key]) }
+
+  it 'is not valid when invalid RSA key is provided' do
+    record = validatable.new('invalid RSA key')
+
+    validator.validate(record)
+
+    aggregate_failures do
+      expect(record).not_to be_valid
+      expect(record.errors[:signing_key]).to include('is not a valid RSA key')
+    end
+  end
+
+  it 'is valid when valid RSA key is provided' do
+    record = validatable.new(OpenSSL::PKey::RSA.new(1024).to_pem)
+
+    validator.validate(record)
+
+    expect(record).to be_valid
+  end
+end