Add Notation data support in subpackets

[wussler]

see #27

[YarmoM]

Dear maintainers, will this get reviewed?

[wiktor-k]

Hi folks! 👋

Some people brought it to my attention that notation support is critical to their use case, e.g. writing Keyoxide-compatible tools (Keybase-like decentralized bidirectional proof systems) in Go.

If conflicts are the only thing preventing the merge I know some people even have fixed that in their forks (@omz13) and could raise a fix-up PR if that's the only concern.

Would that be an acceptable resolution? Thanks for your time! CC: @twiss, @wussler.

(I'll use this occasion to plug an ad that Sequoia PGP has a nice and full support for notations! 😅 haha)

[wussler]

Hi @wiktor-k, thank you for your interest.

We've seen there is quite some push for this feature, and will take care of merging this PR!

[wiktor-k]

Thank you, very, very much! 🙇 Have a nice day!

[twiss]

Hey 👋 Thanks!

I think there is one thing missing which is some handling of known / unknown notations. Currently this code will accept all notations but we should reject them if they're critical and unknown.

In OpenPGP.js we have config.knownNotations for this purpose, we could add something similar here.

[wiktor-k]

Good point @twiss, I should have remembered that! 😅

[twiss]

Right :D Thanks for pushing this forward in all our implementations ^.^

[omz13]

The Notations structure does not contain any exportable fields... should they be exported? Just thinking about this because I'm dumping some entities as JSON data and the Notations entry is there but an empty object because nothing to export (even though there's some notation data present).

[twiss]

Yeah, let's change it to

type Notation struct {
	Name          string
	Value         []byte
	HumanReadable bool
	Critical      bool
}

and get rid of the getters, that will make the API a bit simpler as well.

[omz13]

Yeah, let's change it to

type Notation struct {
	Name          string
	Value         []byte
	HumanReadable bool

IsHumanReadable?

Critical bool

IsCritical?

}

and get rid of the getters, that will make the API a bit simpler as well.

Yep. No need for getters.

[twiss]

IsHumanReadable?

Critical bool

IsCritical?

Yeah, maybe that's better 👍

[wiktor-k]

I just noticed (for the record) that my old commit contains quite a few examples that could be used in unit tests for critical notations here:

openpgpjs/openpgpjs@8279939#diff-c789f741e120e8bd74404f74450907c81bcc9918eb3de78037b04b15552290e7R470-R637

[wussler]

Leaving this on hold until we find a way to pass the notation data for signature validation in key parsing

[twiss]

I've added config.KnownNotations, a map[string]bool indicating which notation names are known (i.e. allowed for critical notations). This is then checked when reading a message / verifying a signature, both for the signature itself and all relevant key binding signatures. Let me know if anyone sees an issue with this API!

@wiktor-k thanks for the link to the test case, I've added it here as well :)

[wiktor-k]

Thanks everyone! 👏 Great to see this finally being in! 👍

Have a great evening! 👋

[twiss]

Thanks, you too :)

[omz13]

Many many many thanks!