New module_utils openssh #213

Ajpantuso · 2021-03-28T20:33:06Z

SUMMARY

Utilities for working with OpenSSH key pairs abstracted from cryptography.
The intention is to offer these utilities as an alternative to invoking ssh-keygen.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugins/module_utils/crypto/cryptography_openssh.py
tests/unit/plugins/module_utils/crypto/test_cryptography_openssh.py

ADDITIONAL INFORMATION

This exposes two classes, Asymmetric_Keypair for encapsulating asymmetric keypairs supported by cryptography and OpenSSH_Keypiar for the OpenSSH encoded/formatted representation of a keypair, mimicking some of the features offered by ssh-keygen.

Attributes of both classes for the most part are exposed as read-only properties to reinforce that keys are immutable.
However comments applied to OpenSSH formatted public keys can be updated.

Since ed25519 is only available from cryptography >=2.6 the utilities will only successfully import if that condition is met.
A soft requirement is cryptography >=3.0 for OpenSSH formatted private keys, but ssh-keygen is not consistent about formatting private keys this way depending on version either.

Text encoding is set to UTF-8 globally in the module and could be changed to ASCII if neccessary.

SHA256 is used as the standard hash algorithm to align with current ssh-keygen versions.

I removed a couple of unit tests for directly comparing ssh-keygen to cryptography generated keys and vise versa because I wasn't sure how best to implement command invocation outside of an Ansible Module. (Direct comparisons are positive in my local test environment for whatever that is worth)

Definitely looking for input on this one as this is for the most part just a straight encapsulation of the cryptography library with most of the effort spent on how to present it as a utility in this collection.

…ader

plugins/module_utils/crypto/cryptography_openssh.py

felixfontein · 2021-03-29T17:17:34Z

plugins/module_utils/crypto/cryptography_openssh.py

+                else:
+                    self.__size = self.__algorithm_parameters[self.__keytype]['default_size']
+
+                self.__generate_keypair()


I think it would be better to have class methods with speaking names to do these tasks, i.e. Asymmetric_Keypair.load() to load a keypair and Asymmetric_Keypair.generate() to generate one.

Have you thought about this one?

I did remove _keypair from these method names, but I may be missing the meaning of your suggestion.
Was the intention that these be readable as actions solely in the context of initialization or that the methods also be exposed for end users? I intentionally wanted to present asymmetric keypairs as immutable single-use objects hence why the loading/generating mechanisms are obscured.

I would offer them as class methods, i.e. you can do AsymmetricKeypair.load(filename) to load a keypair, which will return an AsymmetricKeypair instance. This is more expressive then using AsymmetricKeypair(path=filename) IMO.

(See for example https://code-maven.com/slides/python/class-methods-alternative-constructor for this kind of pattern.)

Ok, that does open the possibility of an empty object and makes the faux overloaded constructor less confusing. I will spend some time on that. Thanks

You could declare the constructor private (by docstring, no idea whether Python allows to do that explicitly without using ugly hacks) to avoid having an empty keypair.

I am considering the possibility of an empty object a benefit.
That would allow for the truthiness of the class to be evaluated in a more obvious (pythonic) way.

Hmm, I think it's a lot easier to let only None represent an empty key, instead of having an instance of the class which can represent an empty key. While such an empty value could simplify some cases, it in general makes code more complicated, since it is harder to ensure that a key is actually specified. (For example, it is basically impossible to specify this requirement with the type system, which is IMO a huge disadvantage.)

plugins/module_utils/crypto/cryptography_openssh.py

felixfontein · 2021-04-06T19:48:11Z

plugins/module_utils/openssh/cryptography_openssh.py

+
+class Asymmetric_Keypair(object):
+    def __init__(self, path=None, keytype='rsa', size=None, passphrase=None):
+        """Container for newly generated asymmetric keypairs or those loaded from existing files


This line should be part of the class's docstring, not of the __init__ docstring. It describes what the class does, not what __init__ does. (Same for the other class.)

Ok, generate/load are now exposed as class methods and docstrings are cleaned up.
Updated unit tests to use generate/load as well just for illustration.

…od comments

felixfontein · 2021-04-11T13:31:54Z

The change c227733 is not what I meant. It basically reverts the PR to what it was at the beginning (with the class methods thin wrappers around __init__).

The __init__ function should only look like this:

    def __init__(self, keytype, privatekey, publickey, size):
        """
           :keytype: One of rsa, dsa, ecdsa, ed25519
           :privatekey: The cryptography pivate key class
           :publickey: The cryptography public key class
           :size: The key size
        """
        self.__encryption_algorithm = serialization.NoEncryption()
        self.__keytype = keytype
        self.__privatekey = privatekey
        self.__publickey = publickey
        self.__size = size

The 'hard work' should be done by Asymmetric_Keypair.generate and Asymmetric_Keypair.load. They do the work, and once done call __init__ to hand over the generated / loaded data.

Ajpantuso · 2021-04-11T16:51:08Z

The change c227733 is not what I meant. It basically reverts the PR to what it was at the beginning (with the class methods thin wrappers around __init__).

The __init__ function should only look like this:
    def __init__(self, keytype, privatekey, publickey, size):
        """
           :keytype: One of rsa, dsa, ecdsa, ed25519
           :privatekey: The cryptography pivate key class
           :publickey: The cryptography public key class
           :size: The key size
        """
        self.__encryption_algorithm = serialization.NoEncryption()
        self.__keytype = keytype
        self.__privatekey = privatekey
        self.__publickey = publickey
        self.__size = size
The 'hard work' should be done by Asymmetric_Keypair.generate and Asymmetric_Keypair.load. They do the work, and once done call __init__ to hand over the generated / loaded data.

I was avoiding that because it required re-scoping the helper/utility functions, but the changes are incorporated as of the latest commit.

As a side note, how do I clear this sanity check?
ERROR: plugins/module_utils/openssh/cryptography_openssh.py:87:0: traceback: NameError: name 'padding' is not defined

I didn't change how padding is imported, but previously it was referred to in an instance variable and now it's being referred to in a class variable.

Also the affected sign method which requires padding to be defined works as expected.

Ajpantuso · 2021-04-12T18:24:05Z

/rebuild_failed

felixfontein · 2021-04-12T18:48:36Z

/rebuild_failed doesn't work here (since we don't have a bot here), but I've restarted them manually :)

felixfontein

Sorry it took so long to review this!

felixfontein · 2021-05-02T12:45:47Z

plugins/module_utils/openssh/cryptography_openssh.py

+        elif isinstance(privatekey, ec.EllipticCurvePrivateKey):
+            keytype = 'ecdsa'
+        elif isinstance(privatekey, Ed25519PrivateKey):
+            keytype = 'ed25519'


This needs a final else block. It should either raise an exception, or set keytype to a concrete value.

felixfontein · 2021-05-02T12:47:21Z

plugins/module_utils/openssh/cryptography_openssh.py

+
+           :signature: signature to verify
+           :data: byteslike data signed by the provided signature
+        """


I think it is a better API to let verify() raise an exception in case verification fails, and return nothing in case validation succeeds, i.e. do the same thing as cryptography also does.

felixfontein · 2021-05-02T12:48:19Z

plugins/module_utils/openssh/cryptography_openssh.py

+    @classmethod
+    def encode_openssh_privatekey(cls, asym_keypair):


Suggested change

@classmethod

def encode_openssh_privatekey(cls, asym_keypair):

@staticmethod

def encode_openssh_privatekey(asym_keypair):

felixfontein · 2021-05-02T12:48:35Z

plugins/module_utils/openssh/cryptography_openssh.py

+    @classmethod
+    def encode_openssh_publickey(cls, asym_keypair, comment):


Suggested change

@classmethod

def encode_openssh_publickey(cls, asym_keypair, comment):

@staticmethod

def encode_openssh_publickey(asym_keypair, comment):

felixfontein · 2021-05-02T12:49:50Z

plugins/module_utils/openssh/cryptography_openssh.py

+        self.__comment = comment
+
+    def __eq__(self, other):
+        return self.fingerprint == other.fingerprint and self.comment == other.comment


I would rather compare the keys themselves, instead of only comparing the fingerprint. Or are some of the key objects not comparable?

(Also this ignores the passphrase.)

felixfontein · 2021-05-02T12:51:04Z

plugins/module_utils/openssh/cryptography_openssh.py

+        """
+
+        self.__asym_keypair.update_passphrase(passphrase)
+        self.__openssh_privatekey = type(self).encode_openssh_privatekey(self.__asym_keypair)


Suggested change

self.__openssh_privatekey = type(self).encode_openssh_privatekey(self.__asym_keypair)

self.__openssh_privatekey = OpenSSH_Keypair.encode_openssh_privatekey(self.__asym_keypair)

Ajpantuso · 2021-05-02T13:23:37Z

Sorry it took so long to review this!

No worries, this is definitely not a high priority and the reviews are very helpful.

Everything suggested makes sense and I will incorporate the changes.

Regarding the __eq__ method it was definitely the lazy way to do it and yes ed25519 keys have no implementation for comparison as far as I can tell (See here), but for the other key types it's simple to compare on the constituent components of the keys. (See here and here for examples with RSA)

I can investigate more on the special case of ed25519 and probably just ask over on the cryptography side if they have any suggestions.

Thanks.

felixfontein · 2021-05-02T13:26:40Z

Ah, that reminds me that I already had to solve that problem once ;-) See cryptography_compare_public_keys in plugins/module_utils/crypto/cryptography_support.py. I think comparing public keys suffices as well, since different private keys should not result in the same public key.

… load bug

Ajpantuso · 2021-05-02T20:25:21Z

Ah, that reminds me that I already had to solve that problem once ;-) See cryptography_compare_public_keys in plugins/module_utils/crypto/cryptography_support.py. I think comparing public keys suffices as well, since different private keys should not result in the same public key.

Thanks, I made use of your example.
Seems like CI couldn't pull 'coverage' again, but otherwise I think CI should clear.
Besides the suggested changes I also:

Stopped encoding passphrase input to bytes to allow the existing validation on the Cryptography side to run naturally
Fixed a bug for loading ed25519 keys caused by the private key class not having a key_size attribute
Cleaned up some comments
Added an explicit test for various comparison scenarios

felixfontein

Let's get this merged! 🎉

felixfontein · 2021-05-03T19:11:03Z

@Ajpantuso thanks for contributing this!

Ajpantuso · 2021-05-03T19:35:40Z

Let's get this merged! 🎉

Thanks for your help!

Ajpantuso added 3 commits March 28, 2021 14:50

Adding openssh utils and unit tests

0a0c687

Adding changelog fragment and correcting RSA default size

b743543

Adding changelog fragment

71c6aba

Ajpantuso marked this pull request as ready for review March 28, 2021 21:12

Ajpantuso added 2 commits March 29, 2021 09:46

Added passphrase update, test cases, and check for SSH private key lo…

7bea795

…ader

corrected ecdsa type when loading

2188554

felixfontein reviewed Mar 29, 2021

View reviewed changes

Ajpantuso added 4 commits March 29, 2021 16:14

Resolving inital review comments

a84f0bb

Fixed import in unit tests

6d2eb88

Cleaning up validation functions

77253bb

Separating private/public key related errors; Adding verify method

725b466

felixfontein reviewed Apr 6, 2021

View reviewed changes

felixfontein mentioned this pull request Apr 7, 2021

Next release: 1.7.0 #209

Closed

11 tasks

felixfontein changed the title ~~New module_utils cryptography_openssh~~ New module_utils openssh Apr 7, 2021

Ajpantuso added 2 commits April 10, 2021 17:22

Expressed generate/load functions as classmethods and cleaned up meth…

c227733

…od comments

Added support for loading asymmetric key pairs of PEM and DER formats

2e95204

Ajpantuso added 3 commits April 11, 2021 11:28

Refactored loading/generation for Asym keypairs into classmethods

9881739

Rescoped helper functions and classmethods for OpenSSH Keypair

5da8ba0

Corrected docstring for OpenSSH_Keypair.generate()

b1f80a7

Fixed import errors for sanity tests

be42d76

felixfontein reviewed May 2, 2021

View reviewed changes

Ajpantuso added 2 commits May 2, 2021 17:04

Improvements to comparison, key verification, and password validation

bf2ecca

Added comparison tests, simplified password validation, fixed Ed25519…

d6df24a

… load bug

Adding additional equivalence tests with passphrases

fe4d605

felixfontein approved these changes May 3, 2021

View reviewed changes

felixfontein merged commit 37c1540 into ansible-collections:main May 3, 2021

renovate bot mentioned this pull request Aug 25, 2021

Update dependency community.crypto to v1.8.0 estenrye/slides#11

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module_utils openssh #213

New module_utils openssh #213

Ajpantuso commented Mar 28, 2021 •

edited

Loading

felixfontein Mar 29, 2021

felixfontein Apr 6, 2021

Ajpantuso Apr 6, 2021

felixfontein Apr 6, 2021

felixfontein Apr 6, 2021

Ajpantuso Apr 6, 2021

felixfontein Apr 6, 2021

Ajpantuso Apr 6, 2021

felixfontein Apr 6, 2021

felixfontein Apr 6, 2021

Ajpantuso Apr 10, 2021

felixfontein commented Apr 11, 2021

Ajpantuso commented Apr 11, 2021

Ajpantuso commented Apr 12, 2021

felixfontein commented Apr 12, 2021

felixfontein left a comment

felixfontein May 2, 2021

felixfontein May 2, 2021

felixfontein May 2, 2021

felixfontein May 2, 2021

felixfontein May 2, 2021

felixfontein May 2, 2021

Ajpantuso commented May 2, 2021

felixfontein commented May 2, 2021

Ajpantuso commented May 2, 2021

felixfontein left a comment

felixfontein commented May 3, 2021

Ajpantuso commented May 3, 2021

		@classmethod
		def encode_openssh_privatekey(cls, asym_keypair):

		@classmethod
		def encode_openssh_publickey(cls, asym_keypair, comment):

	self.__openssh_privatekey = type(self).encode_openssh_privatekey(self.__asym_keypair)
	self.__openssh_privatekey = OpenSSH_Keypair.encode_openssh_privatekey(self.__asym_keypair)

New module_utils openssh #213

New module_utils openssh #213

Conversation

Ajpantuso commented Mar 28, 2021 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Apr 11, 2021

Ajpantuso commented Apr 11, 2021

Ajpantuso commented Apr 12, 2021

felixfontein commented Apr 12, 2021

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ajpantuso commented May 2, 2021

felixfontein commented May 2, 2021

Ajpantuso commented May 2, 2021

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented May 3, 2021

Ajpantuso commented May 3, 2021

Ajpantuso commented Mar 28, 2021 •

edited

Loading