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

Add fallback routines for empty secret cases #31499

Merged
merged 5 commits into from
Oct 17, 2022
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Mar 9, 2022

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

@juliushaertl
Copy link
Member Author

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

@juliushaertl
Copy link
Member Author

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

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

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>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
@blizzz blizzz mentioned this pull request Mar 31, 2022
This was referenced Apr 7, 2022
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
@blizzz blizzz mentioned this pull request Aug 24, 2022
This was referenced Aug 30, 2022
@blizzz blizzz mentioned this pull request Sep 9, 2022
@CarlSchwan
Copy link
Member

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

It's better than the current state

@CarlSchwan CarlSchwan marked this pull request as ready for review September 13, 2022 11:05
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@CarlSchwan CarlSchwan merged commit 9919116 into master Oct 17, 2022
@CarlSchwan CarlSchwan deleted the bugfix/empty-secret branch October 17, 2022 14:02
@CarlSchwan
Copy link
Member

/backport to stable25

@CarlSchwan
Copy link
Member

Manual backport #35605

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants