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 Agent broken decrypt button #10638
base: develop
Are you sure you want to change the base?
Fix SSH Agent broken decrypt button #10638
Conversation
The way the key is handled makes this rather confusing. Comment, encrypted, publickey, etc. We shouldn't have to check so many conditions to display simple data. How can we improve this overall? @hifi |
c1701e9
to
cd9077e
Compare
The presentation of data in Keepassxc is identical to that of openSSH tools and is closely related to the SSH private key file format. Naturally, if the private key is password-free, all information is directly visible. Currently, the only way to simplify the display of SSH key information in Keepassxc would be to decrypt the keys when opening the entry. However, this presents a major drawback:
This means that an entry containing an SSH key encoded with a high number of KDF rounds would take several seconds to open. Simplifying the display of SSH keys in Keepassxc would therefore negatively impact the overall responsiveness of the application. @hifi had already noted this issue here: Changing the behavior and display of information is not just a bug fix. This PR is simply aimed at fixing the decrypt button. :) |
That is not what I was saying. The underlying data structure that contains the keys has a confusing API. |
Do you want me to look into refactoring this part with a "case switch" , for example ? |
No. For example, all these checks for comment existing and decrypted and such... those should be done within the key.comment() function which would return the appropriate response given the conditions of the key. Doing all those checks at the UI layer is confusing and non-repeatable in case we want to implement the same behavior elsewhere. You should simply have to do the Could also be done in a new function that makes it obvious we are preparing data for display: key.resolveComment() or key.displayComment() |
Ok, I'll see if I can do that within a reasonable amount of time. |
I removed the key checks from the UI layer without creating new functions. Your idea of putting this in the If we do that, we might as well go all the way and move this part of if (!key.parsePKCS1PEM(privateKeyData)) {
m_error = key.errorString();
return false;
}
if (key.encrypted() && (decrypt || key.publicKey().isEmpty())) {
if (!key.openKey(password)) {
m_error = key.errorString();
return false;
}
}
if (key.comment().isEmpty()) {
key.setComment(username);
}
if (key.comment().isEmpty()) {
key.setComment(fileName);
} But that's no longer a fix..." |
Yah I like these changes now, no need to go further at this point. Thank you! |
Thank you for your help. |
Doesn't need to hold up this PR, translations are independent of pull requests. However you do need to |
8b171cc
to
82b479b
Compare
Format done ! |
43a227a
to
db7c6da
Compare
db7c6da
to
0bbce15
Compare
The reason is that the activation condition is:
if (!key.comment().isEmpty() || !key.encrypted())
.But the comment is never empty because when opening the key we have:
I also took the opportunity to modify the text to be copied to the clipboard when the key is not decrypted.
Indeed, the undecrypted private key exposes the public key in clear text, but not its comment.
setPlainText(tr("(encrypted)"));
to
setPlainText(key.publicKey() + tr("(comment is encrypted)"));
Testing strategy
Tested manually with ed25519 crypted key.
Type of change