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

Fix SSH certificate fingerprint encoding #207

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

hslatman
Copy link
Member

@hslatman hslatman commented Mar 28, 2023

Before the change the fingerprint for an SSH certificate would
include the bytes that encode the certificate. An SSH fingerprint
should only be based on its public key.

This commit checks if the SSH key is actually an SSH certificate
and will fingerprint just the SSH certificate public key instead
of the entire certificate contents.

Before this commit, the fingerprint for an SSH certificate would
include the bytes that encode the certificate. An SSH fingerprint
should only be based on its public key.

This commit checks if the SSH key is actually an SSH certificate
and will fingerprint just the SSH certificate public key instead
of the entire certificate contents.
@hslatman
Copy link
Member Author

hslatman commented Mar 28, 2023

Example cert:

$ step ssh inspect example-cert.pub
example-cert.pub:
        Type: [email protected] user certificate
        Public key: ECDSA-CERT SHA256:RvkDPGwl/G9d7LUFm1kmWhvOD9I/moPq4yxcb0STwr0
        Signing CA: ECDSA SHA256:AXEctpST7/1MfakrLrE+xrtF8Eixh6YsmqNaxiN6AFI (using ecdsa-sha2-nistp256)
        Key ID: "herman"
        Serial: 14376510....

Fingerprint before patching:

$ step ssh fingerprint example-cert.pub
256 SHA256:YuV7pyvW7Jp4iEwddOsMw+EdugrhKGqOKh6o+PP0xeg herman (ECDSA-CERT)

After patching:

$ step ssh fingerprint example-cert.pub
256 SHA256:RvkDPGwl/G9d7LUFm1kmWhvOD9I/moPq4yxcb0STwr0 herman (ECDSA-CERT)

Output for SSH agent:

$ ssh-add -l
256 SHA256:RvkDPGwl/G9d7LUFm1kmWhvOD9I/moPq4yxcb0STwr0 herman (ECDSA)
256 SHA256:RvkDPGwl/G9d7LUFm1kmWhvOD9I/moPq4yxcb0STwr0 herman (ECDSA-CERT)

@hslatman hslatman requested a review from maraino March 28, 2023 21:13
@hslatman hslatman enabled auto-merge March 28, 2023 21:14
Copy link
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

I'm not sure about this.

For X.509 certificates we differentiate the fingerprint of a certificate than the fingerprint of the key. And getting the fingerprint of the key is as easy as inspecting the certificate.

SSH does shows the fingerprint of the key on both ssh-add -l or ssh-keygen -l -f foo-cert.pub.

@hslatman
Copy link
Member Author

hslatman commented Mar 28, 2023

My patched logic returns the same result as ssh-add -l when doing step ssh list. It confused @andysmallstep that step ssh list returned something different than ssh-add -l (while ssh-add -L and step ssh list --raw did/do have the same output), which prompted me to fix this.

I could change it to only do this logic for step ssh list, but I think that'll be confusing in the end, because you would get different fingerprints for different use cases / commands.

The output of step ssh fingerprint could be used when someone wants to configure a server with a trusted fingerprint. If it isn't the same as the one created/shown by OpenSSH, that may lead to confusion too.

I'm aware there's different logic for fingerprinting public keys vs. certificates for X.509, but I don't think that outweighs the fact that other tools seem to generate the fingerprint always for the public key from an SSH certificate, instead of over the entire certificate contents.

If there's a use for a fingerprint over the entire certificate, I think that can be done with an additional flag. In my opinion it shouldn't be the default for the reasons mentioned above.

@maraino
Copy link
Contributor

maraino commented Mar 30, 2023

@hslatman, Are you going to create a new method to get the fingerprint for the certificate instead of the key as part of this PR?

And yes, there are uses for looking for a certificate fingerprint. It is the only way to differentiate two certificates that share the same key. So that it can be used for indexing.

@hslatman
Copy link
Member Author

Yes, will add it to this PR. Will be a new function, so that we don't need to change the signature of the current one, as you suggested.

What do you think would be a good name for the flag (which will go in a PR for the CLI)? Something like --full, --complete, --digest or --contents? Or maybe just --certificate? The latter one would work better for step ssh fingerprint; less so for step ssh list. I like --contents and --digest best, I think. We could also add a flag --alg to change the algorithm used.

@maraino
Copy link
Contributor

maraino commented Mar 30, 2023

Probably --certificate and just for step ssh fingerprint. There's no need to add --alg, but if we do we probably want to add that to step crypto key fingerprint too.

The `FormatCertificateFingerprint` allows fingerprints for SSH
certificates to be calculated. As opposed to the `FormatFingerprint`
function, the new function will always include all certificate
bytes as opposed to just the marshaled public key when calculating
the fingerprint.

The default behavior for `FormatFingerprint` now behaves like other
systems, such as `ssh-add -l`.
@hslatman
Copy link
Member Author

@maraino I added a new function for fingerprinting certificates: a917ed9. I think it's good to be strict and to check the type, so that an error can be returned when one's trying to fingerprint a certificate, but operating on a key.

@hslatman hslatman requested a review from maraino April 11, 2023 14:11
@hslatman hslatman merged commit c06930f into master Apr 11, 2023
@hslatman hslatman deleted the herman/fix-ssh-certificate-fingerprint branch April 11, 2023 22:30
hslatman added a commit to smallstep/cli that referenced this pull request Apr 12, 2023
With the changes from smallstep/crypto#207,
the default behavior of `step ssh fingerprint` changes to be like
the behavior of `ssh-add` (and similar tools). When a fingerprint
is determined for an SSH certificate, the fingerprint will only
include the bytes of the public key. With the `--certificate` flag,
a user can create a fingerprint for the entire SSH certificate
contents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants