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

Add local file embedding for SSH keys and systemd units #289

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

Okeanos
Copy link
Contributor

@Okeanos Okeanos commented Nov 14, 2021

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.

@ashcrow ashcrow requested a review from travier November 15, 2021 14:43
travier
travier previously approved these changes Nov 15, 2021
Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice. I'm not familiar enough with the Butane code base to see if there are potential issues here but this LGTM.

base/v0_5_exp/schema.go Outdated Show resolved Hide resolved
@Okeanos
Copy link
Contributor Author

Okeanos commented Nov 16, 2021

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

@Okeanos Okeanos force-pushed the ssh-keys-179 branch 2 times, most recently from 24d312e to 147fbdc Compare November 20, 2021 17:24
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

So far we've avoided breaking changes within a Butane variant. On the FCOS side this PR would require us to bump the spec to 2.0.0, and on the OpenShift side we don't even really have a mechanism for breaking changes, since the spec versions are tied to OpenShift versions. I don't think this feature is significant enough to merit breaking existing syntax.

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.

docs/config-openshift-v4_10-exp.md Outdated Show resolved Hide resolved
docs/config-fcos-v1_5-exp.md Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate_test.go Outdated Show resolved Hide resolved
@travier
Copy link
Member

travier commented Nov 29, 2021

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.

@travier travier self-requested a review November 29, 2021 09:10
@Okeanos
Copy link
Contributor Author

Okeanos commented Nov 29, 2021

@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
Copy link
Contributor

bgilbert commented Dec 1, 2021

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 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.

@travier
Copy link
Member

travier commented Dec 1, 2021

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
Copy link
Member

travier commented Dec 1, 2021

What this could potentially look like:

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

@bgilbert
Copy link
Contributor

bgilbert commented Dec 1, 2021

@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
Copy link
Contributor Author

Okeanos commented Dec 3, 2021

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.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick first pass while looking for the TranslationSet problem.

base/v0_5_exp/schema.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
@Okeanos
Copy link
Contributor Author

Okeanos commented Dec 4, 2021

@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.

base/v0_5_exp/translate.go Outdated Show resolved Hide resolved
@Okeanos Okeanos force-pushed the ssh-keys-179 branch 2 times, most recently from cff7820 to 04ed05a Compare December 7, 2021 17:53
@Okeanos
Copy link
Contributor Author

Okeanos commented Dec 7, 2021

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
Copy link
Contributor Author

Okeanos commented Jan 23, 2022

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.

@Okeanos
Copy link
Contributor Author

Okeanos commented Nov 19, 2022

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 Okeanos force-pushed the ssh-keys-179 branch 2 times, most recently from 0036e2f to e2beec7 Compare November 19, 2022 17:06
@Okeanos
Copy link
Contributor Author

Okeanos commented Nov 19, 2022

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 Okeanos requested review from bgilbert and removed request for lucab, marmijo and prestist January 6, 2023 20:31
@Okeanos
Copy link
Contributor Author

Okeanos commented Feb 1, 2023

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

@Okeanos Okeanos force-pushed the ssh-keys-179 branch 2 times, most recently from b076232 to ba4d4f0 Compare February 10, 2023 20:32
Sort upgrade documentation alphabetically and move OpenShift after Flatcar.
Allow embedding SSH keys via file references by supporting:

- multiple file references similar to inline keys
- multiple keys per file (one per line)
- combinations of inline keys and file embeds
Allow embedding systemd units via file references. A systemd unit has to be
either inlined or loaded from an external file – combinations are not possible.
@Okeanos Okeanos mentioned this pull request Mar 4, 2023
39 tasks
@prestist prestist mentioned this pull request Mar 6, 2023
@travier travier requested a review from prestist March 10, 2023 09:18
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

base/v0_5_exp/translate.go Show resolved Hide resolved
base/v0_5_exp/translate.go Show resolved Hide resolved
base/v0_5_exp/translate.go Show resolved Hide resolved
base/v0_5_exp/translate_test.go Show resolved Hide resolved
base/v0_5_exp/translate.go Show resolved Hide resolved
base/v0_5_exp/validate.go Show resolved Hide resolved
config/common/errors.go Show resolved Hide resolved
docs/config-openshift-v4_13-exp.md Show resolved Hide resolved
docs/release-notes.md Show resolved Hide resolved
@bgilbert bgilbert merged commit 9342f39 into coreos:main Mar 14, 2023
@bgilbert
Copy link
Contributor

For the record, the merge commit 9342f39 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
Copy link
Contributor

Followup in #441.

@Okeanos
Copy link
Contributor Author

Okeanos commented Mar 14, 2023

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
Copy link
Member

travier commented Mar 15, 2023

Thanks a lot @Okeanos & @bgilbert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants