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

Decide how to handle user SSH keys #139

Closed
bgilbert opened this issue Jan 26, 2019 · 17 comments
Closed

Decide how to handle user SSH keys #139

bgilbert opened this issue Jan 26, 2019 · 17 comments

Comments

@bgilbert
Copy link
Contributor

Situation

On Container Linux, SSH keys are stored in ~/.ssh/authorized_keys.d and then merged into the ~/.ssh/authorized_keys file that's read by sshd. Ignition writes SSH keys from the Ignition config into ~/.ssh/authorized_keys.d/coreos-ignition (coreos/ignition#710) and coreos-metadata writes keys from cloud provider metadata into ~/.ssh/authorized_keys.d/coreos-metadata. Afterward, each tool runs some code to update the authorized_keys file: Ignition has an implementation in Go, and coreos-metadata in Rust.

There is also a standalone tool, update-ssh-keys, that ships with (and uses) the Rust library. It's the documented method for adding SSH keys on Container Linux. It has the following usage message:

Usage: update-ssh-keys [-l] [-u user] [-a name file1... | -d name]
Options:
    -u USER     Update the given user's authorized_keys file [core]
    -a NAME     Add the given keys, using the given name to identify them.
    -A NAME     Add the given keys, even if it was disabled with '-D'
    -n          When adding, don't replace an existing key with the given name.
    -d NAME     Delete keys identified by the given name.
    -D NAME     Disable the given set from being added with '-a'
    -l          List the names and number of keys currently installed.
    -h          This ;-)

This tool provides a consistent way for different systems to add ssh public
keys to a given user account, usually the default current user.
If -a, -A, -d, nor -D are provided then the authorized_keys file is simply
regenerated using the existing keys.

With the -a option keys may be provided as files on the command line. If no
files are provided with the -a option the keys will be read from stdin.

It's worth noting that update-ssh-keys (and the Rust library) clobbers authorized_keys whenever authorized_keys.d already exists. This makes naive key management tools deceptively unusable, since they'll appear to work but their changes might be reverted later.

update-ssh-keys currently does not ship in Fedora CoreOS. This doesn't prevent users from modifying authorized_keys directly, but authorized_keys.d fragments from Ignition and coreos-metadata will remain around to confuse the user. Also, on platforms where coreos-metadata-sshkeys@.service runs on every boot (coreos/afterburn#138), coreos-metadata will clobber user changes to authorized_keys, leaving no straightforward way to modify the active keys on those platforms.

Options

  1. Ship update-ssh-keys in Fedora CoreOS and proceed as on Container Linux. Document that distros using Ignition or coreos-metadata should ship update-ssh-keys.
    🡆 SSH key management will require using update-ssh-keys, unlike on other Fedora editions.
  2. Same, but ship update-ssh-keys in Fedora generally.
    🡆 Without Ignition or coreos-metadata, update-ssh-keys won't be mandatory, but users can use it if they wish.
  3. Drop the authorized_keys.d concept. Modify Ignition and coreos-metadata to write to authorized_keys directly. That seems okay for Ignition, since it runs once from the initramfs. It's less great for coreos-metadata, which runs on every boot on some clouds and needs to replace keys fetched on a previous boot. It also could race with hypothetical services that modify authorized_keys.
    🡆 authorized_keys UX will be the same as on other distros. The implementation in coreos-metadata will be awkward and racy. The convenience of the command-line tool will be lost unless it's updated for the single-file model and shipped in FCOS.
  4. Implement an AuthorizedKeysCommand that concatenates the contents of authorized_keys.d at runtime (and probably also authorized_keys for compatibility with other tools). sshd will run this as a single configured user; root is easiest, but the tool should probably be SELinux-confined. Delete code from Ignition and coreos-metadata that merges authorized_keys.d into authorized_keys. Document that distros using Ignition or coreos-metadata must use this AuthorizedKeysCommand.
    🡆 Adding keys will work as on other distros, with the addition of authorized_keys.d functionality. Finding/removing existing keys might be unintuitive, since a key not in authorized_keys might still be in authorized_keys.d. The convenience of the command-line tool will be lost unless it's updated and shipped in FCOS.
  5. Modify OpenSSH, either upstream or downstream, to accept a directory in AuthorizedKeysFile.
    🡆 Same consequences as above.
@bgilbert bgilbert added this to Proposed in Fedora CoreOS preview via automation Jan 26, 2019
@cgwalters
Copy link
Member

It's probably worth noting explicitly there's a patch already written for option 5 (nice work Luca!). But yeah...I think while the .d directory is a good idea, my 2¢ here is to just rewrite authorized_keys for now until that patch gets upstream.

(Also worth noting the machine-config-operator for RHCOS today just rewrites authorized_keys directly; if we were to continue with the .d approach we should also ship update-ssh-keys there)

@lucab
Copy link
Contributor

lucab commented Jan 26, 2019

For completeness, support for this in sshd was my preferred approach. The patch above was glued together to probe upstream stance on this, and reaction was not very warm. We can maybe try to propose/push for it again, but as it currently stands I'm not positive it will get in at anytime.

@dustymabe
Copy link
Member

maybe we can go for option 4 while we wait for option 5 ?

@dustymabe
Copy link
Member

Another option may be:

AuthorizedKeysFile .ssh/authorized_keys .ssh/ignition_authorized_keys .ssh/coreos-metadata_authorized_keys

@bgilbert
Copy link
Contributor Author

Yeah, I think my preference is option 4 pending option 5. Option 3 feels pretty clunky, and update-ssh-keys is a non-standard workflow step that seems good to eliminate. The implementation of option 4 seems scary from a security perspective, though.

@dustymabe, I really like the AuthorizedKeysFile approach to buy time. We'd just use

AuthorizedKeysFile .ssh/authorized_keys .ssh/authorized_keys.d/ignition .ssh/authorized_keys.d/coreos-metadata

until the requisite code becomes available.

@dustymabe
Copy link
Member

@dustymabe, I really like the AuthorizedKeysFile approach to buy time. We'd just use

AuthorizedKeysFile .ssh/authorized_keys .ssh/authorized_keys.d/ignition .ssh/authorized_keys.d/coreos-metadata

until the requisite code becomes available.

ahh yes. hardcoding them (as I suggested) but sticking them in the authorized_keys.d/ directory already, even though that fuctionality hadn't made it upstream, would allow us to seamlessly switch over to AuthorizedKeysFile .ssh/authorized_keys ssh/authorized_keys.d/ in the future.

This is another case where sshd config dropins would be useful.

@dustymabe
Copy link
Member

Are we ok with the solution in #139 (comment) as the short term solution and then planning for #5 for the long term solution?

If yes, does this mean we don't need https://github.com/coreos/update-ssh-keys and we could modify coreos-metadata to not depend on it?

@bgilbert
Copy link
Contributor Author

bgilbert commented Jan 30, 2019

@dustymabe I'm okay with that if #5 is accepted upstream. If not, I think we should pursue #4 in the medium term. It doesn't seem great for #139 (comment) to persist indefinitely.

update-ssh-keys could potentially be useful as a management tool (that no longer rewrote authorized_keys), but I think we should avoid pursuing that unless there's a clear need.

So the work items would be:

  • Consider renaming authorized_keys.d fragment for spec 3.0 ignition#710
  • Update AuthorizedKeysFile in Fedora CoreOS and Red Hat CoreOS
  • Drop update-ssh-keys functionality from Ignition and coreos-metadata. coreos-metadata would need it conditionalized for CL.
  • Document in Ignition and coreos-metadata upstream repos that a custom AuthorizedKeysFile directive is required
  • Continue pursuing upstream sshd patch

@dustymabe
Copy link
Member

@dustymabe I'm okay with that if #5 is accepted upstream. If not, I think we should pursue #4 in the medium term. It doesn't seem great for #139 (comment) to persist indefinitely.

If #5 is accepted upstream I think we should be able to get it backported to fedora's SSH (hopefully). If we don't hear anything back from upstream you prefer #4 ?

update-ssh-keys could potentially be useful as a management tool (that no longer rewrote authorized_keys), but I think we should avoid pursuing that unless there's a clear need.

So it sounds like regardless of the option we pick we have eliminated options that include update-ssh-keys and should start pursuing removing it as a dependency from ignition/coreos-metadata so we can drop it?

@bgilbert
Copy link
Contributor Author

If we don't hear anything back from upstream you prefer #4?

Yeah, I think so. Users will plausibly expect that they can add other files to the .d directory, and it's the right mechanic to support.

So it sounds like regardless of the option we pick we have eliminated options that include update-ssh-keys and should start pursuing removing it as a dependency from ignition/coreos-metadata so we can drop it?

👍

@ajeddeloh
Copy link
Contributor

Using an AuthorizedKeysCommand will conflict with google oslogin, since they also want to set that to their own helper.

@bgilbert
Copy link
Contributor Author

In principle, our AuthorizedKeysCommand could also call out to another one.

@bgilbert bgilbert removed the meeting topics for meetings label Feb 12, 2019
bgilbert added a commit to coreos/fedora-coreos-config that referenced this issue Mar 27, 2019
Ignition 2 and Afterburn no longer sync their authorized_keys.d fragment
files to ~/.ssh/authorized_keys.  Eventually we should have an
AuthorizedKeysCommand that walks authorized_keys.d, but meanwhile we'd
like the tools to still work.

coreos/fedora-coreos-tracker#139
@bgilbert bgilbert moved this from Selected to In Progress in Fedora CoreOS preview Mar 27, 2019
@bgilbert bgilbert added this to Proposed in Fedora CoreOS stable via automation May 2, 2019
@bgilbert bgilbert moved this from In Progress to Done in Fedora CoreOS preview May 2, 2019
@bgilbert bgilbert moved this from Proposed to Selected in Fedora CoreOS stable May 2, 2019
@ibotty
Copy link

ibotty commented Jul 1, 2019

Note that regarding option 5 there has been a review on upstream openssh.
https://bugzilla.mindrot.org/show_bug.cgi?id=2755#c8
Maybe it's not that far off.

@bgilbert
Copy link
Contributor Author

Initial implementation of option 4 in coreos/ssh-key-dir#1.

@bgilbert
Copy link
Contributor Author

@bgilbert
Copy link
Contributor Author

Merged in coreos/fedora-coreos-config#498. @lucab is still pursuing option 5 upstream, and we can switch over to that patch if it lands in sshd.

@bgilbert bgilbert added status/pending-testing-release Fixed upstream. Waiting on a testing release. status/pending-next-release Fixed upstream. Waiting on a next release. labels Jun 29, 2020
@dustymabe
Copy link
Member

The fix for this went into testing stream release 32.20200629.2.0. Please try out the new release and report issues.

@dustymabe dustymabe removed status/pending-next-release Fixed upstream. Waiting on a next release. status/pending-testing-release Fixed upstream. Waiting on a testing release. labels Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

6 participants