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

Comments of keys not shown #348

Closed
Hackerpcs opened this issue Jul 18, 2022 · 23 comments
Closed

Comments of keys not shown #348

Hackerpcs opened this issue Jul 18, 2022 · 23 comments

Comments

@Hackerpcs
Copy link

Hackerpcs commented Jul 18, 2022

As described here
#341 (comment)

until this build
https://github.com/dlech/KeeAgent/actions/runs/2290793867
I can see the comments of the keys

previous
previous2

on the latest v0.13.1 I can't

latest
latest2

@dlech
Copy link
Owner

dlech commented Jul 18, 2022

Which private key file format are you using?

@Hackerpcs
Copy link
Author

-----BEGIN OPENSSH PRIVATE KEY-----
foobarbaz
-----END OPENSSH PRIVATE KEY-----

@dlech
Copy link
Owner

dlech commented Jul 18, 2022

KeeAgent currently uses only the public key comment. For this file format, the comment is encrypted, so it is only available after the private key has been decrypted. As a workaround, you might be able to add a .pub file containing the public key and the comment as an attachment so that KeeAgent will pick up the comment.

We could also consider adding a Comment property to SshPrivateKey (to be populated here) for private key comments and use this as a fallback when a public key comment is not provided.

@Hackerpcs
Copy link
Author

My keys aren't encrypted, they are generated with this command without entering a passphrase

ssh-keygen -t rsa -b 4096 -C "comment" -f "key_rsa4096"

It worked till that version (which I currently use) though, couldn't it be done in the stable also like it was there?

@dlech
Copy link
Owner

dlech commented Jul 19, 2022

Most people seem to use encrypted keys and decrypting the key even when it is not being used caused performance problems for newer keys formats with CPU intensive key derivation functions. Fixing that problem has lead to this problem.

We could add a hack that to the effect of "if the key is not encrypted, use the comment", but that would only work for people with unencrypted keys. Attaching a separate public key file that includes the comment is a more universal workaround.

@egfx-notifications
Copy link
Contributor

I just updated from KeeAgent 0.12.1 to 0.13.1 and can confirm @Hackerpcs findings

I have both private and public keys in KeePass and configured for KeeAgent usage. Starting with 0.13.1 I no longer see the comment

  • in the key selection screen I use when an identities list is prompted (Comment column is empty)
  • in the confirmation prompt for key usage (it only displays key '', this is especially frustrating, as now I only have the fingerprint to check which key is about to be used)
  • in the key usage windows notification (it only displays key '')

If I go back to KeeAgent 0.12.1 it works again.

@Hackerpcs
Copy link
Author

Personally I use this build
https://github.com/dlech/KeeAgent/actions/runs/2290793867
that both works (regarding issue #341, Windows 10 21h2) and has comments.

@egfx-notifications
Copy link
Contributor

Didn't manage to do a git bisect yet, but looking at the commits since 0.12.1 I consider this the most likely subject
1227354

@egfx-notifications
Copy link
Contributor

Personally I use this build https://github.com/dlech/KeeAgent/actions/runs/2290793867 that both works (regarding issue #341, Windows 10 21h2) and has comments.

That would be two commits before the commit I suspect, so that sounds legit

@egfx-notifications
Copy link
Contributor

KeeAgent currently uses only the public key comment. For this file format, the comment is encrypted, so it is only available after the private key has been decrypted. As a workaround, you might be able to add a .pub file containing the public key and the comment as an attachment so that KeeAgent will pick up the comment.

We could also consider adding a Comment property to SshPrivateKey (to be populated here) for private key comments and use this as a fallback when a public key comment is not provided.

@dlech Looking at the diff between 0.12.1 and 0.13.1 I think I understand what you meant with the encrypted comment. Unfortunately the public key attachment I also have in place is not helping because the code only ever checks for a public key attachment if the private key doesn't have a public key included. So my readable public key attachment is ignored, because a public key is already present in the private key, but I can't access the comment because it is encrypted.

@mplattner
Copy link

mplattner commented Aug 5, 2022

I have the same problem: since the update to v0.13.1.0 my keys' comments are not displayed anymore, beside for one old and used rsa1024 key. All keys (including the old one) are encrypted.

@attisan
Copy link

attisan commented Aug 18, 2022

I have the same problem: since the update to v0.13.1.0 my keys' comments are not displayed anymore, beside for one old and used rsa1024 key. All keys (including the old one) are encrypted.

same here. in my case public key comments for ssh-ed25519 aren't showing up anymore

@KuttKatrea
Copy link
Contributor

What about a setting to use the entry title as the comment? Is that feasible @dlech?

Wouldn't require a public key nor decrypting the private key

@dlech
Copy link
Owner

dlech commented Aug 24, 2022

That sounds like a good idea for a fallback.

@KuttKatrea
Copy link
Contributor

That sounds like a good idea for a fallback.

I created a PR:
#355

@attisan
Copy link

attisan commented Aug 24, 2022

I wonder if this (perhaps optionally) shouldn't be the default as it is more accessible / editable.

@mplattner
Copy link

The comment of my key (ed25519) is still not shown in v0.13.2. The comment field is just empty. It does work correctly with v.0.12.1.

@dlech
Copy link
Owner

dlech commented Sep 26, 2022

This issue was closed since the workaround of including a .pub file is fixed. The breaking change of encrypted comments not working as before for some keys is an unfortunate side effect of performance improvements, so can't technically be fixed without causing performance issues again.

We should probably start a new issue for the suggestion in #348 (comment) (with relevant discussion in #355) as an alternate solution if using .pub files is not sufficient.

@mplattner
Copy link

As @egfx-notifications mentioned above, using v0.13.2, adding a .pub file does not fix this for me - the comment is still empty.

The pub file contains ssh-ed25519 AAAA...V mycomment.
@egfx-notifications said its due to the public key being included in the private key file. I found no way to remove the public key from the private key file.

@egfx-notifications
Copy link
Contributor

The fix provided in 90a0ce2 is incomplete.
The Read() method in SshAgentLib only uses the provided public key if a public key is not included in the private key, so WithPublicKey() method needs to be used instead.

I can work on a PR later today.

egfx-notifications added a commit to egfx-notifications/KeeAgent that referenced this issue Oct 4, 2022
@jaxFF
Copy link

jaxFF commented Oct 15, 2022

Hi.

I'm still facing the same issue on the most recent KeeAgent build (0.13.2.0) with my ed25519 keypair. My pub key looks as follows, I believe there may be an issue with parsing spaces in the comment or something.

ssh-ed25519 AA....zq user@desktop commentpt2 anotherpartof comment 20221015

I'm not familiar with the codebase and couldn't easily push a PR. If someone was willing to create a PR that would allow the plugin to fall back to using the title AND OR comment for the password entry, that would be wonderful, as I use the same comment for my entry as the one stored in my pub key.

I think it would be more accessible to users if you could specify to use the password entry's name OR comment, for the key info comment.

This is how the key with the comment is appearing in KeeAgent and my ssh-agent.
(screenshots redacted)

The most recent PR regarding this issue does not fix it. I'll probably revert to 0.12.1 for now as previously suggested.

@dlech
Copy link
Owner

dlech commented Oct 15, 2022

Since this issue has been closed, can you please start a new issue and attach an example key that reproduces the issue?

@jaxFF
Copy link

jaxFF commented Oct 15, 2022

Since this issue has been closed, can you please start a new issue and attach an example key that reproduces the issue?

Sure thing, please view issue #369. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants