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

use vcsrepo "${basedir}/puppetboard" with group "$group" #349

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janit42
Copy link

@janit42 janit42 commented Feb 2, 2022

Pull Request (PR) description

the vcsrepo resource which checks out puppetboard uses $group (which it already required in previous versions) instead of the default group (usually root)

the vcsrepo resource which checks out puppetboard uses $group (which it already required in previous versions) instead of the default group (usually root)
Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

I realize it now but this is an insecure setup that we do here by default when using install_from => 'vcsrepo'. The application runs as the puppetboard user so this user should rather not have write access to the files. That way if some security issue allows remote code execution / unauthorized file write, Puppetbord will not be able to replace it's code with malicious one for pivoting / exploitation.

I would rather suggest removing the owner or explicitly setting it to root or some $deploy_user which defaults to undef, and same for the group.

What do you think?

@janit42
Copy link
Author

janit42 commented Feb 6, 2022

What do you think?

I think you have a point here !

However, there are applications which might actually need write access to some of their own config files (e.g. to modify their settings via a web interface). I'm not sure about puppetboard, but as there are no config options in the web interface it might be fine and desirable to limit access to read-only.

If the module was changed to have most of the files owned by root or an optional deploy_user, it would probably be necessary to set the group allowing read access to the files deployed below $basedir (like the settings file) - these resources seem also to be defined without explicitly setting the group in the current state of the module.

@vox-pupuli-tasks
Copy link

Dear @janit42, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants