Closed arudzitis-stripe closed 1 year ago
Hey :wave: Thanks for the PR and the detailed design considerations!
- The regular expression on the Signature is a
*string
. Alternatively, we could go with""
being treated the same as a nil value, but unlike theTrustAmount
, there was no other value to lean on for a hint of intention.
Hmm. I think we could go either way with this; on the one hand I would say that since an empty regular expression normally matches anything, it's equivalent to the regex not being there, so we could use the empty string. On the other hand, the RFC says:
A regular expression is zero or more branches, separated by |. It matches anything that matches one of the branches.
Which would seem to imply that the empty regex doesn't match anything. I think this might just be an oversight, but to avoid the confusion we could stick with a pointer, yeah.
- Signature subpackets can each optionally be
critical
. It doesn't look like this library has a pattern for preserving thecritical
flag after parsing it, or allowing the user to indicate whether it is critical. I replicated behavior we had elsewhere and just hard coded this subpacket to critical upon serialization, because that's what I've seen in the wild and also what makes sense to me in this context, but I'm open to looking into a larger change.
Yeah, I think this is reasonable :+1: Since a regex reduces the scope of the signature, it's important that it's understood by the other implementations when using it, so always marking it critical makes sense.
- The RFC specifies the regex is a null-terminated string, but
gpg
doesn't appear to expect the string to be null-terminated (the null is shown in the user output, and is not required to parse the outputs). I've followed the RFC, but let me know if you have other thoughts.
Here I'm even more skeptical of the RFC. Coincidentally I also questioned this on the OpenPGP WG mailing list here, but didn't get any response to that particular question. Do you happen to know what GnuPG or other implementations generate when creating a signature with a regex? If they don't generate a trailing \0
, I wouldn't do so here either.
If I create a key using GnuPG, it does appear to append a \0
. (GnuPG doesn't let you specify a regex; it just asks for a domain and generates the regex.)
Example:
critical hashed subpkt 6 len 25 (regular expression: "<[^>]+[@.]example\x5c.com>$\0")
Hello, it is me again. You may remember me from pull request such as https://github.com/ProtonMail/go-crypto/pull/138.
I noticed when reviewing the issue with the above pull request (https://github.com/ProtonMail/go-crypto/issues/86), it referenced a slightly different issue in the original Go library: https://github.com/golang/go/issues/20686, which is that we're also missing support for Regular Expression packets (which work hand-in-hand with the trust packets), so I thought I would throw that in as well, if you're interested.
The shape of the change is very similar to https://github.com/ProtonMail/go-crypto/pull/138, but I wanted to call out a few design decisions:
*string
. Alternatively, we could go with""
being treated the same as a nil value, but unlike theTrustAmount
, there was no other value to lean on for a hint of intention.critical
. It doesn't look like this library has a pattern for preserving thecritical
flag after parsing it, or allowing the user to indicate whether it is critical. I replicated behavior we had elsewhere and just hard coded this subpacket to critical upon serialization, because that's what I've seen in the wild and also what makes sense to me in this context, but I'm open to looking into a larger change.gpg
doesn't appear to expect the string to be null-terminated (the null is shown in the user output, and is not required to parse the outputs). I've followed the RFC, but let me know if you have other thoughts.Data used for test case: