Closed Bug 774157 Opened 12 years ago Closed 12 years ago

Add an ssh module and use it for extra known_hosts for linux-foopy

Categories

(Infrastructure & Operations Graveyard :: CIDuty, task)

x86_64
Windows 7
task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch [puppet] v1 (obsolete) — Splinter Review
So, I learned that stuff required for linux-foopy was failing out, and this was actually for hosts that most machines don't need to access.

Instead of just inflating known_hosts, I decided to split the ssh stuff into its own module, and use the (new) concat plugin (which we added for motd) to do it.

I'm going to give kim a shot at reviewing this, but feel free to divert to dustin. And dustin should feel free to make additional comments on it.
Attachment #642463 - Flags: review?(kmoir)
This is a great idea, and well-executed!

A few thoughts.  First, put known_hosts entries in /etc/ssh/ssh_known_hosts, rather than the per-user known_hosts.  This has the advantage of not being edited by manual runs of SSH (I'm seeing a lot of puppet runs where ~/.ssh/known_hosts is reverted), and also makes the hosts entries available to other users.

The ssh module could be a nice utility module, if it didn't depend on users::builder.  Perhaps the homedir information could be passed *to* it, so that it can in principle configure any number of users.  For example:

ssh::user_config {
    $::config::builder_user:
        strict_host_keys => true;
}

and, elsewhere (e.g., when configuring a crontab)

ssh::user_authorized_keys {
    "${::config::builder_user}|dustins_key":
        key => "...";
}

Both have the tricky bit of needing to translate the username into a home directory, though.  Maybe it's best left for a second round, then, unless you see an easy solution.  I did verify that things like file { "~root/.ssh/authorized_keys": .. } don't work.
Blocks: 777466
Attached patch [puppet] v2Splinter Review
Adjusted for bitrot, made into parameterized classes.

(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> This is a great idea, and well-executed!
> 
> A few thoughts.  First, put known_hosts entries in /etc/ssh/ssh_known_hosts,
> rather than the per-user known_hosts.  This has the advantage of not being
> edited by manual runs of SSH (I'm seeing a lot of puppet runs where
> ~/.ssh/known_hosts is reverted), and also makes the hosts entries available
> to other users.

This is a future intention, but not in this patch. Right now we only have the requirement for the builder user, and this is a requirement for linux foopy. We also have .ssh/config Batchmode set, so we don't auto-add known_hosts anyway.

> The ssh module could be a nice utility module, if it didn't depend on
> users::builder.  Perhaps the homedir information could be passed *to* it, so
> that it can in principle configure any number of users.

This is now (sorta) done, but we still currently hard-code to users::builder here.

> ssh::user_authorized_keys {
>     "${::config::builder_user}|dustins_key":
>         key => "...";
> }

Also future improvement ;-)
Attachment #642463 - Attachment is obsolete: true
Attachment #642463 - Flags: review?(kmoir)
Attachment #646274 - Flags: review?(dustin)
Comment on attachment 646274 [details] [diff] [review]
[puppet] v2

Ship it.  Reassign this to me and I'll loop back later to make it look more like I suggested in comment 1.
Attachment #646274 - Flags: review?(dustin) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
oops, this is still open for docs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Resolving in favor of Docs Bug 816921
Blocks: 816921
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Product: Release Engineering → Infrastructure & Operations
Product: Infrastructure & Operations → Infrastructure & Operations Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: