From 8dc2b93ca2719c9adbd7bcf80be031ad972feb15 Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Mon, 8 Sep 2025 19:29:15 -0700 Subject: [PATCH 1/5] More secure default server setting Under some fairly obscure conditions, defaulting the server setting to 'puppet' can be a security concern. Because the worst case scenario involves a user accidentally running `puppet agent -t` on an untrusted network, this PR removes the default completely when non-root. Otherwise, it just prints a deprecation warning. Fixes https://github.com/voxpupuli/security-tracking/issues/22 --- lib/puppet/defaults.rb | 15 +++++++++++++-- spec/unit/defaults_spec.rb | 16 +++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb index 87a8b10c50..fc17bcd355 100644 --- a/lib/puppet/defaults.rb +++ b/lib/puppet/defaults.rb @@ -1650,8 +1650,19 @@ def self.initialize_default_settings!(settings) :desc => "The root directory of devices' $confdir.", }, :server => { - :default => "puppet", - :desc => "The primary Puppet server to which the Puppet agent should connect.", + :default => '', # use an empty string so dependent settings can resolve without crashing + :desc => "The primary Puppet server to which the Puppet agent should connect.", + :call_hook => :on_initialize_and_write, + :hook => proc do |value| + if value.empty? + if Puppet.features.root? + Puppet.deprecation_warning('OpenVox will not default to `server=puppet` as of version 9.0. Please update your configuration appropriately.') + Puppet.settings[:server] = 'puppet' + else + Puppet.deprecation_warning('"server" must be specified when running as a non-privileged user. (Did you mean to run as root?)') + end + end + end }, :server_list => { :default => [], diff --git a/spec/unit/defaults_spec.rb b/spec/unit/defaults_spec.rb index 9f322b38ec..e7375ea634 100644 --- a/spec/unit/defaults_spec.rb +++ b/spec/unit/defaults_spec.rb @@ -8,6 +8,20 @@ end end + describe 'server' do + it 'should default to `puppet` when root' do + allow(Puppet.features).to receive(:root?).and_return(true) + Puppet.initialize_settings + expect(Puppet.settings[:server]).to eq('puppet') + end + + it 'should default to empty value when non-root' do + expect(Puppet).to receive(:deprecation_warning) + Puppet.initialize_settings + expect(Puppet.settings[:server]).to eq('') + end + end + describe 'strict' do it 'should accept the valid value :off' do expect {Puppet.settings[:strict] = 'off'}.to_not raise_exception @@ -146,7 +160,7 @@ describe "deprecated settings" do it 'does not issue a deprecation warning by default' do - expect(Puppet).to receive(:deprecation_warning).never + expect(Puppet).to receive(:deprecation_warning).with(/"server" must be specified when running as a non-privileged user/) Puppet.initialize_settings end From 1b6e9a429803c45b98e4b6f2efb78227c2fbf64a Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Mon, 15 Sep 2025 19:56:16 -0700 Subject: [PATCH 2/5] Better way to do it This moves the check and warnings to the http service so it only screams if you actually use it. This avoids, for example, the spurious deprecation message when running `puppet config set` to set this in the first place! --- lib/puppet/defaults.rb | 13 +------------ lib/puppet/http/service.rb | 18 ++++++++++++++++++ spec/unit/defaults_spec.rb | 23 ++++++++++++++++------- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb index fc17bcd355..c77957351c 100644 --- a/lib/puppet/defaults.rb +++ b/lib/puppet/defaults.rb @@ -1650,19 +1650,8 @@ def self.initialize_default_settings!(settings) :desc => "The root directory of devices' $confdir.", }, :server => { - :default => '', # use an empty string so dependent settings can resolve without crashing + :default => Puppet.features.root? ? 'puppet': '', # use an empty string so dependent settings can resolve without crashing :desc => "The primary Puppet server to which the Puppet agent should connect.", - :call_hook => :on_initialize_and_write, - :hook => proc do |value| - if value.empty? - if Puppet.features.root? - Puppet.deprecation_warning('OpenVox will not default to `server=puppet` as of version 9.0. Please update your configuration appropriately.') - Puppet.settings[:server] = 'puppet' - else - Puppet.deprecation_warning('"server" must be specified when running as a non-privileged user. (Did you mean to run as root?)') - end - end - end }, :server_list => { :default => [], diff --git a/lib/puppet/http/service.rb b/lib/puppet/http/service.rb index e031450506..2efc1acf3f 100644 --- a/lib/puppet/http/service.rb +++ b/lib/puppet/http/service.rb @@ -32,6 +32,24 @@ class Puppet::HTTP::Service # # @api private def self.create_service(client, session, name, server = nil, port = nil) + # this is the entry point for creating all services, check and issue warning here. + unless Puppet.settings.set_by_config? :server + if Puppet.features.root? + Puppet.deprecation_warning('OpenVox will not default to `server=puppet` as of version 9.0. Please update your configuration appropriately.') + else + Puppet.deprecation_warning('OpenVox no longer defaults to `server=puppet` when running as a non-privileged user. (Did you mean to run as root?)') + + case name + when :ca + raise ArgumentError, 'Neither `server` nor `ca_server` is specified.' unless Puppet.settings.set_by_config? :ca_server + when :report + raise ArgumentError, 'Neither `server` nor `report_server` is specified.' unless Puppet.settings.set_by_config? :report_server + when :fileserver, :puppet, :puppetserver + raise ArgumentError, 'Required setting `server` is not specified.' + end + end + end + case name when :ca Puppet::HTTP::Service::Ca.new(client, session, server, port) diff --git a/spec/unit/defaults_spec.rb b/spec/unit/defaults_spec.rb index e7375ea634..1c320f79fc 100644 --- a/spec/unit/defaults_spec.rb +++ b/spec/unit/defaults_spec.rb @@ -11,14 +11,25 @@ describe 'server' do it 'should default to `puppet` when root' do allow(Puppet.features).to receive(:root?).and_return(true) - Puppet.initialize_settings - expect(Puppet.settings[:server]).to eq('puppet') + foo = Puppet::Settings.new + Puppet.initialize_default_settings!(foo) + expect(foo[:server]).to eq('puppet') end it 'should default to empty value when non-root' do - expect(Puppet).to receive(:deprecation_warning) - Puppet.initialize_settings - expect(Puppet.settings[:server]).to eq('') + allow(Puppet.features).to receive(:root?).and_return(false) + foo = Puppet::Settings.new + Puppet.initialize_default_settings!(foo) + expect(foo[:server]).to eq('') + end + + it 'should fail when trying to establish a compiler connection without setting `server`' do + allow(Puppet.features).to receive(:root?).and_return(false) + expect { + client = Puppet.runtime[:http] + session = client.create_session + service = session.route_to(:puppet) + }.to raise_exception ArgumentError, /Required setting/ end end @@ -160,8 +171,6 @@ describe "deprecated settings" do it 'does not issue a deprecation warning by default' do - expect(Puppet).to receive(:deprecation_warning).with(/"server" must be specified when running as a non-privileged user/) - Puppet.initialize_settings end end From 1209fb976261d831c6e900747835f5f88c4c6979 Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Mon, 15 Sep 2025 20:02:51 -0700 Subject: [PATCH 3/5] revert some unintentional changes --- lib/puppet/defaults.rb | 4 ++-- spec/unit/defaults_spec.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb index c77957351c..445697f835 100644 --- a/lib/puppet/defaults.rb +++ b/lib/puppet/defaults.rb @@ -1650,8 +1650,8 @@ def self.initialize_default_settings!(settings) :desc => "The root directory of devices' $confdir.", }, :server => { - :default => Puppet.features.root? ? 'puppet': '', # use an empty string so dependent settings can resolve without crashing - :desc => "The primary Puppet server to which the Puppet agent should connect.", + :default => Puppet.features.root? ? 'puppet': '', # use an empty string so dependent settings can resolve without crashing + :desc => "The primary Puppet server to which the Puppet agent should connect.", }, :server_list => { :default => [], diff --git a/spec/unit/defaults_spec.rb b/spec/unit/defaults_spec.rb index 1c320f79fc..3b82f6f9fc 100644 --- a/spec/unit/defaults_spec.rb +++ b/spec/unit/defaults_spec.rb @@ -172,6 +172,7 @@ describe "deprecated settings" do it 'does not issue a deprecation warning by default' do Puppet.initialize_settings + expect(Puppet).to receive(:deprecation_warning).never end end From ce46f5f634054cbc672ba9d85a23dd4ca12bea57 Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Mon, 15 Sep 2025 20:05:32 -0700 Subject: [PATCH 4/5] revert some unintentional changes --- spec/unit/defaults_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/unit/defaults_spec.rb b/spec/unit/defaults_spec.rb index 3b82f6f9fc..5a1fd6a644 100644 --- a/spec/unit/defaults_spec.rb +++ b/spec/unit/defaults_spec.rb @@ -171,8 +171,9 @@ describe "deprecated settings" do it 'does not issue a deprecation warning by default' do - Puppet.initialize_settings expect(Puppet).to receive(:deprecation_warning).never + + Puppet.initialize_settings end end From 9c47f711ebdc1dd643a8ba121ac5977c57b9a44f Mon Sep 17 00:00:00 2001 From: Ben Ford Date: Mon, 15 Sep 2025 20:11:10 -0700 Subject: [PATCH 5/5] don't scream at me rubocop --- lib/puppet/defaults.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb index 445697f835..4296506f18 100644 --- a/lib/puppet/defaults.rb +++ b/lib/puppet/defaults.rb @@ -1650,7 +1650,7 @@ def self.initialize_default_settings!(settings) :desc => "The root directory of devices' $confdir.", }, :server => { - :default => Puppet.features.root? ? 'puppet': '', # use an empty string so dependent settings can resolve without crashing + :default => Puppet.features.root? ? 'puppet' : '', # use an empty string so dependent settings can resolve without crashing :desc => "The primary Puppet server to which the Puppet agent should connect.", }, :server_list => {