v1_5_exp: Add sugar to create user.cfg

[saqibali-2k]

We want to add sugar to butane which will allow users to indirectly
modify the GRUB configuration. The sugar added here will abstract
the mounting of /boot and will allow users to create /boot/grub2/user.cfg
which is sourced by grub.cfg.

[bgilbert]

Let's mention coreos/fedora-coreos-tracker#134 in the commit message.

[bgilbert]

Thanks for handling this! There are some details, but this has the right general idea. 👍

[travier]

Can you paste here an example config?
Can you also make a PR for the docs with that example?
Thanks!

[jlebon]

This is essentially making user.cfg a public API that users can edit directly via Ignition. Should we enhance grub.cfg to source a specific e.g. password.cfg file dedicated to password protection so that a user can both use this sugar and add their own user.cfg? The ideal would be a user.cfg.d/ but it doesn't seem like source supports directories or globs.

[bgilbert]

As I mentioned in #339 (comment), I think we should actively discourage users from making arbitrary changes to their GRUB configuration. By limiting the available sugar and not documenting how to make arbitrary changes, we can make user.cfg a semi-private API between FCOS and Butane. We can always add additional sugar later, including support for arbitrary config fragments if we really want to.

[saqibali-2k]

Example Config:

variant: fcos
version: 1.5.0-experimental
grub:
  users:
    - user: root
      key: grub.pbkdf2.sha512.10000.874A958E526409...

Note: user and key will be changed to name and password_hash respectively

[saqibali-2k]

Rebased baseutil.MakeDataURL changes and pushed documentation updates.

[bgilbert]

Code generally looks good! Just a pile of small cleanups. 🧹

[bgilbert]

Currently, if a parent Butane config and a child Butane config both specify grub.users, then when Ignition merges the resulting Ignition configs at runtime, the parent users will be completely overwritten by the child. This is surprising, since it violates the usual config-merging semantics of replacing existing list entries and appending new ones.

In principle we could provide the usual semantics by having the sugar use storage.files.append instead of storage.files.contents, and using set superusers="$superusers ...". A couple concerns though:

• This assumes that GRUB overwrites an existing password if it sees a second password_pbkdf2 command for a given user. @saqibali-2k, could you verify this?
• This wouldn't work in openshift because the MCO doesn't support append. I'm not sure that should stop us though. The openshift variant can always have special-case code to look for user.cfg and move its append directive to contents. @jlebon, any thoughts?

[saqibali-2k]

• This assumes that GRUB overwrites an existing password if it sees a second password_pbkdf2 command for a given user. @saqibali-2k, could you verify this?

Verified this: the old password does not work after a second password_pbkdf2 command is given.

[bgilbert]

(And I assume it's okay to list the same user twice in superusers.)

[saqibali-2k]

(And I assume it's okay to list the same user twice in superusers.)

This is true, but this resets any previously defined password for that user. So in practice if no new password is set, we just get a username where the password has been thrown away.

[bgilbert]

To be clear, are you saying that set superusers="foo" deletes any previous password for foo?

I think that's actually okay. As long as we define password_hash as required, we won't add a user to superusers unless we're also setting a password for it.

[bgilbert]

Thanks for switching to append. This should currently fail validation in the openshift experimental spec. Want to add some translation code there to find the user.cfg entry and convert its append to contents?

[bgilbert]

Thanks for making the additional changes!

[bgilbert]

Almost there; just some small fixes.

[bgilbert]

This is probably okay as-is, but there's an unnecessary implicit assumption that getAllPaths() returns paths in a consistent order. Possible cleanup: call getAllPaths once, drop the prefixPaths calls, and then loop over paths:

ts.AddTranslation(prefixPath(path, fromPrefix.Path...), prefixPath(path, toPrefix.Path...))

[bgilbert]

It turns out that I've asked you to add a bug here, which is that both sides of the resulting translation are now using ToTag. I won't ask you to fix it though; I'll handle that in a followup.

[bgilbert]

Followup in #357.

[bgilbert]

Thanks for all your hard work on this! 💯 🎆

[travier]

Awesome!