Skip to content

Fixes #39657 - drop sshkey gem and replace with ssh-keygen cmd - #11185

Open
lzap wants to merge 4 commits into
theforeman:developfrom
lzap:sshkey1
Open

Fixes #39657 - drop sshkey gem and replace with ssh-keygen cmd#11185
lzap wants to merge 4 commits into
theforeman:developfrom
lzap:sshkey1

Conversation

@lzap

@lzap lzap commented Aug 19, 2026

Copy link
Copy Markdown
Member

For FIPS/PQC, we want to remove all SSH related rubygems that do not provide all required features. More generally, we would like to simplify security stack in Foreman and leverage OpenSSH library of the system.

This is a multi-commit PR that does the following:

  • Refactors relevant code from model/validator into a service class SshKey (no changes)
  • Covers it with unit tests with all supported modern ssh keys as fixtures (no changes)
  • Rewrite the implementation to using ssh-keygen instead and add generate method

As you can see, there is one functional change that comes with the refactoring. The original rubygem returned key lengths in total number of bits, whereas ssh-keygen returns number of bits of elyptic curves (or something like that - not important). Therefore, length do change and now they reflect what OpenSSH returns which is for the good. Those lengths are not used for anything other than showing this in the UI/API.

Additionally, there was no generate method that would allow for safe and resilient SSH key generation and Foreman plugins do use this gem. This patch provides that method and tests so plugins can use the new generate method. It is also used in test factories where we generate about one dozen of keys each run. I double checked that this is safe on modern kernels and tests would not get stuck in case of low entropy.

Example use: https://github.com/theforeman/foreman_google/blob/3e6a3612056539bbb46c897354e1cdf0c835bace/app/models/foreman_google/gce.rb#L101

Foreman Packaging reviewers, here:

@lzap
lzap requested a review from a team as a code owner August 19, 2026 14:38
Comment thread app/services/foreman/provision/ssh_key.rb Outdated
Comment thread db/migrate/20200127103144_ssh_keys_fingerprints_sha1.rb
@stejskalleos
stejskalleos self-requested a review August 26, 2026 08:37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit:

In config/initializers/inflections.rb is the following comment:

  # Causes an overlap between ::SSHKey and the SshKey model
  # inflect.acronym 'SSH' # Secure SHell

Is that still relevant, or can we remove it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is still relevant, I thought about either renaming the package/namespace or leaving the inflection as-is.

Comment thread app/services/foreman/provision/ssh_key.rb Outdated
Comment thread app/services/foreman/provision/ssh_key.rb
@lzap

lzap commented Sep 1, 2026

Copy link
Copy Markdown
Member Author

@stejskalleos

Copy link
Copy Markdown
Contributor

@lzap can you please rebase the commits?
Code LGTM. Once the CI is green, I'll merge it.

lzap and others added 4 commits September 7, 2026 10:49
Pass -v to ssh-keygen when Rails runs in development so key
generation and fingerprinting are easier to debug locally.

Co-authored-by: Cursor <cursoragent@cursor.com>
@lzap

lzap commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

Rebased onto develop, I am keeping four commits squash at will.

Comment thread Gemfile
gem 'roadie-rails', '~> 3.0'
gem 'deacon', '~> 1.0'
gem 'mail', '~> 2.7'
gem 'sshkey', '~> 2.0'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this PR removes the sshkey gem dependency, should we also remove the obsolete rubygem-sshkey?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ogajduse ogajduse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Packaging ACK. This should be merged alongside with theforeman/foreman-packaging#14089

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