Connection using certificates not working after restart

[wdehoog]

Expected behaviour

Using the installation wizard I import my certificate and complete the setup. After the client or my pc restarts the connection must be restored.

Actual behaviour

The connection is not restored and cannnot be. I have to remove the account and recreate it. The next restart the same problem occurs.

Server configuration

Operating system: ubuntu 18.04
Web server: apache2
Database: sqlite
PHP version: php 7.2
ownCloud version: 13.06 (nextcloud)

Client configuration

Client version: 2.5.0
Operating system: windows 10

Note that the Nextcloud client suffers from the same problem.

I have build branch 2.4 of the nextcloud client (probably not much different from the owncloud one) (QT 5.11.1 MinGW 32 bits). When building qtkeychain with -DUSE_CREDENTIAL_STORE=OFF the certificates are saved in the .cfg file (as PEM) and are loaded after a restart so login works.

[ckamm]

@wdehoog Does storing the basic credentials work (test with demo.owncloud.org demo/demo)? If disabling the credential store in qtkeychain helps this might be a bug upstream. :/

Could you provide log files? https://doc.owncloud.org/desktop/2.5/troubleshooting.html#logging-to-a-temporary-directory

[wdehoog]

@ckamm yes when using -DUSE_CREDENTIAL_STORE=OFF storing the basic credentials still works. I agree the problem is likely to be upstream. I am really sorry but the coming days I have no time to reinstall and provide logs.

[ckamm]

@wdehoog Ok! I was wondering whether with the credential store enabled in qtkeychain it was only the certificate storing that was broken or also the basic http credential saving.

[wdehoog]

@ckamm that is hard to tell since the credentials are only used after the certificates are ok and since the certificates could not be loaded (they were not saved) the credentials are not asked for.

Anyway I just wanted to let you know there is a problem and building the client in a slightly other way could solve it.

[guruz]

@wdehoog Then please test if the credential saving works when you add a sample account from https://demo.owncloud.com/login (see that URL in browser for credentials)

[guruz]

I have build branch 2.4 of the nextcloud client

Ooops. Wrong repo then here. Please build from https://github.com/owncloud/client.git .. also you mentioned 2.5 so you need to use the 2.4 branch.

Please re-open if ownCloud client also has issues.

[wdehoog]

@guruz why did you close this? I am reporting an issue with the owncloud client. I installed it using ownCloud-2.5.0.10598.msi. Since it has the same problem as the nextcloud client I reported the issue here.

I have built the nextcloud client and there adding the flag -DUSE_CREDENTIAL_STORE=OFF when building qkeychain worked so I thought maybe it also works for your owncloud client and you could benefit from it.

[guruz]

@wdehoog Thank you for trying to help. Are you saying you have the problem when you use ownCloud-2.5.0.10598.msi? (not self-compiled)

[wdehoog]

@guruz yes. When I use the client installed from ownCloud-2.5.0.10598.msi the problem happens. To be complete I did not test it on owncloud server but on a nextcloud server.

However the problem is on the client side. As said before building qkeychain so that it does not use the windows credentials store for saving the certificates made the nextcloud client working again (the 2.4 one that is, the 2.5 version has other problems). It can now save and load the certificates. I think it is likely that this other way of building will also fix this issue with the owncloud client.

[ckamm]

The problem is that using the windows credential store is the correct thing to do. Without it, qtkeychain uses a QSettings based plain text store (with encrypted data). The actual bug needs to be found and fixed; I may try to reproduce on windows.

[wdehoog]

@ckamm In my view the correct way would be to use the Windows Certificate Store. Not the Credentials one.
For most users it would also mean they do not have to import a certificate since it is probably already there.

Saving the certificate as a PEM in plain text file is not really a problem if the file is correctly protected.

[ckamm]

I tested this with current master on Windows 10 and for me the password, certificate and key are stored correctly in the windows credential store.

[wdehoog]

I just installed ownCloud-2.6.0.11309.11168-daily20190114.msi. Removed my account and try to add a new one. This time I could not even log in. After trying to importing my certificates (p12):

01-15 11:59:05:071 [ warning sync.networkjob ]:	SslHandshakeFailedError:  "SSL handshake failed"  : can be caused by a webserver wanting SSL client certificates
01-15 11:59:05:072 [ warning sync.networkjob ]:	QNetworkReply::NetworkError(SslHandshakeFailedError) "SSL handshake failed" QVariant(Invalid)
01-15 11:59:05:073 [ warning sync.networkjob.checkserver ]:	error: status.php replied  0 ""

[ckamm]

@wdehoog Thank you for trying again! Unfortunately the particular daily build you were using had a bug in the wizard that made it not properly set up the account (#6911). Tomorrow's daily build will have the fix.

A useful thing to try besides the new version would be to check whether your credential store can properly store credentials for accounts that don't need client certificates. Like trying to connect to demo.owncloud.com (demo/demo) and then looking for owncloud* entries in the windows credential store manager.

[wdehoog]

Sorry. Today the location I look for daily builds seems empty: http://download.owncloud.com/desktop/daily/
Should I look elsewhere?

[wdehoog]

@ckamm Today I tested using ownCloud-2.5.3.10770-daily20190117-setup.exe. Creating the account works, sync works, After stopping the client and restart login fails. See log:
owncloud-client-failed-login-after-restart.txt

[ckamm]

@wdehoog :/

Do you see the owncloud... entries in your credential store?

[ckamm]

@wdehoog A logging patch has now been merged into the 2.5 branch. If you have time, could you see whether that gets you any information about why the writes fail?

[guruz]

@wdehoog Note this will be only in TOMORROW's daily builds.
https://download.owncloud.com/desktop/daily/

[wdehoog]

@ckamm I just tested ownCloud-2.5.3.11409.11254-daily20190131.msi. nothing changed.
I removed the account and recreated it (account-setup.txt) it worked and sync finished. Then I restarted the owncloud client (after-restart.txt) and it could not connect anymore.

account-setup.txt
after-restart.txt

[ckamm]

@wdehoog Thanks for the data! I didn't expect a behavior change, but the logging has improved. I now see these warnings:

01-31 10:24:43:850 [ warning sync.credentials.http ]:	Could not write client cert to credentials 7 "Encryption failed"
01-31 10:24:43:882 [ warning sync.credentials.http ]:	Could not write client key to credentials 7 "Encryption failed"

That means it's indeed a bug with writing the cert and key to storage. Unfortunately this error is a generic one upstream in QtKeychain:

    if (!CredWriteW(&cred, 0)) {
        q->emitFinishedWithError( OtherError, tr("Encryption failed") ); //TODO more details available?

Reading the CredWrite() documentation I think the most likely cause is that the credential blob is too large, the documentation says

This member cannot be larger than CRED_MAX_CREDENTIAL_BLOB_SIZE (512) bytes.

and presumably your cert is longer than that?

Storing the certificate itself in the credential store is odd in the first place, like you pointed out earlier. The client might store the cert file on disk or look for a different storage mechanism.

EDIT: Hmm, no, the key storage also fails. Must be another reason for the error. It would be excellent if I could reproduce it locally. Would that be an option? If so, email details to mail at ckamm de.

[wdehoog]

Our certificate is probably bigger. Don't know what is stored but with some metadata 512 is filled easily. Please check your email.

[guruz]

@ckamm http://doc.qt.io/archives/qt-4.8/qbytearray.html#qCompress ? Storing on disk gives us other trouble (and is unsafe for the key)

[guruz]

Another idea would be to use https://docs.microsoft.com/de-de/windows/desktop/api/wincred/nf-wincred-credprotectw to encrypt the cert+key and then base64 encode it and store it in owncloud.cfg.
(but compressing might be easier..are we trying to store binary or text?)

[ckamm]

In tests with my own client certificate Windows successfully accepts a 1233 bytes cert plus a 887 bytes key. But both are over the maximum size documented for this API. @wdehoog what Windows 10 version are you using exactly? Possibly there was a change.

Plans:

• improve qtkeychain error reporting for this case
• If confirmed that its a size issue, revamp client cert storage

EDIT: I verified that @wdehoog's certificate and key are both above 5*512 bytes in size, making this extemely likely to be a size issue.

[ckamm]

This diff does indeed indicate that CRED_MAX_CREDENTIAL_BLOB_SIZE changed from 512 to 5*512 - so the documentation is outdated and that explains why credential saving works for my case.

https://abi-laboratory.pro/compatibility/Windows_6.0_to_Windows_7.0/x86_64/headers_diff/advapi32.dll/diff.html

[ckamm]

QtKeychain error handling improvements: frankosterfeld/qtkeychain#137

[guruz]

@wdehoog Could you try this build with logfile enabled: https://download.owncloud.com/desktop/testing/ownCloud-2.5.3.11457.11287-rc1.msi ?

[wdehoog]

Just did. The lines below show up in the log file. After a restart login still fails.

02-06 09:44:00:503 [ warning sync.credentials.http ]:	Could not write client cert to credentials 7 "Credential size exceeds maximum size of 2560"
...
02-06 09:44:00:537 [ warning sync.credentials.http ]:	Could not write client key to credentials 7 "Credential size exceeds maximum size of 2560"

[ckamm]

@wdehoog See #7048

[wdehoog]

#ckamm Thanks for keep on working on this issue. You solution will work I guess.

Please allow me to make a small remark. I can understand why you are taking this approach. You want a 'small' patch and not to overhaul the certificate handling completely. But I think owncloud should do just that. Stop creating your own certificate store and use the one the platform provides.

[ckamm]

@wdehoog Agreed, this is indeed just a patch to make it work at all with larger certificates. Any change to use the platform store would be larger and target 2.7 earliest.

[lazawan]

@ckamm please could you provide the steps to recreate (they are not clear enough) ?

[ckamm]

@lazawan I don't think retesting this issue is worth it.

• Need an affected Windows version, we don't know which exactly. "Windows 6.0" (by dev counting) Apparently early Win10 versions are affected.
• Need to set up an owncloud server with client certificates that exceed 512 bytes.
• Connect a client to this server and provide the certificate. Restart the client. It should be able to connect.

[wdehoog]

@ckamm Just tested 2.6. It works. Impressive job. Thank you.