[FFe] ssh-import-lp-id: retrieve a key from Launchpad and add to the authorized_keys file

Script attached.

Colin-

Can you comment if this is something that could land in openssh-server, or if we would need to carry this elsewhere?

I'm adding a server-papercuts task, as I think this might be a reasonable candidate, as I mentioned this script at UDS-Dallas.

The code is very simple, and straight-forward. It's really something that could make the Ubuntu Server shine, as being friendly, useful, and easier-to-use, and help further the adoption of Ubuntu's EC2 images over other Linux distributions.

It's also a nice tie-in with Launchpad, as an auxiliary Ubuntu-related service (while not being exclusively tied to Launchpad -- just mangle your URL environment variable and point to your infrastructure's public key server).

attached is my improved version to this, more like a program than a shell script.

hmm... shoudl also probably check that pubkey starts with 'ssh-' before adding it, to avoid some "file not found" HTML getting written to .ssh/authorized_keys.

   if url_encode "$i" && cururl=$(printf "$url" "${_RET}") &&
    pubkey=$(wget --quiet -O- "$cururl") && [ -n "${pubkey}" ] &&
           [ "${pubkey#ssh-}" != "${pubkey}" ]; then

Even small, that's a new feature, so it requires FFe and should probably not be accepted as a papercut ("only bugfixes").

I'd prefer to think about this quite hard before adding it to openssh, value-add or not - the security properties worry me given that this is something that grants access to an account based on data retrieved from a remote system (and not everyone trusts https alone). ssh-copy-id is quite different because it simply pushes data over ssh.

Additional comment to my comment 6 above, just to make clear that the fact that it requires FFe and fails to meet papercuts criteria doesn't prevent this from being a good idea.

I think it would be a very valuable feature for our cloud images, as long as we get the security issues around it right. Since it's a standalone feature, it doesn't affect the stability of the product so I'd support an early FFe for it.

While I can see the utility of this script in certain situations, I'm not sure it is generally useful enough to put in openssh, or even cloud-init. It really feels like it should be in its own package. Also, I think we can assume that someone will one day want to run this as root, since the idea is to share the VM, and people will likely want to share all of it. As such, I reviewed the script and feel several improvements should be made. I don't claim this as an exhaustive review and may have missed something. These are in no particular order. I hope others will review it as well.

1. there is no input validation for the '-u' url. Combined with the 'printf' this is a format string vulnerability. I haven't thought about what all you could do with it, but you really only want %s anyway, so enforce it.

2. only '$i' is url encoded. Since -u contains untrusted input, you should url encode it as well

3. there is no input validation for the '-d' option. I can then do -d '/etc/cron.daily' (or any number of fun things). Note there is no privilege escalation, but there is also no reason why the script should write to '/etc/cron.daily'. I suggest rather than using a directory, you specify a local user. Then use 'getent passwd "$local_user"' to get the $HOME directory, then tack '.ssh' on to that. You can then do fascist input validation on $local_user. You can look at adduser for hints on valid user names

4. http or any cleartext protocol *must not* be allowed otherwise very serious man in the middle scenarios are easily possible

5. There is no validation for '$pubkey', which could lead to an invalid entry in the authorized keys file. This is particularly worrisome since the script trusts the response from the server completely. There are all kinds of things that can happen here, from a DoS with 'from="..,!<you>"' to multiple keys being downloaded.

6. there is a TOCTOU issue on the mkdir and then later with file. For the mkdir, use this instead (I left out '-p' intentionally, since if the user's $HOME as seen through 'getent' doesn't exist, it is probably for a reason):
mkdir -m 0700 "$dir" 2>/dev/null || true
test -d "$dir" || fail "failed to make ${dir}"

For file, I suggest doing something like (notice we can perform validation on the tmp file before committing to authorized keys):
tmp=`mktemp`
trap "rm -f $tmp" EXIT HUP INT QUIT TERM
...
echo "$pubkey" >> "$tmp" || fail "failed to write to ${file}"
validate_authorized_keys "$tmp" || fail "Invalid entry. Aborting"
mv -f "$tmp" "$file"

7. the script should use 'set -e'

8. this line should probably be broken up, with each failure being reported on its own:
if url_encode "$i" && cururl=$(printf "$url" "${_RET}") &&
           pubkey=$(wget --quiet -O- "$cururl") && [ -n "${pubkey}" ]; then

9. I'd like to see enforcement of valid ssl certs from the server, otherwise we can be man-in-the-middled

10. Due to the nature of the script, I'd recommend scrubbing the environment in some way (perhaps 'env -i' before the commands or change to bash and use 'set -p'). If you used bash, you could maybe even get away with using '-r' (restricted shell) if you got rid of the '>>' and used wget to append to the...

Colin-

Thanks for the comment. I agree that such a script should undergo a *thorough* review before being accepted into our openssh-server package.

I also see your point, that ssh-copy-id is doing it's work over ssh itself.

In the default configuration, ssh-import-id works against Launchpad.net, over SSL, with a valid certificate. SSL should prevent both eavesdropping and tampering, end-to-end, between the client and the server, and the server should be authenticated to the client (assuming a valid certificate and a 3rd party certificate authority). An invalid certificate will fail the wget, and prevent a key from being written. This would mean that we'd need to keep our certificate current for Launchpad.net, which I'd hope is a safe assumption. And anyone modifying their URL parameter would need to expect the same from their keyserver.

The SSL assumption is a safe one (in my opinion), as it is the same assumption we make as we conduct all sorts of private, critical business over https everyday.

Also, this script runs as a non-privileged user, modifying their own authorized_keys file, obviously something they could do on their own. I don't think it does anything special, tricky, or exceptional in this way.

Finally, I'd argue that the utility is non-intrusive. It doesn't change the behavior of anything else in SSH, or interrupt any other operations.

I'm certainly willing to work the FFe end of this and try to get it accepted ASAP (Alpha3?), as it's something I really believe can make our Ubuntu Server EC2/UEC images stand out among available images. If that means this script should live in one of our cloud-* packages for Lucid, so be it.

I just thought that openssh-server would be the appropriate home for such a utility (eventually), and I thought I'd start a conversation here.

Thanks,
:-Dustin

Kees-

  * May I ask for your opinion?
  * Do we want it to remain non-trivial to add public keys to authorized_keys? Is there a security reason for doing so?
  * Is this ssh-import-id utility just a bad idea?
  * Do you have security concerns about the key retrieval method?
  * Is SSL and/or Launchpad unsuitable for this sort of thing?
  * Would there be any reason to force the client to authenticate with the server too? (I'd think not, as this is a public key, and an open URI).
  * Is it inadvisable to put such a utility in /usr/bin such that it's discoverable in the default path? Would it be better to hide it away in /usr/lib or something?
  * Is openssh-server the right/wrong place for this utility? Does the answer to that question change whether we're talking about Lucid or Lucid+1?
  * Does this open up new opportunities for abuse somehow?

:-Dustin

On Fri, Feb 19, 2010 at 8:47 AM, Jamie Strandboge <email address hidden> wrote:
> While I can see the utility of this script in certain situations, I'm
> not sure it is generally useful enough to put in openssh, or even cloud-
> init. It really feels like it should be in its own package. Also, I
> think we can assume that someone will one day want to run this as root,
> since the idea is to share the VM, and people will likely want to share
> all of it. As such, I reviewed the script and feel several improvements
> should be made. I don't claim this as an exhaustive review and may have
> missed something. These are in no particular order. I hope others will
> review it as well.

Thanks for the review, Jamie.

> 1. there is no input validation for the '-u' url. Combined with the
> 'printf' this is a format string vulnerability. I haven't thought about
> what all you could do with it, but you really only want %s anyway, so
> enforce it.

Actually, I would like to drop the -u parameter altogether for the
first cut of the utility. Localize the variable entirely, forcing
Launchpad.net as the only valid URL. Users wanting another URL would
need to copy the script and edit it locally.

> 2. only '$i' is url encoded. Since -u contains untrusted input, you
> should url encode it as well

Agreed. Moot point, though, if we drop -u.

> 3. there is no input validation for the '-d' option. I can then do -d
> '/etc/cron.daily' (or any number of fun things). Note there is no
> privilege escalation, but there is also no reason why the script should
> write to '/etc/cron.daily'. I suggest rather than using a directory, you
> specify a local user. Then use 'getent passwd "$local_user"' to get the
> $HOME directory, then tack '.ssh' on to that. You can then do fascist
> input validation on $local_user. You can look at adduser for hints on
> valid user names

Good suggestion. But I'd also like to drop -d for now too, in the
interest of simplicity. I was only using it for testing. I think the
assumption should be that only the user running the script can write
to their own $HOME/.ssh/authorized_keys file (which presumably they
can do already).

> 4. http or any cleartext protocol *must not* be allowed otherwise very
> serious man in the middle scenarios are easily possible

Agreed. The wget I used enforces certificates, and fails if
certificate is invalid. Again, if we drop the -u parameter, and force
the https://launchpad.net... URL, I think we're safe here.

> 5. There is no validation for '$pubkey', which could lead to an invalid
> entry in the authorized keys file. This is particularly worrisome since
> the script trusts the response from the server completely. There are all
> kinds of things that can happen here, from a DoS with 'from="..,!<you>"'
> to multiple keys being downloaded.

Great point. This should be fixed.

> 6. there is a TOCTOU issue on the mkdir and then later with file. For the mkdir, use this instead (I left out '-p' intentionally, since if the user's $HOME as seen through 'getent' doesn't exist, it is probably for a reason):
> mkdir -m 0700 "$dir" 2>/dev/null || true
> test -d "$dir" || fail "failed to make ${dir}"

Thanks, agreed.

> ...

I couldn't remember if wget would error out on an invalid certification, but reading the man page for wget, it seems that as long as wget is compiled with openssl, it will error out (good).
"As of Wget 1.10, the default is to verify the server's certificate against the recognized certificate authorities, breaking the SSL handshake and aborting the download if the verification fails. Although this provides more secure downloads, it does break interoperability with some sites that worked with previous Wget versions, particularly those using self-signed, expired, or otherwise invalid certificates."

I do also want to mention that most of the coding issues I brought up are not significant in the expected usage of a regular user running the command and giving the appropriate options (ie, it is a lot easier to just create a directory with authorized_keys in it rather than subverting this script). Running as root brings a few more concerns, but really it is if/when this script becomes part of a larger system that the issues I pointed out can become serious. Since we don't know how people will be using it, IMHO it is important to program as defensively as possible.

I think it's vitally important to enforce https and to validate the new authorized_keys file, ideally with fingerprint and confirmation (and what about ssh-vulnkey for good measure? Perhaps overkill, but certainly doable).

Standard practice would be to display the ssh key's fingerprint and ask the user to validate it before importing it automatically.

Marc-

I think that's true if you're receiving an arbitrary key from an untrusted source (such as the first time you log into a remote server).

However, in this case, I think:
 a) You're communicating over SSL with a server and a valid certificate (hence, the server is authenticated and attested)
 b) The user who's keys you are retrieving had to authenticate themselves with Launchpad in order to upload their key, all of which was conducted over SSL.

In this case, I think the chain of trust comes down to:
 a) Are you sure you're talking to Launchpad.net?
 b) Are you sure that the user who's key you're retrieving authenticated with Launchpad when uploading these keys?

I believe these are assumptions you and I safely make every day, in the course of our daily work through firefox, dput, apt-get, and various other utilities.

:-Dustin

Attaching updated version based on Jamie's excellent feedback. I believe I have addressed the concerns that he has raised so far.

Dustin,

Now that you've removed the ability to specify an arbitrary URL, I think that's a fair assumption.

Dustin,

Thanks for your work on this. I have a couple of small nits and a bug fix:
1. in url_encode(), error(), warn() and info() you use something like 'printf "ERROR: $@\n"'. It would be better to use something like 'printf "ERROR: %s\n" "$@"'

2. env -i isn't doing what you want here. You prefix env -i at the beginning of a command. Eg, compare the difference between these commands:
$ env -i sh -c "set"
$ sh -c "set"

I think it is probably overkill to do this for every command. The only one I would probably use it on is 'wget', since we are trusting it to handle the ssl certificate properly. Even this is probably a bit pedantic. But, it wouldn't hurt:
if env -i wget --quiet -O- "$url" > "$tmp"; then...

3. there is a bug when calculating 'lines' in validate_keys(). wc returns 0 if it doesn't have a newline, which the wget response may not. I suggest doing:
echo "" >> "$tmp" # needed for wc
if ! validate_keys "$tmp"; then
...

Jamie,

Thanks again.

 (1) printf ... Got it, thanks. Fixed in new upload.
 (2) env ... Ah, I see. Also, fixed in new upload.
 (3) Got it, good catch.

One more thing I found/fixed... The grep for existing keys wasn't working properly. So I fixed it with a unique sort on the file (to make the script idempotent on the same id or multiple identical keys).

I think a name like "ssh-import-launchpad-id" would be more appropriate for something this Launchpad specific.

Par-

Can you name another public server where such keys are available?

I scoured the web and couldn't find one.

But, fine... I'll rename the utility ssh-import-lp-id, noting that it is Launchpad specific. If someone else wants to write a plugin that talks to another well-known public ssh key server, it can grow appendages.

I've created a Launchpad project with this utility, a manpage, and some simple packaging:
 * http://launchpad.net/ssh-import

Test packages are available in:
 * https://edge.launchpad.net/~ssh-import/+archive/ppa/+packages

To the FFe reviewers-
I'm requesting a FFe to get this simple package added to Lucid. It is a non-intrusive utility that allows importing of one or more Launchpad user's registered ssh keys into the current system user's ~/.authorized_keys file. The utility is totally non-intrusive and will significantly enhance the usability of Ubuntu server UEC and EC2 images. The Ubuntu Security team has reviewed the code, and all of their suggestions have been incorporated.

I'll get it seeded at least as a recommends of cloud-init.

Colin-
Assuming the FFe is granted and it makes it into the archive, it would be nice if openssh-server would also carry a recommends or suggests on ssh-import.

This is a self-contained feature which (I assume!) has no impact on the default install, so this is fine for a FFe.

I do have concerns about adding a new source package to the archive just for this script, which I've raised with Dustin on IRC.

Per Steve's feedback, I've move this into the cloud-utils source package, as a binary package called ssh-import.

Thanks!

This bug was fixed in the package cloud-utils - 0.6-0ubuntu1

---------------
cloud-utils (0.6-0ubuntu1) lucid; urgency=low

  * debian/control, debian/ssh-import.install, debian/ssh-import.manpages,
    ssh-import-lp-id, ssh-import-lp-id.1:
    - add a utility and a binary package for conveniently importing
      public ssh keys from Launchpad by a LP user id, LP: #524226
 -- Dustin Kirkland <email address hidden> Tue, 23 Feb 2010 20:32:40 -0600

@Jamie: Re: "my initial reaction is that this is too scary for openssh and cloud-init (and potentially main, but I'll let the MIR team comment on that)"

Note that including it in existing cloud-utils, which is already in main, made it bypass MIR review. So if you still have concerns, you should raise them sooner rather than later :)

On Wed, Feb 24, 2010 at 1:44 AM, Thierry Carrez
<email address hidden> wrote:
> Note that including it in existing cloud-utils, which is already in
> main, made it bypass MIR review.

Thierry-

That's not true.

ssh-import is a separate binary package under cloud-utils. It's in
binary-new now.

Assuming it gets accepted by an AA today, it'll land in Universe.
Then, I will start an MIR thread about getting it added to Main. The
MIR team (Kees, at least, already has a heads-up).

:-Dustin

Usually we don't do a MIR for new binaries if the source is already in Main. Seems appropriate in this case though.

+1 on what ScottK said, no MIR necessary for a new binary package with a small script from "in-house" development.

I think that would be a -1 on what Scott said, since he says an MIR *would* be appropriate here :)

(I tend to agree; being developed in-house doesn't mean it's impervious to security concerns.)

Right, I meant that wrt. the maintainer/support angle. It seems that the entire security team already commented/reviewed here, and got followups from Dustin.

Jamie, Kees, do you have any remaining concerns about this in its current state?

I'm fine with it.