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.

Sign up for GitHub

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

Jump to bottom

Add fallback routines for empty secret cases #31499

Merged
merged 5 commits into from
Oct 17, 2022
Merged

Add fallback routines for empty secret cases #31499

merged 5 commits into from
Oct 17, 2022

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Mar 9, 2022 •
edited
Loading

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

@nickvergessen nickvergessen added this to the Nextcloud 24 milestone Mar 9, 2022
@juliusknorr
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

@juliusknorr
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

@juliusknorr juliusknorr requested a review from CarlSchwan March 10, 2022 10:45
@juliusknorr juliusknorr requested a review from PVince81 March 10, 2022 10:47
CarlSchwan
CarlSchwan approved these changes Mar 10, 2022
View reviewed changes
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

@juliusknorr
Add fallback routines for empty secret cases
81f8719 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Rotate token after fallback
553cb49 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Fix decryption fallback after adding a secret
a6796b4 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/empty-secret branch from 57a7255 to a6796b4 Compare March 10, 2022 13:01
@CarlSchwan
Fix unit tests
01e2a26 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan mentioned this pull request Mar 14, 2022
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
This was referenced Apr 7, 2022
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
Merged
Merged
@blizzz blizzz mentioned this pull request Aug 24, 2022
Merged
This was referenced Aug 30, 2022
Merged
Merged
@blizzz blizzz mentioned this pull request Sep 9, 2022
Merged
@solracsf solracsf mentioned this pull request Sep 12, 2022
Open
9 tasks
@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
@CarlSchwan @nickvergessen
Mark method as deprecated
ef31396 
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
Merged
This was referenced Sep 20, 2022
Closed
Merged
@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

@CarlSchwan CarlSchwan mentioned this pull request Dec 5, 2022
Merged
@joshtrichards joshtrichards mentioned this pull request Jun 27, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@come-nc come-nc come-nc approved these changes

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@ArtificialOwl ArtificialOwl ArtificialOwl approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

Assignees
No one assigned
Labels
2. developing Work in progress bug feature: authentication
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
6 participants
@juliusknorr @CarlSchwan @nickvergessen @ArtificialOwl @come-nc @blizzz