Puppet users - Oct 2008 - Proposed refactoring of ssh_authorized_keys

Hey folks,

Over the past day or so, I''ve been working a lot with the
ssh_authorized_keys module, but found what I believe to be several
design flaws.  I''d be interested to see if anybody else feels bound by
the same limitations and lack of support for what I believe to be
common use cases.  I''ve filed a refactoring ticket, which can be
viewed at http://projects.reductivelabs.com/issues/show/1644.  I''d be
interested to hear any feedback, or any other use cases which I
haven''t encountered for which the community feels there should be
support.

Respectfully,
Ryan

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com
To unsubscribe from this group, send email to
puppet-users+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en
-~----------~----~----~----~------~----~------~--~---

Hi Ryan,

You''re correct, let''s move that discussion into the
mailing-list instead
of chatting in the tickets.

In #1644 you wrote:
> 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 really agree on that point in your design because we''ll lose
granularity. It won''t be possible any more to install keys for a given
user in different parts of a recipe.

IMHO native types in Puppet really have to provide the maximum
granularity possible to be able to solve as many use cases as possible.

I''m still convinced that the right way to fix this issue without losing
granularity is to allows constructs like follows.

ssh_authorized_key{"foo":
  ensure => present,
  key    => "AAA..",
  type   => "rsa",
  user   => "root"
}

ssh_authorized_key{"foo":
  ensure => present,
  key    => "BBB..",
  type   => "dsa",
  user   => "root"
}

In ticket #1531:

The documentation is maybe not clear enough but usually you only have to
set the user attribute and leave the target out. The target is only used
to "force" a specific key file, when sshd looks for keys in a
non-standard location for example.

About the idea of using the key itself, a hash or the fingerprint as
namevar instead of the comment, I don''t see it solving this issue if we
want to keep separate resources for each line in each authorized_keys file.

It''s nice to see people using this code and lead to some constructive
discussion.

François

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com
To unsubscribe from this group, send email to
puppet-users+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en
-~----------~----~----~----~------~----~------~--~---

Hi
> You''re correct, let''s move that discussion into the
mailing-list
> instead of chatting in the tickets.
+1
> In #1644 you wrote:
> 
>> 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 really agree on that point in your design because we''ll
lose
>  granularity. It won''t be possible any more to install keys for a
> given user in different parts of a recipe.
> 
> IMHO native types in Puppet really have to provide the maximum 
> granularity possible to be able to solve as many use cases as
> possible.
+1

especially I''d like to put not every key of a user on every node.
> About the idea of using the key itself, a hash or the fingerprint as 
> namevar instead of the comment, I don''t see it solving this issue
if
> we want to keep separate resources for each line in each
> authorized_keys file.
I don''t like to use the key as an identifier, especially when using the
resource in inheritance.
well I could define an alias, but what''s the point of using then the
key
as name-var?
> It''s nice to see people using this code and lead to some
constructive
>  discussion.
the current type has some drawbacks, which might be due to some puppet
limitations (name-var unique, not connectable). Hower I found my way
round the limitations and currently define a key only once and can
change target etc. with an inheritance chain.

For me it is more important to define a key once and only once, because
then I also have to manage it only once (in case of removal etc.), but
being able to deploy it for different users, nodes etc.
This is for me the important key point.

greets pete


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com
To unsubscribe from this group, send email to
puppet-users+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en
-~----------~----~----~----~------~----~------~--~---

Hey folks,

Before responding to the ticket below, I think I should provide a little 
bit more qualifying information.  I had a discussion with Luke Kanies 
regarding the design of the ssh_authorized_keys type, and the agreement 
we came to was that the key itself would be a good identifier because 
it''s unique.  Yes, it sucks that we have to use the key because
it''s so
long and ugly, but since Puppet by nature is declarative and resource 
identifiers generally have to be unique, it seemed like a good 
candidate.  Here is the conversation:

<rgsteele||work> 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.
           <lak> what would the name of the key be if it''s not
the comment?
<rgsteele||work> lak: The key hash itself.
<rgsteele||work> The comment is way too likely to cause collisions.
<rgsteele||work> The key itself will almost always be unique.
           <lak> rgsteele||work: why would the comment have collisions?
           <lak> and if so, why not just use a comment w/out them?
<rgsteele||work> For example, if you create a dsa and rsa key at the same
time, they''ll both have the same comment.
<rgsteele||work> 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".
<rgsteele||work> And just provide an array of ''target''
files.
<rgsteele||work> This way, you can have multiple files for one key,
without elaborate naming schemes.
<rgsteele||work> And, you don''t have to worry about a composite
key.
<rgsteele||work> 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.
           <lak> if you have multiple keys per comment, then the solution
should be to refactor the type to support that, i would think
           <lak> but maybe not
<rgsteele||work> 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''.
<rgsteele||work> 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.
<rgsteele||work> Especially if they accomplish the same thing.
           <lak> rgsteele||work: i agree that target should support arrays
<rgsteele||work> lak: Then, in that case, can we remove the blocking on
#1531 <http://projects.reductivelabs.com/issues/show/1531> by the ticket
assigned to composite keys?
<rgsteele||work> This way, we can hopefully get it out sooner, since the
implementation is simpler.
           <lak> can you open a different ticket for the target being an
array?
           <lak> that''s essentially orthogonal to what the
namevar is
<rgsteele||work> lak: I''m still not sure we really need composite
keys to solve #1531 <http://projects.reductivelabs.com/issues/show/1531>.
The key alone is all we really need if target supports arrays.
<rgsteele||work> It really makes the big chunk of that ticket moot.
            <lak> i guess you''re right
            <lak> the key still really feels like an attribute rather than
the namevar, but i guess, generally, it''s unique in a given file
<rgsteele||work> 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.
<rgsteele||work> 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)


That being said, the rest of my comments are inline:

Francois Deppierraz wrote:
> Hi Ryan,
>
> You''re correct, let''s move that discussion into the
mailing-list instead
> of chatting in the tickets.
>
> In #1644 you wrote:
>
>   
>> 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 really agree on that point in your design because we''ll
lose
> granularity. It won''t be possible any more to install keys for a
given
> user in different parts of a recipe.
>
> IMHO native types in Puppet really have to provide the maximum
> granularity possible to be able to solve as many use cases as possible.
>   
I think there needs to be a little bit of clarification here.  I''ve 
withdrawn my above opinion because of the limitations of Puppet; it 
would be very hard to specify an array for the keys, the targets, the 
types, etc, and have them all map up properly.  And while I don''t like 
the idea of the key being the resource identifier, it seems like a good 
candidate because it''s unique.
> I''m still convinced that the right way to fix this issue without
losing
> granularity is to allows constructs like follows.
>
> ssh_authorized_key{"foo":
>   ensure => present,
>   key    => "AAA..",
>   type   => "rsa",
>   user   => "root"
> }
>
> ssh_authorized_key{"foo":
>   ensure => present,
>   key    => "BBB..",
>   type   => "dsa",
>   user   => "root"
> }
>
>   
Maybe I''m missing something here, but how is that even possible since 
the namevar is not unique?  Puppet would produce an error when trying to 
declare that second resource, since it has the same name as the first.  
If this worked, I wouldn''t be opposed to it, as it makes identification
of the user associated with the key much easier, but as I understand it, 
that''s not possible.  And even if it were, I would think it would 
certainly confuse people if "all resource identifiers in Puppet must be 
unique, EXCEPT for ssh_authorized_key types".  Additionally, the above 
does not allow me to install the key into multiple files without coming 
up with creative naming schemes.  E.g., I''d end up having to do this:

ssh_authorized_key{"foo_rootkey_somehost":
   key => "AAA...",
   ...
}

ssh_authorized_key{"foo_fookey_somehost":
   key => "AAA...",
...
}

ssh_authorizedKey{"foo_fookey_someotherhost":
   key => "AAA...",
...
}

That requires elaborate naming schemes AND forces you to declare 
multiple resources for no good reason.  Really it''s just a band-aid for
a less than ideal implementation.  And, inheritance chains should really 
not be required for such a simple concept.  Honestly, I believe the 
easiest method here is to allow ''target'' to accept an array of
targets,
for reasons described in my next comment.  It could still accept a 
single value, giving you the same granularity you describe above, plus 
additional flexibility.  And it has the benefit of not breaking any 
existing installations, since all you''re doing is adding the ability to
accept arrays.
> In ticket #1531:
>
> The documentation is maybe not clear enough but usually you only have to
> set the user attribute and leave the target out. The target is only used
> to "force" a specific key file, when sshd looks for keys in a
> non-standard location for example.
>   
Duly noted, but if the key needs to be in more than one authorized_keys 
file, just using the ''user'' attribute doesn''t help
me, unless you want
to make ''user'' accept arrays.  Really, either the
''target'' attribute
should accept an array, as Luke and I discussed (above), or the
''user''
attribute should.  No matter how you slice it, it''s the easiest way to 
assign a key to multiple key files: ONE resource to manage the ONE key, 
and an array of either ''user''s or ''target''s
that it belongs in.
Personally, I think ''target'' is more flexible, since you could
install
to authorized_keys files in non-standard locations or with non-standard 
names.
> About the idea of using the key itself, a hash or the fingerprint as
> namevar instead of the comment, I don''t see it solving this issue
if we
> want to keep separate resources for each line in each authorized_keys file.
>   
I fail to see the problem here.  If each key is a separate resource, 
identified by the key itself, you could then just specify an array of 
''target''s or ''user''s as described above. 
Then, all you''d need is a
''comment'' attribute.  The ''comment''
attribute could default to blank,
and if it exists at the end of the key, the type could use that, or 
alternatively it could obviously be explicitly specified.  Regardless, I 
don''t see any reason why using the key as the identifier mutually 
excludes you from being able to keep separate resources for each line of 
the authorized_keys file.
> It''s nice to see people using this code and lead to some
constructive
> discussion.
>
> François
>   
Yep, that''s the beauty of open source software - it''s uses,
features,
and implementations are influenced by the needs of the community! :)

Respectfully,
Ryan


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com
To unsubscribe from this group, send email to
puppet-users+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en
-~----------~----~----~----~------~----~------~--~---

On Mon, Oct 13, 2008 at 6:16 AM, Ryan Steele <ryan.g.steele@gmail.com>
wrote:
> Maybe I''m missing something here, but how is that even possible
since
> the namevar is not unique?  Puppet would produce an error when trying to
> declare that second resource, since it has the same name as the first.
> If this worked, I wouldn''t be opposed to it, as it makes
identification
> of the user associated with the key much easier, but as I understand it,
> that''s not possible.  And even if it were, I would think it would
> certainly confuse people if "all resource identifiers in Puppet must
be
> unique, EXCEPT for ssh_authorized_key types".  Additionally, the above
I may be missing the point here, but isn''t there a similar exception
already, in the case of exec''s ?

I think the solution here is to have the namevar be the key, as others
have suggested.

--Paul

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To post to this group, send email to puppet-users@googlegroups.com
To unsubscribe from this group, send email to
puppet-users+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/puppet-users?hl=en
-~----------~----~----~----~------~----~------~--~---