Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

[BUG] PGP key with multiple identities fails to import #1735

Closed
msfjarvis opened this issue Feb 17, 2022 · 5 comments · Fixed by #1741
Closed

[BUG] PGP key with multiple identities fails to import #1735

msfjarvis opened this issue Feb 17, 2022 · 5 comments · Fixed by #1741
Labels
A-PGPainless Area: PGPainless-backed PGP C-bug Category: This is a bug E-medium Effort: This requires a fair amount of work P-high Priority: high, must be resolved before next major release
Milestone

Comments

@msfjarvis
Copy link
Member

Describe the bug

A GPG key with more than one identity fails to import with the PGPainless backend

Steps to reproduce

  1. Get test keys from https://msfjarvis.dev/apsg/pass-test/406de9b7f0fc
  2. Attempt to import private key with PGPainless backend

Expected behavior

Key imports successfully

Screenshots

N/A

Device information

  • Device: Pixel 4a
  • OS: Android 12
  • App version: 3939003

Additional context

No response

@msfjarvis msfjarvis added C-bug Category: This is a bug E-medium Effort: This requires a fair amount of work P-high Priority: high, must be resolved before next major release A-PGPainless Area: PGPainless-backed PGP labels Feb 17, 2022
@msfjarvis msfjarvis added this to the v2.0.0 milestone Feb 17, 2022
@vanitasvitae
Copy link

Let me know if I can help you out with anything :)

@msfjarvis
Copy link
Member Author

Let me know if I can help you out with anything :)

PGPainless seems to be doing its job correctly, the failure is in the formatting code here which appears to overflow a Long. Still investigating why that happens.

@moppman
Copy link
Contributor

moppman commented Feb 21, 2022

Continued from here:
I deleted the second identity from the key, but I'm still getting the same error on import, which was surprising to me at first, but then I read that removed identities aren't actually "deleted" but only revoked via special signature.
So the overflow bug would still make sense here. 😵‍💫

@vanitasvitae
Copy link

vanitasvitae commented Feb 21, 2022

I'm still a bit confused... Why does an additional user-id have an impact on key-id calculation? 🤔

Edit: Looking at the code, why are you hex-encoding the key-id here, and then immediately decoding it again to calculate the key-id? Shouldn't just passing keyRing.publicKey.keyID to the KeyId constructor be sufficient?

Still, there might be a bug in the encode/decode function?

I'm not fluent in Kotlin, so maybe I'm missing something?

@msfjarvis
Copy link
Member Author

Edit: Looking at the code, why are you hex-encoding the key-id here, and then immediately decoding it again to calculate the key-id? Shouldn't just passing keyRing.publicKey.keyID to the KeyId constructor be sufficient?

goddamnit 🤦🏼

I wish I could tell what was going on in my head when I wrote that. 1956b66 fixes the failing test for me, once #1741 lands and I get snapshots rolling again this should be fixed.

@msfjarvis msfjarvis linked a pull request Feb 21, 2022 that will close this issue
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-PGPainless Area: PGPainless-backed PGP C-bug Category: This is a bug E-medium Effort: This requires a fair amount of work P-high Priority: high, must be resolved before next major release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants