Bug #1538

Yumrepo sets permissions wrongly on files in /etc/yum.repos.d

Added by Robert Lazzurs over 3 years ago. Updated about 2 years ago.

Status:Closed Start date:08/29/2008
Priority:Normal Due date:
Assignee:Marc Fournier % Done:

0%
Category:yumrepo
Target version:0.25.1
Affected Puppet version:0.24.8 Branch:
Keywords:yumrepo, mode, permissions
Votes: 0

Description

When writing out the repo config files the permissions are set to 600. However a default setup from Fedora or RedHat will have these files set to 644 to allow users to use yum search.

This is how this application should be configured. If you require any further information please let me know.

yumrepo-permission-fix.patch (911 Bytes) Marc Fournier, 12/13/2008 06:32 pm

inifile.patch (1.5 kB) Kjetil Torgrim Homme, 09/15/2009 03:47 pm

Related issues

related to Puppet - Bug #2641: problems with yum-repo provider and noop Duplicate 09/15/2009

History

#1
Updated by James Turnbull over 3 years ago

  • Category set to yumrepo
  • Status changed from Unreviewed to Needs More Information
  • Assignee set to David Lutterkort

David – can you confirm?

#2
Updated by Robert Lazzurs over 3 years ago

Is there anything more you require me to add to this bug report?

Kind regards.

#3
Updated by Luke Kanies over 3 years ago

  • Status changed from Needs More Information to Accepted

It looks like the mode isn’t explicitly set anywhere.

#4
Updated by Marc Fournier over 3 years ago

  • Affected Puppet version changed from 0.24.4 to 0.24.6

It would probably make sense to have files created by yumrepo have 0644 mode. With file mode set to 0600, only root is able to use yum. Normal users cannot do things such as “yum search” or “yum info”. They can usually do this sort of things on a system installed by hand.

Would it be considered ugly to chmod the file somewhere in this code block (from yumrepo.rb) ?

# Store all modifications back to disk 
def self.store
    inifile.store
end

If you’re ok with this idea I can try to make a patch.

#5
Updated by Luke Kanies over 3 years ago

Seems reasonable to me; there isn’t ever any private information in the files, is there?

#6
Updated by Marc Fournier over 3 years ago

luke wrote:

Seems reasonable to me; there isn’t ever any private information in the files, is there?

Except an optional proxy username/password, it doesn’t seem so. Anyway both redhat and fedora ship their repository description files with world readable mode:

marc@lonquimay:/tmp$ rpm2cpio fedora-release-10-1.noarch.rpm | sudo cpio -di
78 blocks
marc@lonquimay:/tmp$ ls -l etc/yum.repos.d/
total 16
-rw-r--r-- 1 root root 1962 2008-12-13 19:19 fedora-rawhide.repo
-rw-r--r-- 1 root root 1090 2008-12-13 19:19 fedora.repo
-rw-r--r-- 1 root root 1108 2008-12-13 19:19 fedora-updates.repo
-rw-r--r-- 1 root root 1181 2008-12-13 19:19 fedora-updates-testing.repo

I’ve attached a simple patch. Luke, do you think this is a suitable way of solving the issue ?

#7
Updated by Robert Lazzurs over 3 years ago

Is this patch going to be accepted into the next version of puppet?

#8
Updated by Luke Kanies over 3 years ago

  • Assignee changed from David Lutterkort to James Turnbull
  • Target version set to 0.24.8

Seems reasonable to include in the next release; sorry for taking so long to merge it.

#9
Updated by Luke Kanies over 3 years ago

  • Status changed from Accepted to Ready For Checkin

#10
Updated by James Turnbull over 3 years ago

  • Status changed from Ready For Checkin to Closed

Pushed in commit:535fa89af0b87f1d514d75040c1da7795d1dc87e in branch 0.24.x

#11
Updated by Kjetil Torgrim Homme over 2 years ago

  • Status changed from Closed to Re-opened

The applied patch breaks the yumrepo provider in a couple of ways.

a) if more than one file needs changes, only one will be written b) if puppetd is run in —noop mode, the loop in inifile.store will fall through and return @files which is not a valid argument for chmod.

The patch doesn’t really solve the original bug, either, since chmod will only be run whenever a repo file is written.

#12
Updated by James Turnbull over 2 years ago

  • Assignee changed from James Turnbull to Marc Fournier
  • Target version changed from 0.24.8 to 0.25.1
  • Affected Puppet version changed from 0.24.6 to 0.24.8

Comments on the issues?

#13
Updated by Kjetil Torgrim Homme over 2 years ago

this patch fixes the backtrace in noop mode, and will do chmod on all files when a change happens. it will not do chmod if the content doesn’t change, though.

#14
Updated by Luke Kanies over 2 years ago

  • Status changed from Re-opened to In Topic Branch Pending Review

#15
Updated by David Lutterkort over 2 years ago

IMHO, the cleanest solution to this would be to only set the mode of newly created .repo files, and to just hardcode that to 0644.

The way I would do this (if I had time), is to modify IniConfig::File::store to something like (changes only in the ‘if dirty’ part)

      def store
        @files.each do |file, lines|
            text = ""
            dirty = false
            lines.each do |l|
                if l.is_a?(Section)
                    dirty ||= l.dirty?
                    text << l.format
                    l.mark_clean
                else
                    text << l
                end
            end
            if dirty
                old = File::exist?(old)
                Puppet::Util::FileType.filetype(:flat).new(file).write(text)
                unless old
                  chmod(file, @newfile_mode)
                end
                return file
            end
        end
    end

and pass @newfile_mode into the IniConfig::File constructor from the yumrepo type (where I’d just hardcode it to 0644 for now)

#16
Updated by Kjetil Torgrim Homme over 2 years ago

David Lutterkort wrote:

IMHO, the cleanest solution to this would be to only set the mode of newly created .repo files, and to just hardcode that to 0644.

I guess this is a philosophical question. in my opinion, if a file is under Puppet control, Puppet should have absolute control. this means that permissions (and ownership) should be checked and changed back if it somehow has changed between runs. perhaps it is better to do this with an explicit

file { “/etc/yum.repos.d”: mode => 0644, owner => root, recurse => true }

although this means I need to add explicit knowledge of that pathname to my configuration, whereas it was a hidden implementation detail before.

Luke/James, let me know how you’d like it, and I’ll respin.

#17
Updated by James Turnbull over 2 years ago

  • Status changed from In Topic Branch Pending Review to Needs More Information

Can someone confirm what the decision is here? Which patch?

#18
Updated by Luke Kanies over 2 years ago

I think the second patch is the one to use. The first one is already applied.

#19
Updated by James Turnbull over 2 years ago

  • Status changed from Needs More Information to Closed

Pushed in commit:fd322daa42f751bfc6200b95e6151540b3eb541e in branch 0.25.x

Also available in: Atom PDF