New cookbook: fb_ssh

[jaymzh]

This is a cookbook to manage SSH using the FB attribute-driven model.

It handles daemon and client configs as well as authorized_keys and
authorized_principals.

There is something worth noting here: the API for public keys uses a data_bag.
Grocery delivery and taste-tester have long supported data_bags even though
FB doesn't use them in prod... but it doesn't matter since FB uses principals in prod
and not keys, so this shouldn't be an issue no matter what.

Here are all the steps to do, internally, to be able to import this.

1. Copy all node.default['fb_ssh']... to also have a node.default['fb_ssh_old']...
2. Add code handle reads, and code-replace it
3. Copy fb_ssh to fb_ssh_old
4. Fix roles and include_recipes to reference fb_ssh_old
5. Delete references to fb_ssh
6. Delete fb_ssh
7. Import new fb_ssh

[davide125]

I think these should probably be configurable (not everybody will have/want the same paths).

[jaymzh]

I disagree... one of the things we've almost never done in fb cookbooks is make configuration paths configurable....

[jaymzh]

rebased and addressed review comments

[jaymzh]

Added windows support

[ericnorris]

Hey there @jaymzh (and @davide125)! Apologies for resurrecting this PR, but I wanted to a) point out what I think is a bug I found, and b) ask if there was any chance this could get merged, as we'd like to use it!

Regarding the bug, I believe that the current default of joining arrays by spaces does not work for the HostKey directive. Here is a snippet of my fb_ssh attributes:

node.default['fb_ssh']['sshd_config'] = {
  'HostKey' => [
    '/etc/ssh/ssh_host_rsa_key',
    '/etc/ssh/ssh_host_ecdsa_key',
    '/etc/ssh/ssh_host_ed25519_key',
  ],
  # ...
}

Using this results in the following error:

FATAL: Chef::Exceptions::ValidationFailed: template[/etc/ssh/sshd_config] (fb_ssh::default line 92) had an error: Chef::Exceptions::ValidationFailed: Proposed content for /etc/ssh/sshd_config failed verification /usr/sbin/sshd -t -f %{path}
STDOUT:
STDERR: /etc/ssh/.chef-sshd_config20220302-2453-dz99m0 line 6: garbage at end of line; "/etc/ssh/ssh_host_ecdsa_key".

I'm not familiar with openssh's codebase, but I believe this is because the HostKey option does not support whitespace-separated arguments. At the time of writing, here is an example of how openssh parses the AcceptEnvoption, which does allow whitespace-separated arguments:

https://github.com/openssh/openssh-portable/blob/379b30120da53d7c84aa8299c26b18c51c2a0dac/servconf.c#L2016

...and here is how it parses HostKey:

https://github.com/openssh/openssh-portable/blob/379b30120da53d7c84aa8299c26b18c51c2a0dac/servconf.c#L1431

If I understand correctly, it's only taking one argument at a time from the HostKey option, vs. parsing them all in the AcceptEnv option. I think that we currently want the default HostKey option, so we can remove the attribute for now, but it'd be nice if this worked.

Finally, regarding merging, at Etsy we've been exploring using Facebook-style cookbooks for freshening up our Chef configuration, and found it really pleasant so far! We'd like to be able to customize our sshd config this way as well, so merging this would allow us to rely on the upstream repo.

Thanks both!

[jaymzh]

Hi @ericnorris - thanks for commenting.

The delay on merging this is because FB has an internal fb_ssh cookbook, so it's a migration for them, and resources on that team are currently a bit limited.

I'm happy to continue keeping this up-to-date in the PR though, and the Vicarious folks are already using it (I just left there, but I wrote it while I was there). FB will get to it, but it'll take some time. I'm hoping to update a bunch of my PRs that submit new cookbooks in the coming weeks now that I have a bit more time.

The HostKey is an interesting one. I think there are only two config keys that require multi-define - HostKey is one, ListenAddress is the other. Perhaps the best solution is to just hard-code those two cases... I can see no way to handle both cases the same and have them be parsed correctly by sshd.

[ericnorris]

Appreciate the fast response @jaymzh! Understood, we have a mechanism for pulling in your fork for a given cookbook (e.g. this and fb_sssd) at a specific commit, so we can keep using that for the time being.

Also appreciate you offering to keep it up to date! I agree, HostKey is interesting. I think your solution makes sense, and I'll add that I think another approach could be to always multi-define (e.g. even for AcceptEnv)? The config would be more verbose, though. I'm fine with either option.

Finally, I'd be curious to see if there's an fb_pamd cookbook that could make its way to this repository, but now I fear I'm asking too much :)

[davide125]

If you'd like to contribute a cookbook to manage PAM, we should be able to merge it without issues. As @jaymzh mentioned, the main blocker to merging this one is refactoring our internal one and migrating off it.

[jaymzh]

I don't think multi-defining works for anything else. For example AuthorizedKeysFile needs space-delimited options. Or consider AuthenticationMethods where you do space-delimited array of comma-delimited options! ;)

So I don't think there's a format that works for everything, unfortunately.

[jaymzh]

I'm gonna get this updated here. I have a solution for the non-space-delimited things, but there's some systemd new stuff that's required to get in here... should have an update today or tomorrow.

[jaymzh]

OK this update properly handles special non-space-delimited keys as well as handles config files that are included from packages.

@ericnorris - would love any feedback or testing.

out of date