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

.cryptkey Error During Non-SSO Account Switching after SSO Login or vice-versa #1746

Closed
avinash-0007 opened this issue Sep 13, 2024 · 16 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@avinash-0007
Copy link

avinash-0007 commented Sep 13, 2024

Description:

We have identified an issue involving the .cryptkey file that causes errors when switching accounts after using both SSO and Non-SSO login methods.

Steps to Reproduce:

  1. Register a new account .

  2. Log in with SSO and add an additional account via SSO. A .cryptkey file is created in storage/domain/user/.cryptkey.

  3. Attempt to switch between the accounts — it works as expected.

  4. Log out and log back in without using SSO.

  5. Attempt to switch accounts again — an error occurs, and the following error is logged:

[2024-09-11 14:19:32.658][b4f90fd4] JSON[INFO]: {"Action":"AccountSwitch","Result":false,"ErrorCode":803,"ErrorMessage":"AccountSwitchFailed[803]","ErrorMessageAdditional":"CryptKeyError[111]","ExceptionCode":0,"epoch":1726064372}
[2024-09-11 14:19:32.694][b4f90fd4] [INFO]: Memory peak usage: 8MB
[2024-09-11 14:19:32.724][b4f90fd4] [INFO]: Time delta: 0.55190300941467
[2024-09-11 14:19:59.935][3e510555] [INFO]: [SM:2.37.2][IP:2401:4900:1c63:4335:c50:b8c2:84fe:e8f3][PID:1192][nginx/1.24.0][fpm-fcgi][Streams:tcp,udp,unix,udg,ssl,tls,tlsv1.0,tlsv1.1,tlsv1.2,tlsv1.3][POST https://xxxxx.com/apps/snappymail/?/Json/&q[]=/0/]
[2024-09-11 14:19:59.975][3e510555] Nextcloud[DEBUG]: integrated
[2024-09-11 14:20:00.034][3e510555] JSON[INFO]: Action: DoAppDelayStart
[2024-09-11 14:20:00.069][3e510555] POST[INFO]: {"Action":"AppDelayStart"}
[2024-09-11 14:20:00.100][3e510555] COOKIE[DEBUG]: set smtoken
[2024-09-11 14:20:00.130][3e510555] JSON[INFO]: {"Action":"AppDelayStart","Result":true,"epoch":1726064400}
[2024-09-11 14:20:00.170][3e510555] [INFO]: Memory peak usage: 6MB
[2024-09-11 14:20:00.204][3e510555] [INFO]: Time delta: 0.43127679824829

   

### **Temporary Workaround:**
- Delete the `.cryptkey` file from `storage/domain/user/.cryptkey`.
- After deleting the `.cryptkey`, delete and re-add the account using Non-SSO login.
- Account switching then works correctly without error.

### **Issue Context:**
This issue seems to be related to the `.cryptkey` file behavior when transitioning between SSO and Non-SSO logins. 



![Screenshot 2024-09-13 at 6 43 34 PM](https://github.com/user-attachments/assets/3229e5f6-dd48-428b-b22e-e162827453a5)



@avinash-0007
Copy link
Author

@the-djmaze Please check this one as cryptkey is not working when we switch between sso to non sso or vice-versa

@avinash-0007 avinash-0007 changed the title .cryptkey Error During Non-SSO Account Switching after SSO Login .cryptkey Error During Non-SSO Account Switching after SSO Login or vice-versa Sep 13, 2024
@the-djmaze
Copy link
Owner

the-djmaze commented Sep 13, 2024

This is correct because in OAUTH the password (access token) constantly changes.

That's why the GMail extension sets something different as "password".

$oPassword = new \SnappyMail\SensitiveString($aUserInfo['id']);

It uses the Gmail userinfo id and that is not stored on the server.
So this is mostly "secure enough" to prevent misuse/decode of the cryptkey outside SnappyMail.

You must use a similar approach to get the cryptkey stable

@the-djmaze the-djmaze added documentation Improvements or additions to documentation bug Something isn't working enhancement New feature or request labels Sep 13, 2024
@avinash-0007
Copy link
Author

@the-djmaze Your fix is not working. It still have the same error.

@the-djmaze
Copy link
Owner

the-djmaze commented Sep 16, 2024

Ok, but the PR can't be in the core.
OC class is not available outside NextCloud.

A better aproach must be made in the extension and Nextcloud code then.

@avinash-0007
Copy link
Author

avinash-0007 commented Sep 16, 2024

@the-djmaze Yes you are right. But we do have some limitation thats why i gone this way:

Even if i do crypt code inside SnappyMailHelper.php

$oStorage = \RainLoop\Api::Actions()->StorageProvider();

$sKey = $oStorage->Get($doLogin, \RainLoop\Providers\Storage\Enumerations\StorageType::ROOT, '.cryptkey');

$uID = \OC::$server->getUserSession()->getUser()->getUID();

if (!$sKey) {
	$sKey = \SnappyMail\Crypt::EncryptToJSON(\sha1($uID . APP_SALT),$uID);
	$oStorage->Put($doLogin, \RainLoop\Providers\Storage\Enumerations\StorageType::ROOT, '.cryptkey', $sKey);
}
$sKey = \SnappyMail\Crypt::DecryptFromJSON($sKey, $uID);
$sCryptKey = new SensitiveString(\hex2bin($sKey));
$oStorage->Put($oAccount, StorageType::SESSION, \RainLoop\Utils::GetSessionToken(),
\SnappyMail\Crypt::EncryptToJSON($uID, $oAccount->CryptKey())
);

Still problem is this:

https://github.com/the-djmaze/snappymail/blob/master/snappymail/v/0.0.0/app/libraries/RainLoop/Model/AdditionalAccount.php#L49

When i have two login option one is with SSO and one is without SSO if CryptKey function gets call from both then
IncPassword
https://github.com/the-djmaze/snappymail/blob/master/snappymail/v/0.0.0/app/libraries/RainLoop/Model/MainAccount.php#L44
is different for sso and non-sso so it will never decrypt correctly in any one case. So the account added with non sso will not switch properly with sso login and vice-versa.

So that is why i modified the code here and i also if you see ELSE section is same. In case of two login (this is oidc_login ) option is enabled this IF section get executed.

I understand we are using it with rainloop core files but in extreme cases this is the possible option.

If you have any suggestion , how can i approach this other then this way then let me know.

@avinash-0007
Copy link
Author

@the-djmaze Is there any suggestion from your end how we can allow both login one with sso and one with non sso to have .crypt key same. Bcoz if i try to setPassword to same in both cases either one autologin stops working.

Even currently with your fix autologin for sso is not working in 2.38.0

@the-djmaze
Copy link
Owner

I want to get something cleared up:

  1. You have an email address something@example.com
  2. User logs in with that email address (no SSO)
  3. User also logs in with SSO and has that email adress

So you use 2 login methods for the same email address.

Correct?

@avinash-0007
Copy link
Author

@the-djmaze Yes, its correct. I have two way to login with same email

@the-djmaze
Copy link
Owner

the-djmaze commented Sep 24, 2024

Yes, its correct. I have two way to login with same email

Then it is not possible.
The cryptkey is encrypted with the "password".
Else it would be useless to secure anything stored on the server.

A better approach would be a system option insecure_cryptkey = on
Then it will be your responsibility that user data is not secure and any security breach you must inform your users their passwords might be harvested.

Using SSO and normal login also pretty defeats the security of SSO.

@the-djmaze the-djmaze removed the bug Something isn't working label Sep 24, 2024
@akhil1508
Copy link

akhil1508 commented Sep 25, 2024

@the-djmaze

You have an email address something@example.com
User logs in with that email address (no SSO)
User also logs in with SSO and has that email adress

Actually, no. For me the problem is:

  1. I have an email address something@example.com which is associated with an SSO account
  2. I login with SSO for that; things just work ✅
  3. I have an additional account that I use for business something@business.com; this doesn't work when using SSO; this one is not associated with any SSO account and is just an email address from some other provider ❌

@akhil1508
Copy link

akhil1508 commented Sep 25, 2024

@the-djmaze @avinash-0007 Would it make sense to decrypt and encrypt again with new access token whenever the access token is updated with a Nextcloud event listener? As we do have an AccessTokenUpdatedEvent(we probably need to modify also the OIDC plugin to output both old and new tokens in the event but it is do-able).

Would it even be do-able as this would be code in the nextcloud integration app and no longer in a snappymail plugin.

And at account addition, we encrypt additional account key with the current access token in the plugin.

@the-djmaze
Copy link
Owner

the-djmaze commented Sep 25, 2024

@the-djmaze @avinash-0007 Would it make sense to decrypt and encrypt again with new access token whenever the access token is updated with a Nextcloud event listener? As we do have an AccessTokenUpdatedEvent(we probably need to modify also the OIDC plugin to output both old and new tokens in the event but it is do-able).

Actually the password should not be the access token.
It should be something that is not known by server and does not change.
But since that is not possible with Nextcloud, my change should be sufficient.

Standalone SnappyMail Google auth extension uses the OpenID Connect id as that never changes and is not stored/logged on the server.

@akhil1508
Copy link

akhil1508 commented Sep 25, 2024

@the-djmaze I understand your change in the most recent commit now. Would you say the insecure_cryptkey is similar to the autologin password encode/decode for nextcloud in

$sPassword = static::decodePassword($sPassword, \md5($sEmail));
where the username/email is used as the salt?

@the-djmaze
Copy link
Owner

the-djmaze commented Sep 25, 2024

Yes. Email address + app salt.

@akhil1508
Copy link

Standalone SnappyMail Google auth extension uses the OpenID Connect id as that never changes and is not stored/logged on the server.

@the-djmaze Isn't the email in 6b03820#diff-cbd3c0fe0c611f44da419210ab32e2b7bcf995aae9517b01fb8287bf1c2eafc7R46 stored on server?

@the-djmaze
Copy link
Owner

the-djmaze commented Sep 25, 2024

Yes, so it reopens the issue:
RainLoop#2134

If there is a way in Nextcloud to have something from OIDC that is not stored on server then it is fine.
But therefore a hook in oidc_login must be available to fetch data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants