From b34f3e782afca911861eb991923ad4e0ffe755f1 Mon Sep 17 00:00:00 2001 From: Imre Farkas Date: Tue, 13 Oct 2020 17:40:16 +0200 Subject: [PATCH] Add FortiAuthenticator as OTP method Currently behind the :forti_authenticator feature flag. --- app/controllers/admin/sessions_controller.rb | 5 +- .../profiles/two_factor_auths_controller.rb | 5 +- app/controllers/sessions_controller.rb | 7 ++- app/models/user.rb | 2 +- app/services/users/validate_otp_service.rb | 25 +++++++++ .../development/forti_authenticator.yml | 7 +++ config/gitlab.yml.example | 15 +++++ config/initializers/1_settings.rb | 7 +++ lib/gitlab/auth/otp/strategies/base.rb | 32 +++++++++++ lib/gitlab/auth/otp/strategies/devise.rb | 15 +++++ .../otp/strategies/forti_authenticator.rb | 41 ++++++++++++++ .../gitlab/auth/otp/strategies/devise_spec.rb | 16 ++++++ .../strategies/forti_authenticator_spec.rb | 55 +++++++++++++++++++ .../users/validate_otp_service_spec.rb | 34 ++++++++++++ spec/spec_helper.rb | 4 ++ 15 files changed, 265 insertions(+), 5 deletions(-) create mode 100644 app/services/users/validate_otp_service.rb create mode 100644 config/feature_flags/development/forti_authenticator.yml create mode 100644 lib/gitlab/auth/otp/strategies/base.rb create mode 100644 lib/gitlab/auth/otp/strategies/devise.rb create mode 100644 lib/gitlab/auth/otp/strategies/forti_authenticator.rb create mode 100644 spec/lib/gitlab/auth/otp/strategies/devise_spec.rb create mode 100644 spec/lib/gitlab/auth/otp/strategies/forti_authenticator_spec.rb create mode 100644 spec/services/users/validate_otp_service_spec.rb diff --git a/app/controllers/admin/sessions_controller.rb b/app/controllers/admin/sessions_controller.rb index 3c811e7c3f52e7..9c378f4c883220 100644 --- a/app/controllers/admin/sessions_controller.rb +++ b/app/controllers/admin/sessions_controller.rb @@ -67,7 +67,10 @@ def user_params end def valid_otp_attempt?(user) - valid_otp_attempt = user.validate_and_consume_otp!(user_params[:otp_attempt]) + otp_validation_result = + ::Users::ValidateOtpService.new(user).execute(user_params[:otp_attempt]) + valid_otp_attempt = otp_validation_result[:status] == :success + return valid_otp_attempt if Gitlab::Database.read_only? valid_otp_attempt || user.invalidate_otp_backup_code!(user_params[:otp_attempt]) diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 7d82d6a3bcb892..e2f8baa822600e 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -47,7 +47,10 @@ def show end def create - if current_user.validate_and_consume_otp!(params[:pin_code]) + otp_validation_result = + ::Users::ValidateOtpService.new(current_user).execute(params[:pin_code]) + + if otp_validation_result[:status] == :success ActiveSession.destroy_all_but_current(current_user, session) Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user| diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ae9744b224acdc..61120c5b7d1278 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -264,8 +264,11 @@ def auto_sign_in_with_provider end def valid_otp_attempt?(user) - user.validate_and_consume_otp!(user_params[:otp_attempt]) || - user.invalidate_otp_backup_code!(user_params[:otp_attempt]) + otp_validation_result = + ::Users::ValidateOtpService.new(user).execute(user_params[:otp_attempt]) + return true if otp_validation_result[:status] == :success + + user.invalidate_otp_backup_code!(user_params[:otp_attempt]) end def log_audit_event(user, resource, options = {}) diff --git a/app/models/user.rb b/app/models/user.rb index 41cb15677a8138..ef77e207215a49 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -793,7 +793,7 @@ def two_factor_enabled? end def two_factor_otp_enabled? - otp_required_for_login? + otp_required_for_login? || Feature.enabled?(:forti_authenticator, self) end def two_factor_u2f_enabled? diff --git a/app/services/users/validate_otp_service.rb b/app/services/users/validate_otp_service.rb new file mode 100644 index 00000000000000..a9ce7959aea2b7 --- /dev/null +++ b/app/services/users/validate_otp_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Users + class ValidateOtpService < BaseService + def initialize(current_user) + @current_user = current_user + @strategy = if Feature.enabled?(:forti_authenticator, current_user) + ::Gitlab::Auth::Otp::Strategies::FortiAuthenticator.new(current_user) + else + ::Gitlab::Auth::Otp::Strategies::Devise.new(current_user) + end + end + + def execute(otp_code) + strategy.validate(otp_code) + rescue StandardError => ex + Gitlab::ErrorTracking.log_exception(ex) + error(message: ex.message) + end + + private + + attr_reader :strategy + end +end diff --git a/config/feature_flags/development/forti_authenticator.yml b/config/feature_flags/development/forti_authenticator.yml new file mode 100644 index 00000000000000..31f5256753f17e --- /dev/null +++ b/config/feature_flags/development/forti_authenticator.yml @@ -0,0 +1,7 @@ +--- +name: forti_authenticator +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45055 +rollout_issue_url: +type: development +group: group::access +default_enabled: false diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index bef6d4e48d1ead..de389514ccea6c 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -1022,6 +1022,21 @@ production: &base # cas3: # session_duration: 28800 + # FortiAuthenticator settings + forti_authenticator: + # Allow using FortiAuthenticator as OTP provider + enabled: false + + # Host and port of FortiAuthenticator instance + # host: forti_authenticator.example.com + # port: 443 + + # Username for accessing FortiAuthenticator API + # username: john + + # Access token for FortiAuthenticator API + # access_token: 123s3cr3t456 + # Shared file storage settings shared: # path: /mnt/gitlab # Default: shared diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index baf728fb0dc89e..affbc85d5a9c97 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -766,6 +766,13 @@ Settings.smartcard['san_extensions'] = false if Settings.smartcard['san_extensions'].nil? end +# +# FortiAuthenticator +# +Settings['forti_authenticator'] ||= Settingslogic.new({}) +Settings.forti_authenticator['enabled'] = false if Settings.forti_authenticator['enabled'].nil? +Settings.forti_authenticator['port'] = 443 if Settings.forti_authenticator['port'].to_i == 0 + # # Extra customization # diff --git a/lib/gitlab/auth/otp/strategies/base.rb b/lib/gitlab/auth/otp/strategies/base.rb new file mode 100644 index 00000000000000..718630e0e31994 --- /dev/null +++ b/lib/gitlab/auth/otp/strategies/base.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + module Otp + module Strategies + class Base + def initialize(user) + @user = user + end + + private + + attr_reader :user + + def success + { status: :success } + end + + def error(message, http_status = nil) + result = { message: message, + status: :error } + + result[:http_status] = http_status if http_status + + result + end + end + end + end + end +end diff --git a/lib/gitlab/auth/otp/strategies/devise.rb b/lib/gitlab/auth/otp/strategies/devise.rb new file mode 100644 index 00000000000000..93068d6c9b0861 --- /dev/null +++ b/lib/gitlab/auth/otp/strategies/devise.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + module Otp + module Strategies + class Devise < Base + def validate(otp_code) + user.validate_and_consume_otp!(otp_code) ? success : error('invalid OTP code') + end + end + end + end + end +end diff --git a/lib/gitlab/auth/otp/strategies/forti_authenticator.rb b/lib/gitlab/auth/otp/strategies/forti_authenticator.rb new file mode 100644 index 00000000000000..fbcb9fd8cdb403 --- /dev/null +++ b/lib/gitlab/auth/otp/strategies/forti_authenticator.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + module Otp + module Strategies + class FortiAuthenticator < Base + def validate(otp_code) + body = { username: user.username, + token_code: otp_code } + + response = Gitlab::HTTP.post( + auth_url, + headers: { 'Content-Type': 'application/json' }, + body: body.to_json, + basic_auth: api_credentials) + + # Successful authentication results in HTTP 200: OK + # https://docs.fortinet.com/document/fortiauthenticator/6.2.0/rest-api-solution-guide/704555/authentication-auth + response.ok? ? success : error(message: response.message, http_status: response.code) + end + + private + + def auth_url + host = ::Gitlab.config.forti_authenticator.host + port = ::Gitlab.config.forti_authenticator.port + path = 'api/v1/auth/' + + "https://#{host}:#{port}/#{path}" + end + + def api_credentials + { username: ::Gitlab.config.forti_authenticator.username, + password: ::Gitlab.config.forti_authenticator.token } + end + end + end + end + end +end diff --git a/spec/lib/gitlab/auth/otp/strategies/devise_spec.rb b/spec/lib/gitlab/auth/otp/strategies/devise_spec.rb new file mode 100644 index 00000000000000..0c88421d456142 --- /dev/null +++ b/spec/lib/gitlab/auth/otp/strategies/devise_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Auth::Otp::Strategies::Devise do + let_it_be(:user) { create(:user) } + let(:otp_code) { 42 } + + subject(:validate) { described_class.new(user).validate(otp_code) } + + it 'calls Devise' do + expect(user).to receive(:validate_and_consume_otp!).with(otp_code) + + validate + end +end diff --git a/spec/lib/gitlab/auth/otp/strategies/forti_authenticator_spec.rb b/spec/lib/gitlab/auth/otp/strategies/forti_authenticator_spec.rb new file mode 100644 index 00000000000000..18fd6d08057af8 --- /dev/null +++ b/spec/lib/gitlab/auth/otp/strategies/forti_authenticator_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Auth::Otp::Strategies::FortiAuthenticator do + let_it_be(:user) { create(:user) } + let(:otp_code) { 42 } + + let(:host) { 'forti_authenticator.example.com' } + let(:port) { '444' } + let(:api_username) { 'janedoe' } + let(:api_token) { 's3cr3t' } + + let(:forti_authenticator_auth_url) { "https://#{host}:#{port}/api/v1/auth/" } + + subject(:validate) { described_class.new(user).validate(otp_code) } + + before do + stub_feature_flags(forti_authenticator: true) + + stub_forti_authenticator_config( + host: host, + port: port, + username: api_username, + token: api_token + ) + + request_body = { username: user.username, + token_code: otp_code } + + stub_request(:post, forti_authenticator_auth_url) + .with(body: JSON(request_body), headers: { 'Content-Type' => 'application/json' }) + .to_return(status: response_status, body: '', headers: {}) + end + + context 'successful validation' do + let(:response_status) { 200 } + + it 'returns success' do + expect(validate[:status]).to eq(:success) + end + end + + context 'unsuccessful validation' do + let(:response_status) { 401 } + + it 'returns error' do + expect(validate[:status]).to eq(:error) + end + end + + def stub_forti_authenticator_config(forti_authenticator_settings) + allow(::Gitlab.config.forti_authenticator).to(receive_messages(forti_authenticator_settings)) + end +end diff --git a/spec/services/users/validate_otp_service_spec.rb b/spec/services/users/validate_otp_service_spec.rb new file mode 100644 index 00000000000000..826755d61452ff --- /dev/null +++ b/spec/services/users/validate_otp_service_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::ValidateOtpService do + let_it_be(:user) { create(:user) } + let(:otp_code) { 42 } + + subject(:validate) { described_class.new(user).execute(otp_code) } + + context 'Devise' do + it 'calls Devise strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::Devise) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once + end + + validate + end + end + + context 'FortiAuthenticator' do + before do + stub_feature_flags(forti_authenticator: true) + end + + it 'calls FortiAuthenticator strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiAuthenticator) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once + end + + validate + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6e41ae3fbfd04e..11a45e005b83bc 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -212,6 +212,10 @@ # for now whilst we migrate as much as we can over the GraphQL stub_feature_flags(merge_request_widget_graphql: false) + # Using FortiAuthenticator as OTP provider is disabled by default in + # tests, until we introduce it in user settings + stub_feature_flags(forti_authenticator: false) + enable_rugged = example.metadata[:enable_rugged].present? # Disable Rugged features by default -- GitLab