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

Import/regenerate server SSH keys #547

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

omertuc
Copy link
Collaborator

@omertuc omertuc commented May 29, 2024

Background / Context

Every server has unique SSH server keys which help clients verify the server's identity (not to be confused with authorized keys, which are the list of public keys of clients that are allowed to log into the server).

Ideally, during IBI, we would regenerate the server SSH keys in the seed to avoid using the seed's SSH keys. This is because we don't want all cluster spun up from the same seed to have the same server SSH keys.

During IBU, we should use the server SSH keys from the original cluster to maintain the same server SSH fingerprint (otherwise, when a user tries to SSH following an IBU upgrade, they will see a warning that the server's fingerprint has changed).

Issue / Requirement / Reason for change

Currently, we're simply using the keys that are present in the seed (ticket MGMT-16818):

@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@

Solution / Feature Overview

During IBI, ask recert to regenerate fresh SSH server keys. In IBU, delete the existing seed keys and copy the server SSH keys from the original cluster.

Implementation Details

Add a new field to the SeedReconfiguration struct to hold the server SSH keys. This field is a list of ServerSSHKey structs, each of which contains the file name and content of the server SSH key.

If the list is empty (indicating IBI), we will ask recert to regenerate the server SSH keys.

The copying of the keys is done simply by LCA's post-pivot phase. We don't use recert for this because it's overkill, just need to delete some files and create new ones, no changes in the etcd database are needed.

Other Information

This work is incomplete, created a separate MGMT-17988 to track the purposeful invalidation of seed server SSH keys. Right now the seed image still contains the true server SSH keys of the seed cluster, which is not ideal as it compromises the security of the seed cluster.

Ideally we would regenerate the seed server SSH keys during seed image generation, and then restore the original server SSH keys once the seed cluster is restored. This would ensure that the seed image does not contain the true server SSH keys of the seed cluster and that the seed cluster's SSH fingerprint remains the same after seed generation.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2024
Copy link
Contributor

openshift-ci bot commented May 29, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the cluster-config-api-changed Cluster config API changed. It's used by other projects. Review to ensure your change is nonbreaking label May 29, 2024
@omertuc omertuc changed the title Import/regenerate server SSH keyr Import/regenerate server SSH keys May 29, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2024
@omertuc omertuc force-pushed the serverssh branch 4 times, most recently from 95aa0fa to 1ccfebf Compare June 3, 2024 07:52
@omertuc omertuc marked this pull request as ready for review June 3, 2024 07:56
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2024
@openshift-ci openshift-ci bot requested review from eranco74 and tsorya June 3, 2024 07:56
@omertuc omertuc marked this pull request as draft June 3, 2024 07:56
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2024
# Background / Context

Every server has unique SSH server keys which help clients verify the
server's identity (not to be confused with authorized keys, which are
the list of public keys of clients that are allowed to log into the
server).

Ideally, during IBI, we would regenerate the server SSH keys in the seed
to avoid using the seed's SSH keys. This is because we don't want all
cluster spun up from the same seed to have the same server SSH keys.

During IBU, we should use the server SSH keys from the original cluster
to maintain the same server SSH fingerprint (otherwise, when a user
tries to SSH following an IBU upgrade, they will see a warning that the
server's fingerprint has changed).

# Issue / Requirement / Reason for change

Currently, we're simply using the keys that are present in the seed (ticket MGMT-16818):

```
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@    WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!     @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
```

# Solution / Feature Overview

During IBI, ask recert to regenerate fresh SSH server keys. In IBU,
delete the existing seed keys and copy the server SSH keys from the
original cluster.

# Implementation Details

Add a new field to the `SeedReconfiguration` struct to hold the server
SSH keys. This field is a list of `ServerSSHKey` structs, each of which
contains the file name and content of the server SSH key.

If the list is empty (indicating IBI), we will ask recert to regenerate
the server SSH keys.

The copying of the keys is done simply by LCA's post-pivot phase. We
don't use recert for this because it's overkill, just need to delete
some files and create new ones, no changes in the etcd database are
needed.

# Other Information

This work is incomplete, created a separate MGMT-17988 to track the
purposeful invalidation of seed server SSH keys. Right now the seed
image still contains the true server SSH keys of the seed cluster,
which is not ideal as it compromises the security of the seed cluster.

Ideally we would regenerate the seed server SSH keys during seed image
generation, and then restore the original server SSH keys once the seed
cluster is restored. This would ensure that the seed image does not
contain the true server SSH keys of the seed cluster and that the seed
cluster's SSH fingerprint remains the same after seed generation.
@omertuc omertuc marked this pull request as ready for review June 4, 2024 10:44
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2024
@omertuc
Copy link
Collaborator Author

omertuc commented Jun 4, 2024

/remove-label cluster-config-api-changed

@openshift-ci openshift-ci bot requested a review from imiller0 June 4, 2024 10:44
@openshift-ci openshift-ci bot removed the cluster-config-api-changed Cluster config API changed. It's used by other projects. Review to ensure your change is nonbreaking label Jun 4, 2024
@omertuc
Copy link
Collaborator Author

omertuc commented Jun 4, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Jun 4, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omertuc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2024
return nil
}

func removeOldServerSSHKeys() error {
Copy link
Contributor

@tsorya tsorya Jun 4, 2024

Choose a reason for hiding this comment

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

maybe lets not bring them from seed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will mention that in the ticket comments

@browsell
Copy link
Collaborator

browsell commented Jun 4, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit c9cd484 into openshift-kni:main Jun 4, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants