puppetlabs / puppetlabs-apt

Puppet module to help manage Apt
https://forge.puppetlabs.com/puppetlabs/apt
Apache License 2.0
215 stars 462 forks source link

" Harden PPA defined type " new commit hardcodes Puppet path #1055

Closed baldurmen closed 1 year ago

baldurmen commented 1 year ago

Describe the Bug

This new commit, merged and released for tag v9.0.0, hardcodes the Puppet path for the $script_path variable, in the manifests/ppa.pp file:

$script_path = "/opt/puppetlabs/puppet/cache/add-apt-repository-${dash_filename_no_specialchars}-${release}.sh"

/opt/puppetlabs/puppet is not the path used by the Debian Puppet packages (also used by Ubuntu). This uses it's /etc/puppet/).

Expected Behavior

The script_path var should either default to the right path by itself, or at least be modifiable by a user variable.

kenyon commented 1 year ago

Should just put the script content directly in the exec resource rather than writing out a script file.

chelnak commented 1 year ago

@baldurmen thanks for reporting this!

It might make sense to use the puppet_vardir from stdlib fact here. That should point to the correct cache directory for os where the agent is running.

e.g.

puppet config print vardir

@kenyon I get what you are saying here but this is something we are explicitly trying to avoid in this case.

chelnak commented 1 year ago

@baldurmen I've submitted a potential fix in #1056 .

FWIW I don't actually get /etc/puppet as the path on my system.. but could be looking in the wrong place!

facter os
{
  architecture => "amd64",
  distro => {
    codename => "focal",
    description => "Ubuntu 20.04.3 LTS",
    id => "Ubuntu",
    release => {
      full => "20.04",
      major => "20.04"
    }
  },
  family => "Debian",
  hardware => "x86_64",
  name => "Ubuntu",
  release => {
    full => "20.04",
    major => "20.04"
  },
  selinux => {
    enabled => false
  }
}
chelnak commented 1 year ago

@baldurmen I wanted to follow up on this.

Have you experienced a failure from the cache path being set to /opt/puppetlabs/puppet/cache?

Something feels strange that the vardir would be set to /etc/puppet.

Check out this blog:

https://puppet.com/blog/magic-directories-guide-to-puppet-directory-structure/

baldurmen commented 1 year ago

Oh, I didn't understand this was a cache dir, I saw opt and I didn't think further.

$ puppet config print vardir
> /var/lib/puppet

Using $cache_path = $facts['puppet_vardir'] sounds like the right thing to do yes.

FWIW I don't actually get /etc/puppet as the path on my system.. but could be looking in the wrong place!

You're probably using the upstream Puppetlabs package? When I said "Debian" packages, I meant installed via apt install puppet.

chelnak commented 1 year ago

Yes that's correct - I generally use the puppet gem... makes sense now. @binford2k mentioned internally that distro packages can differ.

I'll get my PR cleaned up and let @puppetlabs/modules know it's ready for review.

chelnak commented 1 year ago

@baldurmen This should get merged today 👍