Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactors (ready for review) #81

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .sync.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
---
spec/spec_helper.rb:
hiera_config: spec/fixtures/hiera.yaml
coverage_report: true
minimum_code_coverage_percentage: 100
10 changes: 10 additions & 0 deletions data/os/AIX.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/var/ssl/certs'
ca_cert::update_cmd: '/usr/bin/c_rehash'
ca_cert::cert_dir_group: 'system'
ca_cert::cert_dir_mode: '0755'
ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'system'
ca_cert::ca::ca_file_mode: '0644'
ca_cert::ca::ca_file_extension: 'crt'
11 changes: 11 additions & 0 deletions data/os/Archlinux.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/etc/ca-certificates/trust-source/anchors/'
ca_cert::distrusted_cert_dir: '/etc/ca-certificates/trust-source/blacklist'
ca_cert::update_cmd: 'trust extract-compat'
ca_cert::cert_dir_group: 'root'
ca_cert::cert_dir_mode: '0755'
ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0644'
ca_cert::ca::ca_file_extension: 'crt'
10 changes: 10 additions & 0 deletions data/os/Debian.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/usr/local/share/ca-certificates'
ca_cert::update_cmd: 'update-ca-certificates'
ca_cert::cert_dir_group: 'staff'
ca_cert::cert_dir_mode: '2665'
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this default come from?
On a fresh install of ca-certificates in amd/debian:10,amd/debian:11 and amd/debian:12 containers the default is

root@60422e36a680:/# ls -l /usr/local/share/
total 0
drwxr-xr-x. 2 root root 6 May  9 14:06 ca-certificates

ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0444'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not 0644 as default? Fresh debian container once more:
No files at all in /usr/local/share/ca-certificates and certificates in /usr/share/ca-certificates have 0644

root@b3059a7e2497:/# ls -l /usr/share/ca-certificates/mozilla/
total 560
-rw-r--r--. 1 root root 2772 Mar 11  2023  ACCVRAIZ1.crt
-rw-r--r--. 1 root root 1972 Mar 11  2023  AC_RAIZ_FNMT-RCM.crt
-rw-r--r--. 1 root root  904 Mar 11  2023  AC_RAIZ_FNMT-RCM_SERVIDORES_SEGUROS.crt

Copy link
Member

Choose a reason for hiding this comment

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

/usr/local/share/ca-certificates is the right place for certs installed by this module. See the FILES section of man update-ca-certificates.

I'd be fine with 0644 or 0444, but I think we often use 0444 as a way to show that the files are puppet-managed and shouldn't be manually edited.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just referring to /usr/share/ca-certificates because that's provided by the ca-certificates package with mode 0644 as an argument for using the same when adding files to /usr/local/share/ca-certificates :)

ca_cert::ca::ca_file_extension: 'crt'
10 changes: 10 additions & 0 deletions data/os/RedHat.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/etc/pki/ca-trust/source/anchors'
ca_cert::distrusted_cert_dir: '/etc/pki/ca-trust/source/blacklist'
ca_cert::update_cmd: 'update-ca-trust extract'
ca_cert::cert_dir_group: 'root'
ca_cert::cert_dir_mode: '0755'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0644'
ca_cert::ca::ca_file_extension: 'crt'
9 changes: 9 additions & 0 deletions data/os/Solaris.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
ca_cert::trusted_cert_dir: '/etc/certs/CA/'
ca_cert::update_cmd: '/usr/sbin/svcadm restart /system/ca-certificates'
ca_cert::cert_dir_group: 'sys'
ca_cert::cert_dir_mode: '0755'
ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0444'
ca_cert::ca::ca_file_extension: 'pem'
2 changes: 2 additions & 0 deletions data/os/Solaris/11.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
---
ca_cert::supported: true
11 changes: 11 additions & 0 deletions data/os/Suse.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/etc/pki/trust/anchors'
ca_cert::distrusted_cert_dir: '/etc/pki/trust/blacklist'
ca_cert::update_cmd: 'update-ca-certificates'
ca_cert::cert_dir_group: 'root'
ca_cert::cert_dir_mode: '0755'
ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0644'
ca_cert::ca::ca_file_extension: 'crt'
5 changes: 5 additions & 0 deletions data/os/Suse/10.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
ca_cert::trusted_cert_dir: '/etc/ssl/certs'
ca_cert::update_cmd: 'c_rehash'
ca_cert::package_name: 'openssl-certs'
ca_cert::ca::ca_file_extension: 'pem'
5 changes: 5 additions & 0 deletions data/os/Suse/11.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
ca_cert::trusted_cert_dir: '/etc/ssl/certs'
ca_cert::update_cmd: 'c_rehash'
ca_cert::package_name: 'openssl-certs'
ca_cert::ca::ca_file_extension: 'pem'
10 changes: 10 additions & 0 deletions data/os/Ubuntu.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
ca_cert::supported: true
ca_cert::trusted_cert_dir: '/usr/local/share/ca-certificates'
ca_cert::update_cmd: 'update-ca-certificates'
ca_cert::cert_dir_group: 'staff'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why staff as default? The ca-certificates package creates this directory with group root ...

root@cbe1f3cd0871:/# ls -l /usr/local/share/
total 0
drwxr-xr-x. 2 root root 6 May  9 17:31 ca-certificates

ca_cert::cert_dir_mode: '0755'
ca_cert::package_name: 'ca-certificates'
ca_cert::ca::ca_file_group: 'root'
ca_cert::ca::ca_file_mode: '0444'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same reasoning as for Debian. See no reason why this should default to 0444 ..

ca_cert::ca::ca_file_extension: 'crt'
21 changes: 21 additions & 0 deletions hiera.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
version: 5

defaults: # Used for any hierarchy level that omits these keys.
datadir: data # This path is relative to hiera.yaml's directory.
data_hash: yaml_data # Use the built-in YAML backend.

hierarchy:
- name: "osfamily/major release"
paths:
# Used to distinguish between Debian and Ubuntu
- "os/%{facts.os.name}/%{facts.os.release.major}.yaml"
- "os/%{facts.os.family}/%{facts.os.release.major}.yaml"
# Used for Solaris
- "os/%{facts.os.family}/%{facts.kernelrelease}.yaml"
- name: "osfamily"
paths:
- "os/%{facts.os.name}.yaml"
- "os/%{facts.os.family}.yaml"
- name: 'common'
path: 'common.yaml'
61 changes: 24 additions & 37 deletions manifests/ca.pp
Original file line number Diff line number Diff line change
Expand Up @@ -32,38 +32,25 @@
# The installed CA certificate's POSIX filesystem permissions. This uses
# the same syntax as Puppet's native file resource's "mode" parameter.
# (defaults to '0444', i.e. world-readable)
# [*ca_file_extension*]
# File extenstion for the certificate.
#
# === Examples
#
# ca_cert::ca { 'globalsign_org_intermediate':
# source => 'http://secure.globalsign.com/cacert/gsorganizationvalsha2g2r1.crt',
# }
define ca_cert::ca (
Optional[String] $ca_text = undef,
String $source = 'text',
String $ensure = 'trusted',
Boolean $verify_https_cert = true,
Optional[String] $checksum = undef,
Optional[String] $ca_text = undef,
String $source = 'text',
String $ensure = 'trusted',
Boolean $verify_https_cert = true,
Optional[String] $checksum = undef,
Optional[String[1]] $checksum_type = undef,
Optional[String] $ca_file_group = undef,
Optional[String] $ca_file_mode = undef,
String[1] $ca_file_group = lookup('ca_cert::ca::ca_file_group'),
String[1] $ca_file_mode = lookup('ca_cert::ca::ca_file_mode'),
String[1] $ca_file_extension = lookup('ca_cert::ca::ca_file_extension'),
) {
include ca_cert::params
include ca_cert::update
require ca_cert::enable

if $ca_file_group == undef {
$file_group = $ca_cert::params::ca_file_group
} else {
$file_group = $ca_file_group
}

if $ca_file_mode == undef {
$file_mode = $ca_cert::params::ca_file_mode
} else {
$file_mode = $ca_file_mode
}

if ($ensure == 'trusted' or $ensure == 'distrusted') and $source == 'text' and !$ca_text {
fail('ca_text is required if source is set to text')
}
Expand Down Expand Up @@ -94,11 +81,11 @@
}

# Determine Full Resource Name
$resource_name = "${name}.${ca_cert::params::ca_file_extension}"
$resource_name = "${name}.${ca_file_extension}"

$ca_cert = $adjusted_ensure ? {
'distrusted' => "${ca_cert::params::distrusted_cert_dir}/${resource_name}",
default => "${ca_cert::params::trusted_cert_dir}/${resource_name}",
'distrusted' => "${ca_cert::distrusted_cert_dir}/${resource_name}",
default => "${ca_cert::trusted_cert_dir}/${resource_name}",
}

case $adjusted_ensure {
Expand All @@ -112,9 +99,9 @@
source => $source,
path => $ca_cert,
owner => 'root',
group => $file_group,
mode => $file_mode,
notify => Class['ca_cert::update'],
group => $ca_file_group,
mode => $ca_file_mode,
notify => Exec['ca_cert_update'],
}
}
'ftp', 'https', 'http': {
Expand All @@ -124,7 +111,7 @@
checksum => $checksum,
checksum_type => $checksum_type,
allow_insecure => !$verify_https_cert,
notify => Class['ca_cert::update'],
notify => Exec['ca_cert_update'],
}
}
'file': {
Expand All @@ -134,9 +121,9 @@
source => $source_path,
path => $ca_cert,
owner => 'root',
group => $file_group,
mode => $file_mode,
notify => Class['ca_cert::update'],
group => $ca_file_group,
mode => $ca_file_mode,
notify => Exec['ca_cert_update'],
}
}
'text': {
Expand All @@ -145,9 +132,9 @@
content => $ca_text,
path => $ca_cert,
owner => 'root',
group => $file_group,
mode => $file_mode,
notify => Class['ca_cert::update'],
group => $ca_file_group,
mode => $ca_file_mode,
notify => Exec['ca_cert_update'],
}
}
default: {
Expand All @@ -158,7 +145,7 @@
'absent': {
file { $ca_cert:
ensure => absent,
notify => Class['ca_cert::update'],
notify => Exec['ca_cert_update'],
}
}
default: {
Expand Down
23 changes: 0 additions & 23 deletions manifests/enable.pp

This file was deleted.

80 changes: 55 additions & 25 deletions manifests/init.pp
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@
# The ensure parameter to pass to the package resource
# [*package_name*]
# The name of the package(s) to be installed
# [*trusted_cert_dir*]
Phil-Friderici marked this conversation as resolved.
Show resolved Hide resolved
# Absolute directory path to the folder containing trusted certificates.
# [*distrusted_cert_dir*]
# Absolute directory path to the folder containing distrusted certificates.
# [*update_cmd*]
# Command to be used to update CA certificates.
# [*cert_dir_group*]
# The installed trusted certificate's POSIX group permissions. This uses
# the same syntax as Puppet's native file resource's "group" parameter.
# (defaults to 'root' with the exeption of AIX which defaults to 'system').
# [*cert_dir_mode*]
# The installed trusted certificate's POSIX filesystem permissions. This uses
# the same syntax as Puppet's native file resource's "mode" parameter.
# (defaults to '0444', i.e. world-readable).
# [*supported*]
# Boolean to ensure module runs only on supported OS families and versions.
#
# === Examples
#
Expand All @@ -40,37 +56,33 @@
#
# Phil Fenstermacher <[email protected]>
#
# lint:ignore:variable_is_lowercase
class ca_cert (
Boolean $always_update_certs = false,
Boolean $purge_unmanaged_CAs = false, # lint:ignore:variable_contains_upcase
Boolean $install_package = true,
Boolean $force_enable = false,
Hash $ca_certs = {},
String $package_ensure = 'installed',
String $package_name = $ca_cert::params::package_name,
) inherits ca_cert::params {
include ca_cert::params
include ca_cert::update

if $always_update_certs == true {
Exec <| title=='ca_cert_update' |> {
refreshonly => false,
}
Boolean $always_update_certs = false,
Boolean $purge_unmanaged_CAs = false, # lint:ignore:variable_is_lowercase lint:ignore:variable_contains_upcase
Boolean $install_package = true,
Boolean $force_enable = false,
Hash $ca_certs = {},
String $package_ensure = 'installed',
String[1] $package_name = 'ca-certificates',
String[1] $trusted_cert_dir = '/etc/pki/ca-trust/source/anchors',
Optional[String[1]] $distrusted_cert_dir = undef,
String[1] $update_cmd = 'update-ca-trust extract',
String[1] $cert_dir_group = 'root',
String[1] $cert_dir_mode = '0755',
Boolean $supported = false,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I like this supported parameter pattern. I don't think we do this in any of the Vox Pupuli modules. It seems redundant to just listing supported OSes in metadata.json. I suppose you are retaining existing behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is only here to maintain the existing behaviour. Would be more than happy to remove it :)
Let @pcfens decide which route to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree with @kenyon on this one. @pcfens doesn't seem to have any opinion?

) {
if $supported == false {
fail("Unsupported osfamily (${facts['os']['family']}) or unsupported version (${facts['os']['release']['major']})")
}

$trusted_cert_dir = $ca_cert::params::trusted_cert_dir
$cert_dir_group = $ca_cert::params::cert_dir_group
$cert_dir_mode = $ca_cert::params::cert_dir_mode

file { 'trusted_certs':
ensure => directory,
path => $trusted_cert_dir,
owner => 'root',
group => $cert_dir_group,
mode => $cert_dir_mode,
purge => $purge_unmanaged_CAs, # lint:ignore:variable_contains_upcase
recurse => $purge_unmanaged_CAs, # lint:ignore:variable_contains_upcase
purge => $purge_unmanaged_CAs, # lint:ignore:variable_is_lowercase lint:ignore:variable_contains_upcase
recurse => $purge_unmanaged_CAs, # lint:ignore:variable_is_lowercase lint:ignore:variable_contains_upcase
notify => Exec['ca_cert_update'],
}

Expand All @@ -82,8 +94,26 @@
}
}

if !empty($ca_certs) {
create_resources('ca_cert::ca', $ca_certs)
create_resources('ca_cert::ca', $ca_certs)

if ($facts['os']['family'] == 'RedHat' and versioncmp($facts['os']['release']['full'], '7') < 0) {
Phil-Friderici marked this conversation as resolved.
Show resolved Hide resolved
$_enable_command = $force_enable ? {
true => 'update-ca-trust force-enable',
default => 'update-ca-trust enable',
}

exec { 'enable_ca_trust':
command => $_enable_command,
logoutput => 'on_failure',
path => ['/usr/sbin', '/usr/bin', '/bin'],
onlyif => 'update-ca-trust check | grep DISABLED',
}
}

exec { 'ca_cert_update':
command => $update_cmd,
logoutput => 'on_failure',
refreshonly => !$always_update_certs,
path => ['/usr/sbin', '/usr/bin', '/bin'],
}
}
# lint:endignore:variable_is_lowercase
Loading