Add local file embedding for SSH keys and systemd units

[Okeanos]

As described in #179 I added an option to define local SSH authorized keys alongside embedding/inlining them.

The code is mostly derived from the existing Resource translator. It would interesting to add a source translator as well that fetches SSH keys from an URL, e.g. a Github user profile. This is going to get "interesting", though, when multiple keys are defined (see travier's profile). Additionally, all the verification checks the Resource translator maps into the Ignition equivalent would've to be inlined into butane if I am not entirely mistaken.

[Okeanos]

I'll go fix the breaking tests later today, apparently I missed those for some reason. Sorry about that.

[travier]

One option is to deprecate ssh_authorized_keys and create e.g. ssh_authorized_keys_source of type SSHAuthorizedKeyResource. But looking at how Resource is defined now, it's just the underlying Ignition Resource struct with inline and local fields added. Along similar lines, we could add a new field ssh_authorized_keys_local (or ssh_authorized_key_files, depending how we want to be confusing) in the PasswdUser struct. That isn't ideal, but it's minimally invasive. I'm inclined to recommend the latter path, but open to ideas.

If we want to support multiple options like source/inline/local, then I would prefer the deprecation + new entry route. If we think we will only add the files option, then a new option looks like the simplest solution.

I could see the "import SSH public key directly from my GitHub profile" use case having some value thus I don't have a preference.

[Okeanos]

@bgilbert thanks so much for the review and feedback. I'll go over it soon and update the pull request.

Concerning the "multiple keys per file": I can try to accommodate that and would, hence, treat each line in a file as a single key.

Concerning "keys from URL" (such as the GitHub profile link): The checks that are part of (Ignition)Resource such as TLS verification and hash comparisons would have to be either skipped because the Ignition type sshAuthorizedKeys (list of strings) doesn't support them (as it isn't a Resource) or build into butane, correct? Given that, would the correct approach be to add something like sshAuthorizedKeysResource to the Ignition base-spec and target that with the same translation mechanism which already exists for file-resources? This would still require particular handling for "multiple keys per file" (regardless of locally or remotely sourced), though.

[bgilbert]

If we want to support remote URLs, I think it should probably be done natively in the Ignition spec, and then that schema structure should be replicated here. If we handled URL fetching in Butane, some types of resources would be fetched when Butane runs and some when Ignition runs, which would be confusing. For SSH keys, which are security sensitive, it's even more important that users know at what time the keys are bound to a machine.

For previous discussion on expanding the types of resources Ignition can fetch, see https://github.com/coreos/ignition/issues/884, https://github.com/coreos/ignition/pull/986, and https://github.com/coreos/ignition/issues/1097. It would have made sense to support fetching more broadly in the original spec, but I do worry about the amount of churn it would create to add that functionality now.

[travier]

For previous discussion on expanding the types of resources Ignition can fetch, see coreos/ignition#884, coreos/ignition#986, and coreos/ignition#1097. It would have made sense to support fetching more broadly in the original spec, but I do worry about the amount of churn it would create to add that functionality now.

Thanks, looks like this has been well explored before. I think it would be great to have that kind of "all resources are treated equal and you can fetch them from anywhere" but it indeed makes it much more complicated in Ignition, would be a breaking spec change and network failures could make provisioning unreliable.

Thus I'm now in favor of making a couple of smaller additions to the Butane spec only to enable local inclusions only for SSH keys and systemd units.

We could then revisit this once we decide to do an Ignition spec 4.

[travier]

What this could potentially look like:

systemd:
  contents_local (object):
    - path (string):
password:
  users:
    - ssh_authorized_keys_local (list of objects):
      - path (string):

[bgilbert]

@travier Hmm, the extra objects for the _local fields are inconsistent with their non-local counterparts, and with how local is handled in Resource. I was thinking something like this:

systemd:
  units:
    - contents_local (string):
passwd:
  users:
    - ssh_authorized_keys_local (list of strings):

[Okeanos]

I just updated the PR with a WIP implementation of the suggested changes. It doesn't work as expected because the translation set contains the wrong data and I don't know why. I'd appreciate a pointer or two if anyone of you would be so kind.

[Okeanos]

@bgilbert thank you so much for your input. For FCOS it appears to work as expected. However, the config/openshift/v4_10_Exp/translate_test.go test case for TestValidateSupport now fails because for some reason it expects:

Error:  Received unexpected error:
        Missing paths in TranslationSet:
        $.spec.config.passwd.users.0.homeDir
        $.spec.config.passwd.users.0.noCreateHome
        $.spec.config.passwd.users.0.noLogInit
        $.spec.config.passwd.users.0.noUserGroup
        $.spec.config.passwd.users.0.passwordHash
        $.spec.config.passwd.users.0.primaryGroup
        $.spec.config.passwd.users.0.shouldExist

whereas originally it tested for:

$.passwd.users.0.home_dir
$.passwd.users.0.no_create_home
$.passwd.users.0.no_log_init
$.passwd.users.0.no_user_group
$.passwd.users.0.password_hash
$.passwd.users.0.primary_group
$.passwd.users.0.should_exist

I would assume that has something to do with my changes in base/v0_5_exp/translate#translatePasswdUser where I added the translate.MergeP pieces? I assumed I had to declare them to make sure that all existing PasswdUser fields are properly added with their default translators.

[Okeanos]

I believe I have fixed all outstanding issues. Feel free to review as a whole once again.

If there is anything else to change or fix I'll of course take another look at it. Considering that this was part of an effort for myself to better understand butane and in particular try my fingers at #179 I'd be willing to add the same functionality for systemd unit files.

[Okeanos]

I just rebased my pull request against the latest changes from the main branch. As far as I can tell from the test execution it still looks fine.

[abalmos]

As things appear a bit stalled, I'll throw my support for this change. It would really clean up some butane based projects.

Edit: I'm particularly interested in the systemd follow-up PR

[Okeanos]

@bgilbert @travier I had some free time recently and updated this pull request to allow embedding local SSH keys as well as systemd units to cover the whole issue (#179). I know there's a general discussion going on to refactor local file embedding overall – nevertheless, feel free to have another look at this pull request. I'd very much appreciate some feedback and an idea of whether this will be merged at some point or the generalized approach is preferred.

@abalmos FYI.

[travier]

Took a quick look and this LGTM but I'm not up-to-date on Butane code and logic to do an in-depth review. I'll give it a try soon however to see if I catch anything missing/broken.

[travier]

I don't think the discussion about merging config files should impact this PR as far as I understand.

[Okeanos]

Thanks once again so much for this in depth review – very helpful indeed.

I resolved (fingers crossed) everything I specifically didn't leave open this time around I hope. Will add a translation set verification similar to the SSH one to the systemd translation test as well later on.

[Okeanos]

I updated the pull request for local ssh & systemd embedding to incorporate minimal changes for Flatcar configs that were recently introduced. If there's anything else you need please let me know.

[Okeanos]

@bgilbert if there is anything I can do to speed up the review process to get the local file embeds for systemd units and SSH keys merged please let me know. I am really looking forward to getting this merged.

[Okeanos]

@travier @bgilbert I just resolved the merge conflicts for the butane "local ssh and local systemd file embedding"-PR. I am still willing to continue working on this PR, however, I would very much like to receive some general feedback here: do you have a rough idea on when you can start reviewing this again? Are there any things I should be doing differently (or at all) to help you with the review process and final merge? I would really like to see this merged (sooner rather than later) because I have a small project myself that would benefit from this tremendously and I am certain others would like to see this as well. If you, however, already know that this won't be happening for the foreseeable future (or in the way prepared here) that's also okay – in that case close the PR, please. It's clear that all of you have a lot on your plates and the changes here aren't exactly trivial so do take your time. If, in the mean time, you have the chance to give a little feedback – even if it's just a "yeah, still in the backlog; we'll get back to you" – I'd greatly appreciate it.

[travier]

Sorry for the late reply. I really want to have this but I haven't had the time to look at it again. I'll see if we can find someone to review it.

[travier]

Something else that might help with reviewing this change is splitting it into two commits, one for systemd and one for ssh keys.

[Okeanos]

Something else that might help with reviewing this change is splitting it into two commits, one for systemd and one for ssh keys.

I split this commit up into multiple semantic commits that now only contain required changes (changes to the test script beyond supplying data related to the new features have been removed). Each commit should be valid on its own and pass the test script. To simplify the upgrade instructions I put them last and included only a single fully-fledged example for each supported platform (FCOS, Flatcar, OpenShift).

If there's anything else I can do, please let me know and I'll take a look.

[Okeanos]

Thanks for the review; I'll get these fixed shortly – if you need anything else from me to get this merged, please let me know.

In unrelated news: 🎂🥳 – happy birthday PR-289 ;-)

[Okeanos]

I noticed during local tests that parsing SSH files may break unexpectedly because of the uniqueness requirement: including newlines will mean that a newline is added to the sshAuthorizedKeys – a newline character may, because of the aforementioned requirement, only ever by present once in the sshAuthorizedKeys-list. Having multiple will cause a validation error and break the transformation. However, SSH doesn't actually care because it ignores empty lines (comments, blank newlines).

Question: how to handle that? Strip (duplicate) newline list elements silently? Issue an explicit error if they are found (otherwise it'll just say "there were 'duplicate entries'" which is kinda unhelpful)? Put a warning in the migration notices or somewhere else? Just … ignore this issue? Something else?

[Okeanos]

That got rid of the Windows test errors now for good as well. In other news … I just now learned that you had already solved that problem in a different test and prepared things for the eventuality it might happen in a different place as well.

@travier bgilbert told me he informed the team of what's supposed to happen next (now that I addressed … I think 🤞 his review findings). I don't know who exactly, though.

There is one comment where I am not entirely sure what I am supposed to change. I would kindly ask you (as a team) to go over this once more and tell me whether you'd like to defer merging this until later (or whether there is anything else I need to change). I am totally okay with any of these options because I want to make sure that these changes are in the best possible state that they can be.

[Okeanos]

@bgilbert could you provide an update on when I can expect additional feedback? I'd really love to get this merged. Thanks!

[bgilbert]

For the record, the merge commit 9342f39c1cbfba2d12fd7460842ad36a59da3869 has some conflict fixups in it. The GitHub web UI doesn't seem to display those, though.

I noticed during local tests that parsing SSH files may break unexpectedly because of the uniqueness requirement: including newlines will mean that a newline is added to the sshAuthorizedKeys – a newline character may, because of the aforementioned requirement, only ever by present once in the sshAuthorizedKeys-list. Having multiple will cause a validation error and break the transformation. However, SSH doesn't actually care because it ignores empty lines (comments, blank newlines).

Yeah, I'd be inclined to strip blank lines entirely when building a key list from a file.

[bgilbert]

Followup in #441.

[Okeanos]

This looks good overall. There are various small fixes, but I'm not going to ask you to handle them, since you've already been through several long review cycles here. I think we can merge this as-is, and I'll submit a separate PR with some followups. Feel free to add your thoughts there if you'd like.

Thank you so much for all your work to get this landed!

Thanks a lot for your very constructive feedback for this PR and willingness to go through so many iterations and make sure that a good implementation makes it into butane!

I'll be sure to stick around and have a look at what else you're up to here and see whether I can continue to contribute in the future.

[travier]

Thanks a lot @Okeanos & @bgilbert