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:

Bug #1531

ssh_authorized_keys should not use the key 'comment' as a unique identifier (name)

Added by Paul Boven over 7 years ago. Updated almost 2 years ago.

Status:AcceptedStart date:08/25/2008
Priority:NormalDue date:
Assignee:Francois Deppierraz% Done:

0%

Category:-
Target version:-
Affected Puppet version:0.24.4 Branch:
Keywords:

We've Moved!

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

This ticket is now tracked at: https://tickets.puppetlabs.com/browse/PUP-2621


Description

Currently the ssh authorized keys provider uses the ‘comment’ section from an SSH public key as the ‘name’. However, this implies that these comment strings must be unique, while SSH itself imposes no such restriction: in fact, it often happens that users generate both an RSA and a DSA key, which by default will have the same comment.

A better ‘name’ for a key would perhaps be its fingerprint. There is a very small chance of collisions, but using the comment as ‘name’ is certain to generate collisions (for me it already has). Otherwise, the key-string itself should perhaps be the ‘name’ as this is certainly unique.

If a user just changes the ‘name’ of the key in the Puppet manifest, then the other problem is that Puppet (only looking at the ‘name’, not the contents of the key) fails to realize that a key is already in place so you end up with duplicates. The current implementation doesn’t really manage authorized_keys, it only manages the comment section and has no knowledge of the actual key. Using the key fingerprint would require Puppet to be able to actually extract the fingerprint from the key and would be a non-trivial change.


Related issues

Related to Puppet - Bug #2812: ssh_authorized_key fails if existing non-puppet installed... Closed 11/13/2009
Related to Puppet - Bug #2192: ssh_authorized_key barfs on missing comment in existing k... Accepted 04/24/2009
Related to Puppet - Bug #3872: ssh_authorized_key intended behaviour? Needs More Information 05/25/2010
Related to Puppet - Bug #7009: Puppet ssh_authorized_keys fails on one account if key wi... Accepted 04/07/2011
Duplicated by Puppet - Refactor #1644: ssh_authorized_keys needs to be completely refactored Duplicate 10/10/2008
Duplicated by Puppet - Bug #1753: ssh_authorized_key chokes on keys without a comment Duplicate 11/17/2008
Blocked by Puppet - Feature #1621: Composite resource identifier Closed 09/29/2008

History

#1 Updated by Francois Deppierraz over 7 years ago

Yes, I agree that using the comment as identifier is a bad choice.

But using only the fingerprint is equally bad, because the same key can be installed into multiple accounts on the same node.

We should definitely be using fingerprint+target(user) as identifier, but I’m not sure Puppet supports composite identifiers like that. Maybe one of the PuppetMasters can help me on this one ?

#2 Updated by James Turnbull over 7 years ago

  • Status changed from Unreviewed to Accepted
  • Assignee set to Francois Deppierraz

#3 Updated by Ryan Steele over 7 years ago

I think we should have a separate ‘comment’ attribute. It would prevent behavior like this if we want to use the hash, fingerprint, or a composite key:

DEFINITION define config_user_keys ( $user, $type, $ensure = present ) { ssh_authorized_key { $name:

  ensure  => $ensure,
  user    => $user,
  target  => "/home/$user/.ssh/authorized_keys",
  key     => $name,
  type    => $type,

} }

CLASS config_user_keys { “1234123412341234== testkey”:

 user    => "test",
 type    => "rsa",

}

RESULT $ tail -1 /home/test/.ssh/authorized_keys ssh-rsa 1234123412341234== testkey 1234123412341234== testkey

Also, maybe we could have the ‘target’ attribute support an array – then we wouldn’t need to create a composite key, or multiple resources for the same key (only differing by the target aspect of the composite key). Instead, the single resource could ensure the key was in all the targets, and we’d still have the benefit of a unique identifier in the key hash itself.

#4 Updated by Ryan Steele over 7 years ago

I had a conversation with Luke on this topic, included below. Essentially, we both feel that having the ‘target’ attribute support arrays is the easiest fix for this, as it won’t require composite keys, won’t require 50 resources for 50 different authorized_keys files (only ONE, a huge savings), and code-wise should be much simpler to implement. The other important thing to take away from this is the addition of a ‘comment’ attribute. The module could/should default to the empty string (or false) if not present, assign the comment from the end of key if present, or the ability to be manually assigned. Given this, both Luke and I feel the blocking by #1621 should be removed.

 lak: Don't suppose you've had a chance to take a gander at http://projects.reductivelabs.com/issues/show/1531 at all today? If not, no worries.
            what would the name of the key be if it's not the comment?
 lak: The key hash itself.
 The comment is way too likely to cause collisions.
 The key itself will almost always be unique.
            rgsteele||work: why would the comment have collisions?
            and if so, why not just use a comment w/out them?
 For example, if you create a dsa and rsa key at the same time, they'll both have the same comment.
 It seems to me that the easy way around this is to have the key be the unique name, instead of trying to dream up comment naming schemes like "dan_rootkey_somebox", "dan_dankey_somebox", "dan_dankey_someotherbox".
 And just provide an array of 'target' files.
 This way, you can have multiple files for one key, without elaborate naming schemes.
 And, you don't have to worry about a composite key.
 lak: It seems the advantages here over composite keys is that it's simpler to implement, still allows you to have multiple key files per key, and does so using a single resource instead of many.
            if you have multiple keys per comment, then the solution should be to refactor the type to support that, i would think
            but maybe not
 lak: Well, this solution does allow multiple keys per comment, without a huge code overhaul. All that really needs to happen is for the 'target' attribute to support arrays, and behind the scenes, the ssh_authorized_key type could assign the comment to a 'comment' attribute, instead of the current (admittedly poorly) method of assigning it to 'name'.
 Maybe it's just me, but if the user's key belongs in 50 authorized_keys files, it makes way more sense to have one resource with an array of 50 targets, than 50 wholly separate resources.
 Especially if they accomplish the same thing.
            rgsteele||work: i agree that target should support arrays
 lak: Then, in that case, can we remove the blocking on #1531 by the ticket assigned to composite keys?
 This way, we can hopefully get it out sooner, since the implementation is simpler.
            can you open a different ticket for the target being an array?
            that's essentially orthogonal to what the namevar is
 lak: I'm still not sure we really need composite keys to solve #1531. The key alone is all we really need if target supports arrays.
 It really makes the big chunk of that ticket moot.
             i guess you're right
             the key still really feels like an attribute rather than the namevar, but i guess, generally, it's unique in a given file
 lak: I argued that point too, but it didn't go over well. However, the reason I didn't fight it harder is that given the declarative nature of Puppet, you really need a unique identifier. It's not really practical to have the username be the identifier and have arrays of keys, comments, types, because then you have to deal with mapping them properly and it'd get really ugly.
 It is ugly to have such a long hex string be the resource title, but I think it's probably the best option. It sucks in that you have to have multiple blocks for each user (one for each key they have), but I can't really find a good implementation in my mind where the user is the resource title, and that single resource contains all the keys, comments, etc. (even though that may be a more logical grouping)

#5 Updated by Ryan Steele over 7 years ago

After several conversations, Francois and I have agreed that having ‘user’ and ‘target’ accept arrays is the right move. However, the ParsedFile provider does not support modifying multiple files. I spoke with Luke about this, who advised that ParsedFile#prefetch needs to be modified so that it doesn’t assume only one resource per record. Essentially, we need to have that method gather a list of target symbols, and pass that list to self.class.prefetch (instead of the single :name symbol for a lone resource as it does now), which already has support for multiple targets. We’ll probably (maybe?) also have to mess with ParsedFile#flush to accommodate this change. It’s a bit over my head right now as I’m still learning Ruby, but that should get us most of the way there.

#6 Updated by Jesse Wolfe almost 6 years ago

  • Target version set to 2.7.x

#7 Updated by Lukas Hetzenecker over 3 years ago

I tried to work around these issues in a module. It can be found here: https://groups.google.com/d/topic/puppet-users/MfaZEwj4sEc/discussion

The code is located on Github: https://github.com/lukas-hetzenecker/puppet-module-ssh_authorized_key

This module uses the fingerprint as key and allows you to use an array for the user and target attributes. Each target is parsed in an own Fileparser.

#8 Updated by Anonymous over 3 years ago

  • Target version deleted (2.7.x)

#9 Updated by Paul Tötterman almost 2 years ago

Redmine Issue #1531 has been migrated to JIRA:

https://tickets.puppetlabs.com/browse/PUP-2621

Also available in: Atom PDF