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

v3: Rework Armor checksum handling #284

Merged
merged 4 commits into from
Jun 14, 2024
Merged

Conversation

lubux
Copy link
Contributor

@lubux lubux commented Jun 11, 2024

GopenPGP v3 did not produce any armor checksum as recommended by the crypto refresh. Unfortunately, a popular OpenPGP library fails to parse armored messages without a checksum in certain scenarios.

This MR adds armor checksums back per default, but tries to avoid them when generating crypto refresh messages. i.e., generated by v6 keys.

In GopenPGP v3, armor checksums were not produced, following recommendations from the OpenPGP crypto refresh. Unfortunately, a widely used OpenPGP library fails to parse armored messages that lack a checksum in certain scenarios although they should be optional according to the official RFC.

This merge request (MR) reinstates armor checksums by default to ensure compatibility. However, it tries to omit the checksums when generating messages for the crypto-refresh, i.e., those generated with v6 keys.

Changes:

  • API to armor data with the option to remove the checksum
  • All armor functions append a checksum per default for compatibility with certain libraries although the crypto-refresh advises not to.
  • Encryption and Sign handle now append a checksum when armoring. If the produced OpenPGP packets are crypto-refresh packets, the checksum is not appended as mandated by the crypto-refresh.

@lubux lubux added the v3 Targeting GopenPGP v3 label Jun 11, 2024
@lubux lubux requested a review from twiss June 12, 2024 10:53
armor/armor.go Outdated Show resolved Hide resolved
constants/armor.go Outdated Show resolved Hide resolved
armor/armor.go Outdated Show resolved Hide resolved
crypto/encryption_handle.go Outdated Show resolved Hide resolved
crypto/encryption_handle.go Outdated Show resolved Hide resolved
crypto/key.go Outdated Show resolved Hide resolved
crypto/message.go Outdated Show resolved Hide resolved
crypto/sign_handle.go Outdated Show resolved Hide resolved
@lubux lubux force-pushed the feat/adapt-armor-checksum-handling branch 2 times, most recently from 725102d to 5d943a6 Compare June 13, 2024 14:07
@lubux
Copy link
Contributor Author

lubux commented Jun 13, 2024

Thanks for the review @twiss . I addressed the comments.

@lubux lubux force-pushed the feat/adapt-armor-checksum-handling branch from 5d943a6 to 9a7bc77 Compare June 13, 2024 14:13
constants/armor.go Outdated Show resolved Hide resolved
@lubux lubux merged commit 5720941 into v3 Jun 14, 2024
@lubux lubux deleted the feat/adapt-armor-checksum-handling branch June 14, 2024 08:41
lubux added a commit that referenced this pull request Aug 14, 2024
* feat(armor): Armor checksum handling according to the crypto refresh

* docs(readme): Add info about v2 support

* refactor(armor): Improve checksum naming

* refactor(armor): Rename global checksum setting variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 Targeting GopenPGP v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants