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

Port to python-gssapi from pykerberos #529

Closed

Conversation

frozencemetery
Copy link
Contributor

@frozencemetery frozencemetery commented Feb 26, 2018

This PR

  • I've read the DCO.
  • I've read the Coding Guidelines
  • The relevant informations about the changes stands in the commit message, not here in the message of the pull request.
  • Code changes follow the style of the files they change.
  • Code is tested (provide details).

References

Additional information

I'm a drive-by contributor; I work on Kerberos, not mail servers. I am of course happy to test this change, but I don't know how, and I'm not seeing any GSSAPI tests in the tree. How do you recommend I test this change without pointing at a production mailserver that I care about?

(edit: I have tested code; more information in comment below)

@chris001
Copy link
Member

@frozencemetery

I am of course happy to test this change, but I don't know how, and I'm not seeing any GSSAPI tests in the tree. How do you recommend I test this change without pointing at a production mailserver that I care about?

I've shared some generated credentials for several mailboxes I created on the "onmicrosoft" cloud, for the purposes of running automated tests using the built in travis-ci here on github. @nicolas33 has added the cleartext credentials as encrypted credentials in the format expected by travis-ci, to his fork of the repo, and these encrypted credentials are to be inserted as variables in the .travis.yml test script. to cycle thru connecting to the remote mail servers, login with username and password, and send email back and forth between these accounts inboxes, pause, and check and see if the mail arrived as expected, or failed and generated the expected error code.

@nicolas33
Copy link
Member

I can't test this out. Ideally, we would have the travis testing suites ready but we don't. BTW, testing a patch about authentication is not dangerous: either it can authenticate or it can't.

I can think of:

  1. Merging this blindly (risky?).
  2. Find someone who can test the patch.
  3. Rewrite the patch to introduce a new mechanism like pGSSAPI instead of replacing the current GSSAPI. This would be the best approach if the deadline were not so short.

@chris001
Copy link
Member

This PR is a good reason to activate Travis on this repo, and get .travis.yml working, at least to test login.

@nicolas33
Copy link
Member

Do you have a travis yaml file to submit?

@chris001
Copy link
Member

OK I'll get one together test it out and and submit it.

@chris001
Copy link
Member

@nicolas33
I looked at the existing test/README. The test code expects one IMAP account, and one GMail account. Good. For IMAP, will use one of the "onmicrosoft" email inboxes. For the GMail inbox, is there already a gmail account for testing purposes? If no, I'll create one, and transmit the credentials to you via email to add as secure encrypted variables, for use by the new .travis.yml test settings.

@nicolas33
Copy link
Member

There's no online accounts other than those you provided that I'm aware of.

@chris001
Copy link
Member

chris001 commented Mar 1, 2018

@nicolas33
Spent significant time working on the automated testing tonight.

  • Had to re-register a test Outlook IMAP account, as they had expired after 6 months, + register a new test GMail account.
  • Also for GMail, had to generate for XOAUTH2 login, oauth2_client_id, oauth_client_secret and oauth2_refresh_token.
    Questions while in the middle of getting this automated testing to work:
  1. @spaetz @pilou- @dolohow @djl
    Referring to the TestRunner.py you guys contributed to:
    https://github.com/OfflineIMAP/offlineimap/blob/master/test/OLItest/TestRunner.py
  • import imaplib
    This is importing at runtime the system library from /usr/lib/python2.7/imaplib.py !! Which is failing to login.
    Shouldn't this import really be:
    import offlineimap.virtual_imaplib2 as imaplib ?
  1. Another question on your TestRunner.py OLITestLib class:
    After upgrading the import, it's still failing to login, but it gets farther.
  • In your TestRunner.py, why don't you call the same code to login, as used by the real OfflinImap class?!
  • Why is TestRunner.py re-writing the login code, but in a bare minimal form that only understands basic auth LOGIN and cleartext passwords?! As you know, this cleartext "less secure apps" login method generally breaks the GMail recommendations to use TLS encryption, XOAUTH2, and refresh_token.
  • After some hours, I have an identical settings file ~/.offlineimaprc and test/credentials.conf which works great when you invoke the app, ./offlineimap.py, and fails when you invoke the test, python setup.py test. This should not happen?! The same settings file should work the same, and be fully compatible with both the app run normally, and with the test runner!
  • It would be nice to enlist you four guys' experience to fix the OLITestLib class, so that it runs successfully (login successfully by using the same login class as OfflineImap) with a valid config settings file test/credentials.conf === ~/.offlineimaprc. I REALLY don't think in 2018 we should be so backwards as to run our tests, on the testing servers at the famous travis-ci.org which would be connecting to gmail.com and microsoft outlook.com with reckless cleartext login passwords over the internet !!! Yes, I know, it's only testing, the data itself is of no value, but still, it's unnecessary to login via cleartext, this is not 1995, we can do better, we can use encryption, as is expected to do now. Besides, how else do you test the encrypted connection, without having OLITestLib connect to the mail servers thru every supported type of encrypted connection. This PR was originally about the need to test python kerberos GSSAPI, therefore, OLITestLib must use the same login class as used by the OfflineImap main application.
  1. This question is for @nicolas33 . The encrypted variables are all commented out, and besides, they're in only your fork of this repo. https://github.com/nicolas33/offlineimap/blob/ns/ci/.travis.yml Instead, they must be uncommented, and you are the only one (as repo owner) who should push a commit to the upstream master repo, here: https://github.com/OfflineIMAP/offlineimap/blob/master/.travis.yml And then enable this repo on https://Travis-CI.org These encrypted variables are only accessible here, their home repo. A clone or a fork of this home repo will not reproduce these variables to other repos for others to access them programatically at runtime, for security reasons, so, they're safe to upload here.

@nicolas33
Copy link
Member

nicolas33 commented Mar 1, 2018

1. 2. The current test suites is unmaintained. I think it would be easier to introduce a new feature tests suites. Only checking the exit code would be a good starting point, IMHO. Resurrecting the legacy test suites is significant work.

3. Yes, this commit is a WIP started from the accounts info you provided. Feel free to start from this file and rewrite it. ,-)
I'll apply the yaml file to a new temporary branch before getting it into the mainlines. You're right I'll have to activate https://travis-ci.org/OfflineIMAP/offlineimap.

python-gssapi has a visible, active upstream and a more pleasant
interface.  python-gssapi is present in most distributions, while
pykerberos is slated for removal from Fedora/RHEL/CentOS.

Tested-by: Robbie Harwood <rharwood@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
@frozencemetery
Copy link
Contributor Author

Thanks! I tested these changes against my mailserver here (--dry-run) and it's working. I've pushed slight code changes and added my Tested-by.

@chris001
Copy link
Member

chris001 commented Mar 1, 2018

That's great @frozencemetery
I've gotten CI tests running on my fork.
One final step - write the credentials to config file at runtime - and CI tests will automatically test the login methods.
I've configure it so that it tries all 5 supported login methods, one at a time - including GSSAPI.
As long as the remote IMAP server supports the 5 login method, the login tests should pass OK.

nicolas33 pushed a commit that referenced this pull request Mar 2, 2018
python-gssapi has a visible, active upstream and a more pleasant
interface.  python-gssapi is present in most distributions, while
pykerberos is slated for removal from Fedora/RHEL/CentOS.

Github-ref: #529
Tested-by: Robbie Harwood <rharwood@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Signed-off-by: Nicolas Sebrecht <nicolas.s-dev@laposte.net>
@nicolas33
Copy link
Member

Thank you much! Patch applied.
I've added gssapi as optional dependency in the README and requirements.txt.

@frozencemetery I might make a new release soon if you feel this helps your purpose. Let me know.

@chris001 I'm closing this PR. Let's open a new issue/PR for the test suites. Happy to hear there are changes in this area. My gentle advice: start small, very small. Improvements can come later. ,-)

@nicolas33 nicolas33 closed this Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants