Support configuring SSH host certificates.

[lungj]

Proposed Commit Message

Support configuring SSH host certificates.

Support configuring SSH host certificates.

Existing config writes keys to /etc/ssh after deleting files matching
a glob that includes certificate files. Since sshd looks for
certificates in the same directory as the keys, a host certificate
must be placed in this directory. This update enables the certificate's
contents to be specified along with the keys.

Test Steps

To test, specify a host certificate's contents using
KEYTYPE_certificate (e.g., rsa_certificate) next to the public and
private keys normally included in the ssh_keys section of the config
yaml file.

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any documentation accordingly

[TheRealFalcon]

Thanks for the contribution and signing the CLA! The code that is here looks good, but I have a couple of comments/questions around total functionality and testing.

I'm not very familiar with SSH certificates, but wouldn't this change also require a change to sshd_config? For this to work, I had to add:
HostCertificate /etc/ssh/ssh-server-cert.pub

Is there a case where this isn't required? If not, can you also update the sshd_config here as well. We already have helpers for reading and writing:
https://github.com/canonical/cloud-init/blob/master/cloudinit/ssh_util.py#L356

Additionally, we usually ask that changes add/update tests accordingly. We have our ssh unit tests here:
https://github.com/canonical/cloud-init/blob/master/cloudinit/config/tests/test_ssh.py
and integration tests here:
https://github.com/canonical/cloud-init/blob/master/tests/integration_tests/modules/test_ssh_keys_provided.py

Would you be able to add the tests for this functionality?

[lungj]

Thanks for the pointers! I'm new to cloud-init conventions (having just started using it last week) and I had a bootcmd to do the same thing. I see having it done automatically is better :) And this is also my first experience with SSH certificates so I'll try not to mess the code base up too much!

And, yes; I think I can add the tests. I'm not sure if there's anything extra I should do when generating the certificate. For example, should I just sign the certificate with a temporary random SSH signing authority and throw away the keys? Or should I include the original private keys for the CA in case someone wants to write something more complex using the same CA?

[lungj]

I added a test case to tests/integration_tests/modules/test_ssh_keys_provided.py but I was unable to run pytest/tox on that successfully (host running 18.04 with venvs for Python 3.5-3.7). I also don't see the relevant tests running in the Travis CI here. I was able to run tests in cloudinit/config/tests/test_ssh.py with pytest/tox but I didn't see any existing tests for file-writing of keys.

[TheRealFalcon]

And, yes; I think I can add the tests. I'm not sure if there's anything extra I should do when generating the certificate. For example, should I just sign the certificate with a temporary random SSH signing authority and throw away the keys? Or should I include the original private keys for the CA in case someone wants to write something more complex using the same CA?

For testing, I think what you've added and what I'm proposing will work. The onus is on the user to provide the correct keys and cert. We just need to make sure they go to the right place.

I added a test case to tests/integration_tests/modules/test_ssh_keys_provided.py but I was unable to run pytest/tox on that successfully (host running 18.04 with venvs for Python 3.5-3.7). I also don't see the relevant tests running in the Travis CI here.

Yeah, it looks like a change in one of our dependencies broke our integration tests on Friday. A change was merged this morning to pin that dependency to an older version, so if you rebase your branch, they should pass now. The travis job that ran these tests is this one:
https://travis-ci.com/github/canonical/cloud-init/jobs/439799988
but they all failed because of that dependency issue. I double-checked your change locally, and it worked for me.

I was able to run tests in cloudinit/config/tests/test_ssh.py with pytest/tox but I didn't see any existing tests for file-writing of keys.

This is a place we should have had pre-existing tests, but do not. You're correct that no existing tests exist for the block you modified. A test like this should cover both the keys and the certificate:

    @mock.patch(MODPATH + "ug_util.normalize_users_groups")
    @mock.patch(MODPATH + "util.write_file")
    def test_handle_ssh_keys_in_cfg(self, m_write_file, m_nug, m_setup_keys):
        cfg = {"ssh_keys": {}}

        expected_calls = []
        for key_type in GENERATE_KEY_NAMES:
            private_name = "{}_private".format(key_type)
            public_name = "{}_public".format(key_type)
            cert_name = "{}_certificate".format(key_type)

            # Actual key contents don"t have to be realistic
            private_value = "{}_PRIVATE_KEY".format(key_type)
            public_value = "{}_PUBLIC_KEY".format(key_type)
            cert_value = "{}_CERT_KEY".format(key_type)

            cfg["ssh_keys"][private_name] = private_value
            cfg["ssh_keys"][public_name] = public_value
            cfg["ssh_keys"][cert_name] = cert_value

            expected_calls.extend([
                mock.call(
                    '/etc/ssh/ssh_host_{}_key'.format(key_type),
                    private_value,
                    384
                ),
                mock.call(
                    '/etc/ssh/ssh_host_{}_key.pub'.format(key_type),
                    public_value,
                    384
                ),
                mock.call(
                    '/etc/ssh/ssh_host_{}_key-cert.pub'.format(key_type),
                    cert_value,
                    384
                ),
                mock.call(
                    '/etc/ssh/sshd_config',
                    ('HostCertificate /etc/ssh/ssh_host_{}_key-cert.pub'
                     '\n'.format(key_type)),
                    preserve_mode=True
                )
            ])

        m_nug.return_value = ([], {})
        cc_ssh.handle("name", cfg, self.tmp_cloud(distro='ubuntu'), LOG, None)

        for call_ in expected_calls:
            self.assertIn(call_, m_write_file.call_args_list)

If you add that along with the other changes suggested, then I think this PR should be good to go!

[TheRealFalcon]

Good catch with the existing sshd_config! Since the config is fetched through a helper, I think it'd be easiest to mock the helper rather than mocking open directly, though I do like how you accomplished that by opening /dev/null...I hadn't thought of that before. I added a suggestion that mocks the helper. With that, we can also get rid of the _empty_ssh_config function.

Thanks for the GENERATE_KEY_NAMES refactor too. I think that looks good.

[lungj]

Same patches; I Just rebased onto the latest commits in canonical/master.

[lungj]

@TheRealFalcon I guess you'll take it from here? Thanks for the review and tips :)

[TheRealFalcon]

@lungj Yes. We'll need one more approval from a project committer before we can get this merged, but you don't need to do anything to make that happen. Thanks for the contribution!

[blackboxsw]

+1 this looks good to me. Thanks for the doc updates and integration tests here.