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

fwupd 1.3.3 misconfigured ConfigurationDirectory #1479

Closed
superm1 opened this issue Nov 12, 2019 · 39 comments
Closed

fwupd 1.3.3 misconfigured ConfigurationDirectory #1479

superm1 opened this issue Nov 12, 2019 · 39 comments
Assignees
@ahkok
Labels
bug

Comments

@superm1
Copy link

superm1 commented Nov 12, 2019

Upstream fwupd received a bug report that on Clear they have no remotes and can't enable any.
The bug report was fwupd/fwupd#1555.

After some investigation it was noted that the debug logs showed:
FuConfig using config path of /etc/fwupd

As the package is configured with --sysconfdir=/usr/share/fwupd/ this was surprising.

It turns out that it is caused by the ConfigurationDirectory directive in the systemd unit. This takes precedence over sysconfdir.

Systemd provides this directive for unit files to use for their configuration directory in the distribution. So this leaves 3 options that we find. I'll try to compare and contrast them.

  1. Modify the fwupd package to use --sysconfdir=/etc.
    This would make it more like other distributions.
    Configuration files wouldn't be overwritten by distribution updates.
    I think this would fix the problem effectively.

  2. Fix the systemd package. If the clearlinux distribution expects all packages to store data in /usr/share it should be configured such that ConfigurationDirectory is correct.
    This should fix both fwupd and any other package that encounters a similar problem.

  3. In the clearlinux fwupd package comment out the ConfigurationDirectory directive in the fwupd.service unit. This will only fix the fwupd package, but it is the easiest and potentially least contentious option if 1 or 2 are not doable options. It was verified in the upstream bug already to solve the issue.

CC original reporter @Queuecumber and @hughsie
@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@superm1 superm1 mentioned this issue Nov 12, 2019
Open
@superm1
Copy link
Author

superm1 commented Nov 12, 2019

So normally if a user wants to change a configuration file they are supposed to duplicate it from /usr/share to /etc themselves and then modify it in /etc?

@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

fwupdmgr has lots of commands that modify those files. For example:

#fwupdmgr enable-remote lvfs-testing would cause daemon to modify a key in /etc/fwupd/remotes.d/lvfs-testing.conf

and with fwupd 1.3.3 after performing an update you may be prompted to upload a report automatically in the future. Depending what you say a key to be modified in each remote your preference.

So in an ideal scenario how should that work in your mind? Should the fwupdmgr tool know about this stateless configuration and clone the file from /usr/share/fwupd to /etc/fwupd/?

@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@Queuecumber Queuecumber mentioned this issue Nov 12, 2019
Closed
@superm1
Copy link
Author

superm1 commented Nov 12, 2019

More than just a few symlinks to turn on and off remotes, a bunch of individual keys in conf files get modified depending on what user selects.

@superm1
Copy link
Author

superm1 commented Nov 12, 2019 •
edited
Loading

So I guess let me re-iterate back what I think would be ideal to you and make sure I understand and we come up with a workable idea.

  1. You'd like to see fwupd upstream switch from just shipping conffiles at all to everything getting put into /usr/share/fwupd by default and clone that to /etc/ both.
  2. The daemon would load configuration from /usr/share/fwupd first and then read stuff from /etc to override it.
  3. Running fwupdmgr commands that change conf stuff only in /etc.
  4. If stuff in /etc didn't exist it would be cloned from /usr/share
  5. "Regular" distros would handle it exactly the same as before.
  6. Clear would only ship the stuff in /usr/share and the clone stuff would automatically happen as needed.

@hughsie, what are your thoughts on this?

@fenrus75
Copy link
Contributor

as a general thing that's indeed pretty close. there's a lot of benefit from splitting what the base/defaults/etc are from what the user customizations are. CoreOS and systemd started this stateless concept, and it's... really nice in general. For most things it's pretty straightforward, there's very little upstream software that edits the config files directly, and from a sysadmin view as well as update view it's very nice

(yeah there is visudo and a few similar things, but not very many)

@hughsie
Copy link

hughsie commented Nov 12, 2019

As the package is configured with --sysconfdir=/usr/share/fwupd/

I don't think that's going to work with most modern daemons... It's pretty naive to assume that applications that are expecting a writable sysconfdir to store, well, their mutable system config in an immutable directory... I don't think you can just change a few compile flags and pretend that it's all going to work, especially without even testing if the service starts...

You'd like to see fwupd upstream switch from just shipping conffiles at all to everything getting put into /usr/share/fwupd by default and clone that to /etc/ both.

Although I don't agree that it's the applications job to copy files from /usr/etc to /etc if they are missing (as http://0pointer.de/blog/projects/stateless.html would imply) this is something we could add fairly easily.

The daemon would load configuration from /usr/share/fwupd first and then read stuff from /etc to override it.

No, I don't really want the override functionality. I think when /etc/fwupd exists then we disregard the contents of /usr/etc/fwupd completely. If we did the fallback thing, how could the user ever remove a remote (e.g. for compliance reasons) if /usr is immutable?

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

I don't think that's going to work with most modern daemons... It's pretty naive to assume that applications that are expecting a writable sysconfdir to store, well, their mutable system config in an immutable directory...

I agree. I think it would be best to switch to dropping that compile flag until we have a good approach for making fwupd work with your preferred stateless approach. This current solution is broken.

I don't think you can just change a few compile flags and pretend that it's all going to work, especially without even testing if the service starts...

Well to be fair; it does start. Fwupd can start without conffiles. Just remotes don't work because they're not configured :)

Although I don't agree that it's the applications job to copy files from /usr/etc to /etc if they are missing (as http://0pointer.de/blog/projects/stateless.html would imply) this is something we could add fairly easily.

The problem with just copying files if they're missing is that you'll never get them upgraded for new keys that are introduced, changes, or removed. Regular distros handles this with conffile systems that will update conffiles if nothing changed or let you do a merge when installing an updated package.

If we did the fallback thing, how could the user ever remove a remote (e.g. for compliance reasons) if /usr is immutable?

That's true - they couldn't remove it, the best they could do was "disable" it.

@fenrus75
Copy link
Contributor

fenrus75 commented Nov 12, 2019 via email

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

Does systemd actually merge configuration files? I thought if you have something in /etc it just takes precedence over the one in /lib. Which I guess would work for "removing" a remote if the file in /etc/ was just a symlink to /dev/null

Conceptually I like the idea of a remote being disabled or enabled by symlinks, but if we're changing keys in the conffile the first time you use fwupdmgr regarding sending a report what does it buy you? You'd still need to copy the file over to be modified.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

there's a simple way to remove, change, or add individual options by making files in /etc.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

The problem with just copying files if they're missing is that you'll never get them upgraded for new keys that are introduced, changes, or removed.

which is why we prefer methods like include or a seamless overlay type system. :)

@Queuecumber
Copy link

I think this is raising a larger question. From your very nice explanation I think there's an undeniable benefit to the system that clear Linux is pushing. That being said, regardless of the route that fwupd ends up choosing, there's a lot of software that has become more or less standard and that won't work as described and will not be receptive to making changes due to

  1. Having originally been designed in a very different fashion making the update difficult and hard to justify to support a single fledgling distro
  2. Being sparsely maintained due to age
  3. Stubbornness

What is the correct way to handle such programs, presumably they should be made to be able to work under clear linux

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

You'd like to see fwupd upstream switch from just shipping conffiles at all to everything getting put into /usr/share/fwupd by default and clone that to /etc/ both.

No, we don't allow packages to install anything in /etc.

@fenrus75
Copy link
Contributor

Most software in legacy space either is something users never change the config for (easy) or where we add a few small lines in a patch that checks /etc first and falls back to the distro reference if the etc file does not exist.

the more complicated things like httpd have a rich configuration language that actually supports this natively

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

@Queuecumber you raise a good point, but, let's focus on making fwupd work in this issue, instead of it going too far off-topic :)

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

A very simple solution is to not ship any config file at all, and have sane builtin defaults, with /etc/ files being parsed to override those defaults. I can totally be OK with tmpfiles.d or some first-run code creating repo files in /etc/ (there's tmpfiles code to support copying template files, which get run after any swupd update).

But I strongly prefer to keep /etc/ empty unless users explicitly make a config change. /var is an excellent place for daemons to maintain state already. It should be at least considered here.

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

A very simple solution is to not ship any config file at all, and have sane builtin defaults, with /etc/ files being parsed to override those defaults.

Well fwupd does actually work that way for all things that have keys everything except remotes (which is the part that broke here).

I can totally be OK with tmpfiles.d or some first-run code creating repo files in /etc/ (there's tmpfiles code to support copying template files, which get run after any swupd update).

I guess with the way things are built right now your --sysconfdir is completely ignored anyway due to systemd using ConfigurationDirectory. Could you do something with tmpfiles.d to clone everything from /usr/share/fwupd/remotes.d to /etc/fwupd/remotes.d that doesn't exist? That would fix this sufficiently.

But I strongly prefer to keep /etc/ empty unless users explicitly make a config change. /var is an excellent place for daemons to maintain state already. It should be at least considered here.

The state of the remotes (and cached metadata) gets put into /var. /etc/ is remote configuration more than it is state.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

How about, if there's no /etc/fwupd/remote.d, it falls back to the builtin (or OS default) ones?

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

Ah then it can't modify them.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

There's got to be a way to solve this without pre-polluting /etc/.

@bryteise
Copy link
Member

Due to the editing commands, I think more systemd level work is required in fwupd to be stateless. Having a system config directory and admin config directory and only writing overrides/changes to the admin config directory would need to be supported I imagine. Otherwise you are left with the unfortunate scenario of a user initially installing the content and getting /etc populated with content at that time. Then if that content is updated in /usr, it is really not manageable to know if the content in /etc can or should be updated accordingly.

@hughsie
Copy link

hughsie commented Nov 12, 2019

I think more systemd level work is required in fwupd

Such as? Honestly, at this point I'm just fine with fwupd not working in clearlinux as it's clearly not respecting the FHS and expecting apps to do weird things...

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

Removing it would save me work, too. Of course, the better outcome for everyone would be if fwupd could improve things so that /etc doesn't become a zoo again.

@Queuecumber
Copy link

Queuecumber commented Nov 12, 2019 •
edited
Loading

This is coming back to my comment

I worry that you (clear Linux) are being too idealistic, @fenrus75 makes a big assumption, you are going to run into this problem again and again as the distro matures. The only way forward is going to be to relax your constraint or reinvent things. This is fine as an experiment, but the real world requires compromise.

Fwupd has become the standard for delivering firmware updates on Linux. It's not fwupd that is going to suffer from not being included in clear, it's your users.

@hughsie
Copy link

hughsie commented Nov 12, 2019

There's got to be a way to solve this without pre-polluting /etc/

So just to be clear, we're not allowed to install into /etc, not allowed to copy from /usr/etc on first start, and not allowed to write to /usr. We have to start up without any etc contents, and only save changed keys to /var/lib, and also merge in user presets from /etc. If so, that sounds pretty crazy. However, /usr/etc does makes sense from a system-reset point if view.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

@Queuecumber we solve these issues for almost all packages. The benefit of all our users not having to wade through hundreds of files in /etc far outweighs the cost of me spending an hour here or there patching software to read defaults from an OS vendor directory.

So, we're going to solve this for every package, and, upstream projects are more than welcome to chime in and talk to us in here, as the above discussion demonstrates.

So far, I am not deterred yet :)

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

So just to be clear, we're not allowed to install into /etc, not allowed to copy from /usr/etc on first start, and not allowed to write to /usr. We have to start up without any etc contents, and only save changed keys to /var/lib, and also merge in user presets from /etc. If so, that sounds pretty crazy. However, /usr/etc does makes sense from a system-reset point if view.

The copying is ... just circumventing the ideal that /etc only should contain user modified options. You're allowed, but, I really wish you wouldn't :)

If so, that sounds pretty crazy.

It's partially made more complex because the software is architected to combine immutable repo information with user-mutable repo state. Splitting it would be a solution, and would avoid a lot of duplicate information in /etc that users would never modify to begin with.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

e.g. /etc/fwupd/remotes.conf could store the user-modified bits for each remote.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

I assume fwupd is OK with the following configs to be absent from /etc/fwupd:

daemon.conf, redfish.conf, thunderbolt.conf, uefi.conf

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

I assume fwupd is OK with the following configs to be absent from /etc/fwupd:

daemon.conf, redfish.conf, thunderbolt.conf, uefi.conf

Yes

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

great, thanks for the quick response.

I'm pushing fwupd-1.3.3-39 which does the following:

It has this default tmpfiles snipplet:

d /etc/fwupd 0755 root root - -
L /etc/fwupd/pki - - - - /usr/share/fwupd/pki
C /etc/fwupd/remotes.d - - - - /usr/share/defaults/fwupd/remotes.d

On top of that the --sysconfdir option is removed and thus /etc being the default.

Hopefully, that puts everything in place that it "just works".

@superm1
Copy link
Author

superm1 commented Nov 12, 2019

That sounds like a workable consolation, thanks.

@ahkok
Copy link
Contributor

ahkok commented Nov 12, 2019

fwupd-1.3.3-40 has an actually tested tmpfiles config.

@ahkok ahkok added the bug label Nov 12, 2019
@ahkok ahkok self-assigned this Nov 12, 2019
@superm1 superm1 closed this as completed Nov 14, 2019
@superm1
Copy link
Author

superm1 commented Nov 14, 2019

It looks these promoted now, so closing this issue.

@xlfe xlfe mentioned this issue Dec 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ahkok ahkok

Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@bryteise @hughsie @superm1 @fenrus75 @Queuecumber @ahkok