Page MenuHome GnuPG

core: Implement adding ADSKs
ClosedPublic

Authored by TobiasFella on Dec 21 2023, 4:02 PM.

Details

Summary

T6880: GPGME (++/qt): Add support for --quick-add-adsk

  • src/engine-gpg.c: Add and use function for adding ADSKs.
  • src/genkey.c: Prevent error due to no status line.
  • src/gpgme.h.in: Add flag GPGME_CREATE_ADSK --

    This adds the ability to add ADSKs through the gpgme_createsubkey interface. The function must be called with NULL userid, the ADSK fingerprint in algo and the GPGME_CREATE_ADSK flag.
Test Plan

Tested with a basic patch for Kleopatra's key generation command

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

TobiasFella created this revision.

Just a quick first comment: We usually commit core changes and cpp/qt/etc. changes separately. So please split this commit before merging it to master.

Another thing I noticed while browsing the changes: I'm missing a gpgme_op_addadsk_start. In fact, I think gpgme_op_addadsk is actually gpgme_op_addadsk_start and we lack a proper blocking gpgme_op_addadsk which calls _gpgme_wait_one (ctx) after starting the operation.

werner added a subscriber: werner.

I don't think that it is a good idea to have such a specialized API for this task. What we do here is very similar to adding a subkey and as such the APIs should be merged.

This revision now requires changes to proceed.Dec 21 2023, 7:20 PM
TobiasFella retitled this revision from core,qt: Implement adding ADSKs to core: Implement adding ADSKs.
TobiasFella edited the summary of this revision. (Show Details)
TobiasFella edited the test plan for this revision. (Show Details)

Updated as requested. Only contains the core parts; C++/Qt will be uploaded separately

  • Document the new flag/functionality in gpgme.texi
  • Extend run-genkey.c, so that one can easily test this. (This also gives an example how it's used correctly.)
src/engine-gpg.c
2859

Use engine_gpg_t gpg instead of void *engine as gpg_adduid. void *engine should only be used for functions in the engine_ops interface (which receive an opaque engine pointer).

Also, I'd rename algo to adskfpr.

2929

Mention this new mode in the comment preceding this if.

src/genkey.c
449

This looks like a nasty hack. uidmode is documented as "Flag to indicate that a UID is to be added." which is clearly not the case if an ADSK is added. Is this done because of the hack in genkey_status_handler? This sets the uid flag in the result which means "A user id was generated." which is also not the case.

Maybe you need to add a new flag to _gpgme_op_genkey_result. Or maybe we don't really need a result other than success/failure.

This revision now requires changes to proceed.Jan 4 2024, 10:44 AM
src/genkey.c
449

This is just for the hack in genkey_status_handler; without it, we always get a GPG_ERR_GENERAL. We don't care about the flag in _gpgme_op_genkey_result; I didn't notice that at first.

There seems to be a problem with error reporting; errors from GPG (e.g., invalid password) are not coming through. I think this caused by GnuPG not reporting them correctly

src/genkey.c
449

Updated to use a separate flag (which does basically the same thing).

Looks good except for a nitpick and some documentation.

Regarding the error reporting: Check to status messages issued by gpg with gpg --status-fd 2 ‐‐quick‐add‐adsk fpr adskfpr. Sometimes printing a status message is missing in gpg if it wasn't needed until now.

doc/gpgme.texi
4427

The flag should also be documented together with all other GPGME_CREATE_* flags around line 4300.

src/engine-gpg.c
2861

This line is superfluous. Just name the function argument gpg. See the previous function.

@werner Please review the changes again. I think Tobias addressed your comments.

Looks good to me. Thanks.

This revision is now accepted and ready to land.May 13 2024, 12:18 PM