Initial support for using Kerberos. #1261

Merged
merged 1 commit into from Apr 7, 2016

Projects

None yet

7 participants

@funkaoshi
Contributor

Support 3 additional parameters when connecting to hosts: gss-auth,
gss-deleg, gss-kex. These correspond to the equivalent in paramiko. It
does all the hard work.

@funkaoshi
Contributor

Oh, sorry, I didn't mean to start this right away. (I thought I was pulling against our forked repo.)

Anyway, does this look like the right approach? What sort of tests would you guys want here, if any. (I am just passing the various kerberos parameters from the env into paramiko.)

@bitprophet
Member

Testing kerberos is actually super difficult as far as I can tell (we were unable to add meaningful tests to Paramiko's support) so I think we'd have to settle for just "are we passing the right parameters to the Paramiko level" perhaps.

Re: approach, it looks like an okay direction for now, this is basically what I was expecting (see #195 (comment)). Thanks! Please comment again when you're ready for actual review (also note that I have a huge backlog D:)

@bitprophet bitprophet added the Feature label Jan 28, 2015
@funkaoshi
Contributor

@bitprophet Thanks. I was going to bug the other people here to see if they'd do anything differently. I'm using this now and it seems to be working fine. (If that fills you with much confidence. Hah!)

@kyrias
kyrias commented Mar 20, 2015

@funkaoshi So you would consider this ready for merger/review now? If so I think I’ll give it a try.

@funkaoshi
Contributor

@kyrias no one at the office has complained! It's a very small change.

@funkaoshi
Contributor

(Right now for simplicity you can't pick a different host/port.)

@bitprophet bitprophet added this to the 1.11 milestone Mar 24, 2015
@bitprophet
Member

Eager to hear feedback from @kyrias but given how simple it is I'm gonna guess it works fine & any issues will be at the Paramiko level. Added to next feature release milestone.

@funkaoshi
Contributor

Should I rebase this on top of master? (Or one of the release branches?)

Adding gss_host as a parameter might be tricky, because you can run commands against multiple hosts at the same time. (I think that's why I didn't include it originally.)

@kyrias
kyrias commented Apr 1, 2015

Yep, seems to work just fine for me.

@bitprophet
Member

@funkaoshi Feature releases come off of master, so no need to move it to a release branch. Rebase to latest master if you want but the diff is pretty small & github isn't identifying any merge conflicts, so I doubt there's a problem at this point.

@kyrias Thanks! Good to know. Keeping it in that release milestone, not sure when I'll get to it, probably post Pycon or during sprints.

@funkaoshi
Contributor

Sorry, one more thing. To use Kerberos tickets you need to install python-gssapi. Should this become a dependency of Fabric? I don't feel like it should, because how many people actually use Kerberos. On the other hand using --gss-api always fail if python-gssapi isn't available.

@funkaoshi
Contributor

Also i'll be at PyCon too. What!

@bennettaur

This PR changed my life! A few years back ssh-keygen and I had an altercation which left me hospitalized for a month. Ever since then I've been unable to use keypair authentication and assumed my life was over as it was (password auth? HAH, here let me just package my identity up for you to use at will). But now with GSSAPI support I can fab deploy a new life without worry or fear ever again

But in all seriousness +1 on this
@funkaoshi I don't think python-gssapi should be a dependancy for fabric as it's an optional dependancy for paramiko for doer GSS-API stuff. Might be worth it to just mention it in the docs though

@kyrias
kyrias commented Apr 2, 2015

Explicitly erroring out/printing a warning when trying to use do GSS-API stuff without python-gssapi might be good, though the paramiko error, while less nice maybe, is sufficient I guess.

@bitprophet
Member

TBH we should just make the paramiko error better, it's historically been kinda bluh at useful errors and it is one area I want to tidy up as I now own it 😩

@funkaoshi
Contributor

@bitprophet Agreed. Better to do things at the source, usually.

Are you guys (still) at PyCon?

@sivy sivy self-assigned this Apr 13, 2015
@sivy sivy removed the Docs label Apr 13, 2015
@funkaoshi
Contributor

Will someone else write the docs? I'm not sure where to start with that.

@dahlke
dahlke commented Jun 11, 2015

@funkaoshi I'm willing to write 'em up, don't think I can modify your PR though. Seems like the docs update should be bundled with the merge.

@kyrias
kyrias commented Jun 12, 2015

@dahlke You can send a PR to the sdelements/fabric basic-kerberos branch though.

@funkaoshi
Contributor

Oh man it's been months. What do I need to do? Or how can I help you @dahlke? You could do a PR on top of my PR, yeah.

@bitprophet
Member

Barf, yea it's been forever. We even semi-fixed-up GSSAPI after it broke again, in Paramiko. There's still some annoying confusion because of the issue documented in paramiko/paramiko#584 but AFAIK if one has python-gssapi (vs gssapi) installed, it's functional right now.

If this basic "pass the vars through" approach still works with Paramiko 1.15/1.16 + python-gssapi I say we merge it and move from there. I can add a quick note to the install docs pointing at Paramiko's own note about GSSAPI install reqs, that will hopefully catch most users capable of reading. EDIT: this thing here: http://www.paramiko.org/installing.html#optional-dependencies-for-gss-api-sspi-kerberos

@bitprophet
Member

To be clear, what this means is I'd like @funkaoshi or somebody to:

  • Update the PR to latest Fabric master (probably no large differences though...)
  • Update local test environment to have at least Paramiko 1.15.4 if not the just-released 1.16.0
  • Make sure your GSSAPI connection functions
  • Let me know things look good

Then I will merge & tweak our install docs & it'll be ready for next Fab feature release (or I will just make one because big releases are dumb and a very bad habit).

Thanks!

@funkaoshi
Contributor

Thanks. Let me rebase on top of master and see what's the what.

@funkaoshi funkaoshi Initial support for using Kerberos.
Support 3 additional parameters when connecting to hosts: gss-auth,
gss-deleg, gss-kex. These correspond to the equivalent in paramiko. It
does all the hard work.
07b0e41
@funkaoshi
Contributor

Will test things locally here and report back.

@funkaoshi
Contributor

And based on my (non-exhaustive) testing it looks like this continues to work (assuming you have python-gssapi installed as well). We use this fork internally for some DevOps stuff, so I can ask them to update and see if anything breaks for them.

@buckett
buckett commented Dec 10, 2015

It would be really nice to have this in a release, is there anything I can do to help get this merged?

@bitprophet
Member

@buckett see #1261 (comment) - tl;dr additional "I tested this with a recent Paramiko release and a live Kerberos install and it worked for me" notes would be helpful.

That said, @funkaoshi gave a thumbs up so I mostly just need to find time to sprint through the 1.11 milestone that this is part of!

@bitprophet bitprophet merged commit 07b0e41 into fabric:master Apr 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bitprophet bitprophet added a commit that referenced this pull request Apr 7, 2016
@bitprophet bitprophet Changelog re #1261 02d40c1
@sivy sivy was unassigned by bitprophet Apr 7, 2016
@bitprophet
Member

Noticed one big problem with this, which is that it blows up on any Paramiko<1.15 (which is when Kerberos support landed). I'm fixing it up so it only submits those kwargs when they're set to non-None values (and updating the CLI/env defaults to be None instead of False).

This should mean enabling them works as before, but if one doesn't touch them, they're not submitted, and Paramiko e.g. 1.13 won't explode.

@bitprophet bitprophet added a commit that referenced this pull request Apr 7, 2016
@bitprophet bitprophet Gracefully skip GSS-API related kwargs unless set to non-default values.
Allows use of Paramiko<1.15. Re #1261
1ca512d
@funkaoshi
Contributor

Ah, thanks for making the change. Looks good.

@funkaoshi funkaoshi deleted the sdelements:basic-kerberos branch Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment