Fixes #39199 - Prevent spurious calls to the proxy related to pubkey/ca pubkey refresh (#1026)

* Fixes #39199 - Prevent spurious calls to proxy when rendering api responses

The previous "return stored value or fetch a fresh one if missing"
approach used for pubkey and ca_pubkey could cause calls to the proxy in
unexpected situations. This could lead to problems like not being able
to list smart proxies if the proxy which didn't have one of the pubkey
fields filled was down.

This change more strictly separates when values are only read and when
values can be updated as part of reading.

* Refs #39199 - Prevent spurious proxy calls when accessing host parameters

Author: adamruzicka

app/models/concerns/foreman_remote_execution/host_extensions.rb

@@ -109,11 +109,11 @@ def remote_execution_proxies(provider, authorized = true)
 
     def remote_execution_ssh_keys
       # only include public keys from SSH proxies that don't have SSH cert verification configured
-      remote_execution_proxies(%w(SSH Script), false).values.flatten.uniq.map { |proxy| proxy.pubkey if proxy.ca_pubkey.blank? }.compact.uniq
+      remote_execution_proxies(%w(SSH Script), false).values.flatten.uniq.map { |proxy| proxy.pubkey(refresh: false) if proxy.ca_pubkey(refresh: false).blank? }.compact.uniq
     end
 
     def remote_execution_ssh_ca_keys
-      remote_execution_proxies(%w(SSH Script), false).values.flatten.uniq.map { |proxy| proxy.ca_pubkey }.compact.uniq
+      remote_execution_proxies(%w(SSH Script), false).values.flatten.uniq.map { |proxy| proxy.ca_pubkey(refresh: false) }.compact.uniq
     end
 
     def drop_execution_interface_cache

app/models/concerns/foreman_remote_execution/smart_proxy_extensions.rb

@@ -7,12 +7,12 @@ def self.prepended(base)
       end
     end
 
-    def pubkey
-      self[:pubkey] || update_pubkey
+    def pubkey(refresh: true)
+      self[:pubkey] || (refresh && update_pubkey || nil)
     end
 
-    def ca_pubkey
-      self[:ca_pubkey] || update_ca_pubkey
+    def ca_pubkey(refresh: true)
+      self[:ca_pubkey] || (refresh && update_ca_pubkey || nil)
     end
 
     def update_pubkey

app/views/api/v2/smart_proxies/ca_pubkey.json.rabl

@@ -1 +1 @@
-attribute :ca_pubkey => :remote_execution_ca_pubkey
+node(:remote_execution_ca_pubkey) { |p| p.ca_pubkey(refresh: false) }

app/views/api/v2/smart_proxies/pubkey.json.rabl

@@ -1 +1 @@
-attribute :pubkey => :remote_execution_pubkey
+node(:remote_execution_pubkey) { |p| p.pubkey(refresh: false) }

test/unit/concerns/host_extensions_test.rb

@@ -11,8 +11,9 @@ class ForemanRemoteExecutionHostExtensionsTest < ActiveSupport::TestCase
     let(:sshkey) { 'ssh-rsa AAAAB3NzaC1yc2EAAAABJQ foo@example.com' }
 
     before do
-      SmartProxy.any_instance.stubs(:pubkey).returns(sshkey)
-      SmartProxy.any_instance.stubs(:ca_pubkey).returns(nil)
+      host.subnet.remote_execution_proxies.each do |proxy|
+        proxy.update(pubkey: sshkey, ca_pubkey: nil)
+      end
       Setting[:remote_execution_ssh_user] = 'root'
       Setting[:remote_execution_effective_user_method] = 'sudo'
     end
@@ -60,6 +61,17 @@ class ForemanRemoteExecutionHostExtensionsTest < ActiveSupport::TestCase
       User.current = nil
       assert_includes host.remote_execution_ssh_keys, sshkey
     end
+
+    it 'triggers no calls to the proxy' do
+      host.subnet.remote_execution_proxies.each do |proxy|
+        proxy.update(pubkey: nil)
+      end
+
+      SmartProxy.any_instance.expects(:update_pubkey).never
+      SmartProxy.any_instance.expects(:update_ca_pubkey).never
+
+      host.host_param('remote_execution_ssh_keys')
+    end
   end
 
   describe 'has ssh CA key configured' do
@@ -68,8 +80,9 @@ class ForemanRemoteExecutionHostExtensionsTest < ActiveSupport::TestCase
     let(:ca_sshkey) { 'ssh-rsa AAAAB3NzaC1yc2EAAAABJE bar@example.com' }
 
     before do
-      SmartProxy.any_instance.stubs(:pubkey).returns(sshkey)
-      SmartProxy.any_instance.stubs(:ca_pubkey).returns(ca_sshkey)
+      host.subnet.remote_execution_proxies.each do |proxy|
+        proxy.update(pubkey: sshkey, ca_pubkey: ca_sshkey)
+      end
       Setting[:remote_execution_ssh_user] = 'root'
       Setting[:remote_execution_effective_user_method] = 'sudo'
     end
@@ -102,6 +115,17 @@ class ForemanRemoteExecutionHostExtensionsTest < ActiveSupport::TestCase
       assert_includes host.host_param('remote_execution_ssh_ca_keys'), key
       assert_includes host.host_param('remote_execution_ssh_ca_keys'), ca_sshkey
     end
+
+    it 'triggers no calls to the proxy' do
+      host.subnet.remote_execution_proxies.each do |proxy|
+        proxy.update(ca_pubkey: nil)
+      end
+
+      SmartProxy.any_instance.expects(:update_pubkey).never
+      SmartProxy.any_instance.expects(:update_ca_pubkey).never
+
+      host.host_param('remote_execution_ssh_ca_keys')
+    end
   end
 
   context 'host has multiple nics' do

test/unit/concerns/smart_proxy_extensions_test.rb

@@ -0,0 +1,83 @@
+require 'test_plugin_helper'
+
+class ForemanRemoteExecutionSmartProxyExtensionsTest < ActiveSupport::TestCase
+  let(:proxy) { FactoryBot.create(:smart_proxy, :ssh) }
+
+  describe '#pubkey' do
+    context 'when key is cached in the database' do
+      it 'returns the cached key without fetching from proxy' do
+        proxy.expects(:update_pubkey).never
+        assert_equal 'ssh-rsa AAAAB3N...', proxy.pubkey
+        assert_equal 'ssh-rsa AAAAB3N...', proxy.pubkey(refresh: false)
+      end
+    end
+
+    context 'when key is not cached' do
+      before { proxy.update(pubkey: nil) }
+
+      it 'fetches from proxy by default' do
+        proxy.expects(:update_pubkey).returns('ssh-rsa FETCHED...')
+        assert_equal 'ssh-rsa FETCHED...', proxy.pubkey
+      end
+
+      it 'returns nil without fetching when refresh: false' do
+        proxy.expects(:update_pubkey).never
+        assert_nil proxy.pubkey(refresh: false)
+      end
+    end
+  end
+
+  describe '#ca_pubkey' do
+    context 'when key is cached in the database' do
+      before { proxy.update(ca_pubkey: 'ssh-rsa CA_KEY...') }
+
+      it 'returns the cached key without fetching from proxy' do
+        proxy.expects(:update_ca_pubkey).never
+        assert_equal 'ssh-rsa CA_KEY...', proxy.ca_pubkey
+        assert_equal 'ssh-rsa CA_KEY...', proxy.ca_pubkey(refresh: false)
+      end
+    end
+
+    context 'when key is not cached' do
+      before { proxy.update(ca_pubkey: nil) }
+
+      it 'fetches from proxy by default' do
+        proxy.expects(:update_ca_pubkey).returns('ssh-rsa FETCHED_CA...')
+        assert_equal 'ssh-rsa FETCHED_CA...', proxy.ca_pubkey
+      end
+
+      it 'returns nil without fetching when refresh: false' do
+        proxy.expects(:update_ca_pubkey).never
+        assert_nil proxy.ca_pubkey(refresh: false)
+      end
+    end
+  end
+
+  describe '#refresh' do
+    context 'when the key is cached' do
+      before do
+        proxy.update(pubkey: 'ssh-rsa KEY...', ca_pubkey: 'ssh-rsa CA_KEY...')
+      end
+
+      it 'fetches the keys' do
+        proxy.expects(:update_pubkey).once
+        proxy.expects(:update_ca_pubkey).once
+
+        proxy.refresh
+      end
+    end
+
+    context 'when the key is not cached' do
+      before do
+        proxy.update(pubkey: nil, ca_pubkey: nil)
+      end
+
+      it 'fetches the keys' do
+        proxy.expects(:update_pubkey).once
+        proxy.expects(:update_ca_pubkey).once
+
+        proxy.refresh
+      end
+    end
+  end
+end