[PATCH 1/5] Fix lookups for uncommitted keys

[PATCH 1/5] Fix lookups for uncommitted keys

[Konstantin Ryabitsev]

Fix for the case when a key is added the repository but hasn't been
committed yet -- we were looking for it in the wrong subpath.

Signed-off-by: Konstantin Ryabitsev <konstantin.ryabitsev@linux.dev>
---
 patatt/__init__.py | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/patatt/__init__.py b/patatt/__init__.py
index 9602392..460d282 100644
--- a/patatt/__init__.py
+++ b/patatt/__init__.py
@@ -47,7 +47,7 @@ OPT_HDRS = [b'message-id']
 KEYCACHE = dict()
 
 # My version
-__VERSION__ = '0.4.3'
+__VERSION__ = '0.4.4'
 MAX_SUPPORTED_FORMAT_VERSION = 1
 
 
@@ -723,19 +723,21 @@ def get_public_key(source: str, keytype: str, identity: str, selector: str) -> T
         parts = source.split(':', 4)
         if len(parts) < 4:
             raise ConfigurationError('Invalid ref, must have at least 3 colons: %s' % source)
-        gittop = parts[1]
+        gitrepo = parts[1]
         gitref = parts[2]
         gitsub = parts[3]
-        if not gittop:
-            gittop = get_git_toplevel()
-        if not gittop:
-            raise KeyError('Not in a git tree, so cannot use a ref: source')
-
-        gittop = os.path.expanduser(gittop)
-        if gittop.find('$') >= 0:
-            gittop = os.path.expandvars(gittop)
-        if os.path.isdir(os.path.join(gittop, '.git')):
-            gittop = os.path.join(gittop, '.git')
+        if not gitrepo:
+            gitrepo = get_git_toplevel()
+        if not gitrepo:
+            raise KeyError('Not in a git tree, so cannot use a ref:: source')
+
+        gitrepo = os.path.expanduser(gitrepo)
+        if gitrepo.find('$') >= 0:
+            gitrepo = os.path.expandvars(gitrepo)
+        if os.path.isdir(os.path.join(gitrepo, '.git')):
+            gittop = os.path.join(gitrepo, '.git')
+        else:
+            gittop = gitrepo
 
         # it could omit the refspec, meaning "whatever the current ref"
         # grab the key from a fully ref'ed path
@@ -767,8 +769,8 @@ def get_public_key(source: str, keytype: str, identity: str, selector: str) -> T
             logger.debug('KEYSRC  : %s', keysrc)
             return out, 'ref:%s:%s' % (gittop, keysrc)
 
-        # Does it exist on disk in gittop?
-        fullpath = os.path.join(gittop, subpath)
+        # Does it exist on disk but hasn't been committed yet?
+        fullpath = os.path.join(gitrepo, subpath)
         if os.path.exists(fullpath):
             with open(fullpath, 'rb') as fh:
                 logger.debug('KEYSRC  : %s', fullpath)
-- 
2.31.1

[PATCH 2/5] Add "frequently seen commentary"

[Konstantin Ryabitsev]

Patatt hit a few news sources in the past few days, which resulted in
some expected commentary. Add some extra reasoning into the README that
would hopefully provide some answers to questions before they are asked.

Signed-off-by: Konstantin Ryabitsev <konstantin.ryabitsev@linux.dev>
---
 README.rst | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/README.rst b/README.rst
index 924b622..b751b68 100644
--- a/README.rst
+++ b/README.rst
@@ -400,3 +400,96 @@ Submissions must be made under the terms of the Linux Foundation
 certificate of contribution and should include a Signed-off-by: line.
 Please read the DCO file for full legal definition of what that implies.
 
+Frequently seen commentary
+--------------------------
+Why is this library even needed? Why not...
+
+Why not simply PGP-sign all patches?
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+PGP-signing patches causes important problems for reviewers. If a patch
+is inline-signed, then this not only adds textual headers/footers, but
+adds additional escaping in the protected body, converting all '^-'
+sequences into '^- -', which corrupts patches.
+
+MIME-signing is better, but has several other downsides:
+
+- messages are now sent as multipart mime structures, which causes some
+  tooling to no longer properly handle the patch content
+- the signature attachments may be stripped/quarantined by email
+  gateways that don't properly recognize OpenPGP mime signatures
+- the From/Subject headers are rarely included into protected content,
+  even though they are crucial parts of what ends up going into a git
+  commit
+
+These considerations have resulted in many projects specifically
+requesting that patches should NOT be sent PGP-signed.
+
+Why not just rely on proper code review?
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Code review is a crucial step of the development process and patatt does
+not aim to replace it. However, there are several areas where the
+process can be abused by malicious parties in the absence of end-to-end
+cryptographic attestation:
+
+1. A maintainer who struggles with code review volume may delegate parts
+   of their duties to a submaintainer. If that person submits aggregated
+   patch series to the maintainer after performing that work, there must
+   be a mechanism to ensure that none of the reviewed patches have been
+   modified between when they were reviewed by the trusted submaintainer
+   and when the upstream developer applies them to their tree. Up to
+   now, the only mechanism to ensure this was via signed pull requests
+   -- with patatt this is now also possible with regular patch series.
+
+2. It is important to ensure that what developer reviews is what
+   actually ends up being applied to their git tree. Linux development
+   process consists of collecting follow-up trailers (Tested-by,
+   Reviewed-by, etc), so various tooling exists to aggregate these
+   trailers and create the collated patch series containing all
+   follow-up tags (see b4, patchwork, etc). Patatt signing provides a
+   mechanism to ensure that what that developer reviewed and approved
+   and what they applied to their tree is the exact same code and hasn't
+   been maliciously modified in-between review and "git am" (e.g. by
+   archival services such as lore.kernel.org, mail hosting providers,
+   someone with access to the developer's inbox, etc).
+
+3. An attacker may attempt to impersonate a well-known developer by
+   submitting malicious code, perhaps with the hope that it receives
+   less scrutiny and is accepted without rigorous code review. Even if
+   this attempt is unsuccessful (and it most likely would be), this may
+   cause unnecessary reputation damage to the person being impersonated.
+   Cryptographic signatures (and lack thereof) will help the developer
+   quickly establish that the attack was performed without their
+   involvement.
+
+Why not just rely on DKIM?
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+DKIM standard is great, but there are several places where it falls a
+bit short when it comes to patch attestation:
+
+1. The signing is done by the mail gateways that may or may not be
+   properly checking that the "From:" header matches the identity of the
+   authenticated user. For example, a service that allows free account
+   registration may not check that alice@example.org sends outgoing
+   email with "bob@example.org" in the "From:" field, which would allow
+   Alice to impersonate Bob and have the messages arrive with a valid
+   DKIM signature.
+
+2. DKIM is usually seen as merely a spam reduction mechanism, so there's
+   usually little incentive for infrastructure administrators to be too
+   strict about how they handle the private keys used for DKIM signing.
+   Most likely, they are just stored on disk without a passphrase and
+   accessible by the SMTP daemon.
+
+3. DKIM's "relaxed" canonicalization standard for message bodies
+   replaces all multiple whitespace characters with a single space
+   before the body hash is signed. This poses significant problems for
+   patches where whitespace is syntactically significant (Python,
+   Makefiles, etc). A "return True" with a different indent will pass
+   DKIM signature check and may introduce a serious security
+   vulnerability.
+
+4. DKIM doesn't prevent typosquatting attacks. For example, an attacker
+   attempting to impersonate known.developer@companyname.com may send an
+   email from known.developer@company-name.com or any other
+   similar-looking address or domain, with valid DKIM signatures in
+   every case.
-- 
2.31.1

[PATCH 3/5] Handle MIME encoded-word & other header manglings

[Konstantin Ryabitsev]

From: Paul Barker <paul@pbarker.dev>

When testing patatt with patches sent to a sr.ht hosted mailing list, it
was found that long header lines (such as the X-Developer-Signature
line) were re-encoded using the MIME encoded-word syntax (RFC 2047) when
an mbox archive is generated, causing patatt to choke on the resulting
text which looks like this:

    X-Developer-Signature: v=1; a=openpgp-sha256; l=672; h=from:subject;
     bh=C40yOKgIfnNIUP+OW9WyPdBfljkZPpfUL1NepOODlx8=; =?utf-8?q?b=3DowGbwMvMwCF2?=
     =?utf-8?q?w7xIXuiX9CvG02pJDAmb67lTNi0+IeF97TL76vtKD7xjSjaluz0o/KfmZLX8rMi7_?=
     =?utf-8?q?l3M6O0pZGMQ4GGTFFFl2z951+fqDJVt7b0gHw8xhZQIZwsDFKQATydFhZJi+fFfvJ?=
     =?utf-8?q?8+0MF7GrfzWnP?=
     K7mAM/3n/r/UC+bprf6/g114QYGdbHcsaK7b1nanfA4IeZi1V0lL26cruXUWxgSEnNDP1FrAA=

Avoiding this issue by neatly wrapping the X-Developer-Signature header
before sending doesn't appear to be possible without making invasive
changes to git-send-email and/or the Net::SMTP perl module. The header
content generated by patatt is wrapped at 78 characters as can be seen
here from a locally signed patch file:

    X-Developer-Signature: v=1; a=openpgp-sha256; l=672; h=from:subject;
    bh=C40yOKgIfnNIUP+OW9WyPdBfljkZPpfUL1NepOODlx8=;
    b=owGbwMvMwCF2w7xIXuiX9CvG02pJDAmbN1xO2bT4hIT3tcvsq+8rPfCOKdmU7vag8J+ak9XysyLv
    Xs7p7ChlYRDjYJAVU2TZPXvX5esPlmztvSEdDDOHlQlkCAMXpwBMpG0Dw/9Kpzgpc8UsQwOPK/taW6
    dFnZyy5QlXPfNCC4WTc76ft9ZnZJjI37a17fP7sxvclKJ1tm36EhITcK62Pphje9KrmOxMJg4A

Running `git send-email --smtp-debug=1 0001.patch` shows that this is
joined into a single long line before the message is sent:

    Net::SMTP::_SSL=GLOB(0x5646fbdc3ac8)>>> X-Developer-Signature: v=1; a=openpgp-sha256; l=672; h=from:subject; bh=C40yOKgIfnNIUP+OW9WyPdBfljkZPpfUL1NepOODlx8=; b=owGbwMvMwCF2w7xIXuiX9CvG02pJDAmb571P2bT4hIT3tcvsq+8rPfCOKdmU7vag8J+ak9XysyLv Xs7p7ChlYRDjYJAVU2TZPXvX5esPlmztvSEdDDOHlQlkCAMXpwBM5JA3I8O5hP6Tqm7lJst0rldcux 1V7M4q8T5o1fPU6Zs+hxj+SjvN8D/DK3rn8b0m34/Xy388Yeu8jvFdJf/c6Y6LDU7Hulj01nAAAA==

So we need to accept that the X-Developer-Signature line may be quite
long and so may be re-encoded by a mail server or archiver.

The Python email.header module provides the decode_header() and
make_header() functions which can be used to handle MIME encoded-word
syntax or other header manglings which may occur. The decode_header()
function requires a str argument so we must decode our bytes before
using this function. Thankfully, RFC 2822 makes life easy here as it
says that all header content must be composed of US-ASCII characters
(see section 2.2 of the RFC) so decoding is straightforward. The header
content is re-encoded into bytes after un-mangling to avoid having to
modify every other location in patatt where the header content is
accessed.

Signed-off-by: Paul Barker <paul@pbarker.dev>
Signed-off-by: Konstantin Ryabitsev <konstantin.ryabitsev@linux.dev>
Link: https://lore.kernel.org/r/20210531140539.7630-1-paul@pbarker.dev
---
 patatt/__init__.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/patatt/__init__.py b/patatt/__init__.py
index 460d282..b4018ab 100644
--- a/patatt/__init__.py
+++ b/patatt/__init__.py
@@ -91,7 +91,7 @@ class DevsigHeader:
 
     def from_bytes(self, hval: bytes) -> None:
         self.hval = DevsigHeader._dkim_canonicalize_header(hval)
-        hval = re.sub(rb'\s*', b'', hval)
+        hval = re.sub(rb'\s*', b'', self.hval)
         for chunk in hval.split(b';'):
             parts = chunk.split(b'=', 1)
             if len(parts) < 2:
@@ -392,6 +392,15 @@ class DevsigHeader:
 
     @staticmethod
     def _dkim_canonicalize_header(hval: bytes) -> bytes:
+        # Handle MIME encoded-word syntax or other types of header encoding if
+        # present. The decode_header() function requires a str argument (not
+        # bytes) so we must decode our bytes first, this is easy as RFC2822 (sec
+        # 2.2) says header fields must be composed of US-ASCII characters. The
+        # resulting string is re-encoded to allow further processing.
+        if b'?q?' in hval:
+            hval = hval.decode('ascii', errors='ignore')
+            hval = str(email.header.make_header(email.header.decode_header(hval)))
+            hval = hval.encode('utf-8')
         # We only do relaxed for headers
         #    o  Unfold all header field continuation lines as described in
         #       [RFC5322]; in particular, lines with terminators embedded in
-- 
2.31.1

[PATCH 4/5] Make instructions for automatic signing more reliable

[Konstantin Ryabitsev]

From: Paul Barker <paul@pbarker.dev>

We can't assume that the git directory path is '.git' from the root of
the source tree. For example, this is not the correct path if patatt is
checked out as a git submodule. We should use `git rev-parse --git-dir`
to reliably determine the git directory path. We should also surround
the path in quotes in case the user has cloned patatt in a path
containing spaces.

Signed-off-by: Paul Barker <paul@pbarker.dev>
Signed-off-by: Konstantin Ryabitsev <konstantin.ryabitsev@linux.dev>
Link: https://lore.kernel.org/r/20210530163623.926-1-paul@pbarker.dev
---
 README.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/README.rst b/README.rst
index b751b68..de299d1 100644
--- a/README.rst
+++ b/README.rst
@@ -184,8 +184,8 @@ Automatic signing via the sendemail-validate hook
 If everything is working well, you can start automatically signing all
 outgoing patches sent via git-send-email::
 
-    $ echo 'patatt sign --hook "${1}"' > .git/hooks/sendemail-validate
-    $ chmod a+x .git/hooks/sendemail-validate
+    $ echo 'patatt sign --hook "${1}"' > "$(git rev-parse --git-dir)/hooks/sendemail-validate"
+    $ chmod a+x "$(git rev-parse --git-dir)/hooks/sendemail-validate"
 
 PGP vs ed25519 keys considerations
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- 
2.31.1

[PATCH 5/5] Throw a NoKeyError when no matching PGP key

[Konstantin Ryabitsev]

Fix a problem where we incorrectly reported a missing public key for a
failing signature for the cases when the public key is in the default
keyring.

Signed-off-by: Konstantin Ryabitsev <konstantin.ryabitsev@linux.dev>
---
 patatt/__init__.py | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/patatt/__init__.py b/patatt/__init__.py
index b4018ab..f5e0fd9 100644
--- a/patatt/__init__.py
+++ b/patatt/__init__.py
@@ -47,7 +47,7 @@ OPT_HDRS = [b'message-id']
 KEYCACHE = dict()
 
 # My version
-__VERSION__ = '0.4.4'
+__VERSION__ = '0.4.5-dev'
 MAX_SUPPORTED_FORMAT_VERSION = 1
 
 
@@ -69,6 +69,12 @@ class ValidationError(Exception):
         self.errors = errors
 
 
+class NoKeyError(ValidationError):
+    def __init__(self, message: str, errors: Optional[list] = None):
+        super().__init__(message)
+        self.errors = errors
+
+
 class BodyValidationError(ValidationError):
     def __init__(self, message: str, errors: Optional[list] = None):
         super().__init__(message, errors)
@@ -346,6 +352,8 @@ class DevsigHeader:
             ecode, out, err = gpg_run_command(vrfyargs, stdin=bsigdata)
 
         if ecode > 0:
+            if err.find(b'[GNUPG:] NO_PUBKEY '):
+                raise NoKeyError('No matching key found')
             raise ValidationError('Failed to validate PGP signature')
 
         good, valid, trusted, signkey, signtime = DevsigHeader._check_gpg_status(err)
@@ -952,12 +960,14 @@ def validate_message(msgdata: bytes, sources: list, trim_body: bool = False) ->
             attestations.append((RES_VALID, i, signtime, keysrc, algo, errors))
         except ValidationError:
             if keysrc is None:
-                # Not in default keyring
-                errors.append('%s/%s no matching openpgp key found' % (i, s))
-                attestations.append((RES_NOKEY, i, t, None, algo, errors))
-                continue
-            errors.append('failed to validate using %s' % keysrc)
+                errors.append('failed to validate using default keyring')
+            else:
+                errors.append('failed to validate using %s' % keysrc)
             attestations.append((RES_BADSIG, i, t, keysrc, algo, errors))
+        except NoKeyError:
+            # Not in default keyring
+            errors.append('%s/%s no matching openpgp key found' % (i, s))
+            attestations.append((RES_NOKEY, i, t, None, algo, errors))
 
     return attestations
 
-- 
2.31.1