-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Reject blank passwords; use SSLContext
for all urllib.request.urlopen
requests
#321
base: main
Are you sure you want to change the base?
Conversation
Without it connection will fail, at least for url https://login.microsoftonline.com/common/oauth2/v2.0/token and with Python 3.13.1 on Windows. The following error was logged: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: certificate has expired (_ssl.c:1018)
Some clients (old Outlook) try IMAP authentication on first connection without a password, when it was not saved in the client beforehand. So to support scenarios where the client tries to login with IMAP on first connection to email server and OAuth 2.0 authorisation didn't happen yet, the authorisation will be delayed until the correct password is entered in the client. As this password will be used for token encryption/decryption in config file, other protocols, like SMTP, which also needs a password to work, can now use the same password to decrypt the authorisation token.
Thanks for the suggestions here. Re: dd44eca, I'm not quite clear about why this is such an issue. Currently a Re: dc8fb08, I'm intrigued to hear that this makes any difference at all. Using I'm not going to merge this, but would be happy to hear about how your use of SSLContext addresses certificate issues, as this has been a longstanding source of problems for people using outdated clients on older machines that don't always have up-to-date SSL/TLS certificates. |
Thank you for looking into it. Re: dd44eca, When I said "without a password", I actually meant an empty quoted string in the data. So it is a huge issue in the IMAP proxy, because from the client side connection, when the proxy has parsed all data from I have first seen this behaviour when I used the "test account settings" button from account settings wizzard in Outlook. It may be a different behaviour when Outlook just starts and doing a mail sync (not tested yet). But it should also just work for the test button, else it would be a bad user experience, when you are not able to test account settings. I will go into detail for the other commit in the next comment. I don't know yet how long it takes. |
Thanks for following up with this extra detail. This is quite strange client behaviour, but I do accept that it would be better if the proxy didn't treat this as valid. I think that handling this in |
Yes this branch will also fix the issue. Here is an excerpt from debug log file:
|
Re dc8fb08, Yes some statements in your mentioned issues are right about that the environment can affect how the certificates are verified. But in this case it is a default Python setup under Windows. I've found the exact code line inside Python's module where this error originates from. But how Python got into this state is currently incomprehensible for me.
The other errors only differ in the |
Thanks for following up. This reminds me of another certificate issue on Windows and a potential alternative fix. It would be good if your approach fixes things since it is far less hacky than this alternative. Since I don't use Windows I've never experienced this problem myself. Could you provide a standalone minimal code example that reliably triggers this issue so that I can explore further? |
SSLContext
for all urllib.request.urlopen
requests
I've merged the blank password change, and will reopen this for wider visibility in case others have thoughts about the |
success, result = OAuth2Helper.get_oauth2_credentials(username, password) | ||
if success: | ||
# send authentication command to server (response checked in ServerConnection) | ||
# note: we only support single-trip authentication (SASL) without checking server capabilities - improve? | ||
super().process_data(b'%s AUTHENTICATE XOAUTH2 ' % self.authentication_tag.encode('utf-8')) | ||
super().process_data(b'%s\r\n' % OAuth2Helper.encode_oauth2_string(result), censor_server_log=True) | ||
|
||
# because get_oauth2_credentials blocks, the server could have disconnected, and may no-longer exist | ||
if self.server_connection: | ||
self.server_connection.authenticated_username = username | ||
|
||
self.reset_login_state() | ||
if not success: | ||
if not password: | ||
# we can't actually check credentials here, but if password is missing, it's possible that | ||
# the client want to ask for credentials and so tried without password first, in the hope | ||
# that the server will handle it, which it won't for XOAUTH2 - intercept that here and mimic old behavior | ||
self.reset_login_state() | ||
result = '%s: Login failed - the password for account %s is incorrect' % (APP_NAME, username) | ||
error_message = '%s NO %s %s\r\n' % (self.authentication_tag, command.upper(), result) | ||
self.authentication_tag = None | ||
self.send(error_message.encode('utf-8')) | ||
else: | ||
success, result = OAuth2Helper.get_oauth2_credentials(username, password) | ||
if success: | ||
# send authentication command to server (response checked in ServerConnection) | ||
# note: we only support single-trip authentication (SASL) without checking server capabilities - improve? | ||
super().process_data(b'%s AUTHENTICATE XOAUTH2 ' % self.authentication_tag.encode('utf-8')) | ||
super().process_data(b'%s\r\n' % OAuth2Helper.encode_oauth2_string(result), censor_server_log=True) | ||
|
||
# because get_oauth2_credentials blocks, the server could have disconnected, and may no-longer exist | ||
if self.server_connection: | ||
self.server_connection.authenticated_username = username | ||
|
||
self.reset_login_state() | ||
if not success: | ||
error_message = '%s NO %s %s\r\n' % (self.authentication_tag, command.upper(), result) | ||
self.authentication_tag = None | ||
self.send(error_message.encode('utf-8')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsoleted by bf0cc29
Sorry for the late reply. I wasn't able to reproduce the certificate issue with a minimal code example yet. When I revert my changes and try to get a new access token with original proxy code, it just succeeds. Probably the OS sorted it out itself or the server changed the certificate to a version that has no doubt in chain verification. If you want to have a look into my test code, see my gist. Your mentioned issue #209 (comment) is a complete different thing and has nothing to do with my original issue. It has also already resolved with python/cpython#91740 in python versions 3.12.6+, 3.13.0+ and 3.14+. |
I've split the changes into two commits:
Some clients (old Outlook) try IMAP authentication without a password
on first connection, when it was not saved in the client beforehand.
So, to support scenarios where the client should ask for password when it
tries to login with IMAP on first connection to email server and OAuth 2.0
authorisation didn't happen yet, the authorisation will be delayed until the
correct password is entered in the client. As this password will be used for
token encryption/decryption in config file, other protocols, like SMTP,
which also needs a password to work, can now use the same password to
decrypt the authorisation token.
Without it, connection will fail at least for url https://login.microsoftonline.com/common/oauth2/v2.0/token
and with Python 3.13.1 on Windows, then the following error is logged: