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 repair job that will ensure that secret and passwordsalt are set #35368

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Nov 23, 2022

@CarlSchwan
Copy link
Member Author

/backport to stable25

@CarlSchwan CarlSchwan force-pushed the add-repair-job-secret-config branch from ac717c9 to 612bd1a Compare November 23, 2022 15:07
@CarlSchwan CarlSchwan requested review from a team, PVince81, icewind1991 and juliusknorr and removed request for nickvergessen, ChristophWurst, miaulalala and a team November 23, 2022 15:07
@ChristophWurst
Copy link
Member

The secret is generated during installation, isn't it? And for the ancient installations we already had a repair step somewhere. Was that dropped?

If the secret was lost, then any encrypted values where the instance secret was used as password will not be recoverable. We have seen that a few times. So with the automatic regeneration we'll possibly cause that problem.

So I'm thinking if the automatic recreation is really the best approach. The alternative could be to show a big error/warning on the setup page. Explain the situation and admins can first check if they have the secret in a config backup.

Just an idea.

Otherwise 👍

@szaimen
Copy link
Contributor

szaimen commented Nov 23, 2022

The alternative could be to show a big error/warning on the setup page. Explain the situation and admins can first check if they have the secret in a config backup.

@CarlSchwan @juliushaertl wasnt that the idea in the first place?

@szaimen szaimen added this to the Nextcloud 26 milestone Nov 23, 2022
@szaimen szaimen added the 3. to review Waiting for reviews label Nov 23, 2022
@CarlSchwan
Copy link
Member Author

If the secret was lost, then any encrypted values where the instance secret was used as password will not be recoverable. We have seen that a few times. So with the automatic regeneration we'll possibly cause that problem.

We already have a fallback in place where if a password hash or encrypted password was previously encrypted with an empty secret, we check first with the current secret and then with an empty secret. See 81f8719

@ChristophWurst
Copy link
Member

Right. I also now saw the referenced ticket #34780.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🤞

lib/private/Repair/NC25/AddMissingSecretJob.php Outdated Show resolved Hide resolved
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan CarlSchwan force-pushed the add-repair-job-secret-config branch from 612bd1a to 5e725da Compare November 23, 2022 15:51
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
5 participants