Merge ~smoser/cloud-init:fix/1781094-ssh-deletekeys into cloud-init:master

[Scott Moser]

redhat: remove ssh keys on new instance.

This changes redhat's default behavior to remove the ssh keys on
new instance (ssh_deletekeys will now be at its default true value).

On redhat systems, cloud-init.service has both:
  Wants=sshd-keygen.service
  Before=sshd-keygen.serviceh

Which is why 'ssh_genkeytypes' is set to None
(yaml '~' == yaml null == python none).

I've changed that to be null as it seems more clear and we do not
use the tilda anywhere else in configs.

LP: #1781094
rhbz: https://bugzilla.redhat.com/show_bug.cgi?id=1598832

see commit message

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:cfcd65906982f8df63bf64e182ec75ad94c75990
https://jenkins.ubuntu.com/server/job/cloud-init-ci/156/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/156/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Is it worth leaving a reference to this LP: # in the both the systemd unit file and with the genkeytypes?

ssh_genkeytypes: null # LP: #XXXXXXXX

[Scott Moser (smoser)]

I do not think we should leave this line in the file.
Thats confusing. No other distro's cloud.cfg would
have such a line.

[Ryan Harper (raharper)]

On Wed, Aug 1, 2018 at 12:09 PM Scott Moser <email address hidden> wrote:
>
> I do not think we should leave this line in the file.
> Thats confusing. No other distro's cloud.cfg would
> have such a line.
>
>
> Diff comments:
>
> > diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
> > index 5619de3..1fef133 100644
> > --- a/config/cloud.cfg.tmpl
> > +++ b/config/cloud.cfg.tmpl
> > @@ -24,8 +24,6 @@ disable_root: true
> > {% if variant in ["centos", "fedora", "rhel"] %}
> > mount_default_fields: [~, ~, 'auto', 'defaults,nofail', '0', '2']
> > resize_rootfs_tmp: /dev
> > -ssh_deletekeys: 0
> > -ssh_genkeytypes: ~
>
> if we left it as null, it would not generate any keys.
> We need it to generate keys.

Why? isn't ssh-keygen service generating keys?

>
> You are correct in that it will generate the default key
> types for cloud-init, which will not necessarily be the
> same as those in ssh-keygen service.
>
> However, cloud-init will *remove* all ssh host keys that match
> /etc/ssh/ssh_host_*key*
> So we wont' have stale keys sitting around.

Hrm, is the goal then to to have cloud-init wipe *all* keys even if it didn't
generate them on new-instance? That would include any keys generated
by the ssh-keygen service? And if so, is that OK for cloud-init to do?

And from above, if ssh-keygen service is already creating keys, should
cloud-init create keys as well?

[Scott Moser (smoser)]

> On Wed, Aug 1, 2018 at 12:09 PM Scott Moser <email address hidden> wrote:
> >
> > if we left it as null, it would not generate any keys.
> > We need it to generate keys.
>
> Why? isn't ssh-keygen service generating keys?

That is the bug. We/centos/fedora *were* delegating generation of host keys to ssh-keygen. That is problematic as ssh-keygen only generates new keys if keys do not previously exist. That means that if the keys ever exist in an image, every instance from that image will have the same host keys.

> > You are correct in that it will generate the default key
> > types for cloud-init, which will not necessarily be the
> > same as those in ssh-keygen service.
> >
> > However, cloud-init will *remove* all ssh host keys that match
> > /etc/ssh/ssh_host_*key*
> > So we wont' have stale keys sitting around.
>
> Hrm, is the goal then to to have cloud-init wipe *all* keys even if it didn't
> generate them on new-instance? That would include any keys generated
> by the ssh-keygen service? And if so, is that OK for cloud-init to do?

Deleting ssh host keys is somethign that cloud-init has *always* done on first instance, so... yes it is OK for cloud-init to do that. It is possible that someone has ssh host keys that they want to be used for multiple instances.. and cloud-init would stop them. However, that is generally against the design intent of ssh host keys, which are supposed to be per-host.

>
> And from above, if ssh-keygen service is already creating keys, should
> cloud-init create keys as well?

on a clean instance boot (no host keys at all):
 * cloud-init.service will run 'Before=sshd-keygen.service' and will create new host keys.
 * ssh-keygen service will run and see it doesn't have anything to do.

On second boot, cloud-init.service will not re-generate ssh host keys because those are generated on a per-instance basis in cloud-init.

On a *dirty* instance first boot (with previously generated ssh host keys)
 * cloud-init.service will run 'Before=sshd-keygen.service' and will
   * delete the old host keys
   * generate host keys
 * ssh-keygen.service will have nothing to do.

[Ryan Harper (raharper)]

On Thu, Aug 2, 2018 at 11:25 AM Scott Moser <email address hidden> wrote:
>
> > On Wed, Aug 1, 2018 at 12:09 PM Scott Moser <email address hidden> wrote:
> > >
> > > if we left it as null, it would not generate any keys.
> > > We need it to generate keys.
> >
> > Why? isn't ssh-keygen service generating keys?
>
> That is the bug. We/centos/fedora *were* delegating generation of host keys to ssh-keygen. That is problematic as ssh-keygen only generates new keys if keys do not previously exist. That means that if the keys ever exist in an image, every instance from that image will have the same host keys.

Hrm, it sort of feels like we should conflict with ssh-keygen;

>
>
> > > You are correct in that it will generate the default key
> > > types for cloud-init, which will not necessarily be the
> > > same as those in ssh-keygen service.
> > >
> > > However, cloud-init will *remove* all ssh host keys that match
> > > /etc/ssh/ssh_host_*key*
> > > So we wont' have stale keys sitting around.
> >
> > Hrm, is the goal then to to have cloud-init wipe *all* keys even if it didn't
> > generate them on new-instance? That would include any keys generated
> > by the ssh-keygen service? And if so, is that OK for cloud-init to do?
>
> Deleting ssh host keys is somethign that cloud-init has *always* done on first instance, so... yes it is OK for cloud-init to do that. It is possible that someone has ssh host keys that they want to be used for multiple instances.. and cloud-init would stop them. However, that is generally against the design intent of ssh host keys, which are supposed to be per-host.
>
> >
> > And from above, if ssh-keygen service is already creating keys, should
> > cloud-init create keys as well?
>
> on a clean instance boot (no host keys at all):
> * cloud-init.service will run 'Before=sshd-keygen.service' and will create new host keys.
> * ssh-keygen service will run and see it doesn't have anything to do.
>
> On second boot, cloud-init.service will not re-generate ssh host keys because those are generated on a per-instance basis in cloud-init.
>
> On a *dirty* instance first boot (with previously generated ssh host keys)
> * cloud-init.service will run 'Before=sshd-keygen.service' and will
> * delete the old host keys
> * generate host keys
> * ssh-keygen.service will have nothing to do.

Right, that seemed odd to me; why bother running it at all then if
cloud-init is present and handling keygen?
should we conflict?

>
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/349359
> Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/1781094-ssh-deletekeys into cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp

[Scott Moser (smoser)]

I'd really rather not conflict and play a game of roulette with systemd.

[Ryan Harper (raharper)]

On Fri, Aug 3, 2018 at 1:44 PM Scott Moser <email address hidden> wrote:
>
> I'd really rather not conflict and play a game of roulette with systemd.

Can we get a RH/Fedora person to weigh in on this then?

>
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/349359
> Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/1781094-ssh-deletekeys into cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp

[Scott Moser (smoser)]

I really don't know what you're expecting to hear.
cloud-init is guaranteed to run Before ssh-keygen.
Worst case,
 a.) ssh-keygen creates keys that cloud-init didn't write (but since cloud-init deleted *all* keys on new-instance, this actually works pretty well).
 b.) ssh-kegen does nothing and wastes some 'stat' of files that it would have executed.

I guess we could drop-in a 'disable' of keygen-service on package install. that just seems like more work than necessary.

[Ryan Harper (raharper)]

On Fri, Aug 3, 2018 at 3:24 PM Scott Moser <email address hidden> wrote:
>
> I really don't know what you're expecting to hear.

The distro put in ssh-keygen service and presumably has a reason for
doing so. They may have some expected properties of the types of keys
that it generates and cloud-init could generate different keys than
what the distro expected.

So, for images in which there are both cloud-init and ssh-keygen
service, I'd like the distro to make a configuration choice that
reflects the intention of the image builder.

> cloud-init is guaranteed to run Before ssh-keygen.
> Worst case,
> a.) ssh-keygen creates keys that cloud-init didn't write (but since cloud-init deleted *all* keys on new-instance, this actually works pretty well).
> b.) ssh-kegen does nothing and wastes some 'stat' of files that it would have executed.
>
> I guess we could drop-in a 'disable' of keygen-service on package install. that just seems like more work than necessary.

Can you imagine a scenario where having both cloud-init and ssh-keygen
manage keys is complementary (neither a or b look complementary
without further work to understand who created which keys and who is
control of when they get deleted)? If not, then why shouldn't we just
conflict ?

>
> --
> https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/349359
> Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:fix/1781094-ssh-deletekeys into cloud-init:master.
>
> _______________________________________________
> Mailing list: https://launchpad.net/~cloud-init-dev
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~cloud-init-dev
> More help : https://help.launchpad.net/ListHelp

[Ryan Harper (raharper)]

After some further discussion I think it's OK to expect that an image that has cloud-init will regenerate and remove ssh keys (on new instance) this is default cloud-init behavior.

I do think we should Conflict, but I also understand not wanting to change systems which have ssh-keygen which is currently running to a state where it's not run and users being unaware that cloud-init was handling that for them.

[Ryan Harper (raharper)]

review: Approve

diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
index 5619de3..1fef133 100644
--- a/config/cloud.cfg.tmpl
+++ b/config/cloud.cfg.tmpl
@@ -24,8 +24,6 @@ disable_root: true
 {% if variant in ["centos", "fedora", "rhel"] %}
 mount_default_fields: [~, ~, 'auto', 'defaults,nofail', '0', '2']
 resize_rootfs_tmp: /dev
-ssh_deletekeys:   0
-ssh_genkeytypes:  ~
 ssh_pwauth:   0
 
 {% endif %}