Merge branch '338177-jira-connect-support-asymmetric-jwt' into 'master'

Support Jira Connect asymmetric JWTs

See merge request gitlab-org/gitlab!71080

app/controllers/jira_connect/app_descriptor_controller.rb

@@ -32,6 +32,7 @@ class JiraConnect::AppDescriptorController < JiraConnect::ApplicationController
       apiVersion: 1,
       apiMigrations: {
         'context-qsh': true,
+        'signed-install': signed_install_active?,
         gdpr: true
       }
     }

app/controllers/jira_connect/application_controller.rb

@@ -74,4 +74,8 @@ class JiraConnect::ApplicationController < ApplicationController
       params[:jwt] || request.headers['Authorization']&.split(' ', 2)&.last
     end
   end
+
+  def signed_install_active?
+    Feature.enabled?(:jira_connect_asymmetric_jwt)
+  end
 end

app/controllers/jira_connect/events_controller.rb

@@ -3,13 +3,18 @@
 class JiraConnect::EventsController < JiraConnect::ApplicationController
   # See https://developer.atlassian.com/cloud/jira/software/app-descriptor/#lifecycle
 
-  skip_before_action :verify_atlassian_jwt!, only: :installed
-  before_action :verify_qsh_claim!, only: :uninstalled
+  skip_before_action :verify_atlassian_jwt!
+  before_action :verify_asymmetric_atlassian_jwt!, if: :signed_install_active?
+
+  before_action :verify_atlassian_jwt!, only: :uninstalled, unless: :signed_install_active?
+  before_action :verify_qsh_claim!, only: :uninstalled, unless: :signed_install_active?
 
   def installed
-    return head :ok if atlassian_jwt_valid?
+    return head :ok if !signed_install_active? && atlassian_jwt_valid?
+
+    return head :ok if current_jira_installation
 
-    installation = JiraConnectInstallation.new(install_params)
+    installation = JiraConnectInstallation.new(event_params)
 
     if installation.save
       head :ok
@@ -28,7 +33,23 @@ class JiraConnect::EventsController < JiraConnect::ApplicationController
 
   private
 
-  def install_params
+  def event_params
     params.permit(:clientKey, :sharedSecret, :baseUrl).transform_keys(&:underscore)
   end
+
+  def verify_asymmetric_atlassian_jwt!
+    asymmetric_jwt = Atlassian::JiraConnect::AsymmetricJwt.new(auth_token, jwt_verification_claims)
+
+    return head :unauthorized unless asymmetric_jwt.valid?
+
+    @current_jira_installation = JiraConnectInstallation.find_by_client_key(asymmetric_jwt.iss_claim)
+  end
+
+  def jwt_verification_claims
+    {
+      aud: jira_connect_base_url(protocol: 'https'),
+      iss: event_params[:client_key],
+      qsh: Atlassian::Jwt.create_query_string_hash(request.url, request.method, jira_connect_base_url)
+    }
+  end
 end

config/feature_flags/development/jira_connect_asymmetric_jwt.yml

+---
+name: jira_connect_asymmetric_jwt
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71080
+rollout_issue_url:
+milestone: '14.4'
+type: development
+group: group::integrations
+default_enabled: false

lib/atlassian/jira_connect/asymmetric_jwt.rb

+# frozen_string_literal: true
+
+module Atlassian
+  module JiraConnect
+    # See documentation about Atlassian asymmetric JWT verification:
+    # https://developer.atlassian.com/cloud/jira/platform/understanding-jwt-for-connect-apps/#verifying-a-asymmetric-jwt-token-for-install-callbacks
+
+    class AsymmetricJwt
+      include Gitlab::Utils::StrongMemoize
+
+      KeyFetchError = Class.new(StandardError)
+
+      ALGORITHM = 'RS256'
+      PUBLIC_KEY_CDN_URL = 'https://connect-install-keys.atlassian.com/'
+      UUID4_REGEX = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/.freeze
+
+      def initialize(token, verification_claims)
+        @token = token
+        @verification_claims = verification_claims
+      end
+
+      def valid?
+        claims.present? && claims['qsh'] == verification_qsh
+      end
+
+      def iss_claim
+        return unless claims
+
+        claims['iss']
+      end
+
+      private
+
+      def claims
+        strong_memoize(:claims) do
+          _, jwt_headers    = decode_token
+          public_key        = retrieve_public_key(jwt_headers['kid'])
+          decoded_claims, _ = decode_token(public_key, true, **relevant_claims, verify_aud: true, verify_iss: true, algorithm: ALGORITHM)
+
+          decoded_claims
+        rescue JWT::DecodeError, OpenSSL::PKey::PKeyError, KeyFetchError
+        end
+      end
+
+      def decode_token(key = nil, verify = false, **claims)
+        Atlassian::Jwt.decode(@token, key, verify, **claims)
+      end
+
+      def retrieve_public_key(key_id)
+        raise KeyFetchError unless UUID4_REGEX.match?(key_id)
+
+        public_key = Gitlab::HTTP.try_get(PUBLIC_KEY_CDN_URL + key_id).try(:body)
+
+        raise KeyFetchError if public_key.blank?
+
+        OpenSSL::PKey.read(public_key)
+      end
+
+      def relevant_claims
+        @verification_claims.slice(:aud, :iss)
+      end
+
+      def verification_qsh
+        @verification_claims[:qsh]
+      end
+    end
+  end
+end

spec/controllers/jira_connect/app_descriptor_controller_spec.rb

@@ -46,7 +46,8 @@ RSpec.describe JiraConnect::AppDescriptorController do
         apiVersion: 1,
         apiMigrations: {
           'context-qsh': true,
-          gdpr: true
+          gdpr: true,
+          'signed-install': true
         }
       )
 
@@ -89,5 +90,17 @@ RSpec.describe JiraConnect::AppDescriptorController do
         )
       )
     end
+
+    context 'when jira_connect_asymmetric_jwt is disabled' do
+      before do
+        stub_feature_flags(jira_connect_asymmetric_jwt: false)
+      end
+
+      specify do
+        get :show
+
+        expect(json_response).to include('apiMigrations' => include('signed-install' => false))
+      end
+    end
   end
 end

spec/controllers/jira_connect/events_controller_spec.rb

@@ -3,9 +3,49 @@
 require 'spec_helper'
 
 RSpec.describe JiraConnect::EventsController do
+  shared_examples 'verifies asymmetric JWT token' do
+    context 'when token is valid' do
+      include_context 'valid JWT token'
+
+      it 'renders successful' do
+        send_request
+
+        expect(response).to have_gitlab_http_status(:success)
+      end
+    end
+
+    context 'when token is invalid' do
+      include_context 'invalid JWT token'
+
+      it 'renders unauthorized' do
+        send_request
+
+        expect(response).to have_gitlab_http_status(:unauthorized)
+      end
+    end
+  end
+
+  shared_context 'valid JWT token' do
+    before do
+      allow_next_instance_of(Atlassian::JiraConnect::AsymmetricJwt) do |asymmetric_jwt|
+        allow(asymmetric_jwt).to receive(:valid?).and_return(true)
+        allow(asymmetric_jwt).to receive(:iss_claim).and_return(client_key)
+      end
+    end
+  end
+
+  shared_context 'invalid JWT token' do
+    before do
+      allow_next_instance_of(Atlassian::JiraConnect::AsymmetricJwt) do |asymmetric_jwt|
+        allow(asymmetric_jwt).to receive(:valid?).and_return(false)
+      end
+    end
+  end
+
   describe '#installed' do
     let(:client_key) { '1234' }
     let(:shared_secret) { 'secret' }
+
     let(:params) do
       {
         clientKey: client_key,
@@ -14,10 +54,16 @@ RSpec.describe JiraConnect::EventsController do
       }
     end
 
+    include_context 'valid JWT token'
+
     subject do
       post :installed, params: params
     end
 
+    it_behaves_like 'verifies asymmetric JWT token' do
+      let(:send_request) { subject }
+    end
+
     it 'saves the jira installation data' do
       expect { subject }.to change { JiraConnectInstallation.count }.by(1)
     end
@@ -31,13 +77,15 @@ RSpec.describe JiraConnect::EventsController do
       expect(installation.base_url).to eq('https://test.atlassian.net')
     end
 
-    context 'client key already exists' do
-      it 'returns 422' do
-        create(:jira_connect_installation, client_key: client_key)
+    context 'when jira_connect_asymmetric_jwt is disabled' do
+      before do
+        stub_feature_flags(jira_connect_asymmetric_jwt: false)
+      end
 
-        subject
+      it 'saves the jira installation data without JWT validation' do
+        expect(Atlassian::JiraConnect::AsymmetricJwt).not_to receive(:new)
 
-        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        expect { subject }.to change { JiraConnectInstallation.count }.by(1)
       end
     end
 
@@ -49,44 +97,83 @@ RSpec.describe JiraConnect::EventsController do
         }
       end
 
-      it 'validates the JWT token in authorization header and returns 200 without creating a new installation' do
-        create(:jira_connect_installation, client_key: client_key, shared_secret: shared_secret)
-        request.headers["Authorization"] = "Bearer #{Atlassian::Jwt.encode({ iss: client_key }, shared_secret)}"
+      it 'returns 422' do
+        subject
 
-        expect { subject }.not_to change { JiraConnectInstallation.count }
-        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to have_gitlab_http_status(:unprocessable_entity)
+      end
+
+      context 'and an installation exists' do
+        let!(:installation) { create(:jira_connect_installation, client_key: client_key, shared_secret: shared_secret) }
+
+        it 'validates the JWT token in authorization header and returns 200 without creating a new installation' do
+          expect { subject }.not_to change { JiraConnectInstallation.count }
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        context 'when jira_connect_asymmetric_jwt is disabled' do
+          before do
+            stub_feature_flags(jira_connect_asymmetric_jwt: false)
+          end
+
+          it 'decodes the JWT token in authorization header and returns 200 without creating a new installation' do
+            request.headers["Authorization"] = "Bearer #{Atlassian::Jwt.encode({ iss: client_key }, shared_secret)}"
+
+            expect(Atlassian::JiraConnect::AsymmetricJwt).not_to receive(:new)
+
+            expect { subject }.not_to change { JiraConnectInstallation.count }
+
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
       end
     end
   end
 
   describe '#uninstalled' do
-    let!(:installation) { create(:jira_connect_installation) }
-    let(:qsh) { Atlassian::Jwt.create_query_string_hash('https://gitlab.test/events/uninstalled', 'POST', 'https://gitlab.test') }
+    let_it_be(:installation) { create(:jira_connect_installation) }
 
-    before do
-      request.headers['Authorization'] = "JWT #{auth_token}"
+    let(:client_key) { installation.client_key }
+    let(:params) do
+      {
+        clientKey: client_key,
+        baseUrl: 'https://test.atlassian.net'
+      }
     end
 
-    subject(:post_uninstalled) { post :uninstalled }
+    it_behaves_like 'verifies asymmetric JWT token' do
+      let(:send_request) { post :uninstalled, params: params }
+    end
 
-    context 'when JWT is invalid' do
-      let(:auth_token) { 'invalid_token' }
+    subject(:post_uninstalled) { post :uninstalled, params: params }
 
-      it 'returns 403' do
-        post_uninstalled
-
-        expect(response).to have_gitlab_http_status(:forbidden)
-      end
+    context 'when JWT is invalid' do
+      include_context 'invalid JWT token'
 
       it 'does not delete the installation' do
         expect { post_uninstalled }.not_to change { JiraConnectInstallation.count }
       end
+
+      context 'when jira_connect_asymmetric_jwt is disabled' do
+        before do
+          stub_feature_flags(jira_connect_asymmetric_jwt: false)
+          request.headers['Authorization'] = 'JWT invalid token'
+        end
+
+        it 'returns 403' do
+          post_uninstalled
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+
+        it 'does not delete the installation' do
+          expect { post_uninstalled }.not_to change { JiraConnectInstallation.count }
+        end
+      end
     end
 
     context 'when JWT is valid' do
-      let(:auth_token) do
-        Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret)
-      end
+      include_context 'valid JWT token'
 
       let(:jira_base_path) { '/-/jira_connect' }
       let(:jira_event_path) { '/-/jira_connect/events/uninstalled' }
@@ -110,6 +197,36 @@ RSpec.describe JiraConnect::EventsController do
 
         expect(response).to have_gitlab_http_status(:unprocessable_entity)
       end
+
+      context 'when jira_connect_asymmetric_jwt is disabled' do
+        before do
+          stub_feature_flags(jira_connect_asymmetric_jwt: false)
+
+          request.headers['Authorization'] = "JWT #{Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret)}"
+        end
+
+        let(:qsh) { Atlassian::Jwt.create_query_string_hash('https://gitlab.test/events/uninstalled', 'POST', 'https://gitlab.test') }
+
+        it 'calls the DestroyService and returns ok in case of success' do
+          expect_next_instance_of(JiraConnectInstallations::DestroyService, installation, jira_base_path, jira_event_path) do |destroy_service|
+            expect(destroy_service).to receive(:execute).and_return(true)
+          end
+
+          post_uninstalled
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        it 'calls the DestroyService and returns unprocessable_entity in case of failure' do
+          expect_next_instance_of(JiraConnectInstallations::DestroyService, installation, jira_base_path, jira_event_path) do |destroy_service|
+            expect(destroy_service).to receive(:execute).and_return(false)
+          end
+
+          post_uninstalled
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
+        end
+      end
     end
   end
 end

spec/lib/atlassian/jira_connect/asymmetric_jwt_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Atlassian::JiraConnect::AsymmetricJwt do
+  describe '#valid?' do
+    subject(:asymmetric_jwt) { described_class.new(jwt, verification_claims) }
+
+    let(:verification_claims) { jwt_claims }
+    let(:jwt_claims) { { aud: aud, iss: client_key, qsh: qsh } }
+    let(:aud) { 'https://test.host/-/jira_connect' }
+    let(:client_key) { '1234' }
+    let(:qsh) { Atlassian::Jwt.create_query_string_hash('https://gitlab.test/events/installed', 'POST', 'https://gitlab.test') }
+    let(:public_key_id) { '123e4567-e89b-12d3-a456-426614174000' }
+    let(:jwt_headers) { { kid: public_key_id } }
+    let(:private_key) { OpenSSL::PKey::RSA.generate 2048 }
+    let(:jwt) { JWT.encode(jwt_claims, private_key, 'RS256', jwt_headers) }
+    let(:public_key) { private_key.public_key }
+
+    before do
+      stub_request(:get, "https://connect-install-keys.atlassian.com/#{public_key_id}").to_return(body: public_key.to_s, status: 200)
+    end
+
+    it 'returns true when verified with public key from CDN' do
+      expect(JWT).to receive(:decode).twice.and_call_original
+
+      expect(asymmetric_jwt).to be_valid
+
+      expect(WebMock).to have_requested(:get, "https://connect-install-keys.atlassian.com/#{public_key_id}")
+    end
+
+    context 'JWT does not contain a key ID' do
+      let(:public_key_id) { nil }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'JWT contains a key ID that is not a valid UUID4' do
+      let(:public_key_id) { '123' }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'public key can not be retrieved' do
+      before do
+        stub_request(:get, "https://connect-install-keys.atlassian.com/#{public_key_id}").to_return(body: '', status: 404)
+      end
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'retrieving the public raises an error' do
+      before do
+        allow(Gitlab::HTTP).to receive(:get).and_raise(SocketError)
+      end
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'token decoding raises an error' do
+      before do
+        allow(JWT).to receive(:decode).and_call_original
+        allow(JWT).to receive(:decode).with(
+          jwt, anything, true, aud: anything, verify_aud: true, iss: client_key, verify_iss: true, algorithm: 'RS256'
+        ).and_raise(JWT::DecodeError)
+      end
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when iss could not be verified' do
+      let(:verification_claims) { { aud: jwt_claims[:aud], iss: 'some other iss', qsh: jwt_claims[:qsh] } }
+
+      it { is_expected.not_to be_valid }
+    end
+
+    context 'when qsh could not be verified' do
+      let(:verification_claims) { { aud: jwt_claims[:aud], iss: client_key, qsh: 'some other qsh' } }
+
+      it { is_expected.not_to be_valid }
+    end
+  end
+
+  describe '#iss_claim' do
+    subject { asymmetric_jwt.iss_claim }
+
+    let(:asymmetric_jwt) { described_class.new('123', anything) }
+
+    it { is_expected.to eq(nil) }
+
+    context 'when jwt is verified' do
+      before do
+        asymmetric_jwt.instance_variable_set(:@claims, { 'iss' => 'client_key' })
+      end
+
+      it { is_expected.to eq('client_key') }
+    end
+  end
+end