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.

Sign up for GitHub

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

Jump to bottom

Fixes #30962 - fix dhcpd.conf acl #620

Closed
wants to merge 1 commit into from
Closed

Fixes #30962 - fix dhcpd.conf acl #620

wants to merge 1 commit into from

Conversation

m-bucher
Copy link

@m-bucher m-bucher commented Oct 2, 2020

  1. Make sure that we explicitly check ACLs for dhcpd.conf, because the ACLs are not refreshed, if the /etc/dhcp/-directory already has the correct permissions. The most secure way would be to remove the unless-statement, so the ACLs are always (re)set recursively 🤔 .
  2. Make sure the ACLs are set after dhcp puppet-module has finished configuration, before setting the ACLs

Probably needs to be cherry-picked into 1.4-stable-branch.

ekohl
ekohl reviewed Oct 2, 2020
View reviewed changes
manifests/proxydhcp.pp Outdated Show resolved Hide resolved
@m-bucher
Copy link
Author

m-bucher commented Oct 5, 2020 •
edited
Loading

@ekohl any idea how I can fix the tests?

@ekohl
Copy link
Member

ekohl commented Oct 5, 2020

@ekohl any idea how I can fix the tests?

As @tbrisker pointed out, the +x permission causes a weird mode.

ekohl
ekohl reviewed Oct 5, 2020
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this shouldn't be needed in master because the cherry pick to stable was different than master.

The approach in master was to make the directory 0750 and apply ACLs. Then mode of dhcpd.conf can still be 0644 (which the module enforces). That means the ACL on dhcpd.conf is actually irrelevant.

In the stable branch we chose a different approach. To change the mode on /etc/dhcp was considered a major version of the DHCP module and we didn't want to pull that in. (In hindsight, we probably should have to avoid this exact problem.) The solution there was to set dhcpd.conf to mode 0640, unconditionally and rely on the ACLs being recursive.

A better fix would be to set the default for new files (which ACLs can do). That would give a much higher chance that it'll actually work. You also don't have to set it on dhcpd.conf explicitly.

@m-bucher
Copy link
Author

m-bucher commented Oct 5, 2020

I think adding the dependency on the dhcp-module should be enough to fix the problems I experience.
This would mean that we do not have to set the permission on dhcpd.conf explicitly and rely on the recursive setfacl.

I will check this again.
I have not tested with master-branch, yet, I will take a look if I can do so fore my scenario.

@m-bucher
Copy link
Author

m-bucher commented Oct 5, 2020

I think this shouldn't be needed in master because the cherry pick to stable was different than master.

Should I change the base-branch to 14.1-stable then?

@ekohl
Copy link
Member

ekohl commented Oct 5, 2020

The chaining on the DHCP class does make sense even in master. The current version should be committed to master and then cherry picked into 14.1-stable.

@m-bucher
Copy link
Author

m-bucher commented Oct 6, 2020

Regarding the tests, when I run the beaker task locally, I get the following messages for the failing test Scenario: install foreman-proxy with HTTPBoot:

  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp: Operation not supported
  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp/dhcpd.hosts: Operation not supported
  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp/dhcpd6.conf: Operation not supported
  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp/dhcpd.conf: Operation not supported
  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp/scripts: Operation not supported
  Notice: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: setfacl: /etc/dhcp/scripts/README.scripts: Operation not supported
  Error: 'setfacl -R -m u:foreman-proxy:rx /etc/dhcp' returned 1 instead of one of [0]
  Error: /Stage[main]/Foreman_proxy::Proxydhcp/Exec[Allow foreman-proxy to read /etc/dhcp]/returns: change from 'notrun' to ['0'] failed: 'setfacl -R -m u:foreman-proxy:rx /etc/dhcp' returned 1 instead of one of [0]

Could it be that file-ACLs are not supported by the container the test runs in and before it just did not try to apply the file-ACLs, because the directories simply have not been there?

@m-bucher
Copy link
Author

m-bucher commented Oct 6, 2020

Then again, I get the same errors running bundle exec rake beaker locally on the master-branch 😞

@m-bucher
Copy link
Author

m-bucher commented Oct 6, 2020

So comparing the tests from master branch with the ones from this branch, at the second run the file permissions seem to be altered by the dhcp-module (lines 2835-2839):

Notice: /Stage[main]/Dhcp/Concat[/etc/dhcp/dhcpd.conf]/File[/etc/dhcp/dhcpd.conf]/mode: mode changed '0654' to '0644'
Info: Concat[/etc/dhcp/dhcpd.conf]: Scheduling refresh of Service[dhcpd]
Notice: /Stage[main]/Dhcp/Concat[/etc/dhcp/dhcpd.hosts]/File[/etc/dhcp/dhcpd.hosts]/mode: mode changed '0654' to '0644'
Info: Concat[/etc/dhcp/dhcpd.hosts]: Scheduling refresh of Service[dhcpd]
Notice: /Stage[main]/Dhcp/Service[dhcpd]: Triggered 'refresh' from 2 events

Is this not a direct result of the fact that we set x recursively on /etc/dhcp-directory?

m-bucher
m-bucher commented Oct 7, 2020
View reviewed changes
manifests/proxydhcp.pp
@@ -76,8 +76,10 @@
exec { "Allow ${foreman_proxy::user} to read ${path}":
command => "setfacl -R -m u:${foreman_proxy::user}:rx ${path}",
path => ['/bin', '/usr/bin'],
unless => "getfacl -p ${path} | grep user:${foreman_proxy::user}:r-x",
Copy link
Author

Choose a reason for hiding this comment

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

I am pretty sure we have to remove this line, because it prevents the permissions of files within those directories from being fixed, if the permission of the directory has not changed.

Copy link
Member

Choose a reason for hiding this comment

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

Removing this line means it's no longer idempotent.

@ekohl ekohl mentioned this pull request Oct 7, 2020
Merged
@m-bucher
Copy link
Author

m-bucher commented Oct 8, 2020

Closing in favour of #621 and #622

@m-bucher m-bucher closed this Oct 8, 2020
@m-bucher m-bucher deleted the fix_dhcpd.conf_facls branch October 8, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@tbrisker tbrisker tbrisker left review comments

Assignees
No one assigned
Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m-bucher @ekohl @tbrisker @theforeman-bot