search

dev-sec / puppet-os-hardening

This puppet module provides numerous security-related configurations, providing all-round base protection.
http://dev-sec.io/
Apache License 2.0
280 stars 101 forks source link

use new syntax for stub in rspec #259

Closed schurzi closed 3 years ago

schurzi commented 3 years ago

Signed-off-by: Martin Schurz Martin.Schurz@t-systems.com

schurzi commented 3 years ago

the unit test job for Puppet 6 seems to enter an endless loop, I don't know why (https://travis-ci.org/github/dev-sec/puppet-os-hardening/jobs/752127358)

mcgege commented 3 years ago

Hmpf, I'm lost here ... but as it used to work until 2 months ago something elemental must have changed. The first time this test had an error was with the change from puppet 6.18.0 to 6.19.1

schurzi commented 3 years ago

so, now the tests are technically working but the two tests seem to be broken. we could merge this now and use a separate PR to fix them (I already have some Ideas). Or if you like, we can keep this PR and work here till everything checks out.

mcgege commented 3 years ago

@schurzi First of all: Thanks a lot for your effort! I'd prefer to continue on this PR, but we can also merge this and continue then ... your choice.

I'm still wondering why we get these errors (running locally bundle exec rake spec):

Unable to add resolve nil for fact retrieve_system_users: No such file or directory @ rb_sysopen - /etc/user_attr
Unable to add resolve nil for fact home_users: No such file or directory @ rb_sysopen - /etc/user_attr

This looks like test assumes we're on Solaris and tries to work with user attributes ...

tuxmea commented 3 years ago

We have seen the same issue at customers. Seems to be related to a regression regarding facts caching from rspec-puppet-facts.

schurzi commented 3 years ago

so I did dive a 'little' bit deeper into this. The short summary is that it may be not a good idea to use Puppet Types in Facts.

So here is what I did. We are currently using self generated Types in our before block, like Puppet::Type.type(:user).new(name: 'root', ensure: 'present'). To make the tests work like on a real system, we need to extend these user objects with uid, home and possibly some other information. To do that, we would need to add these to the call like Puppet::Type.type(:user).new(name: 'root', ensure: 'present', home:'/root', uid: 0). But there is a problem. When Puppet gets additional properties in a way like this, it adds them as desired state ('should' in the object - see https://www.rubydoc.info/gems/puppet/Puppet/Property).

When we read data via user.retrieve it gets the current value of all properties and not the 'should' value. One can sync the state of properties from should to value with a call to .set or .sync, but these calls also directly trigger a update on the system, meaning they execute useradd/ usermod, which is not a good idea for a testcase.

To counter the execution we could also stub Puppet::Util::Execution.execute to catch all invocations of useradd/ usermod, but this feels like a can of worms. There is no other programatic way I konw of to transfer the properties from 'should' state to the value. Maybe someone more familar with ruby cloud help here?

After I decided, that this is a dead end, I thought about some better ways to do this and came up with the idea to stub read from /etc/passwd. In this case we would include a fixture with a /etc/passwd file that contains our testusers. I think this is a realy neat and clean approach. That way we would use standad Puppet library calls to fill our testdata and get all the desired properties via the default way. so I added this to our before block and removed everything else:

allow(File).to receive(:open).and_call_original
allow(File).to receive(:open)
  .with('/etc/passwd')
  .and_return(File.open("#{File.dirname(__FILE__)}/../../fixtures/etc/passwd"))
allow(File).to receive(:read).and_call_original
allow(File).to receive(:read)
  .with('/etc/passwd')
  .and_return(File.read("#{File.dirname(__FILE__)}/../../fixtures/etc/passwd"))

Both openand read are needed, because the code in Puppet uses different methods for access.

Sadly this is currently not working. There are some possible explainations for this:

To continue here we should first fix the issue with Puppet/RSpec thinking we are running on Solaris. @tuxmea wo you have some other information on this, maybe a issue link?

After we have fixed that, I would really like to explore the possibility of stubbing /etc/passwd

Additional Links:

mcgege commented 3 years ago

@schurzi @tuxmea Any idea on how to continue here?

schurzi commented 3 years ago

I just hat a grat idea on how to change the functions and get rid of the whole problem.

I will need to rewrite the custom facts to not use the Puppet ressource User. A quick test looks promising, but I still need to get the test straight. but this seems very approachable right now.

schurzi commented 3 years ago

so, it seems using the Puppet Types in a fact was our main courlpit here. I changed this to the standard Ruby functions to read /etc/passwd.

As far as I understand this, I is exactly what we want for retireve_system_users. But I'm not completely sure, if it is also usable for home_users, since we do not enumerate users from LDAP for example. I do not know, if the default Puppet User type would have picked these up.

Also the usage of Etc.passwd limits our compatibility to linux, which is currently o problem, since we only support linux.

schurzi commented 3 years ago

regarding integration tests. I think CentOS 6 might be a bit hard to fix and maybe we should drop it from our test suite. CentOS 6 has been deprecated for quite some time. What do you think?

The remaining integration tests seem to be failing because of docker rate limit.

mcgege commented 3 years ago

I think we can drop CentOS 6, this shouldn't matter any more ... I'll run the tests locally tomorrow and try them out

mcgege commented 3 years ago

Great work, @schurzi ! I'd vote for dropping CentOS 6 ... when the tests are running again we should anyway cleanup old + add new operating systems

schurzi commented 3 years ago

now all is green, after ftp5.gwdg.de is up again =)

mcgege commented 3 years ago

Fantastic! Can we merge this?

schurzi commented 3 years ago

well, I'm done. If CI says green, we can merge this any time. We should publish a minor relase though and not a bugfix.

mcgege commented 3 years ago

I'll publish a new release soon, there are two more PRs waiting right now. Thanks again!!