ProtonMail / go-crypto

Fork of go/x/crypto, providing an up-to-date OpenPGP implementation
https://pkg.go.dev/github.com/ProtonMail/go-crypto
BSD 3-Clause "New" or "Revised" License
334 stars 101 forks source link

support UserAttribute, #126

Closed izouxv closed 2 years ago

izouxv commented 2 years ago

and compatible with gunpg 2.3.6.

    if subpacketLengthLength(int(subLen)) != 5 {
        //!!! for compatible with openpgp 2.2.34 & 2.3.6, the content len less than 16320, but packet conent[0] is 255, the verify sig will failed, key can not import,
        needCompatibleGunPG2_3_6 = true
    }
twiss commented 2 years ago

Thanks, but I still would prefer not to merge this workaround together with the user attribute stuff. Could you remove that and if necessary make a separate PR for that?

Also, could you post an example of a "broken" key from GnuPG 2.2.34 or 2.3.6? And, is there an open bug report with the GnuPG project? Perhaps we can fix it there instead of adding a workaround here?

izouxv commented 2 years ago
func TestUserattribute_CompatibleGunPG2_3_6(t *testing.T) {
    _, err := ReadArmoredKeyRing(bytes.NewBufferString((pgpPhotoPublicOpenPGP2_3_6)))
    if err != nil {
        t.Fatal(err)
    }
}

the test case is a broken key. you can debug it , this key is exported from gunpg 2.3.6 you can use gpg command line to refactory it

twiss commented 2 years ago

OK, thanks. I finally understand the issue - when re-serializing, we use a different length encoding for the User Attribute subpacket than was originally used by GnuPG, right? But actually, that is an issue on our side. The key is not broken, it's free to use any length encoding it wants, and we should verify the signature using the original encoding.

So actually, I think the fix is too narrow, and we shouldn't build a "special case" for GnuPG, in general. So, I would just add a LengthAndContents field to OpaqueSubpacket, and set it unconditionally, and leave Contents as a slice of that field, as it is now, so that we don't store the memory twice. And then, always use the original LengthAndContents when serializing it.

izouxv commented 2 years ago

Ok

izouxv commented 2 years ago

HI I rewrite this feature please take a look thank you

twiss commented 2 years ago

Hey :wave: Sorry for the delay. I think this is still more complicated than it needs to be (storing the original length type and then re-creating that). Why not just store the original length+contents in a slice called LengthAndContents, with Contents being a slice of that (i.e. pointing to the same underlying data without the length)?

izouxv commented 2 years ago

OK, it done

izouxv commented 2 years ago

HI Can you verify my PR, thank you

twiss commented 2 years ago

I realize now, though, after looking at the code, that maybe LengthAndContents is not the best name for what I suggested (as it contains not just the length and contents but also the subpacket type). So maybe Serialized is a better name (also shorter). Feel free to use that instead.

Also, could you still again split up this PR into two, with one being the changes to make it compatible with GnuPG, and the other being to expose the user attributes feature in the entity?

izouxv commented 2 years ago

it's done

twiss commented 2 years ago

Thanks! I'll try to take a look at it next week 😊