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

Fix sending mails in combination with CRAM-MD5 authentication #1638

Merged
merged 2 commits into from May 21, 2019

Conversation

Projects
None yet
3 participants
@fredreichbier
Copy link
Member

commented May 21, 2019

Under Python 2, we need to pass passwords as bytestrings, while Python 3 expects passwords as (unicode) strings.

In order to test this, I used a modified version of a fake SMTP server:

git clone https://github.com/fredreichbier/fake-smtp-server
cd fake-smtp-server
npm install .
node ./index.js

starts a SMTP server on port 1025. This server advertises CRAM-MD5:

$ (sleep 1; echo 'EHLO test') | nc localhost 1025
220 teapot ESMTP
250-teapot Nice to meet you, localhost
250-PIPELINING
250-8BITMIME
250-SMTPUTF8
250 AUTH PLAIN LOGIN CRAM-MD5

In privacyIDEA, I now configure a SMTP server on localhost:1025 with username test and password huhu5, and test the configuration.

Without this PR, I get a privacyIDEA error "TypeError: character mapping must return integer, None or unicode". With this PR, privacyIDEA says "Test Email sent successfully", and the fake SMTP server prints:

INFO: CRAM-MD5 SMTP login for user: test password: undefined
INFO: CRAM-MD5 authentication -- is password huhu5 correct?  true

Fixes #1637

Fix sending mails in combination with CRAM-MD5 authentication
Under Python 2, we need to pass passwords as bytestrings, while Python 3
expects passwords as (unicode) strings.

Fixes #1637

@fredreichbier fredreichbier changed the base branch from master to branch-3.0 May 21, 2019

@fredreichbier fredreichbier marked this pull request as ready for review May 21, 2019

@fredreichbier fredreichbier requested a review from privacyidea/core May 21, 2019

@cornelinux
Copy link
Member

left a comment

Please tell me your thoughts.

# Under Python 2, we pass passwords as bytestrings to get CRAM-MD5 to work.
# Under Python 3, we pass passwords as unicode.
if six.PY2:
password = to_utf8(password)

This comment has been minimized.

Copy link
@cornelinux

cornelinux May 21, 2019

Member

I can not tell you why, I feel slightly unpleasent with that.
Could we maybe add a config value, so that the password will not be converted, if the value is set - like

from privacyidea.lib.framework import get_app_config_value

if six.PY2 and get_app_config_value().get("PI_MAIL_UTF8_PASSWORDS", True):
    password = to_utf8(password)

This way could "switch this off", by setting in pi.cfg

PI_MAIL_UTF8_PASSWORDS = False

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier May 21, 2019

Author Member

I think always converting the password to a bytestring under Python 2 should be fine for all three supported authentication mechanisms PLAIN, LOGIN and CRAM-MD5. But OK, better be safe than sorry, I'll add the config option :) But I guess it's ok if we keep this option undocumented for now?

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier May 21, 2019

Author Member

BTW, just wanted to note that even with this change, passwords with non-ASCII characters (like passwörd) won't work, because Python's smtplib doesn't really support them: https://bugs.python.org/issue29750.

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier May 21, 2019

Author Member

I added an option PI_SMTP_PASSWORD_AS_BYTES in c298946. What do you think? :)

This comment has been minimized.

Copy link
@cornelinux

cornelinux May 21, 2019

Member

If smtplib would one day support non-ASCII characters, to_utf8 would immediately work, while to_bytes would not. Right? So I would prefer to_utf8 - if I understand this correctly.

@codecov

This comment has been minimized.

Copy link

commented May 21, 2019

Codecov Report

Merging #1638 into branch-3.0 will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           branch-3.0    #1638      +/-   ##
==============================================
- Coverage       96.97%   96.93%   -0.05%     
==============================================
  Files             148      148              
  Lines           17845    17738     -107     
==============================================
- Hits            17306    17195     -111     
- Misses            539      543       +4
Impacted Files Coverage Δ
privacyidea/lib/smtpserver.py 98.94% <100%> (+0.04%) ⬆️
privacyidea/lib/eventhandler/base.py 95.96% <0%> (-2.69%) ⬇️
privacyidea/api/auth.py 95.53% <0%> (-0.42%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b0054c...c298946. Read the comment docs.

# Under Python 2, we pass passwords as bytestrings to get CRAM-MD5 to work.
# Under Python 3, we pass passwords as unicode.
if six.PY2:
password = to_utf8(password)

This comment has been minimized.

Copy link
@plettich

plettich May 21, 2019

Member

Maybe use to_bytes(), it basically does the same and the name is more intuitive (i think).
I think we should remove to_utf8() in the long run.

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier May 21, 2019

Author Member

Yes, will do!

This comment has been minimized.

Copy link
@fredreichbier

fredreichbier May 21, 2019

Author Member

I changed to_utf8 to to_bytes in c298946.

This comment has been minimized.

Copy link
@cornelinux

cornelinux May 21, 2019

Member

thanks, @plettich for the explanation. So this can be merged.

@cornelinux cornelinux merged commit d31613c into branch-3.0 May 21, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.97%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +3.02% compared to 9b0054c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@cornelinux cornelinux deleted the 1637/smtp-cram-md5 branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.