[MODULES-9726] allow read-only authorized_keys Created: 2019/08/14  Updated: 2020/03/17

Status: Accepted
Project: Modules
Component/s: sshkeys_core
Affects Version/s: None
Fix Version/s: None

Type: New Feature Priority: Major
Reporter: Antoine Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Team: Night's Watch
Sprint: PR - Triage, PR - Triage
QA Risk Assessment: Needs Assessment

 Description   

Module Version: N/A
Puppet Version: Puppet 4.8.2
OS Name/Version: Debian stretch

We have a security policy which says SSH `authorized_keys` files should not be writable by users, so that those keys are solely under the control of admins (and therefore Puppet).

Unfortunately, the way the `ssh_authorized_key` type operates now is that it hardcodes the mode (`0600`) of the file and also the owner (whatever the `user` selected). So an operator has two choices, either:

  • make the file owned by the user, in which case authentication works but the keys are modifiable by the user, or;
  • make the file owned by `root`, in which case the file is not writable by the user but authentication then fails because it's not readable either

Desired Behavior: It should be possible to have read-only SSH keys.

Actual Behavior: SSH keys are either read/write or unreadable.



 Comments   
Comment by Antoine [ 2019/08/14 ]

a workaround is to use a separate `File` resource to change the mode on the generated file. but then it's getting a bit ridiculous the amount of boilerplate one needs to add on top of the `Ssh_authorized_key` resource to make it do the right thing. For example, this is code from a live deployment I have here:

  Ssh_authorized_key <<| tag == 'roles::static_master' or tag == 'roles::static_source' |>>
  user { 'mirroradm':
    ensure         => present,
    purge_ssh_keys => '/etc/ssh/userkeys/mirroradm',
  }
  # work around for https://tickets.puppetlabs.com/browse/MODULES-9726
  file { '/etc/ssh/userkeys/mirroradm':
    owner => 'root',
    mode  => '0555',
  }

And elsewhere, I need to export the key as well:

  $ssh_key = $::ssh_keys_users['mirroradm']['id_rsa.pub']
  # this prefix is because Puppet's ssh_authorized_key can't
  # distinguish keys with the same comment, regardless of where
  # they're stored. that is, even if we use `name` or `title`:
  # https://tickets.puppetlabs.com/browse/MODULES-7610
  # https://tickets.puppetlabs.com/browse/MODULES-7604
  @@ssh_authorized_key { "static_source-${ssh_key['comment']}":
    tag     => 'roles::static_source',
    key     => $ssh_key['key'],
    type    => $ssh_key['type'],
    target  => '/etc/ssh/userkeys/mirroradm',
    user    => 'root',
    options => [
      "command=\"/usr/local/bin/staticsync-ssh-wrap ${::fqdn}\"",
      'no-port-forwarding',
      'no-X11-forwarding',
      'no-agent-forwarding',
      'no-user-rc',
      "from=\"${::ipaddress},${::ipaddress6}\"",
    ],
  }

That's a lot of code!

Compare this to how this would look with a simple `Concat` resource:

$ssh_key = $::ssh_keys_users['mirroradm']['id_rsa.pub']
@@concat::fragment { "$ssh_key['comment']":
  tag => 'roles::static_source',
 target  => '/etc/ssh/userkeys/mirroradm',
  content => "command=\"/usr/local/bin/staticsync-ssh-wrap ${::fqdn}\",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-user-rc,from=\"${::ipaddress},${::ipaddress6}\"" ${ssh_key['key']} ${ssh_key['comment']}"
}

and in the collector:

concat { '/etc/ssh/userkeys/mirroradm':
   owner => root,
   mode => '0555',
}
concat::fragment { 'header':
  target  => '/etc/ssh/userkeys/mirroradm',
  content => "# THIS FILE IS UNDER PUPPET CONTROL. DO NOT EDIT IT HERE.\n",
  order   => '00',
}
Concat::fragment <<| tag == 'roles::static_source' |>>

This seems so much simpler! It reuses a primitive a lot of people know, it will work as expected (ie. it will purge unmanaged entries automatically and have the specified mode) and is much easier to audit than the `ssh_authorized_key` code (which I can barely read).

So I'm really beginning to question the value of this type at all - why do people use this instead of `Concat`? Wouldn't it be easier to rewrite this resource with `Concat` and be done with it? Or is that not proper?

Generated at Sat Aug 08 17:52:03 PDT 2020 using Jira 8.5.2#805002-sha1:a66f9354b9e12ac788984e5d84669c903a370049.