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

Do not treat logo image as a conffile in app code package #5850

Closed
eloquence opened this issue Mar 5, 2021 · 5 comments · Fixed by #6101
Closed

Do not treat logo image as a conffile in app code package #5850

eloquence opened this issue Mar 5, 2021 · 5 comments · Fixed by #6101

Comments

@eloquence
Copy link
Member

eloquence commented Mar 5, 2021

14c8761 (from back in 2015) marked /var/www/securedrop/static/i/logo.png as a conffile, which was appropriate prior to improvements to logo handling like #3057. However, this can cause unexpected behavior like what we observed in #5849.

At this point, if logo.png is not modified by the app code or by end users, it should be safe to no longer treat it as a conffile.

@eloquence eloquence added this to the 1.9.0 milestone Mar 5, 2021
@eloquence
Copy link
Member Author

Since we're aiming to resolve #5849 more broadly by avoiding conffile-related failures during unattended upgrades, there should be no need to make this change immediately for 1.8.0. However, milestoned to 1.9.0 as a near-term cleanup opportunity.

@eloquence
Copy link
Member Author

eloquence commented Apr 15, 2021

One case to keep in mind are SD instances that modified their logo using the old method. At least one org reported that they did not have to re-upload the logo after the 20.04 migration, prior to #5880 being released with 1.8.1. Hypothesis: Their logo.png was overwritten pre-2018, the conffile status preserved that overwritten version release after release, and the restore script copied it back into place on the 20.04 server.

If that hypothesis is correct, removing the conffile status may cause older orgs to lose their custom logo, or could cause package update issues. That said, 1.8.1 (due to the changes in #5880) will only copy custom_logo.png and not logo.png -- any instance migrating from here on will have to re-upload their logo if it wasn't already uploaded using the new method.

@eloquence eloquence modified the milestones: 2.0.0, 2.1.0 May 19, 2021
eloquence added a commit that referenced this issue Sep 21, 2021
@eloquence
Copy link
Member Author

eloquence commented Sep 21, 2021

To recap, if folks updated to Ubuntu 20.04 when SecureDrop 1.8.0 was released and before 1.8.1, and their logo is still at the old URL, their logo will get clobbered if we remove the conffile declaration.

It's easy to verify whether an org will have their logo clobbered by looking at its logo image URL. If the URL is logo.png, it would be overwritten if we stop declaring the logo as a conffile. If the URL is custom_logo.png, it will be preserved.

I know of at least one impacted org. I'll do a bit more diligence and will ask impacted orgs to upload the logo manually, to avoid surprises.

eloquence added a commit that referenced this issue Sep 21, 2021
eloquence added a commit that referenced this issue Sep 21, 2021
eloquence added a commit that referenced this issue Sep 21, 2021
eloquence added a commit that referenced this issue Sep 21, 2021
@eloquence
Copy link
Member Author

I've identified a total of 8 news orgs whose logo resides at the old logo.png address. That's a bit higher than I expected. We could consider an automatic migration (if file does not match stock package contents shasum, copy it to custom_logo.png, perhaps in preinst), but since it's very easy to fix through the web UI, I think a manual heads-up now would probably be sufficient, assuming the change in #6101 looks otherwise good. Thoughts?

@conorsch
Copy link
Contributor

I admit I'm a fan of munging the config as needed in the packaging flow, so that we can confidently reason about known state on all instances. Let's avoid asking admins to mess with the web UI unless absolutely necessary, would be my preference.

eloquence added a commit that referenced this issue Sep 24, 2021
eloquence added a commit that referenced this issue Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants