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

Reject blank passwords; use SSLContext for all urllib.request.urlopen requests #321

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DiablosOffens
Copy link

@DiablosOffens DiablosOffens commented Jan 24, 2025

I've split the changes into two commits:

  • Let IMAP authentication immediately fail when no password was given (dd44eca):
    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.
  • Use SSLContext at all places where SSL connection could be opened (dc8fb08):
    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:

[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: certificate has expired (_ssl.c:1018)

@DiablosOffens DiablosOffens marked this pull request as ready for review January 25, 2025 00:11
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.
@simonrob
Copy link
Owner

Thanks for the suggestions here.

Re: dd44eca, I'm not quite clear about why this is such an issue. Currently a LOGIN without a password gives BAD (from the remote server) in response. Are you using a client that reacts differently to this than it does to NO? My interpretation of the RFC is that the response to a missing password (i.e., an invalid command) should be BAD, and although I can see that this could be debated as it is not explicitly addressed, this is currently the response that both Outlook and Gmail provide. Either way, I think it's easier to let the remote server handle this. This is of course the same behaviour as it would have been before using the proxy, so clients that can't handle this would never have worked anyway. And SMTP/POP already use the same password for the same account – indeed this is a hard requirement.

Re: dc8fb08, I'm intrigued to hear that this makes any difference at all. Using urllib.request.Request does not require a SSLContext to be provided in order to connect to secure remote servers, and the proxy works perfectly fine connecting to such servers as-is (see, for instance, token retrieval). The error you're receiving is about expired certificates, and probably similar to previous issues that have discussed this – I'd recommend taking a look at those.

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.

@simonrob simonrob closed this Jan 26, 2025
@DiablosOffens
Copy link
Author

DiablosOffens commented Jan 27, 2025

Thank you for looking into it.
First of all, my changes are just a quick and dirty hack, so that I get this to work at least. I did not read all RFCs to the end and my Python skills are also not the best, so it's maybe not the final elegant solution. I just shared it here for reference and maybe to start a discussion.

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 LOGIN, it doesn't contact the remote server first. Instead it just goes straight to the authorisation code path (e.g. line #1665) without a password, which also does not send back an error to the client, although it was the very first authorisation. So, the client has no chance to see a BAD or NO or whatever it needs to fulfill the RFC. To make it even worse, the proxy directly encrypts the received token with an empty password when authorisation was successful and saves it into config. So decryption of the token at any point later will give wrong data, even if client now would get the BAD message. SMTP/POP proxies don't suffer from this issue, because they check (lines #2149,2277) the password first before they do the authorisation.

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.

@simonrob
Copy link
Owner

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 get_oauth2_credentials would be a better approach, though – see the reject-blank-password branch. Could you see whether this fixes your client's behaviour?

@DiablosOffens
Copy link
Author

DiablosOffens commented Jan 29, 2025

Yes this branch will also fix the issue. Here is an excerpt from debug log file:

2025-01-29 17:38:30,909: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993)     <-- b'* CAPABILITY IMAP4 IMAP4rev1 AUTH=PLAIN AUTH=XOAUTH2 SASL-IR UIDPLUS ID UNSELECT CHILDREN IDLE NAMESPACE LITERAL+\r\n'
2025-01-29 17:38:30,911: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993) <-- b'* CAPABILITY IMAP4 IMAP4rev1 AUTH=PLAIN SASL-IR UIDPLUS ID UNSELECT CHILDREN IDLE NAMESPACE LITERAL+\r\n'
2025-01-29 17:38:30,912: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993)     <-- b'nh5f OK CAPABILITY completed.\r\n'
2025-01-29 17:38:30,912: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993) <-- b'nh5f OK CAPABILITY completed.\r\n'
2025-01-29 17:38:30,913: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993) --> b'77jx LOGIN [[ Credentials removed from proxy log ]]\r\n'
2025-01-29 17:38:30,914: No password provided for account ...@hotmail.com - aborting login
2025-01-29 17:38:30,914: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993) <-- b'77jx NO LOGIN Email OAuth 2.0 Proxy: Login failed - no password provided for account ...@hotmail.com\r\n'
2025-01-29 17:38:43,306: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993) --> b'l43k LOGIN [[ Credentials removed from proxy log ]]\r\n'
2025-01-29 17:38:44,673: Authorisation request received for ...@hotmail.com (interactive mode)
2025-01-29 17:38:44,680: Please authorise your account ...@hotmail.com from the menu
2025-01-29 17:38:54,626: Waiting for URL matching `redirect_uri`; following browser redirection to login.live.com/[...]
2025-01-29 17:39:01,871: Returning authorisation request result for ...@hotmail.com
2025-01-29 17:39:02,057: Authentication completed for ...@hotmail.com
2025-01-29 17:39:02,786: IMAP (127.0.0.1:52449-{127.0.0.1:1993}-outlook.office365.com:993)     --> b'l43k AUTHENTICATE XOAUTH2 '

@DiablosOffens
Copy link
Author

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.
So fiddle around with the CA and intermediate certificate stores in machine and user context only for one specific program isn't the right fix, as these will be handled by the OS itself with Windows Updates. As a proof that these expired certificates are not picked by default for certificate verification, when there are new certificates at the same time, look at all other native Windows programs. Both certificates (new and expired) share the same attributes for the Subject Key Identifier and Authority Key Identifier (OID values: 2.5.29.14 and 2.5.29.35), to create the relation to each other.
I tested the same URL in Edge Chromium and in a custom PowerShell Script by using the default HttpWebRequest APIs from .NET, all without any verification issues. It is only Python itself or how the Python APIs for SSL are used, which does strange things here.

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.
FYI: The SSLContext object is not provided to urllib.request.Request in my code, but is passed directly to urllib.request.urlopen as parameter. What actual impacts this has also goes beyond my knowledge. I was just using ssl.create_default_context to create a default context with only 2 different options (purpose=ssl.Purpose.SERVER_AUTH , minimum_version = ssl.TLSVersion.TLSv1_2). I was in the impression that this will still verify the certificates, but uses other rules to select the certificates from the chain. But as I said earlier, my Python skills are not good enough to verify this yet.
With the current environment and without my fix applied I also get errors in the Windows Event Log for CAPI2 (event id 60), at the same time when this certificate verification fails. Here is 1 example as XML excerpt from the several errors:

...
<UserData>
  <CertificateStore>
    <Store type="CERT_STORE_PROV_SYSTEM_A" constant="9" location="CERT_SYSTEM_STORE_SERVICES_ID">CA</Store> 
    <Flags value="58000" CERT_STORE_READONLY_FLAG="true" /> 
    <EventAuxInfo ProcessName="python3.13.exe" /> 
    <CorrelationAuxInfo TaskId="{A02E84FA-B718-414C-8461-318D6710D9FD}" SeqNumber="1" /> 
    <Result value="80070057">Falscher Parameter.</Result> 
  </CertificateStore>
</UserData>

The other errors only differ in the location attribute and content (CA/ROOT) of the Store tag.
The corresponding Windows API that logs these type of errors is CertOpenStore.
So I suspect that at least Python is doing here something wrong.

@simonrob
Copy link
Owner

simonrob commented Feb 3, 2025

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?

simonrob added a commit that referenced this pull request Feb 4, 2025
@simonrob simonrob changed the title Some fixes for blocking issues Reject blank passwords; use SSLContext for all urllib.request.urlopen requests Feb 4, 2025
@simonrob
Copy link
Owner

simonrob commented Feb 4, 2025

I've merged the blank password change, and will reopen this for wider visibility in case others have thoughts about the SSLContext change you've proposed. Just to reiterate, though – I'd like to be able to reproduce this issue before making this change.

@simonrob simonrob reopened this Feb 4, 2025
Comment on lines -1681 to +1715
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'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obsoleted by bf0cc29

@DiablosOffens
Copy link
Author

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+.

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.

2 participants