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

Fix of deprecation warning #312

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

Fix of deprecation warning #312

wants to merge 1 commit into from

Conversation

amorphina
Copy link

Pull Request (PR) description

Fix of deprecation warning:
Warning: The source_permissions parameter is deprecated. Explicitly set owner, group, and mode.
(file: .../manifests/ca.pp, line: 127)

This Pull Request (PR) fixes the following issues

Replaced:
source_permissions => 'use',
With:
owner => 'root',
mode => '0755',

This is tested on puppet-agent 5.5.7-1 on Ubuntu Xenial, puppetserver 5.3.6-1 Ubuntu Xenial.

…er is deprecated. Explicitly set `owner`, `group`, and `mode`.
@@ -60,7 +60,8 @@
ensure => directory,
recurse => true,
links => 'follow',
source_permissions => 'use',
owner => 'root',
mode => '0755',
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR. Is this the correct mode to use? This directory resource has recurse => true, so I think this mode would make all the files under the directory executable. (But maybe that's what's needed? Could you confirm?)

Puppet automatically sets directory browse permissions, so maybe 0644 would be better?
https://puppet.com/docs/puppet/5.5/types/file.html#file-attribute-mode

When specifying numeric permissions for directories, Puppet sets the search permission wherever the read permission is set.

Copy link
Member

Choose a reason for hiding this comment

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

One of the test failures is due to the now excessive indentation of the parameters in this resource. The acceptance test failures seem to be related to the new modes.

Copy link
Member

Choose a reason for hiding this comment

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

Notice: /Stage[main]/Main/Openvpn::Server[test_openvpn_server]/Openvpn::Ca[test_openvpn_server]/File[/etc/openvpn/test_openvpn_server/easy-rsa/keys/vpnclienta.key]/mode: mode changed '0600' to '0755'
https://travis-ci.org/voxpupuli/puppet-openvpn/jobs/458818494#L1619

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

There are key files that had a very restrictive mode 0600 now being set to 0755. These files should not be executable, but more of a problem is that they're now world readable.

At this stage, I'm not too sure what the best solution is. I wonder why puppet deprecated source_permissions. It looked quite useful here.

@@ -60,7 +60,8 @@
ensure => directory,
recurse => true,
links => 'follow',
source_permissions => 'use',
owner => 'root',
mode => '0755',
Copy link
Member

Choose a reason for hiding this comment

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

Notice: /Stage[main]/Main/Openvpn::Server[test_openvpn_server]/Openvpn::Ca[test_openvpn_server]/File[/etc/openvpn/test_openvpn_server/easy-rsa/keys/vpnclienta.key]/mode: mode changed '0600' to '0755'
https://travis-ci.org/voxpupuli/puppet-openvpn/jobs/458818494#L1619

@bastelfreak bastelfreak added needs-work not ready to merge just yet tests-fail labels Nov 26, 2018
Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Hi @amorphina thank you for the PR.
There is a bunch of failing tests because proposed code is not idempotent.

@alexjfisher
Copy link
Member

alexjfisher commented Nov 26, 2018

There is a bunch of failing tests because proposed code is not idempotent.

@Dan33l Do you know why though? Does the service automatically chmod some files when it starts or something?

@Dan33l
Copy link
Member

Dan33l commented May 15, 2019

@Dan33l Do you know why though? Does the service automatically chmod some files when it starts or something?

@alexjfisher This PR force mode => '0755', and the client key is created by easyrsa with mode 0600 and changed during the second run to 0755.
More than idempotency issue, it is safe that a private key use mode 0600 and not 0755.

@amorphina are you yet interested by this PR ?

@amorphina
Copy link
Author

@Dan33l I am still interested by it.
Can we create a separate file resource to manage the keys directory, something like:

file { "${etc_directory}/openvpn/${name}/easy-rsa/keys" :
  ensure => directory,
  mode   => '0600',
}

This way we will change the mode for all files/dirs to 755 without the keys directory.

Also by default puppet pushes the mode for directories from 6(rw) to 7(rwx), thus the directory ${etc_directory}/openvpn/${name}/easy-rsa/keys will automatically be set to 700, while the files inside it should remain with 600.

@Dan33l
Copy link
Member

Dan33l commented Mar 5, 2020

The source_permissions parameter will be undeprecated :
https://tickets.puppetlabs.com/browse/PUP-10253

@alexjfisher
Copy link
Member

The source_permissions parameter will be undeprecated :
https://tickets.puppetlabs.com/browse/PUP-10253

Has that been agreed?

@Dan33l
Copy link
Member

Dan33l commented Mar 6, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-work not ready to merge just yet tests-fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants