Skip to content

Commit

Permalink
(MODULES-10996) Improve GPG key import check
Browse files Browse the repository at this point in the history
The puppet GPG signing key with ID 4528b6cd9e61ef26 had a subkey in it
until February 2021. This caused GPG checks on systems with RPM versions
that do not support subkeys[1] (SLES 11 and EL 5) to fail.

We added this GPG key in the puppet_agent module in January, and
included it in the 4.4.0 release of the module.

We discovered the subkey issue in February and promptly removed the
subkey from the existing key. The new key is available since version
4.5.0 of the puppet_agent module.

This module imports GPG keys based on their ID. Since in our case both
the good key and the bad key have the same ID, the module will not
import the correct key if the bad one is already installed (or any other
key with the same ID for that matter).

To circumvent this, we now specifically compare the contents of the GPG
key from the RPM database with the contents of the GPG key laid by
Puppet in `/etc/pki/rpm-gpg`. If any differences are found, the imported
key is purged and reimported, which should ensure that the key shipped
in the module is identical to the from the RPM database.

[1] https://technosorcery.net/blog/2010/10/pitfalls-with-rpm-and-gpg/
  • Loading branch information
GabrielNagy committed Apr 20, 2021
1 parent 64e6530 commit c793fd0
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 86 deletions.
26 changes: 26 additions & 0 deletions files/rpm_gpg_import_check.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#!/bin/bash
# shellcheck disable=SC2086

ACTION=$1
GPG_HOMEDIR=$2
GPG_KEY_PATH=$3

GPG_ARGS="--homedir $GPG_HOMEDIR --with-colons"
GPG_BIN=$(command -v gpg || command -v gpg2)

if [ -z "${GPG_BIN}" ]; then
echo Could not find a suitable gpg command, exiting...
exit 1
fi

GPG_PUBKEY=gpg-pubkey-$("${GPG_BIN}" ${GPG_ARGS} "${GPG_KEY_PATH}" 2>&1 | grep ^pub | cut -d':' -f5 | cut --characters=9-16 | tr '[:upper:]' '[:lower:]')

if [ "${ACTION}" = "check" ]; then
# This will return 1 if there are differences between the key imported in the
# RPM database and the local keyfile. This means we need to purge the key and
# reimport it.
diff <(rpm -qi "${GPG_PUBKEY}" | "${GPG_BIN}" ${GPG_ARGS}) <("${GPG_BIN}" ${GPG_ARGS} "${GPG_KEY_PATH}")
elif [ "${ACTION}" = "import" ]; then
(rpm -q "${GPG_PUBKEY}" && rpm -e --allmatches "${GPG_PUBKEY}") || true
rpm --import "${GPG_KEY_PATH}"
fi
39 changes: 16 additions & 23 deletions manifests/osfamily/redhat.pp
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,11 @@
$_sslclientkey_path = undef
}

# Fedora doesn't ship with a gpg binary, only gpg2
if $::operatingsystem == 'Fedora' {
$gpg_cmd = 'gpg2'
} else {
$gpg_cmd = 'gpg'
}

$legacy_keyname = 'GPG-KEY-puppet'
$legacy_gpg_path = "/etc/pki/rpm-gpg/RPM-${legacy_keyname}"
$keyname = 'GPG-KEY-puppet-20250406'
$gpg_path = "/etc/pki/rpm-gpg/RPM-${keyname}"
$gpg_homedir = '/root/.gnupg'
$gpg_keys = "file://${legacy_gpg_path}
file://${gpg_path}"

Expand All @@ -103,17 +97,6 @@
source => "puppet:///modules/puppet_agent/${legacy_keyname}",
}

# Given the path to a key, see if it is imported, if not, import it
$legacy_gpg_pubkey = "gpg-pubkey-$(echo $(${gpg_cmd} --with-colons ${legacy_gpg_path} 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))"

exec { "import-${legacy_keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "rpm --import ${legacy_gpg_path}",
unless => "rpm -q ${legacy_gpg_pubkey}",
require => File[$legacy_gpg_path],
logoutput => 'on_failure',
}

file { $gpg_path:
ensure => present,
owner => 0,
Expand All @@ -122,12 +105,22 @@
source => "puppet:///modules/puppet_agent/${keyname}",
}

# Given the path to a key, see if it is imported, if not, import it
$gpg_pubkey = "gpg-pubkey-$(echo $(${gpg_cmd} --with-colons ${gpg_path} 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))"
exec { "import-${keyname}":
file { "${::env_temp_variable}/rpm_gpg_import_check.sh":
ensure => file,
source => 'puppet:///modules/puppet_agent/rpm_gpg_import_check.sh',
mode => '0755',
}
-> exec { "import-${legacy_keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "${::env_temp_variable}/rpm_gpg_import_check.sh import ${gpg_homedir} ${legacy_gpg_path}",
unless => "${::env_temp_variable}/rpm_gpg_import_check.sh check ${gpg_homedir} ${legacy_gpg_path}",
require => File[$legacy_gpg_path],
logoutput => 'on_failure',
}
-> exec { "import-${keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "rpm --import ${gpg_path}",
unless => "rpm -q ${gpg_pubkey}",
command => "${::env_temp_variable}/rpm_gpg_import_check.sh import ${gpg_homedir} ${gpg_path}",
unless => "${::env_temp_variable}/rpm_gpg_import_check.sh check ${gpg_homedir} ${gpg_path}",
require => File[$gpg_path],
logoutput => 'on_failure',
}
Expand Down
22 changes: 11 additions & 11 deletions manifests/osfamily/suse.pp
Original file line number Diff line number Diff line change
Expand Up @@ -85,22 +85,22 @@
source => "puppet:///modules/puppet_agent/${legacy_keyname}",
}

# Given the path to a key, see if it is imported, if not, import it
$legacy_gpg_pubkey = "gpg-pubkey-$(echo $(gpg --homedir ${gpg_homedir} --with-colons ${legacy_gpg_path} 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))"
$gpg_pubkey = "gpg-pubkey-$(echo $(gpg --homedir ${gpg_homedir} --with-colons ${gpg_path} 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))"

exec { "import-${legacy_keyname}":
file { "${::env_temp_variable}/rpm_gpg_import_check.sh":
ensure => file,
source => 'puppet:///modules/puppet_agent/rpm_gpg_import_check.sh',
mode => '0755',
}
-> exec { "import-${legacy_keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "rpm --import ${legacy_gpg_path}",
unless => "rpm -q ${legacy_gpg_pubkey}",
command => "${::env_temp_variable}/rpm_gpg_import_check.sh import ${gpg_homedir} ${legacy_gpg_path}",
unless => "${::env_temp_variable}/rpm_gpg_import_check.sh check ${gpg_homedir} ${legacy_gpg_path}",
require => File[$legacy_gpg_path],
logoutput => 'on_failure',
}

exec { "import-${keyname}":
-> exec { "import-${keyname}":
path => '/bin:/usr/bin:/sbin:/usr/sbin',
command => "rpm --import ${gpg_path}",
unless => "rpm -q ${gpg_pubkey}",
command => "${::env_temp_variable}/rpm_gpg_import_check.sh import ${gpg_homedir} ${gpg_path}",
unless => "${::env_temp_variable}/rpm_gpg_import_check.sh check ${gpg_homedir} ${gpg_path}",
require => File[$gpg_path],
logoutput => 'on_failure',
}
Expand Down
47 changes: 15 additions & 32 deletions spec/classes/puppet_agent_osfamily_redhat_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
:architecture => 'x64',
:puppet_master_server => 'master.example.vm',
:clientcert => 'foo.example.vm',
:env_temp_variable => '/tmp',
}
end

Expand All @@ -24,39 +25,21 @@
super().merge(:operatingsystem => os, :operatingsystemmajrelease => osmajor)
end

if os == 'Fedora' then
it { is_expected.to contain_exec('import-GPG-KEY-puppet-20250406').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg2 --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406]',
'logoutput' => 'on_failure',
}) }

it { is_expected.to contain_exec('import-GPG-KEY-puppet').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg2 --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet]',
'logoutput' => 'on_failure',
}) }
else
it { is_expected.to contain_exec('import-GPG-KEY-puppet-20250406').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406]',
'logoutput' => 'on_failure',
}) }
it { is_expected.to contain_exec('import-GPG-KEY-puppet-20250406').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => '/tmp/rpm_gpg_import_check.sh import /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => '/tmp/rpm_gpg_import_check.sh check /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406]',
'logoutput' => 'on_failure',
}) }

it { is_expected.to contain_exec('import-GPG-KEY-puppet').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet]',
'logoutput' => 'on_failure',
}) }
end
it { is_expected.to contain_exec('import-GPG-KEY-puppet').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => '/tmp/rpm_gpg_import_check.sh import /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => '/tmp/rpm_gpg_import_check.sh check /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet]',
'logoutput' => 'on_failure',
}) }

context 'with manage_pki_dir => true' do
['/etc/pki', '/etc/pki/rpm-gpg'].each do |path|
Expand Down
36 changes: 16 additions & 20 deletions spec/classes/puppet_agent_osfamily_suse_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
:operatingsystemmajrelease => '12',
:architecture => 'x86_64',
:puppet_master_server => 'master.example.vm',
:env_temp_variable => '/tmp',
:clientcert => 'foo.example.vm',
}

Expand Down Expand Up @@ -50,19 +51,24 @@
}
end

it { is_expected.to contain_file('/tmp/rpm_gpg_import_check.sh').with({
'ensure' => 'file',
'source' => 'puppet:///modules/puppet_agent/rpm_gpg_import_check.sh',
'mode' => '0755',
}) }

it { is_expected.to contain_exec('import-GPG-KEY-puppet').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --homedir /root/.gnupg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'command' => '/tmp/rpm_gpg_import_check.sh import /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => '/tmp/rpm_gpg_import_check.sh check /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet]',
'logoutput' => 'on_failure',
}) }

it { is_expected.to contain_exec('import-GPG-KEY-puppet-20250406').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --homedir /root/.gnupg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'command' => '/tmp/rpm_gpg_import_check.sh import /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => '/tmp/rpm_gpg_import_check.sh check /root/.gnupg /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406]',
'logoutput' => 'on_failure',
}) }
Expand Down Expand Up @@ -196,22 +202,6 @@
})
end

it { is_expected.to contain_exec('import-GPG-KEY-puppet').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --homedir /root/.gnupg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet]',
'logoutput' => 'on_failure',
}) }

it { is_expected.to contain_exec('import-GPG-KEY-puppet-20250406').with({
'path' => '/bin:/usr/bin:/sbin:/usr/sbin',
'command' => 'rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406',
'unless' => "rpm -q gpg-pubkey-$(echo $(gpg --homedir /root/.gnupg --with-colons /etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406 2>&1 | grep ^pub | awk -F ':' '{print \$5}' | cut --characters=9-16 | tr '[:upper:]' '[:lower:]'))",
'require' => 'File[/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406]',
'logoutput' => 'on_failure',
}) }

context 'with manage_pki_dir => true' do
['/etc/pki', '/etc/pki/rpm-gpg'].each do |path|
it { is_expected.to contain_file(path).with({
Expand All @@ -229,6 +219,12 @@

it { is_expected.to contain_class("puppet_agent::osfamily::suse") }

it { is_expected.to contain_file('/tmp/rpm_gpg_import_check.sh').with({
'ensure' => 'file',
'source' => 'puppet:///modules/puppet_agent/rpm_gpg_import_check.sh',
'mode' => '0755',
}) }

it { is_expected.to contain_file('/etc/pki/rpm-gpg/RPM-GPG-KEY-puppet-20250406').with({
'ensure' => 'present',
'owner' => '0',
Expand Down
5 changes: 5 additions & 0 deletions spec/classes/puppet_agent_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ def global_facts(facts, os)
if os =~ %r{sles}
{
is_pe: true,
env_temp_variable: '/tmp',
operatingsystemmajrelease: facts[:operatingsystemrelease].split('.')[0],
}
elsif os =~ %r{redhat|centos|fedora|scientific|oracle}
{
env_temp_variable: '/tmp',
}
elsif os =~ %r{solaris}
{
is_pe: true,
Expand Down

2 comments on commit c793fd0

@vchepkov
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GabrielNagy , this code places a permanent file into /tmp filesystem which is either tmpfs or cleaned regularly, so file will be re-created again and again causing unnecessary configuration drift. IMHO, a better place for this helper should be somewhere in /usr/local/bin/ or /opt/puppetlabs/bin/

@GabrielNagy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vchepkov, indeed /opt/puppetlabs sounds like a more suitable place. When implementing this I followed the same pattern as the other scripts (like install_puppet.ps1, osx_install.sh, etc), but failed to notice that the GPG check here would be executed on each run, whereas the other scripts get executed only if an upgrade is required.

Please sign in to comment.