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

Dev: utils: Avoid hardcoding the ssh key type as RSA #1600

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

liangxin1300
Copy link
Collaborator

@liangxin1300 liangxin1300 commented Nov 1, 2024

Problem

Changes include:

  • Avoid hardcoding the ssh key type as RSA
  • Introduced a new function ssh_key.fetch_public_key_list to fetch public keys from local or remote, return as public key path list or public key content list
  • In KeyFileManager, use class variable to store the key type instead of hardcoding it as RSA
  • In bootstrap.swap_public_ssh_key function, Change the parameter name from 'add' to 'generate_key_on_remote' to make it more descriptive
  • In the process of join_ssh, no need to set 'generate_key_on_remote' to True, as the key is already generated on init node
  • Improve shell script in generate_ssh_key_pair_on_remote to avoid hardcoding the key type as RSA
  • Remove unused code

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 88.23529% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.26%. Comparing base (917b0d9) to head (ece148f).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
crmsh/ssh_key.py 87.50% 2 Missing ⚠️
crmsh/utils.py 33.33% 2 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
integration 54.50% <88.23%> (-0.01%) ⬇️
unit 52.13% <32.35%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
crmsh/bootstrap.py 88.63% <100.00%> (-0.04%) ⬇️
crmsh/ssh_key.py 79.76% <87.50%> (+5.39%) ⬆️
crmsh/utils.py 66.54% <33.33%> (-0.16%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

In utils.ssh_copy_id_no_raise, the ssh key type is hardcoded as RSA.
Then the join process will fail if the existing key type is not RSA.

Also see:
ClusterLabs#1504 (comment)
In KeyFileManager, use class variable to store the key type instead of
hardcoding it as RSA.
@liangxin1300 liangxin1300 force-pushed the 20241031_key_issue_1504 branch 4 times, most recently from 0ea9007 to 1249b0b Compare November 4, 2024 06:09
Replace remote_public_key_from as ssh_key.fetch_public_key_list
…tion

- Change the parameter name from 'add' to 'generate_key_on_remote',
  which is more descriptive.
- In the process of join_ssh, no need to set 'generate_key_on_remote'
  to True, as the init node already has the public key.
@liangxin1300 liangxin1300 force-pushed the 20241031_key_issue_1504 branch 2 times, most recently from 28aca1d to 919b465 Compare November 4, 2024 07:55
@liangxin1300 liangxin1300 marked this pull request as ready for review November 4, 2024 14:17
:param with_content: whether to return the content of the public key files,
default is False

:return: a list of public key files if with_content is False, otherwise a list of public key strings
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. It is a bad idea to return vaules in a same type with different meanings. Please use Key in function signature and use KeyFile and InMemoryKey correspondingly. Or separate these 2 different usage into 2 functions.
  2. typing.List[str] is deprecated. Use list[str] instead.

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.

2 participants