Improve error reporting when the verification key is invalid

achille-roussel commented 8 months ago

When setting a string that doesn't contain the ---- BEGIN PUBLIC KEY ---- prefix, the program crashes due to accessing a method that doesn't exist, for example:

from fastapi import FastAPI
from dispatch.fastapi import Dispatch

app = FastAPI()

dispatch = Dispatch(app,
  endpoint="https://420-69-24-7-365.ngrok-free.app/",
  api_key="apikeygoeshereapikeygoeshereapikeygoeshere",
  verification_key="YXdlcG9ya2F3ZW9ya3Bva2FwZ29rcGFvc2twYXNlb3JrZmRhc2RmYXNhd3M=")

Traceback (most recent call last):
  File "/opt/homebrew/bin/uvicorn", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/main.py", line 418, in main
    run(
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/main.py", line 587, in run
    server.run()
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/server.py", line 62, in run
    return asyncio.run(self.serve(sockets=sockets))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 685, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/server.py", line 69, in serve
    config.load()
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/config.py", line 458, in load
    self.loaded_app = import_from_string(self.app)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/lib/python3.12/site-packages/uvicorn/importer.py", line 21, in import_from_string
    module = importlib.import_module(module_str)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 995, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "/Users/achilleroussel/go/src/github.com/stealthrocket/dispatch-py/start.py", line 6, in <module>
    dispatch = Dispatch(app,    endpoint="https://420-69-24-7-365.ngrok-free.app/",                    api_key="apikeygoeshereapikeygoeshereapikeygoeshere", verification_key="YXdlcG9ya2F3ZW9ya3Bva2FwZ29rcGFvc2twYXNlb3JrZmRhc2RmYXNhd3M=")
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/achilleroussel/go/src/github.com/stealthrocket/dispatch-py/src/dispatch/fastapi.py", line 126, in __init__
    base64_key = base64.b64encode(verification_key.public_bytes_raw()).decode()
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'str' object has no attribute 'public_bytes_raw'

We should report an error that informs users about what they misconfigured instead of something that looks like an internal bug.

kevburnsjr commented 8 months ago

I'm not sure the prefix is the issue. This code also produces the same error (when using real values).

from fastapi import FastAPI
from dispatch.fastapi import Dispatch

app = FastAPI()

dispatch = Dispatch(app,
    endpoint="https://420-69-24-7-365.ngrok-free.app",
    api_key="apikeygoeshereapikeygoeshereapikeygoeshere",
    verification_key="-----BEGIN PUBLIC KEY-----\nYXdlcG9ya2F3ZW9ya3Bva2FwZ29rcGFvc2twYXNlb3JrZmRhc2RmYXNhd3M=\n-----END PUBLIC KEY-----\n",
)

chriso commented 8 months ago

FYI that verification_key has type Ed25519PublicKey | None, so mypy or an IDE would catch this issue.

At this stage, the user needs to either parse the PEM key with public_key_from_bytes(bytes) -> Ed25519PublicKey:

from dispatch.signature import public_key_from_bytes

... or similarly by using base64.b64decode if the key is in base64 format.

Given that (AFAIK) there are no base64 keys which are also valid PEM keys, and vice versa, we could widen the type of verification_key to Ed25519PublicKey | bytes | str | None and automatically detect the type of key the user has provided.

achille-roussel commented 8 months ago

@chriso you suggestion to widen the accepted types seems right to me:

it makes sense to be able to pass the same value to the DISPATCH_VERIFICATION_KEY environment variable and the verification_key argument (e.g., a string)
I have gotten tricked by this same problem before, it's unfortunate that Python is so permissive that the problem triggers deep within the abstraction layers
it seems wrong to require users to know about Ed25519PublicKey which is a bit of an implementation detail

It's true that dispatch.signature simplifies that, but it seems we could help users a lot more if we weren't asking them to deal with this complexity, what do you think?

chriso commented 8 months ago

I agree, let's make the change :+1: sorry it wasn't clear from the previous comment, but I was trying to highlight what the problem was today and suggest that we should widen the type (as long as it's possible to disambiguate key types).

dispatchrun / dispatch-py

Improve error reporting when the verification key is invalid #115