Implement rnp_signature_get_features and rnp_key_set_features.

[kaie]

See issue #2178

[kaie]

FYI, I have written this code on top of the v0.17.0 release, then merged to the main branch.

[codecov]

Codecov Report

Attention: 90 lines in your changes are missing coverage. Please review.

Comparison is base (2cf8fd2) 77.07% compared to head (ecf055e) 77.33%.

Files	Patch %	Lines
src/lib/pgp-key.cpp	0.00%	57 Missing ⚠️
src/lib/rnp.cpp	0.00%	30 Missing ⚠️
src/librepgp/stream-sig.cpp	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2179      +/-   ##
==========================================
+ Coverage   77.07%   77.33%   +0.26%     
==========================================
  Files         194      176      -18     
  Lines       37257    34724    -2533     
==========================================
- Hits        28715    26854    -1861     
+ Misses       8542     7870     -672     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ni4]

@kaie
While PR overall looks good (except missing tests coverage), my main concern is that proposed FFI API which overwrites signature interferes with already planned one (which would create new signature and update it instead of overwriting). Since our goal is to make API as much permanent as possible this may cause problems in the future. Again, I would prefe to get away from already coded practice to overwrite signature and to use more flexible 'create-copy-if-needed-update-delete old if needed' approach.

rnp_signature_get_features() looks good, maybe just change to uin32_t and add some defines with constants for possible features.

[kaie]

Thanks Nickolay for your feedback.

If you prefer that we avoid the SET function, then I'm ok to do the following approach:

• in RNP, implement the GET function
• ideally, release a v0.17.1 that includes the new API, but if necessary, Thunderbird can locally add the upstream patch
• in Thunderbird, refuse to import keys that have certain feature flags set

[kaie]

I assume it's cleaner to close this PR, and open a fresh PR with only the GET code.

[ni4]

Thanks Nickolay for your feedback.

If you prefer that we avoid the SET function, then I'm ok to do the following approach:

* in RNP, implement the GET function

* ideally, release a v0.17.1 that includes the new API, but if necessary, Thunderbird can locally add the upstream patch

We plan to release v0.18 in a month or two, so maybe patch approach would be simpler.

* in Thunderbird, refuse to import keys that have certain feature flags set

This would cause problem for all the users who recently generated keys with GnuPG and attempt to import them, and it would be even worse then SHA1 issue few years, given that many want to have single secret key for Thunderbird and for GnuPG/other applications.

Btw, just realized that it is already possible to retrieve features via the packet dump of exported signature. However, that would require some coding and parsing JSON like the following one:

  {
    "header":{
      "offset":76,
      "tag":2,
      "tag.str":"Signature",
      "raw":"8899",
      "length":153,
      "partial":false,
      "indeterminate":false
    },
    "version":4,
    "type":19,
    "type.str":"Positive User ID certification",
    "algorithm":22,
    "algorithm.str":"EdDSA",
    "hash algorithm":10,
    "hash algorithm.str":"SHA512",
    "subpackets":[
      {
        "type":33,
        "type.str":"issuer fingerprint",
        "length":21,
        "hashed":true,
        "critical":false,
        "fingerprint":"bebafb4b7760c81f9f4f2c5b9b7b86b3dcf73c3c"
      },
      {
        "type":2,
        "type.str":"signature creation time",
        "length":4,
        "hashed":true,
        "critical":false,
        "creation time":1707233645
      },
      {
        "type":27,
        "type.str":"key flags",
        "length":1,
        "hashed":true,
        "critical":false,
        "flags":3,
        "flags.str":[
          "certify",
          "sign"
        ]
      },
      {
        "type":9,
        "type.str":"key expiration time",
        "length":4,
        "hashed":true,
        "critical":false,
        "key expiration":94608000
      },
      {
        "type":11,
        "type.str":"preferred symmetric algorithms",
        "length":4,
        "hashed":true,
        "critical":false,
        "algorithms":[
          9,
          8,
          7,
          2
        ],
        "algorithms.str":[
          "AES-256",
          "AES-192",
          "AES-128",
          "TripleDES"
        ]
      },
      {
        "type":34,
        "type.str":"preferred AEAD algorithms",
        "length":1,
        "hashed":true,
        "critical":false,
        "algorithms":[
          2
        ],
        "algorithms.str":[
          "OCB"
        ]
      },
      {
        "type":21,
        "type.str":"preferred hash algorithms",
        "length":5,
        "hashed":true,
        "critical":false,
        "algorithms":[
          10,
          9,
          8,
          11,
          2
        ],
        "algorithms.str":[
          "SHA512",
          "SHA384",
          "SHA256",
          "SHA224",
          "SHA1"
        ]
      },
      {
        "type":22,
        "type.str":"preferred compression algorithms",
        "length":3,
        "hashed":true,
        "critical":false,
        "algorithms":[
          2,
          3,
          1
        ],
        "algorithms.str":[
          "ZLib",
          "BZip2",
          "ZIP"
        ]
      },
      {
        "type":30,
        "type.str":"features",
        "length":1,
        "hashed":true,
        "critical":false,
        "mdc":true,
        "aead":true,
        "v5 keys":true
      },
      {
        "type":23,
        "type.str":"key server preferences",
        "length":1,
        "hashed":true,
        "critical":false,
        "no-modify":true
      },
      {
        "type":16,
        "type.str":"issuer key ID",
        "length":8,
        "hashed":false,
        "critical":false,
        "issuer keyid":"9b7b86b3dcf73c3c"
      }
    ],
    "lbits":"e464",
    "material":{
      "r.bits":252,
      "s.bits":256
    }
  },

[kaie]

If I cannot fix the key on import, I must refuse the import.

It would be noted in TB release notes that it's no longer possible to import such keys into Thunderbird.

Users who want to share their key with Thunderbird will need to use gpg commands to edit their key to make it compatible, prior to exporting from GnuPG and importing it into Thunderbird.

[kaie]

good idea to use the packet dump, but it isn't very elegant, because I would have to look for unknown string keys in the output, in addition to aead/mdc/v5, to be complete.

Checking for additional bits in an integer is simpler.

[kaie]

New read-only pull request is #2188

[kaie]

In my opinion, the rejection to import keys exported from GnuPG 2.4 with --rfc480bis enabled, would be less serious than the past SHA1 you mentioned.

In that scenario, users had already an imported keys, which suddently no longer worked. Which means, we broke their existing Thunderbird setup.

In this new scenario, the suggested approach would only reject import, so it would become clear something is incompatible based on a nonworking import.

I think I'd accept the fact that such a key has already been imported, and continue to use it - until we're able to automatically fix it in the future.

[ni4]

Good point about the already imported keys vs new keys, didn't think this way. Maybe I'm too stick to idea 'don't touch something which worked for a while and didn't give any complaints'.