Leave unhashed subpackets as-is when re-serializing signatures#1561
Leave unhashed subpackets as-is when re-serializing signatures#1561twiss merged 3 commits intoopenpgpjs:mainfrom
Conversation
wussler
force-pushed
the
fix-serialization-embedded-sig
branch
from
17dd14b to
4fadec5
Compare
|
Hey 👋 Thanks for the PR, but this fix is a bit too specific / hacky, in my opinion. Instead, we should create the unhashed subpackets during signing, and just serialize them as they were during serialization. To achieve that, you could split the |
|
Sure, I'll split the packet serialization. The original intention was to keep the PR minimal, but I understand that it looks too specific indeed. |
wussler
force-pushed
the
fix-serialization-embedded-sig
branch
3 times, most recently
from
320a496 to
13873c4
Compare
wussler
force-pushed
the
fix-serialization-embedded-sig
branch
from
13873c4 to
ab07ffd
Compare
| if (!allowedUnhashedSubpackets.has(type)) { | ||
| this.unhashedSubpackets.push(bytes.subarray(mypos, bytes.length)); | ||
| return; | ||
| } | ||
| this.allowedUnhashedSubpackets.push(bytes.subarray(mypos, bytes.length)); |
There was a problem hiding this comment.
Tbh, I would just change the condition and store all of them in unhashedSubpackets. There's nothing saying that those are only the disallowed ones, it's just that for the others there was no reason (until now) to store them there. It's technically a small breaking change, but imo we change it in a minor release.
| if (!allowedUnhashedSubpackets.has(type)) { | |
| this.unhashedSubpackets.push(bytes.subarray(mypos, bytes.length)); | |
| return; | |
| } | |
| this.allowedUnhashedSubpackets.push(bytes.subarray(mypos, bytes.length)); | |
| this.unhashedSubpackets.push(bytes.subarray(mypos, bytes.length)); | |
| if (!allowedUnhashedSubpackets.has(type)) { | |
| return; | |
| } |
There was a problem hiding this comment.
I tried this approach, but it doesn't work, because when we overwrite the allowedUnhashedSubpackets we do not intend to rewrite all the unhashedSubpackets, but just the one the implementation understands (i.e. the allowed ones).
In other words, there might be some packet in the unhashed section that we do not want to overwrite when re-signing, therefore we need to store them separately or flag them as "to be overwritten" or "not to be overwritten" on re-sign.
There was a problem hiding this comment.
OK, but.. do we rely on preserving unhashed subpackets when re-signing somewhere?
Re-signing in general isn't really something that's often done in OpenPGP.js; we leave the signatures alone by default (unlike go-crypto).
There was a problem hiding this comment.
Yes, we had dozens of tests failing by not preserving the unhashed subpackets. I didn't inspect most of them, since I'm not very familiar with the codebase, but it seemed to be a pretty rooted assumption.
twiss
changed the title
2 participants
When an embedded signature is already present in the hashed part it will be serialized twice when exporting an already signed public key.