-
Notifications
You must be signed in to change notification settings - Fork 256
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
tests: rm /intg/test_ssh_pubkey.py #7608
base: master
Are you sure you want to change the base?
Conversation
danlavu
commented
Sep 19, 2024
- multihost/ipa/test_misc.py functionally covers this scenario
- test_ssh_sighup offers minimal value and should be dropped now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Multihosts tests aren't run in upstream PR CI |
ACK, I extended the framework to provide user SSH key functionality. I need to test it, and I can then write a quick test to cover this. |
2e7f2cf
to
62111d9
Compare
depends on SSSD/sssd-test-framework#131 , tests are going to fail. |
Though... sss_ssh_authorizedkeys is no longer providing output. Need to figure out why. |
2033e67
to
9907312
Compare
1e3e7c9
to
c2070e1
Compare
eda1acb
to
52e7a05
Compare
The failure is not related to the patch. |
def test_ssh_pubkey_retrieve_cert(add_user_with_ssh_cert): | ||
""" | ||
Test that we can retrieve an SSH public key derived from a cert in ldap. | ||
Compare with the sshpubkey derived via ssh-keygen, they should match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are removing test coverage for public key derived from certificate, is it okay? ldap_user_certificate = userCertificate;binary
in format_basic_conf method above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okay, because most users would use ssh-keygen, but I maybe overlooking something because I'm not all that familiar with smartcards. Is it a common scenario to take a certificate from a smartcard and convert it to an SSH key? Is it even important? Maybe it's already covered, @spoore1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The certificate has to be converted into a public key for use with SSH. We have some coverage downstream but, it's going to be a while still before that can be ported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spoore1 Can we plan it for a smartcard test, and push this forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
test_ssh_pubkey_retrieve_cert
tests the feature to generate an ssh-key from the public-key of the Smartcart so that the Smartcard can be used for ssh pubkey authentication. Wouldn't it be possible to check this feature in a similar way as the integration test, i.e. use the certificates generated by the SSSD test CA and compare the output of sss_ssh_authorized_keys
with the corresponding ssh-keys also generated by the SSSD test CA?
bye,
Sumit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be to make it more realistic. It's not just this test that requires SSH keys. We plan on using both SSH keys and certificates in two different customer scenarios. Other tests use SSH keys to test things like GSSAPI, so the effort to add the function to the framework isn't at all wasted.
Back from vacation, will look at this, this week. |
* multihost/ipa/test_misc.py functionally covers this scenario
52e7a05
to
89c16b6
Compare
CI failures are not relevant to this PR. |