Fixes #29974 - allow subnet helpers #7713

lzap · 2020-06-01T13:06:30Z

SSIA

theforeman-bot · 2020-06-01T13:06:41Z

ekohl · 2020-06-01T13:19:45Z

app/models/smart_proxy.rb

@@ -172,6 +172,6 @@ def get_features
  end

  class Jail < ::Safemode::Jail
-    allow :id, :name
+    allow :id, :name, :hostname, :hosts_count


Can it be a problem that Katello also defines the hostname?
https://github.com/Katello/katello/blob/a9abccc2c10f5f2c1194d564834d131d57b28401/app/models/katello/concerns/smart_proxy_extensions.rb#L336-L338

Either way, after merging this that can be removed.

why do we need the hosts_count?

I will do a followup PR into Katello.

That hosts_count was added by accident, not needed.

Done: Katello/katello#8748

timogoebel · 2020-06-02T12:38:39Z

app/models/smart_proxy.rb

@@ -172,6 +172,6 @@ def get_features
  end

  class Jail < ::Safemode::Jail
-    allow :id, :name
+    allow :id, :name, :hostname, :hosts_count, :setting


Does setting allow to retrieve any secret settings?

It allows retrieving settings of a smart proxy feature. I don't think those contain any secrets

Should we perhaps expose those explicitly? Features are just strings and exposed on the individual proxies without any authentication so that's IMHO safe. If you're talking about exposed settings from a proxy then those can contain private information and they're only available via an authenticated endpoint (though I'm not aware of any secrets being passed now).

From my testing machine:

SmartProxyFeature.all.each { |f| puts "#{f.feature.name}: #{f.settings}" } Pulp: {"pulp_url"=>"https://centos7-katello-nightly-stable.example.com/pulp"} Pulpcore: {"pulp_url"=>"https://centos7-katello-nightly-stable.example.com", "mirror"=>false, "content_app_url"=>"https://centos7-katello-nightly-stable.example.com/pulp/content"} TFTP: {"tftp_servername"=>nil} Puppet CA: {"puppet_url"=>"https://centos7-katello-nightly-stable.example.com:8140", "use_provider"=>["puppetca_hostname_whitelisting", "puppetca_http_api"]} Puppet: {"puppet_url"=>"https://centos7-katello-nightly-stable.example.com:8140", "use_provider"=>["puppet_proxy_puppet_api"]} Logs: {} Templates: {} HTTPBoot: {"http_port"=>8000, "https_port"=>9090}

it looks like those don't contain any secrets, but that is no guarrantee that they never will

https://github.com/theforeman/smart_proxy_pulp/blob/55ff29f4cfab971f71bc00273c7623ee8dc5af96/lib/smart_proxy_pulp_plugin/pulpcore_plugin.rb#L17-L18 does expose a username/password. It's mostly used in the development workflow if you don't have certificates set up so you won't see it in production, but exposing this is a security risk.

Another thing to consider is that these settings are IMHO internal implementation details. You should always wrap them in high level logic. For example, the puppet server url has a one that also provides compatibility with older releases which didn't expose this:

foreman/app/models/concerns/host_common.rb

Lines 95 to 100 in c356041

def puppet_server_uri

return unless puppet_proxy

url = puppet_proxy.setting('Puppet', 'puppet_url')

url ||= "https://#{puppet_proxy}:8140"

URI(url)

end

That particular helper should be exposed rather than raw settings.

👎 on exposing setting.

Ok how about that, I have implemented whitelisting.

adamruzicka · 2020-06-03T12:56:38Z

app/models/smart_proxy.rb

+    "https_port",
+  ]
+  def safe_setting(feature, setting)
+    raise ::Foreman::Exception.new(N_("Setting is not whitelisted: %s"), ALLOWED_SETTINGS) unless ALLOWED_SETTINGS.include?(setting)


This shows what settings are allowed, but not which setting was actually requested

::Foreman::Exception.new(N_("Setting is not whitelisted: %s"), ALLOWED_SETTINGS).message => "ERF42-4295 [Foreman::Exception]: Setting is not whitelisted: [\"http_port\", \"https_port\"]"

Fixing and adding tests for this.

lzap · 2020-06-03T13:07:14Z

Rebased, can I get a quick blessing for this @tbrisker and @ekohl ? This patch is actually needed to have bootdisk working with EFI HTTP BOOT. Added tests so we are safe.

ekohl

👎 because I still think explicit wrappers are better. For example, this doesn't look at the feature at all. You will also end up with duplicated logic.

foreman/app/models/operatingsystem.rb

Lines 229 to 240 in c356041

    
           if host.subnet&.httpboot? 
        
             if host.pxe_loader =~ /UEFI HTTPS/ 
        
               port = host.subnet.httpboot.setting(:HTTPBoot, 'https_port') || raise(::Foreman::Exception.new(N_("HTTPS boot requires proxy with httpboot feature and https_port exposed setting"))) 
        
             else 
        
               port = host.subnet.httpboot.setting(:HTTPBoot, 'http_port') || raise(::Foreman::Exception.new(N_("HTTP boot requires proxy with httpboot feature and http_port exposed setting"))) 
        
             end 
        
             hostname = URI.parse(host.subnet.httpboot.url).hostname 
        
             self.class.all_loaders_map(architecture, "#{hostname}:#{port}")[host.pxe_loader] 
        
           else 
        
             raise(::Foreman::Exception.new(N_("HTTP UEFI boot requires proxy with httpboot feature"))) if host.pxe_loader =~ /UEFI HTTP/ 
        
             self.class.all_loaders_map(architecture)[host.pxe_loader] 
        
           end

That logic block contains a lot of the same things. We've already seen multiple times that users couldn't even manage to debug that error message. If we expose raw settings it'll be even harder. The wrapper can raise a proper error.

lzap · 2020-06-04T06:45:48Z

Okay, I've extracted this into two methods. How about now?

lzap · 2020-06-04T07:56:49Z

Tests are passing now @ekohl

adamruzicka

It exposes all the settings we need in a safe manner without exposing things we don't need. 👍 from me

ekohl

Thanks! @tbrisker any last thoughts?

timogoebel · 2020-06-04T09:48:29Z

app/models/smart_proxy.rb

@@ -100,6 +100,14 @@ def setting(feature, setting)
    smart_proxy_features.find_by(:feature_id => Feature.find_by(:name => feature)).try(:settings).try(:[], setting)
  end

+  def httpboot_http_port
+    setting(:HTTPBoot, 'http_port') || raise(::Foreman::Exception.new(N_("HTTP boot requires proxy with httpboot feature and http_port exposed setting")))


Do we need to check if the proxy has the appropriate feature?

IIRC the data model is that the settings are stored in the Feature model. So having a setting implies the feature. It may be useful to check it separate when you want to raise more explicit exceptions. I can imagine a FeatureMissingException or something. However, that's not a requirement for this PR.

The code actually does this. Method settings uses try. If you scroll to the right, you will see the error saying either feature is missing or port is not exposed.

adamruzicka · 2020-06-05T06:09:23Z

Katello failures seem unrelated, merging

adamruzicka · 2020-06-05T06:10:02Z

Thank you @lzap!

(cherry picked from commit 89c1e28)

lzap · 2020-06-05T07:52:25Z

CP 2.1: Fixes #29974 - allow subnet helpers (CP 2.1) #7722

(cherry picked from commit 89c1e28)

theforeman-bot added Needs testing Not yet reviewed labels Jun 1, 2020

lzap mentioned this pull request Jun 1, 2020

Fixes #4413 - UEFI support for full and generic theforeman/foreman_bootdisk#93

Merged

ekohl reviewed Jun 1, 2020

View reviewed changes

lzap force-pushed the allow-bootdisk-efi-macros-29974 branch from f436b8d to e30c1ce Compare June 2, 2020 09:03

timogoebel reviewed Jun 2, 2020

View reviewed changes

tbrisker assigned adamruzicka Jun 3, 2020

lzap force-pushed the allow-bootdisk-efi-macros-29974 branch from e30c1ce to 1194a15 Compare June 3, 2020 12:52

adamruzicka reviewed Jun 3, 2020

View reviewed changes

lzap force-pushed the allow-bootdisk-efi-macros-29974 branch from 1194a15 to 5361a33 Compare June 3, 2020 13:07

ekohl requested changes Jun 3, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jun 3, 2020

Fixes #29974 - allow subnet helpers

f74beb3

lzap force-pushed the allow-bootdisk-efi-macros-29974 branch from 5361a33 to f74beb3 Compare June 4, 2020 06:43

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jun 4, 2020

lzap mentioned this pull request Jun 4, 2020

Fixes #30010 - removed proxy.hostname from safemode Katello/katello#8748

Merged

adamruzicka approved these changes Jun 4, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 4, 2020

ekohl approved these changes Jun 4, 2020

View reviewed changes

timogoebel reviewed Jun 4, 2020

View reviewed changes

adamruzicka merged commit 89c1e28 into theforeman:develop Jun 5, 2020

lzap deleted the allow-bootdisk-efi-macros-29974 branch June 5, 2020 07:49

lzap added a commit to lzap/foreman that referenced this pull request Jun 5, 2020

Fixes #29974 - allow subnet helpers (theforeman#7713)

e8b79e0

(cherry picked from commit 89c1e28)

tbrisker pushed a commit that referenced this pull request Jun 7, 2020

Fixes #29974 - allow subnet helpers (#7713)

6c50021

(cherry picked from commit 89c1e28)

MariaAga pushed a commit to MariaAga/foreman that referenced this pull request Jun 17, 2020

Fixes #29974 - allow subnet helpers (theforeman#7713)

9c71362

anandkumaragrawal mentioned this pull request Sep 10, 2020

updating my fork (#1) #7975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #29974 - allow subnet helpers #7713

Fixes #29974 - allow subnet helpers #7713

lzap commented Jun 1, 2020

theforeman-bot commented Jun 1, 2020

ekohl Jun 1, 2020

tbrisker Jun 1, 2020

lzap Jun 3, 2020

lzap Jun 4, 2020

timogoebel Jun 2, 2020

adamruzicka Jun 3, 2020

ekohl Jun 3, 2020

adamruzicka Jun 3, 2020 •

edited

Loading

ekohl Jun 3, 2020

lzap Jun 3, 2020

adamruzicka Jun 3, 2020 •

edited

Loading

lzap Jun 3, 2020

lzap commented Jun 3, 2020

ekohl left a comment

lzap commented Jun 4, 2020

lzap commented Jun 4, 2020

adamruzicka left a comment

ekohl left a comment

timogoebel Jun 4, 2020

ekohl Jun 4, 2020

lzap Jun 4, 2020 •

edited

Loading

adamruzicka commented Jun 5, 2020

adamruzicka commented Jun 5, 2020

lzap commented Jun 5, 2020

	def puppet_server_uri
	return unless puppet_proxy
	url = puppet_proxy.setting('Puppet', 'puppet_url')
	url \|\|= "https://#{puppet_proxy}:8140"
	URI(url)
	end

	if host.subnet&.httpboot?
	if host.pxe_loader =~ /UEFI HTTPS/
	port = host.subnet.httpboot.setting(:HTTPBoot, 'https_port') \|\| raise(::Foreman::Exception.new(N_("HTTPS boot requires proxy with httpboot feature and https_port exposed setting")))
	else
	port = host.subnet.httpboot.setting(:HTTPBoot, 'http_port') \|\| raise(::Foreman::Exception.new(N_("HTTP boot requires proxy with httpboot feature and http_port exposed setting")))
	end
	hostname = URI.parse(host.subnet.httpboot.url).hostname
	self.class.all_loaders_map(architecture, "#{hostname}:#{port}")[host.pxe_loader]
	else
	raise(::Foreman::Exception.new(N_("HTTP UEFI boot requires proxy with httpboot feature"))) if host.pxe_loader =~ /UEFI HTTP/
	self.class.all_loaders_map(architecture)[host.pxe_loader]
	end

Fixes #29974 - allow subnet helpers #7713

Fixes #29974 - allow subnet helpers #7713

Conversation

lzap commented Jun 1, 2020

theforeman-bot commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamruzicka Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Jun 3, 2020

ekohl left a comment

Choose a reason for hiding this comment

lzap commented Jun 4, 2020

lzap commented Jun 4, 2020

adamruzicka left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

adamruzicka commented Jun 5, 2020

adamruzicka commented Jun 5, 2020

lzap commented Jun 5, 2020

adamruzicka Jun 3, 2020 •

edited

Loading

adamruzicka Jun 3, 2020 •

edited

Loading

lzap Jun 4, 2020 •

edited

Loading