Page MenuHome GnuPG
Differential D570

core: Implement adding ADSKs
Needs ReviewPublic
Actions

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

Details

Reviewers
ikloecker
• werner
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
2860

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.Mon, Apr 8, 10:22 AM

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

Revision Contents
Changeset List

PathSize
doc/
9 lines
src/
33 lines
8 lines
1 line
tests/
41 lines

Diff 1550

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