From 96d528cd1dfa4ce8cf416e9a0e65b950f7d0c2fa Mon Sep 17 00:00:00 2001 From: Nick Carboni Date: Thu, 16 Feb 2017 13:46:56 -0500 Subject: [PATCH 1/2] Remove the mechanisms around "configuring" central admin This was put in place because the authentication method we were using involved encrypting a token using a remote region's v2_key. The initial assumption was that regions could have different keys so we needed to do some configuration to fetch the remote key for each subscription. That assumption has been proven incorrect; all regions in the enterprise should be using the same v2_key. This allows us to authenticate with remote regions as soon as replication is configured. --- app/models/miq_region.rb | 66 +------------- .../mixins/inter_region_api_method_relay.rb | 5 -- app/models/pglogical_subscription.rb | 1 - spec/factories/authentication.rb | 2 - spec/models/miq_region_spec.rb | 90 ------------------- .../inter_region_api_method_relay_spec.rb | 49 ++++------ spec/models/pglogical_subscription_spec.rb | 17 ---- 7 files changed, 19 insertions(+), 211 deletions(-) diff --git a/app/models/miq_region.rb b/app/models/miq_region.rb index caca72d3174..9c2e7bf0114 100644 --- a/app/models/miq_region.rb +++ b/app/models/miq_region.rb @@ -20,7 +20,6 @@ class MiqRegion < ApplicationRecord after_save :clear_my_region_cache acts_as_miq_taggable - include AuthenticationMixin include UuidMixin include NamingSequenceMixin include AggregationMixin @@ -32,7 +31,6 @@ class MiqRegion < ApplicationRecord alias_method :all_storages, :storages PERF_ROLLUP_CHILDREN = [:ext_management_systems, :storages] - AUTHENTICATION_TYPE = "system_api".freeze def database_backups DatabaseBackup.in_region(region_number) @@ -244,69 +242,13 @@ def remote_ws_url hostname && URI::HTTPS.build(:host => hostname).to_s end - def generate_auth_key_queue(ssh_user, ssh_password, ssh_host = nil) - args = [ssh_user, MiqPassword.try_encrypt(ssh_password)] - args << ssh_host if ssh_host - - MiqQueue.put_unless_exists( - :class_name => self.class.name, - :instance_id => id, - :queue_name => "generic", - :method_name => "generate_auth_key", - :args => args - ) - end - - def generate_auth_key(ssh_user, ssh_password, ssh_host = remote_ws_address) - key = remote_region_v2_key(ssh_user, MiqPassword.try_decrypt(ssh_password), ssh_host) - - auth = AuthToken.new - auth.auth_key = key - auth.name = "Region #{region} API Key" - auth.resource = self - auth.authtype = AUTHENTICATION_TYPE - auth.save! - end - - def remove_auth_key - authentication_delete(AUTHENTICATION_TYPE) - end - - def verify_credentials(_auth_type = nil, _options = nil) - # TODO: verify the key against the remote api using the api client gem - true - end - - def auth_key_configured? - authentication_token(AUTHENTICATION_TYPE).present? - end - def api_system_auth_token(userid) token_hash = { :server_guid => remote_ws_miq_server.guid, :userid => userid, :timestamp => Time.now.utc } - encrypt(token_hash.to_yaml) - end - - def required_credential_fields(_type) - [:auth_key] - end - - def encrypt(string) - region_v2_key = authentication_token(AUTHENTICATION_TYPE) - raise "No key configured for region #{region}. Configure Central Admin to fetch the key" if region_v2_key.nil? - - file = Tempfile.new("region_auth_key") - begin - file.write(region_v2_key) - file.close - key = EzCrypto::Key.load(file.path) - MiqPassword.new.encrypt(string, "v2", key) - ensure - file.unlink - end + MiqPassword.encrypt(token_hash.to_yaml) end def self.api_system_auth_token_for_region(region_id, user) @@ -392,10 +334,4 @@ def perf_capture_always=(options) def clear_my_region_cache MiqRegion.my_region_clear_cache end - - def remote_region_v2_key(ssh_user, ssh_password, ssh_host) - require 'net/scp' - key_path = "/var/www/miq/vmdb/certs/v2_key" - Net::SCP.download!(ssh_host, ssh_user, key_path, nil, :ssh => {:password => ssh_password}) - end end diff --git a/app/models/mixins/inter_region_api_method_relay.rb b/app/models/mixins/inter_region_api_method_relay.rb index e6630b9c235..789ae722da9 100644 --- a/app/models/mixins/inter_region_api_method_relay.rb +++ b/app/models/mixins/inter_region_api_method_relay.rb @@ -58,11 +58,6 @@ def api_relay_class_method(method, action = method) def self.api_client_connection_for_region(region_number, user = User.current_userid) region = MiqRegion.find_by(:region => region_number) - unless region.auth_key_configured? - _log.error("Region #{region_number} is not configured for central administration") - raise "Region #{region_number} is not configured for central administration" - end - url = region.remote_ws_url if url.nil? _log.error("The remote region [#{region_number}] does not have a web service address.") diff --git a/app/models/pglogical_subscription.rb b/app/models/pglogical_subscription.rb index 25a9e67366a..43659f93071 100644 --- a/app/models/pglogical_subscription.rb +++ b/app/models/pglogical_subscription.rb @@ -62,7 +62,6 @@ def self.save_all!(subscription_list) def delete pglogical.subscription_drop(id, true) - MiqRegion.find_by_region(provider_region).remove_auth_key MiqRegion.destroy_region(connection, provider_region) if self.class.count == 0 pglogical.node_drop(MiqPglogical.local_node_name, true) diff --git a/spec/factories/authentication.rb b/spec/factories/authentication.rb index e2da8086ed3..c76cf4b0710 100644 --- a/spec/factories/authentication.rb +++ b/spec/factories/authentication.rb @@ -105,6 +105,4 @@ factory :ansible_network_credential, :parent => :automation_manager_authentication, :class => "ManageIQ::Providers::AnsibleTower::AutomationManager::NetworkCredential" - - factory :auth_token, :class => "AuthToken" end diff --git a/spec/models/miq_region_spec.rb b/spec/models/miq_region_spec.rb index caf4449e937..96f38a88f46 100644 --- a/spec/models/miq_region_spec.rb +++ b/spec/models/miq_region_spec.rb @@ -154,76 +154,10 @@ end end - describe "#generate_auth_key" do - let(:remote_region) { FactoryGirl.create(:miq_region) } - let(:remote_key) { "this is the encryption key!" } - - before { EvmSpecHelper.create_guid_miq_server_zone } - - it "stores an authentication key" do - require 'net/scp' - host = "remote-region.example.com" - password = "mypassword" - user = "admin" - - expect(Net::SCP).to receive(:download!) - .with(host, user, "/var/www/miq/vmdb/certs/v2_key", nil, :ssh => {:password => password}) - .and_return(remote_key) - - remote_region.generate_auth_key(user, password, host) - - expect(remote_region.authentication_token("system_api")).to eq(remote_key) - end - end - - describe "#auth_key_configured?" do - let(:remote_region) { FactoryGirl.create(:miq_region) } - let(:remote_key) { "this is the encryption key!" } - - before { EvmSpecHelper.create_guid_miq_server_zone } - - it "returns true if a key is configured" do - FactoryGirl.create( - :auth_token, - :resource_id => remote_region.id, - :resource_type => "MiqRegion", - :auth_key => remote_key - ) - - expect(remote_region.auth_key_configured?).to be true - end - - it "returns false if a key is not configured" do - expect(remote_region.auth_key_configured?).to be false - end - end - - describe "#remove_auth_key" do - let(:remote_region) { FactoryGirl.create(:miq_region) } - let(:remote_key) { "this is the encryption key!" } - - before { EvmSpecHelper.create_guid_miq_server_zone } - - it "removes a key if configured" do - FactoryGirl.create( - :auth_token, - :resource_id => remote_region.id, - :resource_type => "MiqRegion", - :auth_key => remote_key, - :authtype => "system_api" - ) - expect(remote_region.auth_key_configured?).to be true - remote_region.remove_auth_key - remote_region.reload - expect(remote_region.auth_key_configured?).to be false - end - end - describe "#api_system_auth_token" do it "generates the token correctly" do user = "admin" server = FactoryGirl.create(:miq_server, :has_active_webservices => true) - expect(region).to receive(:authentication_token).and_return(File.read(Rails.root.join("certs/v2_key"))) token = region.api_system_auth_token(user) token_hash = YAML.load(MiqPassword.decrypt(token)) @@ -234,30 +168,6 @@ end end - describe "#required_credential_fields" do - it "checks the right credential fields" do - expect(region.required_credential_fields(:system_api)).to eq([:auth_key]) - end - end - - describe "#encrypt" do - let(:region) { FactoryGirl.create(:miq_region, :region => ApplicationRecord.my_region_number) } - - it "correctly encrypts a string using the authentication token" do - expect(region).to receive(:authentication_token).and_return(File.read(Rails.root.join("certs/v2_key"))) - - a_string = "this should be encrypted correctly" - enc = region.encrypt(a_string) - other_string = MiqPassword.decrypt(enc) - - expect(other_string).to eq(a_string) - end - - it "raises if no key is configured" do - expect { region.encrypt("a string") }.to raise_error(RuntimeError) - end - end - context "ConfigurationManagementMixin" do describe "#settings_for_resource" do it "returns the resource's settings" do diff --git a/spec/models/mixins/inter_region_api_method_relay_spec.rb b/spec/models/mixins/inter_region_api_method_relay_spec.rb index 30168d9f95c..fb7bdca0778 100644 --- a/spec/models/mixins/inter_region_api_method_relay_spec.rb +++ b/spec/models/mixins/inter_region_api_method_relay_spec.rb @@ -176,41 +176,28 @@ def expect_api_call(expected_action, expected_args = nil) expect(MiqRegion).to receive(:find_by).with(:region => region_number).and_return(region) end - context "with authentication configured" do - before do - expect(region).to receive(:auth_key_configured?).and_return true - end - - it "opens an api connection to that address when the server has an ip address" do - require "manageiq-api-client" - - server.ipaddress = "192.0.2.1" - server.save! - - expect(User).to receive(:current_userid).and_return(request_user) - expect(region).to receive(:api_system_auth_token).with(request_user).and_return(region_auth_token) - - client_connection_hash = { - :url => "https://#{server.ipaddress}", - :miqtoken => region_auth_token, - :ssl => {:verify => false} - } - expect(ManageIQ::API::Client).to receive(:new).with(client_connection_hash).and_return(api_connection) - described_class.api_client_connection_for_region(region_number) - end - - it "raises if the server doesn't have an ip address" do - expect { - described_class.api_client_connection_for_region(region_number) - }.to raise_error("Failed to establish API connection to region #{region_number}") - end + it "opens an api connection to that address when the server has an ip address" do + require "manageiq-api-client" + + server.ipaddress = "192.0.2.1" + server.save! + + expect(User).to receive(:current_userid).and_return(request_user) + expect(region).to receive(:api_system_auth_token).with(request_user).and_return(region_auth_token) + + client_connection_hash = { + :url => "https://#{server.ipaddress}", + :miqtoken => region_auth_token, + :ssl => {:verify => false} + } + expect(ManageIQ::API::Client).to receive(:new).with(client_connection_hash).and_return(api_connection) + described_class.api_client_connection_for_region(region_number) end - it "raises without authentication configured" do - expect(region).to receive(:auth_key_configured?).and_return false + it "raises if the server doesn't have an ip address" do expect { described_class.api_client_connection_for_region(region_number) - }.to raise_error("Region #{region_number} is not configured for central administration") + }.to raise_error("Failed to establish API connection to region #{region_number}") end end diff --git a/spec/models/pglogical_subscription_spec.rb b/spec/models/pglogical_subscription_spec.rb index d865c34dffc..a92a1fd94ce 100644 --- a/spec/models/pglogical_subscription_spec.rb +++ b/spec/models/pglogical_subscription_spec.rb @@ -370,23 +370,6 @@ def with_an_invalid_remote_schema sub.delete end - - it "removes the region authentication key if present" do - allow(pglogical).to receive(:subscriptions).and_return(subscriptions, [subscriptions.last]) - expect(pglogical).to receive(:subscription_drop).with("region_#{remote_region1}_subscription", true) - - EvmSpecHelper.create_guid_miq_server_zone - auth = FactoryGirl.create( - :auth_token, - :resource_id => remote_region.id, - :resource_type => "MiqRegion", - :auth_key => "this is the encryption key!", - :authtype => "system_api" - ) - - sub.delete - expect(AuthToken.find_by_id(auth.id)).to be_nil - end end describe "#validate" do From 37f854ef441cf6109418d2f870d6eb2375c82602 Mon Sep 17 00:00:00 2001 From: Nick Carboni Date: Fri, 17 Feb 2017 17:29:12 -0500 Subject: [PATCH 2/2] Add a data migration to remove previously saved region auth records The AuthenticationMixin was added to MiqRegion specifically for saving these records. Because of this we can just remove all the authentication records that point to an MiqRegion resource. --- ...remove_central_admin_region_auth_records.rb | 7 +++++++ ...e_central_admin_region_auth_records_spec.rb | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 db/migrate/20170217220618_remove_central_admin_region_auth_records.rb create mode 100644 spec/migrations/20170217220618_remove_central_admin_region_auth_records_spec.rb diff --git a/db/migrate/20170217220618_remove_central_admin_region_auth_records.rb b/db/migrate/20170217220618_remove_central_admin_region_auth_records.rb new file mode 100644 index 00000000000..34a2b5fba6b --- /dev/null +++ b/db/migrate/20170217220618_remove_central_admin_region_auth_records.rb @@ -0,0 +1,7 @@ +class RemoveCentralAdminRegionAuthRecords < ActiveRecord::Migration[5.0] + class Authentication < ActiveRecord::Base; end + + def up + Authentication.where(:resource_type => 'MiqRegion').delete_all + end +end diff --git a/spec/migrations/20170217220618_remove_central_admin_region_auth_records_spec.rb b/spec/migrations/20170217220618_remove_central_admin_region_auth_records_spec.rb new file mode 100644 index 00000000000..a676afd845d --- /dev/null +++ b/spec/migrations/20170217220618_remove_central_admin_region_auth_records_spec.rb @@ -0,0 +1,18 @@ +require_migration + +describe RemoveCentralAdminRegionAuthRecords do + let(:authentication_stub) { migration_stub(:Authentication) } + + migration_context :up do + it "removes rows that point to MiqRegion" do + authentication_stub.create!(:resource_type => "MiqRegion") + authentication_stub.create!(:resource_type => "MiqRegion") + authentication_stub.create!(:resource_type => "Provider") + + migrate + + expect(authentication_stub.count).to eq(1) + expect(authentication_stub.first.resource_type).to eq("Provider") + end + end +end