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

feat: Add support for the ssh_key parameter for credentials #247

Merged
merged 6 commits into from
Jul 17, 2023

Conversation

abellotti
Copy link
Contributor

@abellotti abellotti commented Jun 15, 2023

feat: Add support for the ssh_key parameter for credentials

  • Allows us to specify an ssh_key instead of a password or ssh_keyfile.
  • It is supported for both credential creation and edits.

This enhancement is to support Quipucords' quipucords/quipucords#2390

$ qpc cred add --help
usage: qpc cred add [-h] --name NAME --type TYPE [--username USERNAME] (--password | --sshkeyfile FILENAME | --sshkey | --token)
                    [--sshpassphrase] [--become-method BECOME_METHOD] [--become-user BECOME_USER] [--become-password]

options:
  -h, --help            show this help message and exit
  --name NAME           Credential name.
  --type TYPE           Type of credential. Valid values: ansible, network, openshift, satellite, vcenter.
  --username USERNAME   User name for authenticating against the target system.
  --password            Password for authenticating against the target system.
  --sshkeyfile FILENAME
                        File that contains the SSH key.
  --sshkey              The SSH Private Key.
  --token               Authentication token.
  --sshpassphrase       SSH passphrase for authenticating against the target system.
  --become-method BECOME_METHOD
                        The method to become for network privilege escalation. Valid values: sudo, su, pbrun, pfexec, doas, dzdo, ksu, runas.
  --become-user BECOME_USER
                        The user to become when running a privileged command during network scan.
  --become-password     The privilege escalation password to be used when running a network scan.

The usage for specifying an ssh key is prompted like the password or SSH Key passphrase. It is a non-echoed multiline input that gets ended with a Control-D.

$ deploy/qpc cred add --name testing1 --type network --username abellott --sshpassphrase --sshkey
Provide a Private SSH Key followed by a Control-D.
Private SSH Key: 
Provide a passphrase for the SSH Key.
Password: 
$

Similarly with updates where just the credential name and sshkey need to be specified:

$ deploy/qpc cred edit --name testing1 --sshkey
Provide a Private SSH Key followed by a Control-D.
Private SSH Key: 
$

When not attaching a tty to the the docker image, the sshkey can be passed to qpc via stdin, either as a pipe to qpc or via a heredoc as follows:

$ cat ~/.ssh/qpc_client_rsa | deploy/qpc --no-tty cred edit --name testing1 --sshkey

or via a heredoc:

$ deploy/qpc --no-tty cred edit --name testing1 --sshkey<<END
-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAABFwAAAAdzc2gtcn
NhAAAAAwEAAQAAAQEAxOUoCsRav3IO6GCBSNXh0EnJS07UWjT2sswRZHvBK6WWTML9j7SS
...
-----END OPENSSH PRIVATE KEY-----
END

The above methods will work for both credential add or edits.

Note that if an sshkey is sent in via stdin, no other password type parameters can be prompted for by qpc as there will not be a tty attached. If both an sshkey and an sshpassphrase need to be entered in the same qpc command execution, this must be done with a tty attached, so no stdin pipe allowed for the sshkey. Automating such cases may require the use of expect with qpc or using the quipucords API directly.

Note: once this is a go, tests need to be added and the man page will also be updated in this PR.

To-do's:

  • Add tests
  • Update the qpc man page

@abellotti abellotti marked this pull request as draft June 15, 2023 03:42
@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2023

Codecov Report

Merging #247 (b0124c4) into main (10b14c3) will decrease coverage by 0.31%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   93.86%   93.55%   -0.31%     
==========================================
  Files          67       67              
  Lines        2802     2841      +39     
==========================================
+ Hits         2630     2658      +28     
- Misses        172      183      +11     
Impacted Files Coverage Δ
qpc/cred/utils.py 82.14% <72.72%> (-2.18%) ⬇️
qpc/cred/add.py 100.00% <100.00%> (ø)
qpc/cred/edit.py 89.79% <100.00%> (+0.21%) ⬆️
qpc/messages.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@bruno-fs bruno-fs left a comment

Choose a reason for hiding this comment

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

I like the approach, I couldn't think of a better way to handle the limitation of running the CLI on a container.

Unfortunately the UX is not great, but honestly I think this is the best we can have while distributing the CLI as a container...

qpc/cred/utils.py Outdated Show resolved Hide resolved
@abellotti abellotti changed the title feat: Add support for the ssh_keyvalue parameter for credentials feat: Add support for the ssh_key parameter for credentials Jul 11, 2023
@abellotti abellotti force-pushed the support_ssh_keyvalues branch 8 times, most recently from 01c4e65 to a770451 Compare July 14, 2023 04:28
@abellotti abellotti marked this pull request as ready for review July 14, 2023 04:30
Copy link
Contributor

@bruno-fs bruno-fs 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 hard work =D 👏🏽

BTW, I experimented with these changes and I think the UX is pretty solid 👍🏽 👍🏽 👍🏽

qpc/cred/add.py Outdated Show resolved Hide resolved
- Allows us to specify an ssh_key instead of a password or ssh_keyfile.
- It is supported for both credential creation and edits.
- Reducing the complexity of get_password by breaking out the logic
  for the individual credential attributes into their own functions.
- Updated the input for sshkey to support entering
  data via stdin/heredoc. This requires running podman
  without the -t (attach tty) option.

  Note that not attaching a tty, one cannot enter password
  like parameters (password, sshpassphrase, etc), so a tty
  check is added to verify and enforce such cases.
- in preparation for adding the ssh_key credential tests
  updated the cred tests from unittest to pytest.
- Adding ssh_key related tests for the credentials
  add and edit CLI.
- updated the qpc man page for the new --sshkey option of the
  ssh_key credential attribute.
- Renaming the qpc tests from their tests_* unittest names
  to the pytest tests_* convention.
- combining password, become_password and ssh_passphrase to
  use a common _get_attribute_value method.
Copy link
Member

@nicolearagao nicolearagao left a comment

Choose a reason for hiding this comment

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

LGTM

@abellotti abellotti merged commit 7e6ed88 into quipucords:main Jul 17, 2023
4 checks passed
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