Add support for SSH certificates using ecdsa-sk or ed25519-sk public keys #813

jnohlgard · 2024-10-29T10:47:03Z

SUMMARY

Fixes #796

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

module_utils.openssh.certificate

ADDITIONAL INFORMATION

The openssh.certificate module can both sign using FIDO2 SSH keys and create signatures for FIDO2 SSH keys, but the overall task results in a failure status because it can not understand the certificate it has created.
This PR adds two new types of public keys and certificates to the load function which match the ssh-keygen key types ecdsa-sk and ed25519-sk.

Steps to reproduce

This requires a hardware FIDO2 key, e.g. Yubikey.

Create FIDO2 keys using ssh-keygen -t ecdsa-sk -f /tmp/id_ecdsa_sk -N '' and ssh-keygen -t ecdsa-sk -f /tmp/id_ed25519_sk -N '', then use the example below to sign the ECDSA key using the Ed25519 key.

`sign-key.yml`

---
- name: Sign OpenSSH user key
  hosts: localhost
  gather_facts: false
  tasks:
    # Generate an OpenSSH user certificate that is valid for 1 hour from now and will be regenerated
    # if it is valid for less than 10 minutes from the time the module is being run
    - name: Certify OpenSSH user key using master key
      community.crypto.openssh_cert:
        type: user
        signing_key: /tmp/id_ed25519_sk
        public_key: /tmp/id_ecdsa_sk.pub
        path: /tmp/id_ecdsa_sk-cert.pub
        valid_from: -1m
        valid_to: +1h
        valid_at: +10m
        ignore_timestamps: true
        principals:
          - myname
        options:
          - clear
          - permit-pty
          - no-touch-required

$ ansible-playbook -c local -i /dev/null -v sign-key.yml

Without this PR:

No config file found; using defaults
[WARNING]: provided hosts list is empty, only localhost is available. Note that the
implicit localhost does not match 'all'

PLAY [Sign OpenSSH user key] **********************************************************

TASK [Certify OpenSSH user key using master key] **************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "Unable to read new certificate: Invalid certificate format identifier: b'[email protected]'"}

PLAY RECAP ****************************************************************************
localhost                  : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

The full traceback is:

  File "/tmp/ansible_community.crypto.openssh_cert_payload_kpyxp1cv/ansible_community.crypto.openssh_cert_payload.zip/ansible_collections/community/crypto/plugins/modules/openssh_cert.py", line 469, in _generate
    self.data = OpensshCertificate.load(self.path)
                ~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
  File "/tmp/ansible_community.crypto.openssh_cert_payload_kpyxp1cv/ansible_community.crypto.openssh_cert_payload.zip/ansible_collections/community/crypto/plugins/module_utils/openssh/certificate.py", line 482, in load
    raise ValueError("Invalid certificate format identifier: %s" % format_identifier)

With this PR applied:

No config file found; using defaults
[WARNING]: provided hosts list is empty, only localhost is available. Note that the
implicit localhost does not match 'all'

PLAY [Sign OpenSSH user key] **********************************************************

TASK [Certify OpenSSH user key using master key] **************************************
changed: [localhost] => {"changed": true, "filename": "/tmp/id_ecdsa_sk-cert.pub", "info": ["Type: [email protected] user certificate", "Public key: ECDSA-SK-CERT SHA256:5+FIpbU8zV3jqDqNLoC8XIS2vfeoGNxARJVVhucoR8c", "Signing CA: ED25519-SK SHA256:rQJvUPDwKZOst1h5MnDrum4bhEU6KnEi5HNb+4qYYm0 (using [email protected])", "Key ID: \"\"", "Serial: 0", "Valid: from 2024-10-29T11:33:01 to 2024-10-29T12:34:01", "Principals: myname", "Critical Options: (none)", "Extensions: no-touch-required permit-pty"], "type": "user"}

PLAY RECAP ****************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

…keys Fixes ansible-collections#796

felixfontein

Thanks a lot for your contribution! I've taken a first look and added some first comments.

Please also add a changelog fragment, and note that the sanity tests are currently failing.

felixfontein · 2024-10-30T17:29:11Z

plugins/module_utils/openssh/certificate.py

@@ -145,7 +151,8 @@ def format_datetime(dt, date_format):
            elif dt == _FOREVER:
                result = 'forever'
            else:
-                result = dt.isoformat().replace('+00:00', '') if date_format == 'human_readable' else dt.strftime("%Y%m%d%H%M%S")
+                result = dt.isoformat().replace('+00:00', '') if date_format == 'human_readable' else dt.strftime(
+                    "%Y%m%d%H%M%S")


Would you mind undoing reformatting changes to unrelated code?

felixfontein · 2024-10-30T17:32:48Z

plugins/module_utils/openssh/certificate.py

+        return fingerprint(writer.bytes())
+
+    @abc.abstractmethod
+    def write_public_key_params(self, writer):


This is a breaking API change. You need to remove @abc.abstractmethod and add in a docstring that in community.crypto 3.0.0 this will become an abstract method.

Also you should add a docstring here explaining what this function is expected to do.

@jnohlgard I think it would be better to just introduce the feature in this PR and move the refactor to a follow up if necessary. As @felixfontein is indicating you would force this feature to wait until the next major release if it includes breaking changes.

felixfontein · 2024-10-30T17:33:45Z

plugins/module_utils/openssh/certificate.py

    def public_key_fingerprint(self):
+        if self.public_key_type_string is None:
+            return b''


Returning b'' instead of None is also an API breakage, I would add a docstring that specifies that None will change to b'' in community.crypto 3.0.0.

b'' is actually consistent with the existing behavior.self.public_key_type_string is a new property added with this refactor which is why there is a None check now.

Ajpantuso

Overall looks good, but as mentioned in the review comments putting the breaking changes into a separate PR is probably best.

Ajpantuso · 2024-10-30T18:13:13Z

plugins/module_utils/openssh/certificate.py

    def public_key_fingerprint(self):
+        if self.public_key_type_string is None:
+            return b''


b'' is actually consistent with the existing behavior.self.public_key_type_string is a new property added with this refactor which is why there is a None check now.

Ajpantuso · 2024-10-30T18:15:35Z

plugins/module_utils/openssh/certificate.py

+        return fingerprint(writer.bytes())
+
+    @abc.abstractmethod
+    def write_public_key_params(self, writer):


@jnohlgard I think it would be better to just introduce the feature in this PR and move the refactor to a follow up if necessary. As @felixfontein is indicating you would force this feature to wait until the next major release if it includes breaking changes.

Ajpantuso · 2024-10-30T18:18:02Z

plugins/module_utils/openssh/certificate.py

        if any([self.e is None, self.n is None]):
-            return b''
+            return


Returning early here without throwing an exception or any indication of missing data will result in an invalid fingerprint now instead of b''. So if you really want this refactor I would throw an exception and catch it in public_key_fingerprint to handle the error properly.

Same goes for the other concrete implementations.

felixfontein · 2024-11-11T17:27:46Z

@jnohlgard ping

Add support for SSH certificates using ecdsa-sk or ed25519-sk public …

b28f247

…keys Fixes ansible-collections#796

felixfontein reviewed Oct 30, 2024

View reviewed changes

Ajpantuso reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for SSH certificates using ecdsa-sk or ed25519-sk public keys #813

Add support for SSH certificates using ecdsa-sk or ed25519-sk public keys #813

jnohlgard commented Oct 29, 2024

felixfontein left a comment

felixfontein Oct 30, 2024

felixfontein Oct 30, 2024

Ajpantuso Oct 30, 2024

felixfontein Oct 30, 2024

Ajpantuso Oct 30, 2024

Ajpantuso left a comment

Ajpantuso Oct 30, 2024

Ajpantuso Oct 30, 2024

Ajpantuso Oct 30, 2024

Ajpantuso Oct 30, 2024

felixfontein commented Nov 11, 2024

Add support for SSH certificates using ecdsa-sk or ed25519-sk public keys #813

Are you sure you want to change the base?

Add support for SSH certificates using ecdsa-sk or ed25519-sk public keys #813

Conversation

jnohlgard commented Oct 29, 2024

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Steps to reproduce

sign-key.yml

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

Ajpantuso 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

felixfontein commented Nov 11, 2024

`sign-key.yml`