use new syntax for stub in rspec

[schurzi]

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

[schurzi]

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]

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]

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]

@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]

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

[schurzi]

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:

• The problem discovered by @mcgege also has some other implications, mainly that the default way of generating the user objects is not working. I only get the error: Provider user_role_add is not functional on this host
• I do not know/understand if the user objects are generated when Puppet is loaded, or if they are regenerated on each test run (they are not facts, and thus I do not know how to clear them)
• Puppet also uses a C library function getpwent to query /etc/passwd I will need to find a way to stub this, if this is relevant

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:

• stub read from /etc/passwd: https://github.com/puppetlabs/puppet/blob/4ed7f371da8314e0e1192d6c8684b4cb59ba6cd3/spec/unit/provider/user/useradd_spec.rb#L340
• idea to stub getpwent: https://github.com/puppetlabs/puppet/blob/e4a654400a4c1ad3b5bc73fb558d28219a4626b2/spec/unit/type/exec_spec.rb#L285

[mcgege]

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

[schurzi]

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]

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]

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]

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]

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]

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

[mcgege]

Fantastic! Can we merge this?

[schurzi]

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]

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