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

ipa role: adding sshpubkey parameter to users #131

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

danlavu
Copy link

@danlavu danlavu commented Oct 4, 2024

No description provided.

@danlavu danlavu force-pushed the roles-ipa-user_pubkey branch 5 times, most recently from a770d46 to 7eb6ee8 Compare October 7, 2024 05:27
@danlavu
Copy link
Author

danlavu commented Oct 7, 2024

I'm still getting comfortable working with multiple commits and amending changes to previous commits. I have an extra commit I was trying to squash.

@danlavu danlavu force-pushed the roles-ipa-user_pubkey branch 3 times, most recently from 94f4d22 to e3529a6 Compare October 7, 2024 19:51
@danlavu
Copy link
Author

danlavu commented Oct 7, 2024

tests that use this enhancement SSSD/sssd#7608

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a few small things to check on.

sssd_test_framework/roles/client.py Outdated Show resolved Hide resolved
sssd_test_framework/utils/tools.py Outdated Show resolved Hide resolved
sssd_test_framework/utils/tools.py Show resolved Hide resolved
sssd_test_framework/utils/tools.py Outdated Show resolved Hide resolved
spoore1
spoore1 previously approved these changes Oct 10, 2024
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 786 to 788
self.user = user
self.path = path
self.file = file
Copy link
Member

Choose a reason for hiding this comment

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

This won't work if you call keygen multiple times.

You can:

  • return (pubkey, privatekey) pair instead of self
  • or return a new object e.g. SSHKey that will hold the information

Copy link
Author

@danlavu danlavu Oct 14, 2024

Choose a reason for hiding this comment

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

I never intended it to be called keygen multiple times without changing the file name... like
https://github.com/SSSD/sssd/blob/99073123c4727a005d7f72b1a784c4474eb1231d/src/tests/system/tests/test_ipa.py#L193

I'm a little confused; I do have an object, SSHKeysUtils, that should be created each time, that contains the user, path and file. Get returns a tuple of the public and private keys.

Copy link
Member

@pbrezina pbrezina Oct 15, 2024

Choose a reason for hiding this comment

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

But SSHKeysUtils is not create multiple times, it is created only once for each test.

If you want it to be created multiple times, then you should do it via @property. It also should not inherit from MultihostUtility, but from object in this case.

@property
def keygen(...) -> SSHKeyUtils:
    return SSHKeyUtils(...)

public_key, private_key = client.tools.keygen.generate()

Or we can leave it as is (single object) but then it should not set an internal state.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the class inheritance and made keygen a property that calls SSHKeyUtils

@danlavu danlavu force-pushed the roles-ipa-user_pubkey branch 2 times, most recently from 11ce091 to a3af139 Compare October 16, 2024 02:15
@danlavu danlavu requested a review from pbrezina October 16, 2024 02:16
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Just a question about the class name.

sssd_test_framework/utils/tools.py Show resolved Hide resolved
Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

Hi Dan, it looks good, thank you. See nitpicks inside.

sssd_test_framework/utils/tools.py Show resolved Hide resolved
sssd_test_framework/utils/tools.py Outdated Show resolved Hide resolved
sssd_test_framework/utils/tools.py Show resolved Hide resolved
Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

LGTM

@pbrezina pbrezina merged commit 84aadec into SSSD:master Oct 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants