Merge #1820

1820: Re-enable signature health checks with a faster library r=rehandalal a=mythmon

As a reminder, we disabled these checks because they were too slow. The [library](https://pypi.org/project/ecdsa/) we were using to validate signatures is a pure Python library, and is not focused on speed. I added [a new library](https://pypi.org/project/fastecdsa/) that is more focused on speed, though it includes C dependencies, which might make our life harder.

The new library is significantly less friendly, as well. I had to jump through some more hoops to actually use it, and I actually still use the slow library to decode the signature into the format expected by the new one. I think that's probably fine for now, but I could work on removing it entirely if it's important.

To test the speed increase, I created 100 signed recipes, re-enabled the test, and timed the signature verification check before and after this change.

Before:

```
Checked signatures for 100 recipes in 0:00:20.166276 (4.96 checks/second)
```

After:

```
Checked signatures for 100 recipes in 0:00:00.913976 (109 checks/second)
```

I think an almost 22x speed up should make it ok to re-enable these checks. :smile: 	

I haven't tested the Docker build yet. Given that the new library has a C component, I suspect it might fail to install on some systems. On my laptop, I had to install `gmp-devel` to build the library. I expect CI will catch this issue though, so I'm not worried.

Co-authored-by: Mike Cooper <[email protected]>
Co-authored-by: Michael Cooper <[email protected]>

Author: bors[bot]

Dockerfile

@@ -4,7 +4,7 @@ RUN groupadd --gid 10001 app && useradd -g app --uid 10001 --shell /usr/sbin/nol
 RUN apt-get update && \
     apt-get upgrade -y && \
     apt-get install -y --no-install-recommends \
-    gcc libpq-dev curl apt-transport-https libffi-dev openssh-client gnupg
+    gcc libpq-dev curl apt-transport-https libffi-dev openssh-client gnupg python-dev libgmp3-dev
 
 # Install node from NodeSource
 RUN curl -s https://deb.nodesource.com/gpgkey/nodesource.gpg.key | apt-key add - && \

normandy/recipes/checks.py

@@ -156,8 +156,7 @@ def geoip_db_is_available(app_configs, **kwargs):
 
 def register():
     register_check(actions_have_consistent_hashes)
-    # Temporarily disabled, see Issue #900.
-    # register_check(recipe_signatures_are_correct)
-    # register_check(action_signatures_are_correct)
+    register_check(recipe_signatures_are_correct)
+    register_check(action_signatures_are_correct)
     register_check(signatures_use_good_certificates)
     register_check(geoip_db_is_available)

normandy/recipes/signing.py

@@ -5,8 +5,10 @@
 import re
 from datetime import datetime
 
-import ecdsa
 import requests
+import ecdsa.util
+import fastecdsa.ecdsa
+from fastecdsa.encoding.pem import PEMEncoder
 from pyasn1.codec.der.decoder import decode as der_decode
 from pyasn1.codec.native.encoder import encode as python_encode
 from pyasn1_modules import rfc5280
@@ -102,40 +104,34 @@ def verify_signature(data, signature, pubkey):
     If the signature is valid, returns True. If the signature is invalid, raise
     an exception explaining why.
     """
+    # Data must be encoded as bytes
     if isinstance(data, str):
         data = data.encode()
 
-    # Add data template
+    # Content signature implicitly adds a prefix to signed data
     data = b"Content-Signature:\x00" + data
 
-    try:
-        verifying_pubkey = ecdsa.VerifyingKey.from_pem(pubkey)
-    except binascii.Error as e:
-        if e.args == ("Incorrect padding",):
-            raise WrongPublicKeySize()
-        else:
-            raise
-    except IndexError:
-        raise WrongPublicKeySize()
+    # fastecdsa expects ASCII armored keys, but ours is unarmored. Add the
+    # armor before passing the key to the library.
+    EC_PUBLIC_HEADER = "-----BEGIN PUBLIC KEY-----"
+    EC_PUBLIC_FOOTER = "-----END PUBLIC KEY-----"
+    verifying_pubkey = PEMEncoder.decode_public_key(
+        "\n".join([EC_PUBLIC_HEADER, pubkey, EC_PUBLIC_FOOTER])
+    )
 
     try:
         signature = base64.urlsafe_b64decode(signature)
+        signature = ecdsa.util.sigdecode_string(signature, order=ecdsa.curves.NIST384p.order)
     except binascii.Error as e:
         if BASE64_WRONG_LENGTH_RE.match(e.args[0]):
             raise WrongSignatureSize("Base64 encoded signature was not a multiple of 4")
         else:
             raise
-
-    verified = False
-
-    try:
-        verified = verifying_pubkey.verify(signature, data, hashfunc=hashlib.sha384)
-    except ecdsa.keys.BadSignatureError:
-        raise SignatureDoesNotMatch()
     except AssertionError as e:
-        # The signature verifier has a clause like
+        # The signature decoder has a clause like
         #     assert len(signature) == 2*l, (len(signature), 2*l)
-        # Check that the AssertionError is consistent with that
+        # If the AssertionError is consistent with that signature, translate it
+        # to a nicer error. Otherwise re-raise.
         if (
             len(e.args) == 1
             and isinstance(e.args[0], tuple)
@@ -147,8 +143,12 @@ def verify_signature(data, signature, pubkey):
         else:
             raise
 
+    verified = fastecdsa.ecdsa.verify(
+        signature, data, verifying_pubkey, curve=fastecdsa.curve.P384, hashfunc=hashlib.sha384
+    )
+
     if not verified:
-        raise BadSignature()
+        raise SignatureDoesNotMatch()
 
     return True
 

requirements/default.txt

@@ -219,4 +219,9 @@ untangle==1.1.1 \
     --hash=sha256:e7cfa1ad57707e6b74cfea8b9fc50f7cbe9bbaf18401cc9d72192002bcd80bcb
 drf-yasg==1.14.0 \
     --hash=sha256:a276bc90af1902b1bd9c11927f75658da1a62aad2d87022ae5f653106ed09a17 \
-    --hash=sha256:ca17555127f8ac59d51c2bf721eff83b46ae20a8626c033f186a4c4d7d6053f4
+    --hash=sha256:ca17555127f8ac59d51c2bf721eff83b46ae20a8626c033f186a4c4d7d6053f4
+fastecdsa==1.7.1 \
+    --hash=sha256:ca3b70122a95a310020758924f0772527bf4758b830460b87fa948d87ca2205d \
+    --hash=sha256:018c5aed286ccc81d858593a08fb5600bead4183969f934fec05aaa2249f0c3f \
+    --hash=sha256:3e493b03050484c4f48ba2a96933908baea49d64a12358e22fd9bff38c483bbd
+