From 406ce19d8eee520f7ad99aa05d81e34aded46eb0 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Wed, 20 Feb 2019 11:21:52 +0800 Subject: [PATCH] Create Jira app descriptor Also handle lifecycle callbacks and verification of JWT from Atlassian Verifies based on stored shared secret and also checks if the query string is not tampered --- config/application.rb | 2 + config/routes.rb | 1 + db/schema.rb | 8 ++ .../jira_connect/app_descriptor_controller.rb | 58 ++++++++++++ .../jira_connect/application_controller.rb | 51 +++++++++++ .../jira_connect/configuration_controller.rb | 30 ++++++ .../jira_connect/events_controller.rb | 28 ++++++ ee/app/models/jira_connect_installation.rb | 11 +++ ee/config/routes/jira_connect.rb | 15 +++ ...44405_create_jira_connect_installations.rb | 21 +++++ ee/lib/atlassian/jwt.rb | 65 +++++++++++++ .../app_descriptor_controller_spec.rb | 38 ++++++++ .../configuration_controller_spec.rb | 52 +++++++++++ .../jira_connect/events_controller_spec.rb | 91 +++++++++++++++++++ .../factories/jira_connect_installation.rb | 9 ++ ee/spec/lib/atlassian/jwt_spec.rb | 33 +++++++ .../models/jira_connect_installation_spec.rb | 14 +++ 17 files changed, 527 insertions(+) create mode 100644 ee/app/controllers/jira_connect/app_descriptor_controller.rb create mode 100644 ee/app/controllers/jira_connect/application_controller.rb create mode 100644 ee/app/controllers/jira_connect/configuration_controller.rb create mode 100644 ee/app/controllers/jira_connect/events_controller.rb create mode 100644 ee/app/models/jira_connect_installation.rb create mode 100644 ee/config/routes/jira_connect.rb create mode 100644 ee/db/migrate/20190218144405_create_jira_connect_installations.rb create mode 100644 ee/lib/atlassian/jwt.rb create mode 100644 ee/spec/controllers/jira_connect/app_descriptor_controller_spec.rb create mode 100644 ee/spec/controllers/jira_connect/configuration_controller_spec.rb create mode 100644 ee/spec/controllers/jira_connect/events_controller_spec.rb create mode 100644 ee/spec/factories/jira_connect_installation.rb create mode 100644 ee/spec/lib/atlassian/jwt_spec.rb create mode 100644 ee/spec/models/jira_connect_installation_spec.rb diff --git a/config/application.rb b/config/application.rb index ead1ac2d4bc75d..62f7275d8c8f91 100644 --- a/config/application.rb +++ b/config/application.rb @@ -111,6 +111,7 @@ class Application < Rails::Application # - Webhook URLs (:hook) # - Sentry DSN (:sentry_dsn) # - File content from Web Editor (:content) + # - Jira shared secret (:sharedSecret) # # NOTE: It is **IMPORTANT** to also update gitlab-workhorse's filter when adding parameters here to not # introduce another security vulnerability: https://gitlab.com/gitlab-org/gitlab-workhorse/issues/182 @@ -125,6 +126,7 @@ class Application < Rails::Application trace variables content + sharedSecret ) # Enable escaping HTML in JSON. diff --git a/config/routes.rb b/config/routes.rb index d47ec7c864bc3c..60b74a78ec4bdf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -91,6 +91,7 @@ draw :operations draw :instance_statistics draw :smartcard + draw :jira_connect if ENV['GITLAB_ENABLE_CHAOS_ENDPOINTS'] get '/chaos/leakmem' => 'chaos#leakmem' diff --git a/db/schema.rb b/db/schema.rb index 9a65135e6991e3..05c987be53bdec 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1582,6 +1582,14 @@ t.index ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree end + create_table "jira_connect_installations", id: :bigserial, force: :cascade do |t| + t.string "client_key" + t.string "encrypted_shared_secret" + t.string "encrypted_shared_secret_iv" + t.string "base_url" + t.index ["client_key"], name: "index_jira_connect_installations_on_client_key", unique: true, using: :btree + end + create_table "keys", force: :cascade do |t| t.integer "user_id" t.datetime "created_at" diff --git a/ee/app/controllers/jira_connect/app_descriptor_controller.rb b/ee/app/controllers/jira_connect/app_descriptor_controller.rb new file mode 100644 index 00000000000000..bed45c6428ac5d --- /dev/null +++ b/ee/app/controllers/jira_connect/app_descriptor_controller.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +# This returns an app descriptor for use with Jira in development mode +# For the Atlassian Marketplace, a static copy of this JSON is uploaded to the marketplace +# https://developer.atlassian.com/cloud/jira/platform/app-descriptor/ + +class JiraConnect::AppDescriptorController < JiraConnect::ApplicationController + skip_before_action :verify_atlassian_jwt! + + def show + render json: { + name: "GitLab for Jira (#{Gitlab.config.gitlab.host})", + description: 'Integrate commits, branches and merge requests from GitLab into Jira', + key: "gitlab-jira-connect-#{Gitlab.config.gitlab.host}", + baseUrl: jira_connect_base_url, + lifecycle: { + installed: relative_to_base_path(jira_connect_events_installed_path), + uninstalled: relative_to_base_path(jira_connect_events_uninstalled_path) + }, + vendor: { + name: 'GitLab', + url: 'https://gitlab.com' + }, + authentication: { + type: 'jwt' + }, + scopes: %w(READ WRITE DELETE), + apiVersion: 1, + modules: { + jiraDevelopmentTool: { + key: 'gitlab-development-tool', + application: { + value: 'GitLab' + }, + name: { + value: 'GitLab' + }, + url: 'https://gitlab.com', + logoUrl: view_context.image_url('gitlab_logo.png'), + capabilities: %w(branch commit pull_request) + }, + postInstallPage: { + key: 'gitlab-configuration', + name: { + value: 'GitLab Configuration' + }, + url: relative_to_base_path(jira_connect_configuration_path) + } + } + } + end + + private + + def relative_to_base_path(full_path) + full_path.sub(/^#{jira_connect_base_path}/, '') + end +end diff --git a/ee/app/controllers/jira_connect/application_controller.rb b/ee/app/controllers/jira_connect/application_controller.rb new file mode 100644 index 00000000000000..8ca1a22a30b822 --- /dev/null +++ b/ee/app/controllers/jira_connect/application_controller.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +class JiraConnect::ApplicationController < ApplicationController + include Gitlab::Utils::StrongMemoize + + skip_before_action :authenticate_user! + before_action :check_feature_flag_enabled! + before_action :verify_atlassian_jwt! + + attr_reader :current_jira_installation + + private + + def check_feature_flag_enabled! + render_404 unless Feature.enabled?(:jira_connect_app) + end + + def verify_atlassian_jwt! + return render_403 unless atlassian_jwt_valid? + + @current_jira_installation = installation_from_jwt + end + + def atlassian_jwt_valid? + return false unless installation_from_jwt + + # Verify JWT signature with our stored `shared_secret` + payload, _ = Atlassian::Jwt.decode(auth_token, installation_from_jwt.shared_secret) + + # Make sure `qsh` claim matches the current request + payload['qsh'] == Atlassian::Jwt.create_query_string_hash(request.method, request.url, jira_connect_base_url) + rescue JWT::DecodeError + false + end + + def installation_from_jwt + return unless auth_token + + strong_memoize(:installation_from_jwt) do + # Decode without verification to get `client_key` in `iss` + payload, _ = Atlassian::Jwt.decode(auth_token, nil, false) + JiraConnectInstallation.find_by_client_key(payload['iss']) + end + end + + def auth_token + strong_memoize(:auth_token) do + params[:jwt] || request.headers['Authorization']&.split(' ', 2)&.last + end + end +end diff --git a/ee/app/controllers/jira_connect/configuration_controller.rb b/ee/app/controllers/jira_connect/configuration_controller.rb new file mode 100644 index 00000000000000..cd7c70b8760253 --- /dev/null +++ b/ee/app/controllers/jira_connect/configuration_controller.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +class JiraConnect::ConfigurationController < JiraConnect::ApplicationController + before_action :allow_rendering_in_iframe + + def show + sample_html = <<~HEREDOC + + + + + + + +
+

Hello from GitLab!

+
+ + + HEREDOC + + render html: sample_html.html_safe + end + + private + + def allow_rendering_in_iframe + response.headers.delete('X-Frame-Options') + end +end diff --git a/ee/app/controllers/jira_connect/events_controller.rb b/ee/app/controllers/jira_connect/events_controller.rb new file mode 100644 index 00000000000000..2f9b04fb27563f --- /dev/null +++ b/ee/app/controllers/jira_connect/events_controller.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class JiraConnect::EventsController < JiraConnect::ApplicationController + skip_before_action :verify_authenticity_token + skip_before_action :verify_atlassian_jwt!, only: :installed + + def installed + if JiraConnectInstallation.create(install_params) + head :ok + else + head :unprocessable_entity + end + end + + def uninstalled + if current_jira_installation.destroy + head :ok + else + head :unprocessable_entity + end + end + + private + + def install_params + params.permit(:clientKey, :sharedSecret, :baseUrl).transform_keys(&:underscore) + end +end diff --git a/ee/app/models/jira_connect_installation.rb b/ee/app/models/jira_connect_installation.rb new file mode 100644 index 00000000000000..b6b312bd8400c1 --- /dev/null +++ b/ee/app/models/jira_connect_installation.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class JiraConnectInstallation < ApplicationRecord + attr_encrypted :shared_secret, + mode: :per_attribute_iv, + algorithm: 'aes-256-gcm', + key: Settings.attr_encrypted_db_key_base_32 + + validates :client_key, :shared_secret, presence: true + validates :base_url, presence: true, public_url: true +end diff --git a/ee/config/routes/jira_connect.rb b/ee/config/routes/jira_connect.rb new file mode 100644 index 00000000000000..6f22993182b2ac --- /dev/null +++ b/ee/config/routes/jira_connect.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +namespace :jira_connect do + # This is so we can have a named route helper for the base URL + root to: proc { [404, {}, ['']] }, as: 'base' + + get 'app_descriptor' => 'app_descriptor#show' + + namespace :events do + post 'installed' + post 'uninstalled' + end + + get 'configuration' => 'configuration#show' +end diff --git a/ee/db/migrate/20190218144405_create_jira_connect_installations.rb b/ee/db/migrate/20190218144405_create_jira_connect_installations.rb new file mode 100644 index 00000000000000..c31d60e72b199e --- /dev/null +++ b/ee/db/migrate/20190218144405_create_jira_connect_installations.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateJiraConnectInstallations < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :jira_connect_installations, id: :bigserial do |t| + t.string :client_key + t.string :encrypted_shared_secret + t.string :encrypted_shared_secret_iv + t.string :base_url + end + + add_index :jira_connect_installations, :client_key, unique: true + end +end diff --git a/ee/lib/atlassian/jwt.rb b/ee/lib/atlassian/jwt.rb new file mode 100644 index 00000000000000..89c67e8aa2400e --- /dev/null +++ b/ee/lib/atlassian/jwt.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +# This is based on https://bitbucket.org/atlassian/atlassian-jwt-ruby +# which is unmaintained and incompatible with later versions of jwt-ruby + +module Atlassian + module Jwt + class << self + CANONICAL_QUERY_SEPARATOR = '&' + ESCAPED_CANONICAL_QUERY_SEPARATOR = '%26' + + def decode(token, secret, validate = true, options = {}) + options = { algorithm: 'HS256' }.merge(options) + ::JWT.decode(token, secret, validate, options) + end + + def encode(payload, secret, algorithm = 'HS256', header_fields = {}) + ::JWT.encode(payload, secret, algorithm, header_fields) + end + + def create_query_string_hash(http_method, uri, base_uri = '') + Digest::SHA256.hexdigest( + create_canonical_request(http_method, uri, base_uri) + ) + end + + private + + def create_canonical_request(http_method, uri, base_uri) + uri = URI.parse(uri) unless uri.is_a?(URI) + base_uri = URI.parse(base_uri) unless base_uri.is_a?(URI) + + [ + http_method.upcase, + canonicalize_uri(uri, base_uri), + canonicalize_query_string(uri.query) + ].join(CANONICAL_QUERY_SEPARATOR) + end + + def canonicalize_uri(uri, base_uri) + path = uri.path.sub(/^#{base_uri.path}/, '') + path = '/' if path.nil? || path.empty? + path = '/' + path unless path.start_with? '/' + path.chomp!('/') if path.length > 1 + path.gsub(CANONICAL_QUERY_SEPARATOR, ESCAPED_CANONICAL_QUERY_SEPARATOR) + end + + def canonicalize_query_string(query) + return '' if query.nil? || query.empty? + + query = CGI.parse(query) + query.delete('jwt') + + query.each do |k, v| + query[k] = v.map { |a| CGI.escape a }.join(',') if v.is_a?(Array) + query[k].gsub!('+', '%20') # Use %20, not CGI.escape default of "+" + query[k].gsub!('%7E', '~') # Unescape "~" + end + + query = Hash[query.sort] + query.map { |k, v| "#{CGI.escape k}=#{v}" }.join('&') + end + end + end +end diff --git a/ee/spec/controllers/jira_connect/app_descriptor_controller_spec.rb b/ee/spec/controllers/jira_connect/app_descriptor_controller_spec.rb new file mode 100644 index 00000000000000..093d3675e4ef02 --- /dev/null +++ b/ee/spec/controllers/jira_connect/app_descriptor_controller_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraConnect::AppDescriptorController do + describe '#show' do + context 'feature disabled' do + before do + stub_feature_flags(jira_connect_app: false) + end + + it 'returns 404' do + get :show + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'feature enabled' do + before do + stub_feature_flags(jira_connect_app: true) + end + + it 'returns JSON app descriptor' do + get :show + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to include( + 'baseUrl' => 'http://test.host/-/jira_connect', + 'lifecycle' => { + 'installed' => '/events/installed', + 'uninstalled' => '/events/uninstalled' + } + ) + end + end + end +end diff --git a/ee/spec/controllers/jira_connect/configuration_controller_spec.rb b/ee/spec/controllers/jira_connect/configuration_controller_spec.rb new file mode 100644 index 00000000000000..5ea80e4662ea70 --- /dev/null +++ b/ee/spec/controllers/jira_connect/configuration_controller_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraConnect::ConfigurationController do + describe '#show' do + context 'feature disabled' do + before do + stub_feature_flags(jira_connect_app: false) + end + + it 'returns 404' do + get :show + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'feature enabled' do + before do + stub_feature_flags(jira_connect_app: true) + end + + context 'without JWT' do + it 'returns 403' do + get :show + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with correct JWT' do + let(:installation) { create(:jira_connect_installation) } + let(:qsh) { Atlassian::Jwt.create_query_string_hash('GET', '/configuration') } + + before do + get :show, params: { + jwt: Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret) + } + end + + it 'returns 200' do + expect(response).to have_gitlab_http_status(200) + end + + it 'removes X-Frame-Options to allow rendering in iframe' do + expect(response.headers['X-Frame-Options']).to be_nil + end + end + end + end +end diff --git a/ee/spec/controllers/jira_connect/events_controller_spec.rb b/ee/spec/controllers/jira_connect/events_controller_spec.rb new file mode 100644 index 00000000000000..cf869404ce52ae --- /dev/null +++ b/ee/spec/controllers/jira_connect/events_controller_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraConnect::EventsController do + context 'feature disabled' do + before do + stub_feature_flags(jira_connect_app: false) + end + + describe '#installed' do + it 'returns 404' do + post :installed + + expect(response).to have_gitlab_http_status(404) + end + end + + describe '#uninstalled' do + it 'returns 404' do + post :uninstalled + + expect(response).to have_gitlab_http_status(404) + end + end + end + + context 'feature enabled' do + before do + stub_feature_flags(jira_connect_app: true) + end + + describe '#installed' do + subject do + post :installed, params: { + clientKey: '1234', + sharedSecret: 'secret', + baseUrl: 'https://test.atlassian.net' + } + end + + it 'saves the jira installation data' do + expect { subject }.to change { JiraConnectInstallation.count }.by(1) + end + + it 'saves the correct values' do + subject + + installation = JiraConnectInstallation.find_by_client_key('1234') + + expect(installation.shared_secret).to eq('secret') + expect(installation.base_url).to eq('https://test.atlassian.net') + end + end + + describe '#uninstalled' do + let!(:installation) { create(:jira_connect_installation) } + let(:qsh) { Atlassian::Jwt.create_query_string_hash('POST', '/events/uninstalled') } + + before do + request.headers['Authorization'] = "JWT #{auth_token}" + end + + subject { post :uninstalled } + + context 'when JWT is invalid' do + let(:auth_token) { 'invalid_token' } + + it 'returns 403' do + subject + + expect(response).to have_gitlab_http_status(403) + end + + it 'does not delete the installation' do + expect { subject }.not_to change { JiraConnectInstallation.count } + 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 + + it 'deletes the installation' do + expect { subject }.to change { JiraConnectInstallation.count }.by(-1) + end + end + end + end +end diff --git a/ee/spec/factories/jira_connect_installation.rb b/ee/spec/factories/jira_connect_installation.rb new file mode 100644 index 00000000000000..f8e7361b763eb8 --- /dev/null +++ b/ee/spec/factories/jira_connect_installation.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :jira_connect_installation do + sequence(:client_key) { |n| "atlassian-client-key-#{n}" } + shared_secret 'jrNarHaRYaumMvfV3UnYpwt8' + base_url 'https://sample.atlassian.net' + end +end diff --git a/ee/spec/lib/atlassian/jwt_spec.rb b/ee/spec/lib/atlassian/jwt_spec.rb new file mode 100644 index 00000000000000..670ff6244a8838 --- /dev/null +++ b/ee/spec/lib/atlassian/jwt_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Atlassian::Jwt do + describe '#create_query_string_hash' do + using RSpec::Parameterized::TableSyntax + + let(:base_uri) { 'https://example.com/-/jira_connect' } + + where(:path, :method, :expected_hash) do + '/events/uninstalled' | 'POST' | '57d5306d4c520456ebb58ac802779232a941e583589354b8a31aa949cdd4c9ae' + '/events/uninstalled/' | 'post' | '57d5306d4c520456ebb58ac802779232a941e583589354b8a31aa949cdd4c9ae' + '/configuration' | 'GET' | 'be30d9dc39ca6a6543a0b05a253ed9aa36d282311af4cecad54b487dffa62769' + '/' | 'PUT' | 'c88c7735138a8806c60f95f0d3e133d1d3d313e2a9d590abbb5f898dabad7b62' + '' | 'PUT' | 'c88c7735138a8806c60f95f0d3e133d1d3d313e2a9d590abbb5f898dabad7b62' + end + + with_them do + it 'generates correct hash with base URI' do + hash = subject.create_query_string_hash(method, base_uri + path, base_uri) + + expect(hash).to eq(expected_hash) + end + + it 'generates correct hash with base URI already removed' do + hash = subject.create_query_string_hash(method, path) + + expect(hash).to eq(expected_hash) + end + end + end +end diff --git a/ee/spec/models/jira_connect_installation_spec.rb b/ee/spec/models/jira_connect_installation_spec.rb new file mode 100644 index 00000000000000..9aba1b6aa1e5a1 --- /dev/null +++ b/ee/spec/models/jira_connect_installation_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe JiraConnectInstallation do + describe 'validations' do + it { is_expected.to validate_presence_of(:client_key) } + it { is_expected.to validate_presence_of(:shared_secret) } + it { is_expected.to validate_presence_of(:base_url) } + + it { is_expected.to allow_value('https://test.atlassian.net').for(:base_url) } + it { is_expected.not_to allow_value('not/a/url').for(:base_url) } + end +end -- GitLab