Do not remove cached credentials when login with wrong password
Adphi opened this issue · 20 comments
First of all, thanks for this project 😀.
I'm trying to configure an office365 proxy for an old application.
The idea is to configure the proxy once, then use only the cached credentials to run in a headless environment.
I was doing some testing to configure the initial credentials when I found that attempting to log in with an invalid password clears the cached access token and refresh token.
The exception is raised when the decryption fails:
email-oauth2-proxy/emailproxy.py
Line 335 in 89f2f7c
and caught as InvalidToken which then deletes the cached credentials instead of sending an unauthorised or invalid crendentials error:
email-oauth2-proxy/emailproxy.py
Lines 342 to 360 in 89f2f7c
How could this be handled so that invalid credentials do not logout a legit user ?
The challenge here is that there is no "correct" password to use with an account, which is why you can use any password you like locally. Invalid credentials might not match the saved values, but there's no real way to find out which ones are preferred if both sets are subsequently authenticated successfully via OAuth 2.0. The current behaviour does however mean that you can get into a situation where multiple competing clients repeatedly invalidate an account if you've set them up with different local passwords.
On balance, I think you're probably right that it would be better to give an error rather than removing the credentials in this situation. I'll make this change in the next update.
I did take a deeper look, here is what I ended up with:
diff --git a/emailproxy.py b/emailproxy.py
index b0f7600..dce72b8 100644
--- a/emailproxy.py
+++ b/emailproxy.py
@@ -322,17 +325,19 @@ class OAuth2Helper:
AppConfig.save()
else:
+ try:
+ access_token = OAuth2Helper.decrypt(cryptographer, access_token)
+ decrypted_refresh_token = OAuth2Helper.decrypt(cryptographer, refresh_token)
+ except InvalidToken as e:
+ Log.error('Invalid password for account', username, '- aborting login', Log.error_string(e))
+ return False, '%s: Login failed - the password for account %s is invalid' % (APP_NAME, username)
if access_token_expiry - current_time < TOKEN_EXPIRY_MARGIN: # if expiring soon, refresh token
- response = OAuth2Helper.refresh_oauth2_access_token(token_url, client_id, client_secret,
- OAuth2Helper.decrypt(cryptographer,
- refresh_token))
+ response = OAuth2Helper.refresh_oauth2_access_token(token_url, client_id, client_secret, decrypted_refresh_token)
access_token = response['access_token']
config.set(username, 'access_token', OAuth2Helper.encrypt(cryptographer, access_token))
config.set(username, 'access_token_expiry', str(current_time + response['expires_in']))
AppConfig.save()
- else:
- access_token = OAuth2Helper.decrypt(cryptographer, access_token)
# send authentication command to server (response checked in ServerConnection) - note: we only support
# single-trip authentication (SASL) without actually checking the server's capabilities - improve?
What do you think ?
Thanks for the suggestion. However, unless you've also changed the line after the diff, this version also removes the normal token usage in the event that there is a valid token present. It would probably be better to remove the token deletion (and authentication recursion) section instead, I think.
However, I'm having second thoughts about entirely removing this behaviour as I think it is useful not to give the impression that accounts with, e.g., a mistyped local password are somehow locked or inaccessible when simply removing the credentials will let you authenticate again. Perhaps a clearer error message and a setting to configure this behaviour might be better?
I'm also curious about the actual real-world likelihood of this happening. Of course, in account setup it can be a common issue due to typos etc, but after this isn't it quite unlikely to occur? Or are you maybe entering your account password each time you check for email?
this version also removes the normal token usage in the event that there is a valid token present
I don't see how... The only thing it does is checking if the password is correct for the encrypted token and refresh token, reject if the password is not valid or continue with the normal behaviour.
I'm also curious about the actual real-world likelihood of this happening
My bad... I didn't give you enough context.
One of my clients has an old php application that uses imap connections with Office 365.
The app can't be updated to use OAuth credentials, so we need to proxy it.
The proxy will run in headless mode in a Docker container.
Since the authentication flow is manual, we don't want an application trying to connect to the proxy to trigger token deletion if the application is configured with the wrong credentials (due to some operator errors).
You can take a look at the changes I did here.
Thanks for following up - and yes, you are correct about your changes still maintaining the correct behaviour (you deleted lines 334 and 335, but I missed that you had moved this to earlier in the process).
Thanks also for the additional context – this makes more sense. I'll think again about how this behaviour could work. My first thought is to keep the current default, but add an option in the configuration file.
I'm curious about your addition of listen_address – this just seems like a duplication of redirect_uri. What situation is this used in?
I'd like to point to your fork in the proxy's readme as an example of using it in a Docker container. Is your repository likely to remain available publicly?
I'm curious about your addition of listen_address – this just seems like a duplication of redirect_uri. What situation is this used in?
The listen_address is used to say to the server to listen to that ip:port which may be different from the redirect_uri which is used in the query url for the callback. In a container the exposed ports are natted. In this case, the http server is also proxied, so I needed a way to say: listen to this address and announce this callback url. Also the usage of redirect_uri as the server address does not support using an url (e.g. https://auth.example.org).
I'd like to point to your fork in the proxy's readme as an example of using it in a Docker container. Is your repository likely to remain available publicly?
Actually, my initial intention was to come back to you with a pull-request with Docker/ docker-compose / Dockerfile support. But it was mostly useless without the listen_address as the auth flow should have been done locally, then use the configuration with access and refresh token to deploy as container...
I'm still confused about listen_address – the whole point of redirect_uri is that you get redirected here automatically after authenticating. Your changes mean that the local server is listening at a different ip:port, but the OAuth 2.0 result is still going to come via a redirect to the original redirect_uri. How do you tie these together – are you manually editing the OAuth result afterwards to point it to listen_address?
Also the usage of redirect_uri as the server address does not support using an url (e.g.
https://auth.example.org).
Are you sure this is the case? The specification seems to suggest this is fine.
I'm still confused about listen_address – the whole point of redirect_uri is that you get redirected here automatically after authenticating.
Yes, but the server may not be exposed directly (when inside a container or on a private network). Let's say that the email-oauth2-proxy redirect_uri is http://10.232.1.20:8443, the server will listen on 10.232.1.20:8443, when the container port is mapped, it will become the machine ip and mapped port, e.g. 192.168.42.112:443. Then there will be a NAT rule on the router or a reverse proxy that will redirect traffic there...
So if you give the local address to the oauth server in the redirect_uri the browser will never be able to reach the callback uri.
Your changes mean that the local server is listening at a different ip:port, but the OAuth 2.0 result is still going to come via a redirect to the original redirect_uri
yes, that is the whole point.
Also the usage of redirect_uri as the server address does not support using an url (e.g. https://auth.example.org).
Are you sure this is the case? The specification seems to suggest this is fine.
You are right, OAuth does.
What I mean is, as you parse the redirect_uri:
email-oauth2-proxy/emailproxy.py
Lines 380 to 385 in dc8b717
and then use it to tell the server where to listen:
email-oauth2-proxy/emailproxy.py
Lines 401 to 403 in dc8b717
it is expected to be an ip:port.
In that context, redirect_uri does not support using an url (e.g. https://auth.example.org).
I understand that the initial project goal is to provide a local proxy (with a minimal gui) to run on a workstation, but when used in a headless / server context, we cannot use localhost or a local ip as it may not be directly accessible from the browser.
Ok, that makes sense. Like you say, a lot of this comes from the proxy's original design of a simple local GUI, but I appreciate it is also very useful in a server context. I think we can find a way to incorporate this.
Out of interest, how are you actually achieving authentication? #33 has a discussion of improving this in a headless context – do you have a better solution, or are you just doing this manually before deploying (and then re-deploying when authentication is required again)?
The device flow may be interesting to avoid having to create a server for the oauth flow. But it would still require user interaction, and following an url (probably written in the logs / stdout).
I don't know if office365 or Gmail provides a machine to machine flow.
Also, I know office365 allows the password grant, which is kind of deprecated, but the client app could continue to use the email / password and the proxy use that for the password grant flow.
Thanks – I think for the time being the manual approach prior to deployment is still the most appropriate then.
One more thing re: listen_address – is this something that you might want to set uniquely per server/account, or is it more of a global setting for the proxy?
I think it might be good to share the oauth client server for all configured accounts providers by email domain and not having to configure separate accounts.
The issue 42 branch now has both of these features: 43bfe51 adds a configuration file option to enable or disable credential deleting when an invalid password is provided, and 02636f0 adds redirect_listen_address.
It proved overly complex/brittle to be worth sharing the listen address, so this is just duplicated in each account as with all of the other configuration options. I think this addresses both suggestions now, though so it'd be great to get your feedback.
Just following up on this – I intend to merge this branch unless I hear otherwise.
In that context,
redirect_uridoes not support using an url (e.g. https://auth.example.org).
Could you clarify what you mean here? You can use a URL via, e.g., redirect_uri = https://auth.example.org:443. The same works for, e.g., redirect_listen_address = http://auth.example.org.
In that context, redirect_uri does not support using an url (e.g. https://auth.example.org/).
Could you clarify what you mean here? You can use a URL via, e.g., redirect_uri = https://auth.example.org:443. The same works for, e.g., redirect_listen_address = http://auth.example.org.
I might be wrong, but if you set redirect_listen_address to auth.example.org:443 and there is no record pointing the hostname auth.example.org to a local ip address in the /etc/hosts file, the server will not be able create the listening socket.
Ok, but is this something the proxy could ever actually handle itself? The redirect_listen_address is for a fairly advanced configuration where you're already setting up external redirection or address translation. How else could it do this?
The redirect_listen_address will most likely contain the wildcard ip and a port, for example 0.0.0.0:8080, so it seems to me that 02636f0 is sufficient.
I think all the features mentioned in this discussion are great.
Regarding the password problem, what do you think of the following idea:
When saving the tokens, a hash of the used password is stored in the config. When authenticating, a hash for the given password is generated and compared with the stored one.
@gegu I'm not sure this is necessary, since decrypting the token fails if the password is invalid, which already provides a password validation mechanism.
You're absolutely right, of course.