Add keychain support

[chr4]

Allows a keychain hash to be specified, containing an ssh keypair which
will be deployed to the users homedir.

[ranjib]

this logic is kinda arbitrary :-/

[chr4]

Can you elaborate why?

[ranjib]

you are assuming here that the pub keys will always end in .pub. Any reason to not enforce 0400 in general without any checks?

[chr4]

It's an assumption, but one that is always true when using ssh-keygen unless you manually rename your private key to *.pub. :)

It will probably do no harm to just use 600 or even 400 for all files, the idea was to stick to the defaults (of ssh-keygen) if you can - And that would be 644 for the pubkey and 600 for the private one.

[theckman]

I'm okay making this assumption for now. If it breaks in the future we can remove it.

[chr4]

Rebased this to current master. Any news on merging this @ranjib @fnichol?

[theckman]

I'm okay with leaving the .pub assumption, but I think there are two other changes that should be made before this is merged in.

I'm also looking to update the integration tests to use more recent distributions. Would you be willing to add integration tests for this functionality, including testing that the file modes are as-expected?

[theckman]

Has this behavior been testing during a --why-run when the user doesn't exist yet? I'm thinking you may be vulnerable to the same bug that was fixed in #89.

[chr4]

I suppose that my implementation will have the same issue. Is there a dynamic way of fixing this (without requiring a manual group attribute to be set)?

[theckman]

@chr4 sorry for the delay in getting back to you. Been a busy few weeks. 😒

Looking around this file I think you might be able to work around this by using the @my_home instance variable. It seems like this is used in a few other places where directory creation is handled. I wonder if it snuck in while this PR was sitting open.

Here are some examples of it being used:

• https://github.com/chr4-cookbooks/user/blob/e4959685d6e5db96939c0fb0e6f51bc21901437d/providers/account.rb#L145
• https://github.com/chr4-cookbooks/user/blob/e4959685d6e5db96939c0fb0e6f51bc21901437d/providers/account.rb#L201

[chr4]

The problem with @my_home is, that it's just an educated guess, assuming that the home directory resides in "#{node['user']['home_root']}/#{new_resource.username}".

I've just stumbled over the follwing code snipped in account.rb (which determines the gid), which seems to do exactly what we want:

https://github.com/chr4-cookbooks/user/blob/e4959685d6e5db96939c0fb0e6f51bc21901437d/providers/account.rb#L104-L110

def user_gid
  # this check is needed as the new user won't exit yet
  # in why_run mode, causing an uncaught ArgumentError exception
  Etc.getpwnam(new_resource.username).gid
rescue ArgumentError
  nil
end

I'd suggest adding a user_home method in a similar fashion, which then should be used everywhere instead of @my_home. I'd use something like this:

# Returns the user's home directory, either from the `home` attribute (if specified), or using Etc.getpwnam()
def user_home
  # This check is needed as the new user won't exit yet in why_run mode, 
  # causing an uncaught ArgumentError exception
  new_resource.home || Etc.getpwnam(new_resource.username).dir
rescue ArgumentError
  nil
end

[chr4]

Alternatively, the new_resource.home check can be done outside of the function: new_resource.home || user_home

[chr4]

I've implemented user_home and pushed the changes. This works well for me.

[theckman]

Please use ::File.join().

[chr4]

Ok, push incoming. There's other occasions of paths constructed using string interpolation, though. For example https://github.com/chr4-cookbooks/user/blob/e4959685d6e5db96939c0fb0e6f51bc21901437d/providers/account.rb#L182

Should those be adjusted as well then?

[theckman]

I was looking to get this merged in to master today, and it looks like this introduces some bugs around the user_home/@my_home value.

I specifically merged the master branch in to this branch and tried to run the Ubuntu 16.04 Test Kitchen test (backed by Vagrant) chef exec kitchen converge 'lwrp.*1604'. It ends up with this stacktrace:

• https://gist.github.com/theckman/b07aa8da87ae94331f8e6c64823409eb

I've only looked at it for a few moments, but I think the issue is only when a user_account resource is called without specifying the home directory. Might we need to provide a default value in user_home instead of returning nil?

[theckman]

It might be a good idea to squash the commits of this branch in to a single commit detailing the changes made. We can then rebase that commit against master.

[chr4]

The problem is that when creating a user, the homedir is not existent yet. Therefore @my_home is set to nil when the provider is invoked.
I've added a commit to update @my_home after the user was created.

The lwrp-ubuntu-1604 test converges now.

Note: I've also rebased this onto master.

Please confirm that you want all commits sqashed into one. In my opinion it might be useful to have them seperated. If it makes sense to you, I'll go ahead and squash them, though.

Thanks for reviewing!

[theckman]

@chr4 Sorry for sucking so bad as a maintainer with this change. Personal-life stuff that's now thankfully cleared up. Let me merge this and get a release cut for you ASAP.

[theckman]

@chr4 v0.7.0 is out on the Supermarket.

[chr4]

:) Thanks!

Now only #65 is missing, then I can abadon my fork :)