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

[sailfish-secrets] Properly detect the key ID in case of a subkey. #189

Merged
merged 2 commits into from Sep 20, 2023

Conversation

dcaliste
Copy link
Contributor

Also don't bother the user with a second demand to access the cache passphrase if the first attempt was dismissed.

@pvuorela , if you have time to give a look. Thank you.


// Ugly hack here due to GnuPG being stuck in 2.0.4
// and not having SETKEYINFO command yet. Description may contain "ID xxxxx".
{
const QString &str(self->prompt.message());
int id = str.lastIndexOf("ID ");
int id = str.lastIndexOf(", ID ");
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the content like here actually? Wondering should the comment about "ID ..." be adjusted too to include the comma part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good remark. I've updated the comment with an example of description that can be parsed to extract the key ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose the change here fixes what is says, but for another thing if the content is like above, why is this using lastIndexOf() instead of indexOf()? Quick google on such strings shows instances with email address in the end which theoretically at least might involve the id?

If fixes an issue this could be fine already.

Copy link
Contributor Author

@dcaliste dcaliste Sep 20, 2023

Choose a reason for hiding this comment

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

The problem I got was exactly this one: with the localisation of the description by GnuPG, I ended up parsing something like clé secrète pour l'utilisateur: « Damien Caliste <email> » clé de 2048 bits RSA, ID A099539B, créée le 2018-04-13 (ID clé principale 41123AA4). Which gives, with the code in master branch, an ID like "clé prin". And I saw this string in the UI, wondering what happened.

Indeed, my first intention in using lastIndexOf() was to avoid a possible match of "ID " in the first line containing the name and the email. But then, I didn't though about subkeys... Thus the change to parse the comma with it.

I agree this is very fragile and ugly. GnuPG in a later version is providing the key ID by itself and we don't need to parse a changing string. But this later version is GPLv3, you know.

In case of a subkey, the hack code to detect
the key ID in the pinentry has to be modified
since the description contains both the main
key and the sub key.
In the pinentry, when asking for a passphrase,
the system is asking to access the GnuPG secrets
to read the passphrase from cache if any.

When this confirmation is dismissed by the user
the pinentry should not ask again to access the
secret to cache the passphrase.
@dcaliste
Copy link
Contributor Author

I've added the first line of the description in the comment, so it may justify the use of lastIndexOf().

Copy link
Contributor

@pvuorela pvuorela left a comment

Choose a reason for hiding this comment

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

Ok, fair enough. Thanks.

@pvuorela pvuorela merged commit b19e382 into sailfishos:master Sep 20, 2023
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 this pull request may close these issues.

None yet

2 participants