Create a "ssh_key" module, to allow upload of SSH keys to drupal.org user profiles

Requirements:
- Check for blacklisted keys from the debian openssl debacle
- Good documentation for especially for windows users, with screenshots and a screencast
- Will be provided by one of the to-be-written modules for the git integration
- Allow associating more than one ssh key to a user.
- Associating one ssh key with different d.o users is technically impossible - validate that.
- Validate the field so that you can't enter random stuff in our authorized_keys file - This is _very_ important for security
- Before uploading the ssh key(s) the user has to agree to some legal stuff wrt GPLv2+. It looks like Crell is investigating this and he will us then provide the exact text etc. to use.

I hope that's it for that matter ;)

Ah, the legal stuff is handled there: #720670: Figure out wording of the "Yes, I promise to upload GPLv2+ code" checkbox and the documentation is there: #720666: Document how to create SSH keys on all major platforms

- Before uploading the ssh key(s) the user has to agree to some legal stuff wrt GPLv2+. It looks like Crell is investigating this and he will us then provide the exact text etc. to use.

This is scope creep. No mix-up of unrelated stuff, please.

I updated the original post.

@Damien - why is that scope creep? Or you think some other module should form_alter in that license check-off?

@pwolanin: I would like this to be a generic "ssh key" module. The licence stuff could be added in the Drupal.org customization module.

I agree with @Damien about the scope creep. And furthermore it doesn't make sense to push this agreement on a per-access key basis. It should rather be a requirement for getting the necessary permissions for publishing code on d.o.

I've started on a public key module. Currently takes care of:

- Allow associating more than one ssh key to a user.
- Disallow adding the same key twice by calculating and using the key fingerprint
- Validates the key format

Soon finished with blacklist checking

The work in progress. The blacklisting check is not in place yet, but everything else works: http://github.com/hugowetterberg/sshkey

Zipball: http://github.com/hugowetterberg/sshkey/zipball/master

Okay, blacklisting check in place.

I've created a sshkey module on d.o. now. The vulnerability check for the keys requires ssh-vulnkey, in other words debian/ubuntu. http://drupal.org/project/sshkey

Also see #638206: Add a "SSH public key" field to the user profile since we already have an ssh key profile field. This should replace it.

First, thanks!
I had a (quick) look over the module (haven't tried it out yet) and it seems to be pretty much what I imagined :)
We need a thorough review of the module, which I can't really give because I know too few about drupal.
Given this, I have some questions, though:
I'm unsure about using exceptions for error handling, do we really want this, or would be a drupal_set_message(); return; be a better alternative?
And why do you save the public key to the file system as well as to the database? Isn't it enough to just save it to the database? We need the public keys as strings later anways instead of as a .pub file, so these are pretty useless, imho.
Just from reading the code, it seems that 'edit key' allows you to change the public key, but it _seems_ to not replace a changed public key. And if it does, we need to fire a hook that the old key was (effectively) deleted and that the new key was added (This shouldn't happen if the name of the public key was changed, which we don't want for later usage anyways, except if we want to use it as a comment in .ssh/authorized_keys).
For the hook name in general, I'd prefer a public_key_added over a public_key_created.

@CornI
Re error handling: Exceptions are used in the backend code. And it's the responsibility of the front-end code (page and form validation callbacks) to catch those exceptions and use drupal_set_message or form_set_error where appropriate.

Re files: The reason that I'm saving the files is that it makes it easier to integrate with (unix and OpenSSH) tools that operate on public key files. So instead of having to generate the file when it's needed I went with the strategy of always having it there.

Re editing: I chose to not allow editing of the actual key as it doesn't make sense in the context of keys. If the public key is different it is a different key, so a new one should be created. Non-essential information as name can be edited though.

"So instead of having to generate the file when it's needed I went with the strategy of always having it there."

Do we know if git-server will be in same server as drupal.org site and is it something we need to consider anyway? Since if keys are kept on HD to avoid generating them on fly time-to-time.

This is also something what our webmasters think what they are confortable to allow but if needed keys-folder can be mounted to git server (NFS for example).

Our boxes on drupal.org are Centos and Gentoo. We need to find out if the ssh-vulnkey tool can be used there.

I opened #777980: Figure out how to check Debian/Ubuntu blacklisted keys to figure this out. It seems that the blacklist check is (1) so integrated into the Debian SSH code base that it's not possible to extract properly and (2) so simple that we could implement that using any database easily.

Does the format of public key storage - file vs DB vs ? need to be based on what sort of git/ssh authentication code we are going to use?

My vision of this is that the sshkey module should just store everything in the database. It is simply not its job to generate authorized_keys files or anything like this.

@Damien: I agree, think we should put keys only to database and then figure out what to do with that data so that it's working well (security, performance...) with OpenSSH.

@Corni - why is "- Check for blacklisted keys from the debian openssl debacle" part of the spec?

@Damien I agree with you that it isn't the sshkey module's job to generated authorized_keys files. That's why it doesn't do it. But what it does need to be is to be easy to integrate with systems that actually do generate authorized_keys files. That said, I'm not against storing the full keys in the database, storing them in the file-system might've been premature optimization - will take a look at changing the storage mechanism.

@pwolanin To check if a key has been blacklisted is a sensible feature isn't it? Furthermore it's optional, so if you don't specify a path to vulnkey no check is made. I think that the easiest thing to do at this stage is to open up http://github.com/hugowetterberg/sshkey/blob/master/sshkey.inc#L55 with a hook so that whatever comes out of #777980 can perform any validation needed.

I quess it's just trustable community listing "bad" keys. Has anyone searched for services maintaining blacklists?

Think creating hook is good idea, that gives us modularity.

Quess we should create another project call it "ssh_key_auth" (if you want) what would be responsible authenticating added keys to ssh_key-module.

Some specs (at start):

Module extends ssh_key and is able to validate given key before key is saved to db.
Check following from ssh-key:
1) format (rigth format, lenght etc...)
2) is not used yet as some1 else's key.
3) key is not blacklisted in x (replace x with blacklist of your choice, debian's openssh-blacklist prob)

1) Is already handled by sshkey http://github.com/hugowetterberg/sshkey/blob/master/sshkey.inc#L14 and should probably always be handled there, there's no point in saving invalid public keys.
2) This is also handled by sshkey http://github.com/hugowetterberg/sshkey/blob/master/sshkey.module#L258 and should probably also be handled here in the in the future.
3) This would be the task that the "sshkey_blacklist" / "sshkey_auth" module should perform. Preferably this project could contain modules/functionality for using both debian's and ubuntu's blacklists.

3) I have feeling that ubuntu's and debian's blacklists doesn't differ. But maybe there are also other ssh blacklist's existing what sshkey_blacklist module could support.

well the only blacklist which _would_ matter is the debian/ubuntu one, any other blacklist for ssh keys doesn't make sense.
But okay, if it isn't easy to implement the blacklist check on centos, we can drop it as well, because some time has passed since the incident happened.

There's noo need to drop anything, the check is already implemented, and as previously stated - optional. The pluggable blacklist check should still be implemented as blacklisting can be needed for other reasons.

And optional = external module supporting debian/ubuntus program or ability to download blacklist from debian site and put keys to db ?

You guys may want to support keys in differents formats for user convenience and convert them to the single one (openssh) transparently. Putty e.g. has own format afaik.

subscribing

subscribing

+1

FYI I've been working on a rewrite of Hugo's code to just use the database for storing the keys. It will be flexible enough that we can add a small drupalorg_ssh_public_key module or something to handle any custom validation we want to do.

https://github.com/davereid/ssh_public_key

@Dave Reid: awesome! :-)

subscribing

Tagging

It seems that Hugo have the project on d.o.

@Dave Reid: do you have plans to integrate your work on that project? if not, are you ok if I provide one-patch-to-get-to-your-version on a new issue?

Yeah that's, fine, or I could create a pull request for Hugo's project on drupal.org, or we could co-maintain it, or patch on drupal.org, or I can create a separate projects under the 'ssh_public_key' project since I think that better describes the module.

Hi guys. Please use the existing ssk_key project I'm not territorial about it and would welcome co-maintainers, neither am I adamant about file-based storage.

I took a look at Daves repo, and first off I would ask if we please could reroll some of the changes; as first deleting everything and then adding the renamed files with content changes in a separate commit makes everything a bit hard to follow.

So, if we could hold off the renaming, and maybe skip it altogether and concentrate on the actual features, it will be easier to work on this together.

Cheers,
Hugo

Well, frankly the github code I've been working on has diverged so much from the original code that it's essentially a new module. You're not going to get much a benefit by keeping the same filenames at all.

+1 to everything that Angie said.

Guys, I understand the issue here, but somebody needs to step up and just solve the problem, even at the risk of pissing someone else off. Infra needs to start to evaluating it, we need to roll it into testing profiles, and we NEED the shared issue queue space, since it's where we're managing the entire rest of the migration process. We've got an increasingly real timetable we're working on here, and if we don't have the above process rolling soon (like, in the next week), it has the potential to start throwing our entire timetable off.

The "ssh_public_keys" name also limits us to a module handling only public keys, and we've already discussed in this issue some theoretically possible sub-modules that could do things like handle directly writing authorized_keys, etc.

I've no idea why we would intentionally limit ourselves to only handling public keys;

Ok, although the current code on d.org makes a ton of assumptions for only dealing with public keys. authorized_keys deal with public keys as well. I have no idea in this issue why we would even need to deal with private keys.

I went and renamed the module namespace back to 'sshkey' for easier comparison/patching.
https://github.com/davereid/sshkey

would like to be settled & reintegrated on this by the end of this sprint (dec 17th)

I don't know enough to separate this into issue-sized patches nor whether it's more useful than pulling directly, but here's a patch generated from Dave Reid's work.

I've started looking at how to go about this. There are a lot of changes in Daves fork. Some that I agree with, some that I don't. I think that I'll start with making the keys completely stored in db, which was the original feature request that sparked this. Then we can discuss the pros and cons of the rest of the proposed changes.

This work is really important to the Git migration time line. If there's anything I can do to help get this reintegrated by December 17, please don't hesitate to call on me.

Tagging for Git Sprint 7.

Just added simpletests and a sshkey_blacklist optional sub-module that demonstrates part of the new API in my github repo. Module has also been straight ported to D7 for whenever drupal.org is upgraded to D7.

OK, I've reviewed both Hugo and Dave's versions, and Dave's is closer to what we need for the migration. Most of the reasons for this are already noted somewhere in this issue, but I'll quickly sum up:

• Dave gives us both a entities-based D7 version and a backported D6 version. This has the side benefit of having some very standard CRUD hooks; the ones in Hugo's are less standard. It does make for a kinda weird schema, but it's not so bad.
• I find the separate page for both adding/editing a key in Dave's version to be more intuitive; I also prefer a version where the full pubkey is actually stored in the db and then repopulated into the textfield. Feels more like what I'm used to with other systems that manage ssh keys in a web UI.
• The data that's stored in Dave's schema works for the git migration, _right now_. We really don't have the luxury of time in the migration anymore, so this is significant.
• File-based storage that's directly managed by the sshkey module isn't helpful for what we need.

That said, an sshkey module would be pretty incomplete if it didn't provide some facilities for getting the keys out of the db and into flatfiles. While I'm certain that having the file-writing logic hard-coded into the core sshkey module itself is not a good architecture, it does have to live _somewhere_. My thought: provide some drush commands that do the exporting. I think that most of the logic that Hugo's written for actually doing the writing should be able to be reused, as long as it's moved into drush commands.

Also, I feel like something's missing in all of it: I'd like to see a function that, given a public key/fingerprint, returns a uid, and then maybe a menu callback which exposes that same uid-finding functionality as a web service.

So with all that in mind, here's my official request: I'd like to see Dave's code move in immediately, maybe to a 6.x-2.x branch, with the expectation that the parts of Hugo's code that were cut out (basically, file-writing) can be moved in to drush commands later as time allows. Those additions aren't crucial for the git migration, but are still pertinent in that we need to make an official release of this before we can put it on d.o. So we can't have a full release waiting on those drush commands.

Hugo, I totally understand that this I'm using steamrolling bully tactics here, and I'm really sorry. But we need this, and Dave's works as-is. If you're not comfortable with the plan I've outlined, I understand that - but in that case, I'll have to ask Dave to open a separate project to contain his code, and we'll put that one on d.o.

Let's leave this until Tuesday, Dec. 28th. At that point, if Hugo hasn't given a green-light to my plan in #51, then Dave, please post a separate project.

Also, I feel like something's missing in all of it: I'd like to see a function that, given a public key/fingerprint, returns a uid, and then maybe a menu callback which exposes that same uid-finding functionality as a web service.

You should be able to use:

$key = sshkey_load_by_fingerprint($fingerprint);
return $key->entity_type == 'user' ? $key->entity_id : FALSE;

I'm definitely not going to be a road-block on this one, if Dave's version works and does what you need then I think you should go with it. Other than that it looks like Dave has a lot of time and energy to put into this, and that is definitely valuable in itself. Though I would like someone more to step in as a maintainer to establish a proper review process for what is going to be a central module for the Drupal infrastructure.

Cheers
Hugo

Hugo, thank you SO much for understanding. Could you grant Dave CVS access to the project (if you haven't already), then, and he can take care of shepharding it forward for d.o? We've already started getting it through the hoops necessary to get it into d.o infra - security review, etc.

I don't quite follow what you mean by

Though I would like someone more to step in as a maintainer to establish a proper review process for what is going to be a central module for the Drupal infrastructure.

but hopefully I've addressed it. If not, please clarify and we'll see what we can do to get it taken care of.

Thanks Hugo. I'll start uploading the code into 7.x-2.x and 6.x-2.x branches.

Actually it might probably make more sense to just load the code into the 1.x branch? There's only been one release and it was a 6.x-1.0-unstable1? Hugo what do you think?

Silly delays! :P Just put it in 6.x-2.x for safety and be done with it, please...

Yep, 6.x-2.x is best. I'm actually using this module in a Drupal install that manages ssh access by key for all our servers, and somebody else might be using it too, so it's best if the versions are distinct as this introduces breaking changes.

@sdboyer No problem, I hate prestige-blocking and never intended the module to be a one-man show, I needed it and it needed to be done.

Dave should have all admin privileges for sshkey now

@Hugo THANKS AGAIN! sooooo much!

New branch with a beta release has been committed and download available shortly! Thanks everyone and Hugo for helping on this issue.

Dave Reid++ !!!!!!!