Leave unhashed subpackets as-is when re-serializing signatures

[wussler]

When an embedded signature is already present in the hashed part it will be serialized twice when exporting an already signed public key.

[twiss]

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 writeUnhashedSubPackets function into createUnhashedSubPackets and writeUnhashedSubPackets, and in the latter just write them out, and then in sign set this.unhashedSubpackets = this.createUnhashedSubPackets(). You'd also have to change the calls to writeSubPacket and only write the type and body instead.

[wussler]

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.

[twiss]

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.

Suggested change

	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;
	}

[wussler]

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.

[twiss]

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

[wussler]

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.