Add fallback routines for empty secret cases #31499

juliusknorr · 2022-03-09T09:52:54Z

Make sure to keep authentication working when an instance has been setup without a secret after adding the secret manually afterwards.

Password auth to webdav still works
App password auth to webdav still works
Browser session keeps being active
PublicKeyTokens get rotated if the fallback is hit so their private key is reencrypted with the secret now

Provides a possible migration path for #31492

ToDo

Add migration step to update the config if possible and set a secret before the upgrade
- secret
- paswordsalt

lib/private/Authentication/Token/PublicKeyTokenProvider.php

lib/private/Security/Crypto.php

lib/private/Security/VerificationToken/VerificationToken.php

lib/private/Security/Crypto.php

juliusknorr · 2022-03-10T10:13:20Z

Test results when having an instance without a secret and adding one afterwards:

Users get logged out of their sessions in the browser
Reauthentication with existing password works -> password salt change is confirmed to not be an issue
App passwords still fail

juliusknorr · 2022-03-10T10:41:29Z

Retested with follow up commits:

Password auth to webdav still works
App password auth to webdav still works
Browser session keeps being active
PublicKeyTokens get rotated if the fallback is hit so their private key is reencrypted with the secret now

CarlSchwan

Code looks good and I tested it manually and I didn't get logged when moving from a config without secret to a config.php with a secret

Signed-off-by: Julius Härtl <jus@bitgrid.net>

lib/private/Authentication/Token/PublicKeyTokenProvider.php

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

CarlSchwan · 2022-09-13T11:05:34Z

Should we merge it without the migration path and just add the warning from #31492 instead?

It's better than the current state

Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: Carl Schwan <carl@carlschwan.eu>

CarlSchwan · 2022-10-17T14:03:04Z

/backport to stable25

CarlSchwan · 2022-12-05T14:08:25Z

Manual backport #35605

Before this patch, when decrypting a value without using a password, it would call `decryptWithoutSecret` with the system `secret` as `password`. When this fails, it would retry with an empty string as `password`. This has the practical disadvantage that it can lead to confusing error messages. For example, when using the TOTP app, when the system `secret` is misconfigured, the first invocation will throw a sensible `HMAC does not match.` error, but then it is retried and the retry throws a `Hash_hkdf(): Argument nextcloud#2 ($key) cannot be empty` error causing confusion (e.g. https://help.nextcloud.com/t/hash-hkdf-argument-2-key-cannot-be-empty/192556). Of course this fallback to using an empty string is likely part of some sort of graceful migration from the days when the secret could be empty (e.g. nextcloud#34012, nextcloud#31499). However, taking a wider perspective, such 'fallback logic' in security-critical areas makes things more complex, which is a risk. It's not quite the same scenario, but Heartbleed does come to mind. For this reason, rather than a 'surgical' improvement for the particular case encountered above (increasing complexity further), I think it'd be worth to start considering removing this fallback entirely (perhaps in v32.0.0?) - hence this conversation-starter PR. Signed-off-by: Arnout Engelen <arnout@bzzt.net>