The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using https://tickets.puppetlabs.com. See the following page for information on filing tickets with JIRA:

Refactor #1644

ssh_authorized_keys needs to be completely refactored

Added by Ryan Steele over 7 years ago. Updated about 5 years ago.

Status:DuplicateStart date:10/10/2008
Priority:NormalDue date:
Assignee:Francois Deppierraz% Done:

0%

Category:ssh
Target version:-
Affected Puppet version:0.24.6 Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com


Description

The current implementation of ssh_authorized_keys seems to me to have several glaring design flaws. Take, for example, this following sample use case, as demonstrated by bartc from the IRC channel on Freenode:

$ cat test.pp

!/usr/bin/env puppet

ssh_authorized_key {

"test":
    user    => "bart",
    key     => "foo",
    type    => "ssh-rsa";
"bla":
    user    => "bart",
    key     => "bla",
    type    => "ssh-rsa";

} $ ./test.pp notice: //Ssh_authorized_key[test]/ensure: created notice: //Ssh_authorized_key[bla]/ensure: created $ tail -2 /home/bart/.ssh/authorized_keys ssh-rsa foo test ssh-rsa bla bla

For one, the resource title is used as the comment (and, I see no obvious way to change that). So, if I wanted to use something unique such as the key itself as the resource title, I’d end up with two separate copies of the key on the same line – once for the key, and again for the comment. Additionally, there is no way to add the same key to multiple files, unless I start coming up with creative naming schemes like “jsmith_rootkey_somehost”, “jsmith_jsmithkey_somehost”, “jsmith_jsmithkey_someotherhost”, etc. It seems that a workaround like that is just a band-aid for a very limited mechanism.

In my opinion, the proper design would be to have ONE ssh_authorized_key resource per user, and that you should be able to provide an array for both the “target” and “key” attributes. This way, all the user’s specified keys would be added to all the specified authorized_keys file for the host in question. In it’s current state, ssh_authorized_keys offers me only a fraction of the functionality needed to satisfy what I believe are normal use cases.


Related issues

Duplicates Puppet - Bug #1531: ssh_authorized_keys should not use the key 'comment' as a... Accepted 08/25/2008

History

#1 Updated by James Turnbull over 7 years ago

  • Category set to ssh
  • Status changed from Unreviewed to Needs Decision
  • Assignee set to Francois Deppierraz
  • Priority changed from High to Normal
  • Target version set to 4

#2 Updated by Peter Meier over 7 years ago

[…] unless I start coming up with creative naming schemes like “jsmith_rootkey_somehost”, “jsmith_jsmithkey_somehost”, “jsmith_jsmithkey_someotherhost”, etc. It seems that a workaround like that is just a band-aid for a very limited mechanism.

this can be done by a define and an inheritance chain. but yeah it is hackish

In my opinion, the proper design would be to have ONE ssh_authorized_key resource per user, and that you should be able to provide an array for both the “target” and “key” attributes.

I’d rather like to have one resource per key, as one key is one resource I’d like to manage, but that doesn’t conflict with your idea.

This way, all the user’s specified keys would be added to all the specified authorized_keys file for the host in question. In it’s current state, ssh_authorized_keys offers me only a fraction of the functionality needed to satisfy what I believe are normal use cases.

I think the way to go is to have #1621 being fixed and then we have a lot more possibilities to treat with this problem. As every resource needs an unique identifier it was imho the best solution to take the name variable as a comment.

#3 Updated by Francois Deppierraz over 7 years ago

  • Status changed from Needs Decision to Duplicate

rgsteele wrote:

For one, the resource title is used as the comment (and, I see no obvious way to change that). So, if I wanted to use something unique such as the key itself as the resource title, I’d end up with two separate copies of the key on the same line – once for the key, and again for the comment. Additionally, there is no way to add the same key to multiple files, unless I start coming up with creative naming schemes like “jsmith_rootkey_somehost”, “jsmith_jsmithkey_somehost”, “jsmith_jsmithkey_someotherhost”, etc. It seems that a workaround like that is just a band-aid for a very limited mechanism.

I agree but this point is definitely a duplicate of bug #1531.

In my opinion, the proper design would be to have ONE ssh_authorized_key resource per user, and that you should be able to provide an array for both the “target” and “key” attributes. This way, all the user’s specified keys would be added to all the specified authorized_keys file for the host in question. In it’s current state, ssh_authorized_keys offers me only a fraction of the functionality needed to satisfy what I believe are normal use cases.

I cannot agree on this one, it is IMHO necessary to be able to manage each key independently. If you want to manage the authorized_key file as a single resource, why not use the file type ?

#4 Updated by Ryan Steele over 7 years ago

ctrlaltdel wrote:

rgsteele wrote:

For one, the resource title is used as the comment (and, I see no obvious way to change that). So, if I wanted to use something unique such as the key itself as the resource title, I’d end up with two separate copies of the key on the same line – once for the key, and again for the comment. Additionally, there is no way to add the same key to multiple files, unless I start coming up with creative naming schemes like “jsmith_rootkey_somehost”, “jsmith_jsmithkey_somehost”, “jsmith_jsmithkey_someotherhost”, etc. It seems that a workaround like that is just a band-aid for a very limited mechanism.

I agree but this point is definitely a duplicate of bug #1531.

Agreed, sorry I missed that. I’ve taken this point to that ticket, and made some design suggestions on that point there.

In my opinion, the proper design would be to have ONE ssh_authorized_key resource per user, and that you should be able to provide an array for both the “target” and “key” attributes. This way, all the user’s specified keys would be added to all the specified authorized_keys file for the host in question. In it’s current state, ssh_authorized_keys offers me only a fraction of the functionality needed to satisfy what I believe are normal use cases.

I cannot agree on this one, it is IMHO necessary to be able to manage each key independently. If you want to manage the authorized_key file as a single resource, why not use the file type ?

I don’t want to use the ‘file’ type to manage the authorized_keys file as a whole because I don’t want to have to manage many authorized_keys files per server for every server I manage; in that regard, managing the keys and targets associated with a user is much less cumbersome.

And on one side of the coin, I think that making the key fingerprint (or they key itself) the unique resource makes it harder to identify who it belongs to, and makes the manifests much more verbose when compared to having a single resource per user. If I could instead specify one ssh_authorized_key resource per user, it would be easy for me to see all the keys, types, and targets associated with that user all in one block, instead of many independent blocks (with a lot of redundant information, such as ‘user’, ‘ensure’, ‘comment’, etc).

But on the flip side of the coin, I submit that it would be hard to associate a ‘type’, ‘ensure’, and other key-specific attributes to keys contained in an array, which lends strength to your argument that all keys must be managed independently. So maybe we do in fact have to just live with providing some redundant information if it makes more sense in the big picture.

All that being said, I think this ticket can probably be closed. Although I think having one ssh_authorized_key resource per user would make the configs more concise and logically grouped, in the design sense it might not be the right way to go, and difficult to achieve. And, I’ve taken my first point to #1531.

#5 Updated by Francois Deppierraz over 7 years ago

rgsteele wrote:

All that being said, I think this ticket can probably be closed. Although I think having one ssh_authorized_key resource per user would make the configs more concise and logically grouped, in the design sense it might not be the right way to go, and difficult to achieve. And, I’ve taken my first point to #1531.

Maybe you can simplify your config using a custom type like that.

define my_user($ensure, $ssh_key, $ssh_key_type) {
  user{$name:
    ensure => $ensure,
  }

  ssh_authorized_key{$name,
    ensure => $ensure,
    type   => $ssh_key_type,
    key    => $ssh_key,
    user   => $name,
  }
}

which allows you to write only that for each user.

my_user{"johndoe":
  ensure => present,
  ssh_key_type => "rsa",
  ssh_key => "AAAA....",
}

#6 Updated by Ryan Steele over 7 years ago

That idea doesn’t really work, because it fails to account for the very common case in which you have a variable number of keys per user. It’d be a nice workaround if I could keep the users' keys in the files area under something like files/keys/username, and have Puppet iterate over each file in there, calling ssh_authorized_key on each of those, but it would require both the ability to somehow iterate over each file in the directory and open them up and the ability to parse the keyfiles and separate out the type, key, and comment.

I’m not really sure how to go about tackling that with Puppet, or if I’m even barking up the right tree. It might not be possible with the current design to really automate setting up an arbitrary number of keys for a given user.

#7 Updated by Peter Meier over 7 years ago

rgsteele wrote:

That idea doesn’t really work, because it fails to account for the very common case in which you have a variable number of keys per user. It’d be a nice workaround if I could keep the users' keys in the files area under something like files/keys/username, and have Puppet iterate over each file in there, calling ssh_authorized_key on each of those, but it would require both the ability to somehow iterate over each file in the directory and open them up and the ability to parse the keyfiles and separate out the type, key, and comment.

I’m not really sure how to go about tackling that with Puppet, or if I’m even barking up the right tree. It might not be possible with the current design to really automate setting up an arbitrary number of keys for a given user.

if you don’t mind putting keys in a file, then a clever design of fileresource handling with symlinking (linkfollow option for file type) and a type putting the files together (like the concatenate custom type of DavidS custom module) might be the more approriate solution. but somehow this discussion should be rather on the list.

#8 Updated by Ryan Steele over 7 years ago

immerda wrote:

if you don’t mind putting keys in a file, then a clever design of fileresource handling with symlinking (linkfollow option for file type) and a type putting the files together (like the concatenate custom type of DavidS custom module) might be the more approriate solution. but somehow this discussion should be rather on the list.

Somehow I don’t think that such an arcane methodology is the right one, since I’d have to discard that method in favor of the more supported method that will result from #1531. I think my proposed solution in #1531 might be a better solution than #1621, but I’ll really have to wait and see what Luke and Francois think about it.

#9 Updated by Tim Harper over 7 years ago

  • Affected Puppet version set to 0.24.6

I ran into this problem with this command as well.

I had something like this:

define timcharper_key($ensure = present) { ssh_authorized_key { “$title-timcharper_key”:

user   => $title,
ensure => $ensure,
name   => "timcharper@localhost",
type   => "ssh-rsa",
key    => "AAAAB31nsa3ja/..."

} }

And then, for the node:

node “server” { timcharper_key { “timcharper”: } timcharper_key { “deploy”: } }

I was getting an error here. Even though the title was unique (“timcharper-timcharper_key” and “deploy-timcharper_key”), it was failing because the name was not unique “timcharper@localhost”. A bit surprising!

To fix it, I removed the name attribute, so the comments are being set as “timcharper-timcharper_key” and “deploy-timcharper_key”:

define timcharper_key($ensure = present) { ssh_authorized_key { “$title-timcharper_key”:

user   => $title,
ensure => $ensure,
name   => "timcharper@localhost",
type   => "ssh-rsa",
key    => "AAAAB31nsa3ja/..."

} }

Not a huge deal… but it was surprising and felt like an obstacle that need not exist.

#10 Updated by Francois Deppierraz over 7 years ago

timcharper wrote:

I was getting an error here. Even though the title was unique (“timcharper-timcharper_key” and “deploy-timcharper_key”), it was failing because the name was not unique “timcharper@localhost”. A bit surprising!

The name attribute of all resources of a given type must always be unique in puppet. This is not specific to this type. However, we can debate about whether the comment should be used as name (see Bug #1531).

#11 Updated by James Turnbull about 5 years ago

  • Target version deleted (4)

Also available in: Atom PDF