Page MenuHome GnuPG
Differential D570

core: Implement adding ADSKs
ClosedPublic
Actions

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

Details

Reviewers
• ikloecker
• werner
Maniphest Tasks
T6880: GPGME (++/qt): Add support for --quick-add-adsk
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 requested review of this revision.Dec 21 2023, 4:02 PM
• TobiasFella created this revision.
• ikloecker added a comment.Dec 21 2023, 5:35 PM

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 requested changes to this revision.Dec 21 2023, 7:20 PM
• 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 updated this revision to Diff 1547.Jan 3 2024, 2:25 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

• ikloecker requested changes to this revision.Jan 4 2024, 10:44 AM
  • 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
• TobiasFella added inline comments.Jan 4 2024, 11:41 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.

• TobiasFella updated this revision to Diff 1548.Jan 4 2024, 3:48 PM
• TobiasFella updated this revision to Diff 1549.Jan 4 2024, 4:36 PM
• TobiasFella added a comment.Jan 4 2024, 4:38 PM

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).

• ikloecker added a comment.Jan 5 2024, 10:54 AM

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.

• TobiasFella updated this revision to Diff 1550.Jan 5 2024, 11:50 AM
• ikloecker accepted this revision.Jan 8 2024, 11:13 AM

Looks good.

• TobiasFella mentioned this in D572: cpp,qt: Implement adding ADSKs.Jan 8 2024, 11:39 AM
• ikloecker added a comment.Apr 8 2024, 10:22 AM

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

• werner accepted this revision.May 13 2024, 12:18 PM

Looks good to me. Thanks.

This revision is now accepted and ready to land.May 13 2024, 12:18 PM
• TobiasFella closed this revision.May 22 2024, 10:30 AM
• ikloecker added a task: T6880: GPGME (++/qt): Add support for --quick-add-adsk.Jun 7 2024, 2:43 PM

Revision Contents
Changeset List

PathSize
doc/
4 lines
src/
34 lines
8 lines
1 line
tests/
41 lines

Diff 1549

View Options

doc/gpgme.texi

Loading...
View Options

src/engine-gpg.c

Loading...
View Options

src/genkey.c

Loading...
View Options

src/gpgme.h.in

Loading...
View Options

tests/run-genkey.c

Loading...