Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable GSSAPI tests and add support for the package 'gssapi' #1311

Closed
wants to merge 10 commits into from

Conversation

@akruis
Copy link

@akruis akruis commented Oct 5, 2018

[Maintainer note: the original feature/request ticket for this is #584]

This pull request replaces pull request #1166.

Previously testing of GSSAPI (Kerberos) related functions required an
externally provided Kerberos environment. Therefore all GSSAPI tests were
skipped.

Now the package k5test is used to setup a self-contained Kerberos environment.
Because k5test requires the new GSSAPI, this commit also merges
pull request #1166 and fixes broken GSSAPI test. If k5test is not available
(i.e. on Windows), the tests still get skipped.

The test case test_kex_gss.test_2_gsskex_and_auth_rekey is expected to fail. This needs to be resolved as a separate issue.

This pull request does not change the flow of control (except for the additional case in ssh_gss) and does not touch non-GSSAPI/SSPI related code. Therefore I recommend inclusion in 2.4. With this patch applied, GSSAPI could be tested by travis.

Tanja Huthmacher and others added 5 commits Sep 19, 2018
Detect if "python-gssapi" or "gssapi" is installed at import time
and use the appropriate API
Previously testing of GSSAPI (Kerberos) related functions required an
externally provided Kerberos environment. Therefore all GSSAPI tests were
skipped.

Now the package k5test is used to setup a self-contained Kerberos environment.
Because k5test requires the new GSSAPI, this commit also merges
pull request paramiko#1166 and fixes broken GSSAPI test. If k5test is not available
(i.e. on Windows), the tests still get skipped.

The test case test_kex_gss.test_2_gsskex_and_auth_rekey is expected to fail.
@akruis
Copy link
Author

@akruis akruis commented Oct 5, 2018

@SebastianDeiss Sebastian, provided you have time, could you review this pull request? And could you look at #1312 too?

Anselm Kruis and others added 3 commits Oct 6, 2018
Add two additional test jobs to run GSSAPI related tests with
python 2.7 and 3.6.
@SebastianDeiss
Copy link

@SebastianDeiss SebastianDeiss commented Oct 8, 2018

@akruis I'm quite busy this week, but I try to take a look at it next week.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Oct 16, 2018

FWIW this could probably get pushed onto master; it's in that gray area between bugfix and feature but given it involves dependencies & is a nontrivial shakeup of an established feature, my gut says feature.

But I'll perform any necessary merging/rebasing when I get to it so don't worry about opening yet another separate PR for now 😂

I'm also real happy to see the notes about self-testing using the new library as that's always been a real sore spot for this feature set. Thanks!

@SebastianDeiss
Copy link

@SebastianDeiss SebastianDeiss commented Oct 22, 2018

@akruis LGTM.

@JAORMX
Copy link

@JAORMX JAORMX commented Nov 19, 2018

We need this patch, any chance to get it merged?

@aaron-p-lehmann
Copy link

@aaron-p-lehmann aaron-p-lehmann commented Nov 20, 2018

I'm with @JAORMX , getting this merged and a new egg/wheel made would save me a lot of shenanigans.

@jborean93
Copy link

@jborean93 jborean93 commented Dec 18, 2018

It would be great if this could be merged in at some point. Currently paramiko is used in a few places in Ansible but some other libraries use the newer gssapi library for Kerberos auth support. Due to the conflicts this causes import errors and paramiko cannot be used in those situations.

@ecederstrand
Copy link

@ecederstrand ecederstrand commented May 20, 2019

Ping? We also need this patch. @bitprophet Is there anything we can do to help?

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Looking this over now, thoughts:

  • I like that it adds tests ❤️
  • flake8 found that one of the new methods (ssh_init_sec_context) lacked an internal import from pyasn1
    • Normally, this would just be an understandable braino (those internal imports seem to have been a pattern from the original GSSAPI support patch, because of the optional nature of this feature)
    • However it makes me worry that that part of the code (if not other parts) are not being hit by the new tests 😢
    • I fixed it and pushed to Travis to see how the new tests run, at any rate, but noting this for the record.
  • I'm not doing a deep dive of most of the diff, since at this time I consider the GSSAPI support to be "community supported" and I have bigger fish to fry. Fixed one or two very minor things that jumped out at me and that's it.
  • I'd love it if some of the folks in this ticket (esp @akruis and @SebastianDeiss but any help is good) could triple check the 1311-int branch to make sure that it still works for them (it's this patchset + master merge + my minor tweaks/fixes as above).
    • I may delay merging this until I get at least one firm +1 on that. But if I do, I'll happily put out a Paramiko 2.6 as soon as that +1 occurs 👍

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Interestingly there's already some failures in Travis: https://travis-ci.org/paramiko/paramiko/jobs/539948050

  • Suspect the one about pyasn1 is just a minor oversight and we need the Travis GSSAPI section to pip install pyasn1, though this might be some unrelated issue around Cryptography (which recently-ish stopped using that lib, IIRC, and master Paramiko just landed a change requiring a newer Crypto).
  • The other about SSH keys is weird, we have other unrelated tickets about new key types, but I haven't done anything with them yet, so...??? Maybe it'll magically go away when the other is fixed! That sometimes happens. Computers!

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Also, realized the docs did not get updated here:

  • Clearly we need to s/python-gssapi/gssapi in the install docs
    • Do that
    • though I'll keep mention of python-gssapi since the code went to the effort of remaining backwards compatible.
    • Do we know what the minimum supported version of that new dependency is? I'm going to try for 1.0.0 on up, but...
  • Changelog entry, which I will handle, noting that this is a backwards-compatible change

Also wanted to add an extras_require but then remembered this is one of those thorny platform-specific dependencies: Unix wants gssapi but Windows only wants pywin32 (and for all I know even that's outdated). Not sure if we can specify platforms in extras_require these days so going to punt on that and just tweak our Travis YML.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Digging harder at that pyasn1 issue - I made some faulty assumptions:

  • That gssapi was not already indicated in the .travis.yml - it is
  • That pyasn1 was a dependency of gssapi - it isn't

Turns out, ok: in a31818c (for #1191) I made pyasn1 optional, but I don't seem to ever have added it to the docs. Not sure if this is because it was an implicit dep of python-gssapi or not, but it's moot now I think. Added it to both the install docs and the travis.yml.

bitprophet added a commit that referenced this issue Jun 1, 2019
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Cool, that seems to have fixed all the issues: https://travis-ci.org/paramiko/paramiko/builds/539961575

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 1, 2019

Assuming my full rerun of that build's still green (initially only ran the Kerberos test cells), that means what's left is:

  • Want others to triple check the branch still kosher for them
  • Would like a sanity check on whether the gssapi above 1.0.0 in the install docs is accurate or if we need to scope it to eg >=1.5.0

@akruis
Copy link
Author

@akruis akruis commented Jun 4, 2019

Hi Jeff, I'm out of office this week, but I'll have a look at it during my leave next week.

akruis referenced this issue Jun 19, 2019
Worries me that it was not caught by the implementer though,
doesn't that mean this method was not called by their testing?
@akruis
Copy link
Author

@akruis akruis commented Jun 19, 2019

Assuming my full rerun of that build's still green (initially only ran the Kerberos test cells), that means what's left is:

* Want others to triple check the branch still kosher for them

I compared branch 1311-int with my pull request. It looks good. All tests pass.

* Would like a sanity check on whether the `gssapi above 1.0.0` in the install docs is accurate or if we need to scope it to eg `>=1.5.0`

We (s+c) use version 1.4.1 of the new gssapi. Therefore I propose >=1.4.1, see pull request #1461.

Anselm Kruis and others added 2 commits Jun 19, 2019
Worries me that it was not caught by the implementer though,
doesn't that mean this method was not called by their testing?

(cherry picked from commit 2c15c19)
@ecederstrand
Copy link

@ecederstrand ecederstrand commented Jun 20, 2019

Seems tests are now green except for f3af0b3 which introduced a flake8 error (line too long).

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

Back on this, thanks @akruis for the thumbs up & doc tweaks.

With fresh eyes, and spurred by that doc update, I am re-examining whether we can use extras_require here. There's two obvious ways we actually can:

  • Just offer two different extras, eg paramiko[gssapi] for Unix and paramiko[gssapi-win] or something for Windows. Not ideal but still saves folks one or two minor steps.
  • I'd forgotten that modern setup.py includes those semicolon-delimited dependency specifiers: https://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-platform-specific-dependencies - suspect we can use this to avoid the previous bullet point.
    • I don't have a good Windows testbed to triple check, but can do my usual, which is to ensure it works OK for Unix and hope one of our Windows users files a bug (and uh, that I actually see that bug report 😒)
    • Also, I'm assuming that extras_require and the platform-specific extension both work in the same spot...guess we'll find out.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

Cool, that (ecae381) seems to Just Work on my macOS system 👍

  • Regular pip install -e . is a noop
  • pip install -e .[gssapi] starts installing pyasn1 and gssapi dependencies, but not pywin32.
  • Obviously this does not prove that it works on Windows, but see above.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

Merged latest master in and fixed conflicts (and saw / fixed the flake8 failure @ecederstrand noticed). Now merging back into master & pushing - this should be done now. Will see whether I can easily bundle something else into a 2.6 release and then put that out. Thanks all!

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 21, 2019

Ah, this won't autoclose because it's targeting 2.4 branch for w/e reason.

@bitprophet bitprophet closed this Jun 21, 2019
@ecederstrand
Copy link

@ecederstrand ecederstrand commented Jul 5, 2019

My GitHub skills are failing me. Does this mean that the gssapi package is supported in the latest release (2.6)?

@ploxiln
Copy link
Contributor

@ploxiln ploxiln commented Jul 6, 2019

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants