search

AdRoll / hologram

Easy, painless AWS credentials on developer laptops.
Apache License 2.0
803 stars 42 forks source link

Support the authorized_keys public key format in addition to the existing one #31

Closed copumpkin closed 9 years ago

copumpkin commented 9 years ago

Should resolve #13. The fallthrough logic should be safe, since I doubt there's any chance of something getting misinterpreted as the wrong format :smile:

I'm not a go master, so I don't know if the fallthrough nested if is idiomatic. I welcome feedback if there's anything that needs fixing!

copumpkin commented 9 years ago

I also have confirmation from GitHub that their implementation of the sshPublicKey LDAP attribute expects the OpenSSH authorized_keys format specifically, which should serve as additional motivation for this functionality :smile:

frangarciam commented 9 years ago

Nice work! Just added a trivial comment and this will be good to merge!

frangarciam commented 9 years ago

Oh, I knew I was forgetting something!

This PR adds support to read keys in authorized_keys format from ldap, but doesn't actually provide any way to put them there in that format. If you use hologram-authorize it will put the keys in our current format, so for completeness we should probably add a switch so keys can be send in that format instead of the default one. Relevant code can be found at https://github.com/AdRoll/hologram/blob/master/cmd/hologram-authorize/main.go#L41

This should be a relatively simple change, I'd be happy to help if you need a hand.

copumpkin commented 9 years ago

Ah yes, I've been adding keys to users with another mechanism, since I didn't even realize hologram-authorize was a thing! I saw evidence of it in the server handler but didn't know how to talk to it :smile:

Now that I look more carefully at it, I don't think its approach to passwords will work for me, which will make it fairly hard to test properly. As far as I can tell, you hash the password on the client side and then ask the LDAP server for it on the server side and check that the hashes match. Active Directory will unfortunately (or fortunately!) not let you retrieve user passwords from the server at all, no matter your permissions, so I'm not able to hash them myself. Any ideas?

frangarciam commented 9 years ago

You're absolutely right, this is hologram doing the wrong thing, so I've created https://github.com/AdRoll/hologram/issues/36 to fix it.

Regarding how to test your changes, it's not ideal but the interesting part is actually handled in usercache.go, from hologram-authorize we really just need to send the public key in one format or another. So as long as tests pass I can do any additional testing before merging the PR.

copumpkin commented 9 years ago

I think I've fixed the outstanding issues now! Let me know if anything else seems weird or wrong :smile:

frangarciam commented 9 years ago

Hmm, I'm still missing the changes to hologram-authorize we discussed?

copumpkin commented 9 years ago

Sorry, I misunderstood your last comment as you saying you didn't need the hologram-authorize support in this PR! Is it really worth adding that before #36 is done, since that'll change how LDAP authentication happens and make it possible for me to actually make sure it works?

frangarciam commented 9 years ago

That's a fair point. I'm happy to merge this so hologram-authorize support can be added later, I'll just make sure https://github.com/AdRoll/hologram/issues/13 stays open until that happens.

Thanks for adding this!

copumpkin commented 9 years ago

Thank you for accepting it! I'll definitely be happy to add the support to hologram-authorize once we sort out how it should work.