Skip to content

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 a team, ChristophWurst, miaulalala and nickvergessen 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.

🤞

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

Development

Successfully merging this pull request may close these issues.

[Bug]: NC25 - The required secret config variable is not configured in the config.php file [Bug]: passwordsalt migration missing

6 participants